From: Thomas Huth <thuth@redhat•com>
To: Nikunj A Dadhania <nikunj@linux•vnet.ibm.com>
Cc: linuxppc-dev@ozlabs•org, benh@kernel•crashing.org, aik@ozlabs•ru,
dvaleev@suse•com
Subject: Re: [PATCH SLOF] disk-label: add support for booting from GPT FAT partition
Date: Wed, 17 Jun 2015 12:22:48 +0200 [thread overview]
Message-ID: <20150617122248.77211775@thh440s> (raw)
In-Reply-To: <1434017929-1792-1-git-send-email-nikunj@linux.vnet.ibm.com>
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?
> 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?
> + 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@-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.
> + 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?
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?
> + 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
next prev parent reply other threads:[~2015-06-17 10:23 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-06-11 10:18 [PATCH SLOF] disk-label: add support for booting from GPT FAT partition Nikunj A Dadhania
2015-06-17 10:22 ` Thomas Huth [this message]
2015-06-17 11:59 ` Nikunj A Dadhania
2015-06-17 12:04 ` Nikunj A Dadhania
2015-06-19 16:27 ` Thomas Huth
2015-06-19 17:47 ` Nikunj A Dadhania
2015-06-18 6:24 ` Nikunj A Dadhania
2015-06-19 16:35 ` Thomas Huth
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20150617122248.77211775@thh440s \
--to=thuth@redhat$(echo .)com \
--cc=aik@ozlabs$(echo .)ru \
--cc=benh@kernel$(echo .)crashing.org \
--cc=dvaleev@suse$(echo .)com \
--cc=linuxppc-dev@ozlabs$(echo .)org \
--cc=nikunj@linux$(echo .)vnet.ibm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox