Implement GPT support #12
Conversation
|
also if you want and run out of time for this I can work on any finishing stuff |
|
Would appreciate that! Sorry for stepping on your toes a bit with this btw, I'd written it a few weeks ago but had been too busy to make the PR >_< |
|
no worries, you got a heck of a lot further than i did |
|
addressed the nits and pushed an update to the PR |
|
Also make sure not to merge this before we have some tests. I'll write some up in a couple of days if you don't get to it first :) |
|
I'll also be switching my os over to this pr branch at some point in a few days to try it out in a real scenario |
|
found a bug in that the BuildInterface doesnt provide quotes around the partition name |
|
i have my os project now successfully booting off of an efi/gpt disk setup using this PR |
|
thoughts on having a flag to mark the protective MBR as bootable? this is required for bios/gpt boots with some bioses |
|
my proposal would be to add a |
I think that should be part of a future PR if it is decided hybrid MBR/GPT would be worth doing. |
| std.mem.writeInt(u64, gpt_header[0x30..0x38], max_partition_lba, .little); // Last usable LBA | ||
| (table.disk_id orelse Guid.rand(random)).write(gpt_header[0x38..0x48]); | ||
| std.mem.writeInt(u64, gpt_header[0x48..0x50], 2, .little); // First LBA of the partition entry array | ||
| std.mem.writeInt(u32, gpt_header[0x50..0x54], @intCast(table.partitions.len), .little); // Number of partition entries |
There was a problem hiding this comment.
I'm not sure about this change you made, I'm pretty sure there was a reason I set this to 128. Please don't merge this before I've had a further look.
Sorry but I'm seeing what you've pushed here, and to me it doesn't make sense. The protective MBR shouldn't ever have the bootable flag set for a GPT system. To quote the UEFI spec: "If set to any value other than 0x00 the behavior of this flag on non-UEFI systems is undefined. Must be ignored by UEFI implementations." |
when booting BIOS/GPT, the value may still be checked by the bios, the efi spec just doesnt define anything about it; i can confirm that seabios (as used by qemu) checks it and setting the flag was required to make my os bootable as bios/gpt, otherwise nothing gets booted. |
|
Wait, you're using this with BIOS? But there's no provision to set the bootloader in this, and I think any BIOS support for this GPT implementation should be dealt with in a separate PR (if at all – why can't you just use MBR?) To clarify, it should be in a separate PR as if we do go ahead with hybrid MBR, this should be considered together, not least because it would possibly require breaking the syntax for the config flag you've used here. This PR's scope is strictly for booting GPT with UEFI, which by the spec should not require the protective MBR bootable flag set. |
Not really tbh. if it works on qemu, i'm fine with keeping it. We can always change it later and adjust if we figured out it's a problem with real hw. Imho we gain a lot if it works both with BIOS and UEFI
because then you can't boot the same disk image on UEFI systems. this way, you can construct a universal boot stick/disk |
A few things need doing before this is good to merge:
Really the GPT code should call into the MBR code to make the protective MBR. I can have a look in a few days at making this change, but happy for someone else to if they get to it before I do.I forgot I already did this.The code might not compile since as I was working on master. I think I've made sure the code is right now, but be sure that they are correct before merging :)