Re: [PATCH SLOF] disk-label: add support for booting from GPT FAT partition

[Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>]

Thomas Huth <thuth@redhat.com> writes:

> On Thu, 11 Jun 2015 15:48:49 +0530
> Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> wrote:
>
>> For a GPT+LVM combination disk, older bootloader that does not support
>> LVM, cannot load kernel from LVM.
>> 
>> The patch add support to read from BASIC_DATA UUID
>> partition. Installer has installed CHRP-BOOT config on a FAT file
>> system.
>> 
>> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
>> ---
>>  slof/fs/packages/disk-label.fs | 72 ++++++++++++++++++++++++++++++++++++++++--
>>  1 file changed, 70 insertions(+), 2 deletions(-)
>> 
>> diff --git a/slof/fs/packages/disk-label.fs b/slof/fs/packages/disk-label.fs
>> index fe1c25e..bf5fb5c 100644
>> --- a/slof/fs/packages/disk-label.fs
>> +++ b/slof/fs/packages/disk-label.fs
>> @@ -266,7 +266,7 @@ CONSTANT /gpt-part-entry
>>  
>>  : try-dos-partition ( -- okay? )
>>     \ Read partition table and check magic.
>> -   no-mbr? IF cr ." No DOS disk-label found." cr false EXIT THEN
>> +   no-mbr? IF false EXIT THEN
>
> Maybe keep the output when "debug-disk-label?" is set?

Sure.

>
>>     count-dos-logical-partitions TO dos-logical-partitions
>>  
>> @@ -408,6 +408,73 @@ AA26         CONSTANT GPT-PREP-PARTITION-4
>>     FALSE
>>  ;
>>  
>> +\ Check for GPT MSFT BASIC DATA GUID - vfat based
>> +EBD0A0A2     CONSTANT GPT-BASIC-DATA-PARTITION-1
>> +B9E5         CONSTANT GPT-BASIC-DATA-PARTITION-2
>> +4433         CONSTANT GPT-BASIC-DATA-PARTITION-3
>> +87C0         CONSTANT GPT-BASIC-DATA-PARTITION-4
>> +68B6B72699C7 CONSTANT GPT-BASIC-DATA-PARTITION-5
>> +
>> +: gpt-basic-data-partition? ( -- true|false )
>> +   block gpt-part-entry>part-type-guid l@-le GPT-BASIC-DATA-PARTITION-1 = IF
>> +      block gpt-part-entry>part-type-guid 4 + w@-le
>> +      GPT-BASIC-DATA-PARTITION-2 = IF
>> +         block gpt-part-entry>part-type-guid 6 + w@-le
>> +         GPT-BASIC-DATA-PARTITION-3 = IF
>> +            block gpt-part-entry>part-type-guid 8 + w@
>
> Don't you have to byte-swap (w@-le) here, too? Looks somehow strange
> that the other UID parts are read byte-swapped but this one is not?

Interesting observation, I had used code from gpt-prep-partition? and
did not doubt the validity of it. But that is how I see it in the memory
though.

4 >  7e50d000 10 dump 
7e50d000: a2 a0 d0 eb e5 b9 33 44 87 c0 68 b6 b7 26 99 c7  ......3D..h..&.. ok
4 >



>
>> +            GPT-BASIC-DATA-PARTITION-4 = IF
>> +               block gpt-part-entry>part-type-guid a + w@
>> +               block gpt-part-entry>part-type-guid c + l@ swap lxjoin
>
> dito ... here again no byteswap?
>
>> +               GPT-BASIC-DATA-PARTITION-5 = IF
>> +                   TRUE EXIT
>> +               THEN
>> +            THEN
>> +         THEN
>> +      THEN
>> +   THEN
>> +   FALSE
>> +;
>
> I'm not a fan of deep nesting, so I'd maybe write this function rather
> like this instead:

Sure, looks much simpler.

