Skip to content
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

GlobalDescriptorTable: option/feature to increase number of entries #333

Closed
SlyMarbo opened this issue Jan 16, 2022 · 12 comments
Closed

GlobalDescriptorTable: option/feature to increase number of entries #333

SlyMarbo opened this issue Jan 16, 2022 · 12 comments

Comments

@SlyMarbo
Copy link

Hi there!

Would it be possible to increase the number of entries in the GDT? If you'd prefer to keep the default at 8, perhaps there could be a feature to increase it (to 16, perhaps)? To be able to use syscall/sysret, I reckon you need at least 7 entries, not including the null first entry. 2 for kernel code/data, 1 for the TSS, 2 for 32-bit user code/data and 2 for 64-bit code/data. If you also want an entry to allow CPU-local data with GS, then the current type panics. It's entirely possible I'm wrong, which would be great.

Thanks for all your hard work.

@Freax13
Copy link
Member

Freax13 commented Jan 16, 2022

Sure, that sounds reasonable!

If you also want an entry to allow CPU-local data with GS, then the current type panics. It's entirely possible I'm wrong, which would be great.

In long mode the x86-64 doesn't need a GDT entry for the for configuring GS segment, you have to configure the GsBase MSR instead. However, AFAICT, x86-64 in compatibility mode does use the GDT for memory segments, so a larger GDT might still be useful (I've never personally done anything with compatibility mode, so please correct me if I'm wrong here).

I believe we could implement this using const generics, allowing the user specify the maximum capacity. feature(const_generics_defaults) would make it possible to keep the current maximum (8) by default, which would also make this a non-breaking change. The downside to this is that feature(const_generics_defaults) won't be stable until Rust 1.59 which won't be released until the end of next month.

Of course we could also do the much simpler thing and simply increase the capacity to some fixed value eg. 16. We don't have any guarantees on the layout of our GDT type, so I guess/hope that changing the capacity won't break anyone's code.

@SlyMarbo
Copy link
Author

In long mode the x86-64 doesn't need a GDT entry for the for configuring GS segment, you have to configure the GsBase MSR instead.

Huh! I'm not sure why but I had become convinced that it was necessary. I've just tested and you're absolutely right. Well that reduces the urgency for me. If it's convenient, or of others ask for more options, great, but I think I'm unblocked here. Thanks for your help :)

@SlyMarbo
Copy link
Author

Ah, I'd forgotten that the TSS takes up two slots in the GDT. Would it still be possible to increase the length?

@Freax13
Copy link
Member

Freax13 commented Jan 16, 2022

feature(const_generics_defaults) would make it possible to keep the current maximum (8) by default, which would also make this a non-breaking change.

Unfortunately, this doesn't work, I misunderstood how much Rust can infer automatically.
feature(const_generics_defaults) makes this work:

let mut gdt: GlobalDescriptorTable = GlobalDescriptorTable::new();

However this does not work:

let mut gdt = GlobalDescriptorTable::new();
error[E0282]: type annotations needed for `GlobalDescriptorTable<{_: usize}>`
 --> src/structures/gdt.rs:39:15
  |
6 | let mut gdt = GlobalDescriptorTable::new();
  |     -------   ^^^^^^^^^^^^^^^^^^^^^^^^^^ cannot infer the value of const parameter `MAX_CAPACITY`
  |     |
  |     consider giving `gdt` the explicit type `GlobalDescriptorTable<MAX_CAPACITY>`, where the const parameter `MAX_CAPACITY` is specified

We could work around this by always returning GlobalDescriptorTable<8> from GlobalDescriptorTable::new and add a different constructor that allows variable sizes until the next breaking change, but this feel a bit too hackish IMO.


To be honest, I'm not so sure anymore if const generics are the right way to solve this: A lot of our users will probably never need anything but the default, so adding const generics would add unnecessary complexity for most. I thought we could avoid this using default values, but as I already said, this turned out to be incorrect.

Simply increasing the capacity will work for now, but only until the next person requires a bigger capacity. This might still be the most pragmatic solution though.

Another possible solution might be to allow creating GDTs of dynamic lengths at runtime by supplying a slice of u64s as backing memory. This would probably need another type though, I don't see how we could integrate this into the existing GDT type.

I'm open to any other suggestions or feedback!

@SlyMarbo
Copy link
Author

That's a shame. I don't want to cause a big change unnecessarily. I would be happy to avoid 32-bit user processes and therefore only need 1 (null) + 2 (kernel code/data) + 2 (TSS) + 2 (user code/data), but my intepretation of sysenter/sysexit and syscall/sysret is that they take different approaches to handling 32-bit and 64-bit user code. I currently use sysexit to start new user threads, then syscall/sysret for syscalls. That may be the wrong approach.

My understanding is that sysexit requires the user code segment to be immediately before the user data segment, as the data segment is inferred by adding 8 to the code segment in the IA32_SYSENTER_CS MSR. As I understand it, syscall/sysret gets the user code segment by adding 16 to IA32_STAR if returning to 64-bit user code, or adding 0 if returning to 32-bit user code, but always gets the user data segment by adding 8 to IA32_STAR, no matter what.

