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

[Thomas Huth]

On Thu, 11 Jun 2015 15:48:49 +0530
Nikunj A Dadhania <nikunj at 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 at 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?

>     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 at -le GPT-BASIC-DATA-PARTITION-1 = IF
> +      block gpt-part-entry>part-type-guid 4 + w at -le
> +      GPT-BASIC-DATA-PARTITION-2 = IF
> +         block gpt-part-entry>part-type-guid 6 + w at -le
> +         GPT-BASIC-DATA-PARTITION-3 = IF
> +            block gpt-part-entry>part-type-guid 8 + w@

Don't you have to byte-swap (w at -le) here, too? Looks somehow strange
that the other UID parts are read byte-swapped but this one is not?

> +            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:

: gpt-basic-data-partition? ( -- true|false )
   block gpt-part-entry>part-type-guid
   dup l at -le GPT-BASIC-DATA-PARTITION-1 <> IF FALSE EXIT THEN
   dup 4 + w at -le GPT-BASIC-DATA-PARTITION-2 <> IF FALSE EXIT THEN
   dup 6 + w at -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.

> +   1 read-sector block gpt>part-entry-lba l at -le

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

By the way, it might be a good idea to add a "x at -le" helper for this to
little-endian.fs.

> +   block-size * to seek-pos
> +   block gpt>part-entry-size l at -le to gpt-part-size
> +   block gpt>num-part-entry l at -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?

> +      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)

> +         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