>
> : gpt-basic-data-partition? ( -- true|false )
>    block gpt-part-entry>part-type-guid
>    dup l@-le GPT-BASIC-DATA-PARTITION-1 <> IF FALSE EXIT THEN
>    dup 4 + w@-le GPT-BASIC-DATA-PARTITION-2 <> IF FALSE EXIT THEN
>    dup 6 + w@-le GPT-BASIC-DATA-PARTITION-3 <> IF FALSE EXIT THEN
>    ...
>    TRUE
> ;
>
> ... but that's just cosmetics.
>
>> +: try-gpt-vfat-partition ( -- okay? )
>> +   \ Read partition table and check magic.
>> +   no-gpt? IF cr ." No GPT disk-label found." cr false EXIT THEN
>
> Not directly related to this patch, but "no-gpt?" seems somewhat
> imprecise to me ... what if the whole MBR is accidentially filled
> with EE for example? That certainly does not indicate a valid GPT,
> does it?
>
> I might be wrong, but as far as I can say that function should also
> check for the aa55 magic at the end of the MBR, shouldn't it?
>
> Also I wonder whether you should check gpt>signature somewhere, either
> in "no-gpt?" or here before touching gpt>part-entry-lba entry below?
> I think that would contribute to the robustness of the code.

Sure, we can make this changes.

>> +   1 read-sector block gpt>part-entry-lba l@-le
>
> gpt>part-entry-lba seems to be an 8-bytes field ... so shouldn't you
> access it with "x@ xbflip" instead?

I will verfy this and make appropriate changes.

>
> By the way, it might be a good idea to add a "x@-le" helper for this to
> little-endian.fs.
>
>> +   block-size * to seek-pos
>> +   block gpt>part-entry-size l@-le to gpt-part-size
>> +   block gpt>num-part-entry l@-le dup 0= IF FALSE EXIT THEN
>> +   1+ 1 ?DO
>> +      seek-pos 0 seek drop
>> +      block gpt-part-size read drop
>
> Can you be sure that gpt-part-size is only smaller than 4096 bytes
> here? If not, you might overflow the block array, don't you?

True.

>> +      gpt-basic-data-partition? IF
>> +         debug-disk-label? IF
>> +            ." GPT LINUX DATA partition found " cr
>> +         THEN
>> +         block gpt-part-entry>first-lba x@ xbflip
>> +         dup to part-start
>> +         block gpt-part-entry>last-lba x@ xbflip
>> +         over - 1+                 ( addr offset len )
>> +         dup block-size * to part-size
>> +         swap                      ( addr len offset )
>> +         block-size * to part-offset
>> +         0 0 seek
>> +         block block-size read
>> +         3drop
>> +         \ block 0 byte 0-2 is a jump instruction in all FAT
>> +         \ filesystems.
>> +         \ e9 and eb are jump instructions in x86 assembler.
>> +         block c@ e9 <> IF
>> +            block c@ eb <>
>> +            block 2+ c@ 90 <> or
>> +            IF false EXIT THEN
>> +         THEN
>
> That's a copy of the code in try-dos-files ... could you please factor
> out that stuff into a separate function to avoid duplicate code? Thanks!
> (especially you also lack a UNLOOP before above EXIT otherwise, I
> think)

Yes.

>
>> +         TRUE
>> +         UNLOOP EXIT
>> +      THEN
>> +      seek-pos gpt-part-size i * + to seek-pos
>> +    LOOP
>> +    FALSE
>> +;
>> +
>>  \ Extract the boot loader path from a bootinfo.txt file
>>  \ In: address and length of buffer where the bootinfo.txt has been loaded to.
>>  \ Out: string address and length of the boot loader (within the input buffer)
>> @@ -493,7 +560,7 @@ AA26         CONSTANT GPT-PREP-PARTITION-4
>>  
>>     debug-disk-label? IF ." Trying CHRP boot " .s cr THEN
>>     1 disk-chrp-boot !
>> -   dup load-chrp-boot-file ?dup 0 <> IF .s cr nip EXIT THEN
>> +   dup load-chrp-boot-file ?dup 0 <> IF nip EXIT THEN
>>     0 disk-chrp-boot !
>>  
>>     debug-disk-label? IF ." Trying GPT boot " .s cr THEN
>> @@ -600,6 +667,7 @@ AA26         CONSTANT GPT-PREP-PARTITION-4
>>  
>>  : try-partitions ( -- found? )
>>     try-dos-partition IF try-files EXIT THEN
>> +   try-gpt-vfat-partition IF try-files EXIT THEN
>>     \ try-iso9660-partition IF try-files EXIT THEN
>>     \ ... more partition types here...
>>     false
>
>  Thomas

Regards
Nikunj