If I understand correctly, that means the intent is that you have both 32-bit and 64-bit segment selectors for user code, passing the 64-bit user code selector and the 32-bit user data selector to Star::write. It would be possible to support only 64-bit user code and therefore have only one pair of user segment selectors, but then sysexit would require the code selector to come first, while sysret would require them to be the other way around. I guess I could try having 3 user segment selectors, not having the second user data selector.

Does that make sense? Would I just be better off using iret to start user code instead of sysexit? If I'm doing something weird and that's the only reason for having more than 8 entries in the GDT I'm happy to do something more sensible. Sorry for the essay.

@Freax13
Copy link
Member

Freax13 commented Jan 16, 2022

I currently use sysexit to start new user threads, then syscall/sysret for syscalls.

sysexit is not supported by AMD CPUs in long mode. Was there a specific reason you used sysexit instead of sysret?

If you only use syscall/sysret and not sysenter/sysexit the following GDT should work:

Index Descriptor Comment
0 empty Not usable
1 KERNEL_CODE64 STAR[47:32] should point here.
2 KERNEL_DATA
3 USER_CODE32 STAR[63:48] should point here.
4 USER_DATA This segment is used in 32 and 64 bit modes.
5 USER_CODE64
6-7 TSS

That being said, I still think adding more capacity could be useful e.g. for setting up setting segments for user space programs in compatibility mode, however this is probably not as urgently needed.

Sorry for the essay.

No need to apologize, you provided a lot of information about your current setup and what you already figured out about what the CPU wants, this is very useful to understand the requirements and possible solutions!

@SlyMarbo
Copy link
Author

sysexit is not supported by AMD CPUs in long mode. Was there a specific reason you used sysexit instead of sysret?

Oh, I didn't realise that! I went with sysexit because it seemed simpler to set the stack pointer. I might as well switch over to sysret.

If you only use syscall/sysret and not sysenter/sysexit the following GDT should work:

Yeah, you're absolutely right. Thanks again for all your help!

@phil-opp
Copy link
Member

We could work around this by always returning GlobalDescriptorTable<8> from GlobalDescriptorTable::new and add a different constructor that allows variable sizes until the next breaking change, but this feel a bit too hackish IMO.

This would be fine in my opinion. Most users would just use the default size through the new contructor and people with different needs could use a different size through a new_with_size constructor. This way, no type annotations would be required.

SlyMarbo added a commit to ProjectSerenity/firefly that referenced this issue Jan 22, 2022
It turns out that we don't need to have a segment selector for GS
to be able to use the GSBase MSR, as discussed in rust-osdev/x86_64#333.
I suspect this is because we now have a kernel data segment selector
for SS, which we didn't when I first added support for CPU-local
data. Removing the extra selector frees up a slot in the GDT.

Signed-off-by: SlyMarbo <the.sly.marbo@googlemail.com>
SlyMarbo added a commit to ProjectSerenity/firefly that referenced this issue Feb 10, 2022
It turns out that we don't need to have a segment selector for GS
to be able to use the GSBase MSR, as discussed in rust-osdev/x86_64#333.
I suspect this is because we now have a kernel data segment selector
for SS, which we didn't when I first added support for CPU-local
data. Removing the extra selector frees up a slot in the GDT.

Signed-off-by: SlyMarbo <the.sly.marbo@googlemail.com>
SlyMarbo added a commit to ProjectSerenity/firefly that referenced this issue Feb 11, 2022
It turns out that we don't need to have a segment selector for GS
to be able to use the GSBase MSR, as discussed in rust-osdev/x86_64#333.
I suspect this is because we now have a kernel data segment selector
for SS, which we didn't when I first added support for CPU-local
data. Removing the extra selector frees up a slot in the GDT.

Signed-off-by: SlyMarbo <the.sly.marbo@googlemail.com>
SlyMarbo added a commit to ProjectSerenity/firefly that referenced this issue Feb 11, 2022
It turns out that we don't need to have a segment selector for GS
to be able to use the GSBase MSR, as discussed in rust-osdev/x86_64#333.
I suspect this is because we now have a kernel data segment selector
for SS, which we didn't when I first added support for CPU-local
data. Removing the extra selector frees up a slot in the GDT.

Signed-off-by: SlyMarbo <the.sly.marbo@googlemail.com>
@josephlr
Copy link
Contributor

So if we had something like this:

pub struct GlobalDescriptorTable<const MAX: usize = 8> {
    table: [u64; MAX],
    next_free: usize,
}

We could just have:

impl GlobalDescriptorTable {
    /// Creates an empty GDT with the default length.
    pub const fn new() -> Self {
        Self::empty()
    }
}

impl<const MAX: usize> GlobalDescriptorTable<MAX> {
    /// Creates an empty GDT.
    #[inline]
    pub const fn empty() -> Self {
        Self {
            table: [0; MAX],
            next_free: 1,
        }
    }
}

That way we keep the reasonable default of 8 for GlobalDescriptorTable::new(), but we can still have everything work with a custom size. Also, this wouldn't even be a breaking change per: https://doc.rust-lang.org/cargo/reference/semver.html#generic-new-default

@phil-opp
Copy link
Member

Sounds good to me!

@josephlr
Copy link
Contributor

Based on #355 it seems like our current MSRV when no features are enabled is Rust 1.57. Default const generic parameters were introduced in Rust 1.59, so this change will have to be on the next branch.

I'll try to get a PR out with this later today.

@josephlr
Copy link
Contributor

Fixed by #360

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants