- 
                Notifications
    You must be signed in to change notification settings 
- Fork 388
Introduce enum-class for memory flags #2329
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's go with Protect here - page protection is well established and originates in concrete CPU features.
| /** Conversion from x86 paging flags to mem::Accessflags **/ | ||
| os::mem::Access to_memflags(Flags f); | ||
| /** Conversion from x86 paging flags to mem::Permission flags **/ | ||
| os::mem::Permission to_memflags(Flags f); | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's go with os::mem::Prot, os::mem::Protect or os::mem::Protection instead - which ever you prefer, aligning with Intel manual and mprotect terminology.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do prefer os::mem::Protection myself, if we have to align ourselves to PROT_* values. I was hoping to use our own constexpr semantics with more extended features. I would like to distinguish between "allow-all" and "undefined protections". There is no way of doing this with a traditional Protection bitmask, since PROT_NONE = 0. Keeping the same name could be confusing.
That is, Protection is the opposite of Permission. Also, related question: do protection flags work the same on all/most architectures?
|  | ||
| Access mem::protect_page(uintptr_t linear, Access flags) | ||
| /* | ||
| * TODO(mazunki): | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't commit todo's in code. And the semantics of protect also covers removing flags:
mprotect() changes the access protections for the calling process's memory pages
From https://man7.org/linux/man-pages/man2/mprotect.2.html
If there are existing conventions in linux / posix that covers the exact same feature, and we're doing exactly the same thing, we should align on the language.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I agree that if one has read the manpage and understand how to use it already the usage of protect_page() makes perfect sense. My argument here is that one shouldn't need to read the manpage to understand what the code does. I feel like mprotect/protect_page are misleading in name, and we have the opportunity to provide a more intuitive API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding TODOs, do you have a better idea for progressive changes and ideas? A TODO is less invasive than an overloaded PR, but more commiting than opening an issue. Do you specifically mean personal TODOs, or TODOs in general?
The reason I tag some of the TODOs by name is partially because I claim I am going to fix them soon-ish, while also wanting some feedback on the idea before I start working on it; but also so that if it's left undone/unimplemented people can know who to ask when they eventually hit the same buggy code.
I do understand and agree it looks ugly; but I think it's better to document things that should be fixed with information that might be useful when starting to work on it, instead of hoping one doesn't forget when the time comes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I agree that if one has read the manpage and understand how to use it already the usage of
protect_page()makes perfect sense. My argument here is that one shouldn't need to read the manpage to understand what the code does. I feel like mprotect/protect_page are misleading in name, and we have the opportunity to provide a more intuitive API.
"More intuitive" is very subjective. We can't know what's more or less intuitive unless we do a survey. So the guideline is that for mechanisms that are already well known and that we create a 1-1 implementation of, we don't reinvent names just because we like them better. That just confuses the literature.
If you're making something new and different, you should make your own names, and then it will be entirely your choice how you name things. Here though, we're not providing the mechanism - the hardware does. We are just exposing it to code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding TODOs, do you have a better idea for progressive changes and ideas? A TODO is less invasive than an overloaded PR, but more commiting than opening an issue. Do you specifically mean personal TODOs, or TODOs in general?
The reason I tag some of the TODOs by name is partially because I claim I am going to fix them soon-ish, while also wanting some feedback on the idea before I start working on it; but also so that if it's left undone/unimplemented people can know who to ask when they eventually hit the same buggy code.
I do understand and agree it looks ugly; but I think it's better to document things that should be fixed with information that might be useful when starting to work on it, instead of hoping one doesn't forget when the time comes.
Open an issue. Issues can be tracked, assigned to people and put into projects. If you want to commit, assign the issue to you.
|  | ||
|  | ||
| // Enable bitwise ops on access flags | ||
| namespace util { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you remind me why you're breaking out a new header for this? The file is only 345 lines
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mainly because most of api/kernel/memory.hpp involves mappings (at a quick glance). I don't think it's particularly big either. I created sys/mman.hpp intending it to correlate one-to-one with sys_mman(0p) but for C++. Do you disagree?
8ca5b18    to
    3aef58d      
    Compare
  
    
This PR introduces enum-class types for memory flags, and acts as a first step refactor for
os::mem::Permission(which used to be called Access).Previously, we have been reusing the same "values" from POSIX which correspond to the mprotect() values, which I don't think makes much sense.
In my head, if I read
Access::none, my intepretation is that all access is denied: which is exactly the opposite of the case. I intend to introducePermission::Emptyto signify an unset permission,Permission::Anyto be equivalent toRead|Write|Execute. There's a set ofTODO(mazunki)objects which I'll handle in a subsequent PR for this unless there's any objections to the idea.I also see there's basically no tests involving
mmap()(only one I can see is a single unit test:util/unit/tar_test.cpp). I will have to introduce more tests in this aspect. @alfreb , you mentioned you built the allocators with a bunch of tests: can you remember if there were any tests beyond the unit tests for{buddy,fixed_list,pmr}_alloc_test.cpp? Could be I'm just blind here.There are a few references to
os::mem::vmmap(). Other than being related to virtual memory, it's not really super clear what this is supposed to do. It seems to just be a generic memory map of the whole memory space. There are far more tests for this, but far more undocumented.After this is properly merged, I also intend to modify the signature of
sys_mmap()to only accept enums. Currently, the implementation is really just an internal typecast: it makes refactoring easier. A benefit of using enum-classes is that we can fail at compile-time if wrong values are provided.I also intend to change the type of the file descriptor to something like an
std::optional<int>or similar. This is C++, we shouldn't need magic values for this sort of thing.