[PATCH SLOF 0/5] GPT fixes/cleanup and LVM support with FAT

[PATCH SLOF 0/5] GPT fixes/cleanup and LVM support with FAT

[Nikunj A Dadhania]

Following patchset implements some improvements and cleanup for the
GPT booting code:

patch 1: Simplify the gpt detection code with lesser scopes and add
         comments.
patch 2: Introduce 8byte LE helpers: x@-le and x!-le
patch 3: As we need to detect FAT partition, implement a helper that
      	 can be used both from GPT code and "try-dos-files"
patch 4: Implement GPT FAT for LVM suport.
patch 5: Make GPT detection code robust 

Nikunj A Dadhania (5):
  disk-label: simplify gpt-prep-partition? routine
  introduce 8-byte LE helpers
  disk-label: introduce helper to check fat filesystem
  disk-label: add support for booting from GPT FAT partition
  disk-label: make gpt detection code more robust

 slof/fs/little-endian.fs       |   6 ++
 slof/fs/packages/disk-label.fs | 141 +++++++++++++++++++++++++++--------------
 2 files changed, 101 insertions(+), 46 deletions(-)

-- 
1.8.3.1

[PATCH SLOF 1/5] disk-label: simplify gpt-prep-partition? routine

[Nikunj A Dadhania]

Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
---
 slof/fs/packages/disk-label.fs | 36 +++++++++++++-----------------------
 1 file changed, 13 insertions(+), 23 deletions(-)

diff --git a/slof/fs/packages/disk-label.fs b/slof/fs/packages/disk-label.fs
index fe1c25e..2305eee 100644
--- a/slof/fs/packages/disk-label.fs
+++ b/slof/fs/packages/disk-label.fs
@@ -352,31 +352,21 @@ CONSTANT /gpt-part-entry
    drop 0
 ;
 
-\ Check for GPT PReP partition GUID
-9E1A2D38     CONSTANT GPT-PREP-PARTITION-1
-C612         CONSTANT GPT-PREP-PARTITION-2
-4316         CONSTANT GPT-PREP-PARTITION-3
-AA26         CONSTANT GPT-PREP-PARTITION-4
-8B49521E5A8B CONSTANT GPT-PREP-PARTITION-5
+\ Check for GPT PReP partition GUID. Only first 3 blocks are
+\ byte-swapped treating last two blocks as contigous for simplifying
+\ comparison
+9E1A2D38            CONSTANT GPT-PREP-PARTITION-1
+C612                CONSTANT GPT-PREP-PARTITION-2
+4316                CONSTANT GPT-PREP-PARTITION-3
+AA268B49521E5A8B    CONSTANT GPT-PREP-PARTITION-4
 
 : gpt-prep-partition? ( -- true|false )
-   block gpt-part-entry>part-type-guid l@-le GPT-PREP-PARTITION-1 = IF
-      block gpt-part-entry>part-type-guid 4 + w@-le
-      GPT-PREP-PARTITION-2 = IF
-         block gpt-part-entry>part-type-guid 6 + w@-le
-         GPT-PREP-PARTITION-3 = IF
-            block gpt-part-entry>part-type-guid 8 + w@
-            GPT-PREP-PARTITION-4 = IF
-               block gpt-part-entry>part-type-guid a + w@
-               block gpt-part-entry>part-type-guid c + l@ swap lxjoin
-               GPT-PREP-PARTITION-5 = IF
-                   TRUE EXIT
-               THEN
-            THEN
-         THEN
-      THEN
-   THEN
-   FALSE
+   block gpt-part-entry>part-type-guid
+   dup l@-le     GPT-PREP-PARTITION-1 <> IF DROP FALSE EXIT THEN
+   dup 4 + w@-le GPT-PREP-PARTITION-2 <> IF DROP FALSE EXIT THEN
+   dup 6 + w@-le GPT-PREP-PARTITION-3 <> IF DROP FALSE EXIT THEN
+       8 + x@    GPT-PREP-PARTITION-4 <> IF FALSE EXIT THEN
+   TRUE
 ;
 
 : load-from-gpt-prep-partition ( addr -- size )
-- 
1.8.3.1

Re: [PATCH SLOF 1/5] disk-label: simplify gpt-prep-partition? routine

[Thomas Huth]

On Mon, 22 Jun 2015 13:29:43 +0530
Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> wrote:

> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
> ---
>  slof/fs/packages/disk-label.fs | 36 +++++++++++++-----------------------
>  1 file changed, 13 insertions(+), 23 deletions(-)
> 
> diff --git a/slof/fs/packages/disk-label.fs b/slof/fs/packages/disk-label.fs
> index fe1c25e..2305eee 100644
> --- a/slof/fs/packages/disk-label.fs
> +++ b/slof/fs/packages/disk-label.fs
> @@ -352,31 +352,21 @@ CONSTANT /gpt-part-entry
>     drop 0
>  ;
>  
> -\ Check for GPT PReP partition GUID
> -9E1A2D38     CONSTANT GPT-PREP-PARTITION-1
> -C612         CONSTANT GPT-PREP-PARTITION-2
> -4316         CONSTANT GPT-PREP-PARTITION-3
> -AA26         CONSTANT GPT-PREP-PARTITION-4
> -8B49521E5A8B CONSTANT GPT-PREP-PARTITION-5
> +\ Check for GPT PReP partition GUID. Only first 3 blocks are
> +\ byte-swapped treating last two blocks as contigous for simplifying
> +\ comparison
> +9E1A2D38            CONSTANT GPT-PREP-PARTITION-1
> +C612                CONSTANT GPT-PREP-PARTITION-2
> +4316                CONSTANT GPT-PREP-PARTITION-3
> +AA268B49521E5A8B    CONSTANT GPT-PREP-PARTITION-4
>  
>  : gpt-prep-partition? ( -- true|false )
> -   block gpt-part-entry>part-type-guid l@-le GPT-PREP-PARTITION-1 = IF
> -      block gpt-part-entry>part-type-guid 4 + w@-le
> -      GPT-PREP-PARTITION-2 = IF
> -         block gpt-part-entry>part-type-guid 6 + w@-le
> -         GPT-PREP-PARTITION-3 = IF
> -            block gpt-part-entry>part-type-guid 8 + w@
> -            GPT-PREP-PARTITION-4 = IF
> -               block gpt-part-entry>part-type-guid a + w@
> -               block gpt-part-entry>part-type-guid c + l@ swap lxjoin
> -               GPT-PREP-PARTITION-5 = IF
> -                   TRUE EXIT
> -               THEN
> -            THEN
> -         THEN
> -      THEN
> -   THEN
> -   FALSE
> +   block gpt-part-entry>part-type-guid
> +   dup l@-le     GPT-PREP-PARTITION-1 <> IF DROP FALSE EXIT THEN
> +   dup 4 + w@-le GPT-PREP-PARTITION-2 <> IF DROP FALSE EXIT THEN
> +   dup 6 + w@-le GPT-PREP-PARTITION-3 <> IF DROP FALSE EXIT THEN
> +       8 + x@    GPT-PREP-PARTITION-4 <> IF FALSE EXIT THEN
> +   TRUE
>  ;
>  
>  : load-from-gpt-prep-partition ( addr -- size )

Also change DROP, FALSE and TRUE to lowercase, as Segher
suggested with patch 3? Apart from that, looks fine to me.

Reviewed-by: Thomas Huth <thuth@redhat.com>

[PATCH SLOF 2/5] introduce 8-byte LE helpers

[Nikunj A Dadhania]

Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
---
 slof/fs/little-endian.fs       | 6 ++++++
 slof/fs/packages/disk-label.fs | 4 ++--
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/slof/fs/little-endian.fs b/slof/fs/little-endian.fs
index f2e4e8d..6b4779e 100644
--- a/slof/fs/little-endian.fs
+++ b/slof/fs/little-endian.fs
@@ -17,6 +17,9 @@ here c@ ef = CONSTANT ?littleendian
 
 ?bigendian [IF]
 
+: x!-le >r xbflip r> x! ;
+: x@-le x@ xbflip ;
+
 : l!-le  >r lbflip r> l! ;
 : l@-le  l@ lbflip ;
 
@@ -47,6 +50,9 @@ here c@ ef = CONSTANT ?littleendian
 
 [ELSE]
 
+: x!-le x! ;
+: x@-le x@ ;
+
 : l!-le  l! ;
 : l@-le  l@ ;
 
diff --git a/slof/fs/packages/disk-label.fs b/slof/fs/packages/disk-label.fs
index 2305eee..2cf1b85 100644
--- a/slof/fs/packages/disk-label.fs
+++ b/slof/fs/packages/disk-label.fs
@@ -384,8 +384,8 @@ AA268B49521E5A8B    CONSTANT GPT-PREP-PARTITION-4
          debug-disk-label? IF
             ." GPT PReP partition found " cr
          THEN
-         block gpt-part-entry>first-lba x@ xbflip
-         block gpt-part-entry>last-lba x@ xbflip
+         block gpt-part-entry>first-lba x@-le
+         block gpt-part-entry>last-lba x@-le
          over - 1+                 ( addr offset len )
          swap                      ( addr len offset )
          block-size * to part-offset
-- 
1.8.3.1

Re: [PATCH SLOF 2/5] introduce 8-byte LE helpers

[Thomas Huth]

On Mon, 22 Jun 2015 13:29:44 +0530
Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> wrote:

> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
> ---
>  slof/fs/little-endian.fs       | 6 ++++++
>  slof/fs/packages/disk-label.fs | 4 ++--
>  2 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/slof/fs/little-endian.fs b/slof/fs/little-endian.fs
> index f2e4e8d..6b4779e 100644
> --- a/slof/fs/little-endian.fs
> +++ b/slof/fs/little-endian.fs
> @@ -17,6 +17,9 @@ here c@ ef = CONSTANT ?littleendian
>  
>  ?bigendian [IF]
>  
> +: x!-le >r xbflip r> x! ;
> +: x@-le x@ xbflip ;
> +
>  : l!-le  >r lbflip r> l! ;
>  : l@-le  l@ lbflip ;
>  
> @@ -47,6 +50,9 @@ here c@ ef = CONSTANT ?littleendian
>  
>  [ELSE]
>  
> +: x!-le x! ;
> +: x@-le x@ ;
> +
>  : l!-le  l! ;
>  : l@-le  l@ ;
>  
> diff --git a/slof/fs/packages/disk-label.fs b/slof/fs/packages/disk-label.fs
> index 2305eee..2cf1b85 100644
> --- a/slof/fs/packages/disk-label.fs
> +++ b/slof/fs/packages/disk-label.fs
> @@ -384,8 +384,8 @@ AA268B49521E5A8B    CONSTANT GPT-PREP-PARTITION-4
>           debug-disk-label? IF
>              ." GPT PReP partition found " cr
>           THEN
> -         block gpt-part-entry>first-lba x@ xbflip
> -         block gpt-part-entry>last-lba x@ xbflip
> +         block gpt-part-entry>first-lba x@-le
> +         block gpt-part-entry>last-lba x@-le
>           over - 1+                 ( addr offset len )
>           swap                      ( addr len offset )
>           block-size * to part-offset

Reviewed-by: Thomas Huth <thuth@redhat.com>

[PATCH SLOF 3/5] disk-label: introduce helper to check fat filesystem

[Nikunj A Dadhania]

Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
---
 slof/fs/packages/disk-label.fs | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/slof/fs/packages/disk-label.fs b/slof/fs/packages/disk-label.fs
index 2cf1b85..e317e93 100644
--- a/slof/fs/packages/disk-label.fs
+++ b/slof/fs/packages/disk-label.fs
@@ -320,6 +320,18 @@ CONSTANT /gpt-part-entry
 
 \ Load from first active DOS boot partition.
 
+: has-fat-filesystem ( block -- true | false )
+   \ block 0 byte 0-2 is a jump instruction in all FAT
+   \ filesystems.
+   \ e9 and eb are jump instructions in x86 assembler.
+   dup c@ e9 <> IF
+      dup c@ eb <> swap
+      2+  c@ 90 <> or
+      IF false EXIT THEN
+   ELSE DROP THEN
+   TRUE
+;
+
 \ NOTE: block-size is always 512 bytes for DOS partition tables.
 
 : load-from-dos-boot-partition ( addr -- size )
@@ -547,15 +559,7 @@ AA268B49521E5A8B    CONSTANT GPT-PREP-PARTITION-4
 
 : try-dos-files ( -- found? )
    no-mbr? IF false EXIT THEN
-
-   \ 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
+   block has-fat-filesystem 0= IF false EXIT THEN
    s" fat-files" (interpose-filesystem)
    true
 ;
-- 
1.8.3.1

Re: [PATCH SLOF 3/5] disk-label: introduce helper to check fat filesystem

[Segher Boessenkool]

On Mon, Jun 22, 2015 at 01:29:45PM +0530, Nikunj A Dadhania wrote:
> +: has-fat-filesystem ( block -- true | false )
> +   \ block 0 byte 0-2 is a jump instruction in all FAT
> +   \ filesystems.

"block" there is not a block number, just a host address.  So it's not
a good name.  Maybe do a better name for this word as well, something
saying it looks at a disk block.

> +   \ e9 and eb are jump instructions in x86 assembler.
> +   dup c@ e9 <> IF
> +      dup c@ eb <> swap
> +      2+  c@ 90 <> or
> +      IF false EXIT THEN
> +   ELSE DROP THEN
> +   TRUE
> +;

Don't write DROP and TRUE in caps please.  The purpose of having the
structure words in caps is to make them stand out more, to make things
more readable; putting other things in caps as well destroys that.

Since you factored this, it becomes more readable if you invert the
conditions:

: fat-bootblock? ( addr -- flag )
   \ byte 0-2 of the bootblock is a jump instruction in
   \ all FAT filesystems.
   \ e9 and eb are jump instructions in x86 assembler.
   dup c@ e9 = IF drop true EXIT THEN
   dup c@ eb = swap 2+ c@ 90 = and ;

(not tested, etc.)


Segher

Re: [PATCH SLOF 3/5] disk-label: introduce helper to check fat filesystem

[Nikunj A Dadhania]

Hi Segher,

Segher Boessenkool <segher@kernel.crashing.org> writes:
> On Mon, Jun 22, 2015 at 01:29:45PM +0530, Nikunj A Dadhania wrote:
>> +: has-fat-filesystem ( block -- true | false )
>> +   \ block 0 byte 0-2 is a jump instruction in all FAT
>> +   \ filesystems.
>
> "block" there is not a block number, just a host address.  So it's not
> a good name.  Maybe do a better name for this word as well, something
> saying it looks at a disk block.

Sure.

>
>> +   \ e9 and eb are jump instructions in x86 assembler.
>> +   dup c@ e9 <> IF
>> +      dup c@ eb <> swap
>> +      2+  c@ 90 <> or
>> +      IF false EXIT THEN
>> +   ELSE DROP THEN
>> +   TRUE
>> +;
>
> Don't write DROP and TRUE in caps please.  The purpose of having the
> structure words in caps is to make them stand out more, to make things
> more readable; putting other things in caps as well destroys that.

Sure, will take care.

> Since you factored this, it becomes more readable if you invert the
> conditions:

Sure.

> : fat-bootblock? ( addr -- flag )
>    \ byte 0-2 of the bootblock is a jump instruction in
>    \ all FAT filesystems.
>    \ e9 and eb are jump instructions in x86 assembler.
>    dup c@ e9 = IF drop true EXIT THEN
>    dup c@ eb = swap 2+ c@ 90 = and ;
>
> (not tested, etc.)
Will test.

Regards,
Nikunj

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

[Nikunj A Dadhania]

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 | 54 +++++++++++++++++++++++++++++++++++++-----
 1 file changed, 48 insertions(+), 6 deletions(-)

diff --git a/slof/fs/packages/disk-label.fs b/slof/fs/packages/disk-label.fs
index e317e93..821e959 100644
--- a/slof/fs/packages/disk-label.fs
+++ b/slof/fs/packages/disk-label.fs
@@ -266,7 +266,10 @@ 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
+       debug-disk-label? IF cr ." No DOS disk-label found." cr THEN
+       false EXIT
+   THEN
 
    count-dos-logical-partitions TO dos-logical-partitions
 
@@ -381,18 +384,38 @@ AA268B49521E5A8B    CONSTANT GPT-PREP-PARTITION-4
    TRUE
 ;
 
-: load-from-gpt-prep-partition ( addr -- size )
+\ Check for GPT MSFT BASIC DATA GUID - fat based
+EBD0A0A2            CONSTANT GPT-BASIC-DATA-PARTITION-1
+B9E5                CONSTANT GPT-BASIC-DATA-PARTITION-2
+4433                CONSTANT GPT-BASIC-DATA-PARTITION-3
+87C068B6B72699C7    CONSTANT GPT-BASIC-DATA-PARTITION-4
+
+: gpt-basic-data-partition? ( -- true|false )
+   block gpt-part-entry>part-type-guid
+   dup l@-le     GPT-BASIC-DATA-PARTITION-1 <> IF DROP FALSE EXIT THEN
+   dup 4 + w@-le GPT-BASIC-DATA-PARTITION-2 <> IF DROP FALSE EXIT THEN
+   dup 6 + w@-le GPT-BASIC-DATA-PARTITION-3 <> IF DROP FALSE EXIT THEN
+       8 + x@    GPT-BASIC-DATA-PARTITION-4 <> IF FALSE EXIT THEN
+   TRUE
+;
+
+\ Can be called from two path
+\ 1) load-from-boot-partition for GPT PReP partition
+\ 2) try-partitions for gpt basic data partition having fat chrp-boot
+\
+: load-from-gpt-partition ( [ addr ] -- size | TRUE )
    no-gpt? IF drop FALSE EXIT THEN
    debug-disk-label? IF
       cr ." GPT partition found " cr
    THEN
-   1 read-sector block gpt>part-entry-lba l@-le
+   1 read-sector block gpt>part-entry-lba x@-le
    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 gpt-prep-partition? IF
+       block gpt-part-size read drop
+       gpt-prep-partition? IF
          debug-disk-label? IF
             ." GPT PReP partition found " cr
          THEN
@@ -404,6 +427,24 @@ AA268B49521E5A8B    CONSTANT GPT-PREP-PARTITION-4
          0 0 seek drop             ( addr len )
          block-size * read         ( size )
          UNLOOP EXIT
+     THEN
+     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 has-fat-filesystem 0= IF false UNLOOP EXIT THEN
+         TRUE
+         UNLOOP EXIT
       THEN
       seek-pos gpt-part-size i * + to seek-pos
    LOOP
@@ -495,11 +536,11 @@ AA268B49521E5A8B    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
-   load-from-gpt-prep-partition
+   load-from-gpt-partition
    \ More boot partition formats ...
 ;
 
@@ -594,6 +635,7 @@ AA268B49521E5A8B    CONSTANT GPT-PREP-PARTITION-4
 
 : try-partitions ( -- found? )
    try-dos-partition IF try-files EXIT THEN
+   load-from-gpt-partition IF try-files EXIT THEN
    \ try-iso9660-partition IF try-files EXIT THEN
    \ ... more partition types here...
    false
-- 
1.8.3.1

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

[Thomas Huth]

On Mon, 22 Jun 2015 13:29:46 +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.

Maybe better: The patch adds support to read from BASIC_DATA UUID
partitions for the case that the OS installer has installed
the 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 | 54 +++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 48 insertions(+), 6 deletions(-)
> 
> diff --git a/slof/fs/packages/disk-label.fs b/slof/fs/packages/disk-label.fs
> index e317e93..821e959 100644
> --- a/slof/fs/packages/disk-label.fs
> +++ b/slof/fs/packages/disk-label.fs
> @@ -266,7 +266,10 @@ 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
> +       debug-disk-label? IF cr ." No DOS disk-label found." cr THEN
> +       false EXIT
> +   THEN
>  
>     count-dos-logical-partitions TO dos-logical-partitions
>  
> @@ -381,18 +384,38 @@ AA268B49521E5A8B    CONSTANT GPT-PREP-PARTITION-4
>     TRUE
>  ;
>  
> -: load-from-gpt-prep-partition ( addr -- size )
> +\ Check for GPT MSFT BASIC DATA GUID - fat based
> +EBD0A0A2            CONSTANT GPT-BASIC-DATA-PARTITION-1
> +B9E5                CONSTANT GPT-BASIC-DATA-PARTITION-2
> +4433                CONSTANT GPT-BASIC-DATA-PARTITION-3
> +87C068B6B72699C7    CONSTANT GPT-BASIC-DATA-PARTITION-4
> +
> +: gpt-basic-data-partition? ( -- true|false )
> +   block gpt-part-entry>part-type-guid
> +   dup l@-le     GPT-BASIC-DATA-PARTITION-1 <> IF DROP FALSE EXIT THEN
> +   dup 4 + w@-le GPT-BASIC-DATA-PARTITION-2 <> IF DROP FALSE EXIT THEN
> +   dup 6 + w@-le GPT-BASIC-DATA-PARTITION-3 <> IF DROP FALSE EXIT THEN
> +       8 + x@    GPT-BASIC-DATA-PARTITION-4 <> IF FALSE EXIT THEN
> +   TRUE
> +;
> +
> +\ Can be called from two path
> +\ 1) load-from-boot-partition for GPT PReP partition
> +\ 2) try-partitions for gpt basic data partition having fat chrp-boot

What did you want to achieve with the above comment? Caller locations
can change with later patches, but it's unlikely that everybody
remembers to update such comments in that case. So unlikely you've got
a good reason for above comment (but in that case, it maybe should be
written in a different way), I'd suggest to drop it.

> +: load-from-gpt-partition ( [ addr ] -- size | TRUE )

What do you mean with addr in square brackets? Is it optional?

>     no-gpt? IF drop FALSE EXIT THEN
>     debug-disk-label? IF
>        cr ." GPT partition found " cr
>     THEN
> -   1 read-sector block gpt>part-entry-lba l@-le
> +   1 read-sector block gpt>part-entry-lba x@-le
>     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 gpt-prep-partition? IF
> +       block gpt-part-size read drop
> +       gpt-prep-partition? IF

You've changed the level of indentation here. Please try to avoid that
(unless you've got a good reason, e.g. because the previous indentation
was obviously wrong)

>           debug-disk-label? IF
>              ." GPT PReP partition found " cr
>           THEN
> @@ -404,6 +427,24 @@ AA268B49521E5A8B    CONSTANT GPT-PREP-PARTITION-4
>           0 0 seek drop             ( addr len )
>           block-size * read         ( size )
>           UNLOOP EXIT
> +     THEN
> +     gpt-basic-data-partition? IF

Hmm, I wonder whether we need a proper coding conventions spec for
writing Forth code ... (at least about the indentation depths ...) ;-)

> +         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 has-fat-filesystem 0= IF false UNLOOP EXIT THEN
> +         TRUE
> +         UNLOOP EXIT
>        THEN
>        seek-pos gpt-part-size i * + to seek-pos
>     LOOP
> @@ -495,11 +536,11 @@ AA268B49521E5A8B    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
> -   load-from-gpt-prep-partition
> +   load-from-gpt-partition

So here the function is called with an "addr" parameter on the stack ...

>     \ More boot partition formats ...
>  ;
>  
> @@ -594,6 +635,7 @@ AA268B49521E5A8B    CONSTANT GPT-PREP-PARTITION-4
>  
>  : try-partitions ( -- found? )
>     try-dos-partition IF try-files EXIT THEN
> +   load-from-gpt-partition IF try-files EXIT THEN

... but here it is called without an "addr" parameter? *Confusing*
How does this work?

 Thomas

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

[Segher Boessenkool]

On Tue, Jun 23, 2015 at 09:34:44AM +0200, Thomas Huth wrote:
> > +: load-from-gpt-partition ( [ addr ] -- size | TRUE )
> 
> What do you mean with addr in square brackets? Is it optional?

And "size | TRUE"?  The code even returns "false" instead, which
usually is a valid size (0).  Just always return a flag?  Or maybe
you mean something like ( -- false | size true ) .  Not going to
read the code, I cannot keep track of the stack, bringing us to...


> Hmm, I wonder whether we need a proper coding conventions spec for
> writing Forth code ... (at least about the indentation depths ...) ;-)

"Write readable code.  That means in part, do not write long definitions
(longer than a few lines)."

There, all coding conventions you'll ever need :-)


Almost all short definitions (with good names!) are easily readable
(with a little effort if the subject matter is tricky).  No longer
definitions are ever readable (well, there are exceptions; not many).

Don't get hung up on "how many spaces should I indent"...  Since your
words are short, you won't have more than two levels of indent anyway :-)

Adding extra spacing to group things is also very helpful.

Minor things...  Most words want a stack comment.  If you need stack
comments inside a definition, it is too complex.  If there is any
significant amount of stack juggling, the word is too complex.  If
the word would be too complex, you need to factor it.  If you cannot
easily split off factors, your solution is too complex.  If it is
hard to think of good names for the factors, that is simply because
naming things is the hardest part of programming (but see also the
previous point).

You also want short words that do one little thing because you _do_
test your code.


Segher

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

[Nikunj A Dadhania]

Segher Boessenkool <segher@kernel.crashing.org> writes:

> On Tue, Jun 23, 2015 at 09:34:44AM +0200, Thomas Huth wrote:
>> > +: load-from-gpt-partition ( [ addr ] -- size | TRUE )
>> 
>> What do you mean with addr in square brackets? Is it optional?
>
> And "size | TRUE"?  The code even returns "false" instead, which
> usually is a valid size (0).  Just always return a flag?  Or maybe
> you mean something like ( -- false | size true ) .  Not going to
> read the code, I cannot keep track of the stack, bringing us to...
>
>
>> Hmm, I wonder whether we need a proper coding conventions spec for
>> writing Forth code ... (at least about the indentation depths ...) ;-)
>
> "Write readable code.  That means in part, do not write long definitions
> (longer than a few lines)."

I ended up here by combining two similar looking words as they were
doing too many similar stuff.

But I guess it ended up being pretty big. I will break it up into
smaller units and resend this patch.

>
> There, all coding conventions you'll ever need :-)
>
>
> Almost all short definitions (with good names!) are easily readable
> (with a little effort if the subject matter is tricky).  No longer
> definitions are ever readable (well, there are exceptions; not many).
>
> Don't get hung up on "how many spaces should I indent"...  Since your
> words are short, you won't have more than two levels of indent anyway :-)
>
> Adding extra spacing to group things is also very helpful.
>
> Minor things...  Most words want a stack comment.  If you need stack
> comments inside a definition, it is too complex.  If there is any
> significant amount of stack juggling, the word is too complex.  If
> the word would be too complex, you need to factor it.  If you cannot
> easily split off factors, your solution is too complex.  If it is
> hard to think of good names for the factors, that is simply because
> naming things is the hardest part of programming (but see also the
> previous point).
>
> You also want short words that do one little thing because you _do_
> test your code.

Regards
Nikunj

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

[Nikunj A Dadhania]

Thomas Huth <thuth@redhat.com> writes:

> On Mon, 22 Jun 2015 13:29:46 +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.
>
> Maybe better: The patch adds support to read from BASIC_DATA UUID
> partitions for the case that the OS installer has installed
> the 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 | 54 +++++++++++++++++++++++++++++++++++++-----
>>  1 file changed, 48 insertions(+), 6 deletions(-)
>> 
>> diff --git a/slof/fs/packages/disk-label.fs b/slof/fs/packages/disk-label.fs
>> index e317e93..821e959 100644
>> --- a/slof/fs/packages/disk-label.fs
>> +++ b/slof/fs/packages/disk-label.fs
>> @@ -266,7 +266,10 @@ 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
>> +       debug-disk-label? IF cr ." No DOS disk-label found." cr THEN
>> +       false EXIT
>> +   THEN
>>  
>>     count-dos-logical-partitions TO dos-logical-partitions
>>  
>> @@ -381,18 +384,38 @@ AA268B49521E5A8B    CONSTANT GPT-PREP-PARTITION-4
>>     TRUE
>>  ;
>>  
>> -: load-from-gpt-prep-partition ( addr -- size )
>> +\ Check for GPT MSFT BASIC DATA GUID - fat based
>> +EBD0A0A2            CONSTANT GPT-BASIC-DATA-PARTITION-1
>> +B9E5                CONSTANT GPT-BASIC-DATA-PARTITION-2
>> +4433                CONSTANT GPT-BASIC-DATA-PARTITION-3
>> +87C068B6B72699C7    CONSTANT GPT-BASIC-DATA-PARTITION-4
>> +
>> +: gpt-basic-data-partition? ( -- true|false )
>> +   block gpt-part-entry>part-type-guid
>> +   dup l@-le     GPT-BASIC-DATA-PARTITION-1 <> IF DROP FALSE EXIT THEN
>> +   dup 4 + w@-le GPT-BASIC-DATA-PARTITION-2 <> IF DROP FALSE EXIT THEN
>> +   dup 6 + w@-le GPT-BASIC-DATA-PARTITION-3 <> IF DROP FALSE EXIT THEN
>> +       8 + x@    GPT-BASIC-DATA-PARTITION-4 <> IF FALSE EXIT THEN
>> +   TRUE
>> +;
>> +
>> +\ Can be called from two path
>> +\ 1) load-from-boot-partition for GPT PReP partition
>> +\ 2) try-partitions for gpt basic data partition having fat chrp-boot
>
> What did you want to achieve with the above comment? Caller locations
> can change with later patches, but it's unlikely that everybody
> remembers to update such comments in that case. So unlikely you've got
> a good reason for above comment (but in that case, it maybe should be
> written in a different way), I'd suggest to drop it.

I will redo this word and split it so that do not have confusing
signature.

>
>> +: load-from-gpt-partition ( [ addr ] -- size | TRUE )
>
> What do you mean with addr in square brackets? Is it optional?
>
>>     no-gpt? IF drop FALSE EXIT THEN
>>     debug-disk-label? IF
>>        cr ." GPT partition found " cr
>>     THEN
>> -   1 read-sector block gpt>part-entry-lba l@-le
>> +   1 read-sector block gpt>part-entry-lba x@-le
>>     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 gpt-prep-partition? IF
>> +       block gpt-part-size read drop
>> +       gpt-prep-partition? IF
>
> You've changed the level of indentation here. Please try to avoid that
> (unless you've got a good reason, e.g. because the previous indentation
> was obviously wrong)

Not intentional, I try to make sure I do not change the indentation.

>
>>           debug-disk-label? IF
>>              ." GPT PReP partition found " cr
>>           THEN
>> @@ -404,6 +427,24 @@ AA268B49521E5A8B    CONSTANT GPT-PREP-PARTITION-4
>>           0 0 seek drop             ( addr len )
>>           block-size * read         ( size )
>>           UNLOOP EXIT
>> +     THEN
>> +     gpt-basic-data-partition? IF
>
> Hmm, I wonder whether we need a proper coding conventions spec for
> writing Forth code ... (at least about the indentation depths ...) ;-)
>
>> +         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 has-fat-filesystem 0= IF false UNLOOP EXIT THEN
>> +         TRUE
>> +         UNLOOP EXIT
>>        THEN
>>        seek-pos gpt-part-size i * + to seek-pos
>>     LOOP
>> @@ -495,11 +536,11 @@ AA268B49521E5A8B    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
>> -   load-from-gpt-prep-partition
>> +   load-from-gpt-partition
>
> So here the function is called with an "addr" parameter on the stack ...
>
>>     \ More boot partition formats ...
>>  ;
>>  
>> @@ -594,6 +635,7 @@ AA268B49521E5A8B    CONSTANT GPT-PREP-PARTITION-4
>>  
>>  : try-partitions ( -- found? )
>>     try-dos-partition IF try-files EXIT THEN
>> +   load-from-gpt-partition IF try-files EXIT THEN
>
> ... but here it is called without an "addr" parameter? *Confusing*
> How does this work?

Yes :-)

Let me rework on this to break it up for readabilty and better stack
debugging.

Regards
Nikunj

[PATCH SLOF 5/5] disk-label: make gpt detection code more robust

[Nikunj A Dadhania]

* Check for Protective MBR Magic
* Check for valid GPT Signature
* Boundary check for allocated block size before reading into the
  buffer

Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
---
 slof/fs/packages/disk-label.fs | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/slof/fs/packages/disk-label.fs b/slof/fs/packages/disk-label.fs
index 821e959..d9c3a8d 100644
--- a/slof/fs/packages/disk-label.fs
+++ b/slof/fs/packages/disk-label.fs
@@ -20,6 +20,7 @@ false VALUE debug-disk-label?
 \ If we ever want to put a large kernel with initramfs from a PREP partition
 \ we might need to increase this value. The default value is 65536 blocks (32MB)
 d# 65536 value max-prep-partition-blocks
+d# 4096 value block-array-size
 
 s" disk-label" device-name
 
@@ -152,8 +153,8 @@ CONSTANT /gpt-part-entry
 : init-block ( -- )
    s" block-size" ['] $call-parent CATCH IF ABORT" parent has no block-size." THEN
    to block-size
-   d# 4096 alloc-mem
-   dup d# 4096 erase
+   block-array-size alloc-mem
+   dup block-array-size erase
    to block
    debug-disk-label? IF
       ." init-block: block-size=" block-size .d ." block=0x" block u. cr
@@ -175,10 +176,18 @@ CONSTANT /gpt-part-entry
    block mbr>magic w@-le aa55 <>
 ;
 
+\
+\ GPT Signature
+\ ("EFI PART", 45h 46h 49h 20h 50h 41h 52h 54h)
+\
+4546492050415254 CONSTANT GPT-SIGNATURE
+
 \ This word returns true if the currently loaded block has _NO_ GPT partition id
 : no-gpt? ( -- true|false )
    0 read-sector
-   1 partition>part-entry part-entry>id c@ ee <>
+   1 partition>part-entry part-entry>id c@ ee <> IF TRUE EXIT THEN
+   block mbr>magic w@-le aa55 <> IF TRUE EXIT THEN
+   1 read-sector block gpt>signature x@ GPT-SIGNATURE <>
 ;
 
 : pc-extended-partition? ( part-entry-addr -- true|false )
@@ -411,6 +420,10 @@ B9E5                CONSTANT GPT-BASIC-DATA-PARTITION-2
    1 read-sector block gpt>part-entry-lba x@-le
    block-size * to seek-pos
    block gpt>part-entry-size l@-le to gpt-part-size
+   gpt-part-size block-array-size > IF
+       cr ." GPT part size exceeds buffer allocated " cr
+       FALSE EXIT
+   THEN
    block gpt>num-part-entry l@-le dup 0= IF FALSE EXIT THEN
    1+ 1 ?DO
       seek-pos 0 seek drop
@@ -646,7 +659,7 @@ B9E5                CONSTANT GPT-BASIC-DATA-PARTITION-2
 
 : close ( -- )
    debug-disk-label? IF ." Closing disk-label: block=0x" block u. ." block-size=" block-size .d cr THEN
-   block d# 4096 free-mem
+   block block-array-size free-mem
 ;
 
 
-- 
1.8.3.1

Re: [PATCH SLOF 5/5] disk-label: make gpt detection code more robust

[Thomas Huth]

On Mon, 22 Jun 2015 13:29:47 +0530
Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> wrote:

> * Check for Protective MBR Magic
> * Check for valid GPT Signature
> * Boundary check for allocated block size before reading into the
>   buffer
> 
> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
> ---
>  slof/fs/packages/disk-label.fs | 21 +++++++++++++++++----
>  1 file changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/slof/fs/packages/disk-label.fs b/slof/fs/packages/disk-label.fs
> index 821e959..d9c3a8d 100644
> --- a/slof/fs/packages/disk-label.fs
> +++ b/slof/fs/packages/disk-label.fs
> @@ -20,6 +20,7 @@ false VALUE debug-disk-label?
>  \ If we ever want to put a large kernel with initramfs from a PREP partition
>  \ we might need to increase this value. The default value is 65536 blocks (32MB)
>  d# 65536 value max-prep-partition-blocks
> +d# 4096 value block-array-size
>  
>  s" disk-label" device-name
>  
> @@ -152,8 +153,8 @@ CONSTANT /gpt-part-entry
>  : init-block ( -- )
>     s" block-size" ['] $call-parent CATCH IF ABORT" parent has no block-size." THEN
>     to block-size
> -   d# 4096 alloc-mem
> -   dup d# 4096 erase
> +   block-array-size alloc-mem
> +   dup block-array-size erase
>     to block
>     debug-disk-label? IF
>        ." init-block: block-size=" block-size .d ." block=0x" block u. cr
> @@ -175,10 +176,18 @@ CONSTANT /gpt-part-entry
>     block mbr>magic w@-le aa55 <>
>  ;
>  
> +\
> +\ GPT Signature
> +\ ("EFI PART", 45h 46h 49h 20h 50h 41h 52h 54h)
> +\
> +4546492050415254 CONSTANT GPT-SIGNATURE
> +
>  \ This word returns true if the currently loaded block has _NO_ GPT partition id
>  : no-gpt? ( -- true|false )
>     0 read-sector
> -   1 partition>part-entry part-entry>id c@ ee <>
> +   1 partition>part-entry part-entry>id c@ ee <> IF TRUE EXIT THEN
> +   block mbr>magic w@-le aa55 <> IF TRUE EXIT THEN
> +   1 read-sector block gpt>signature x@ GPT-SIGNATURE <>

The comment above the function talks about the "currently loaded
block", so I'd maybe avoid to load another sector here.
Maybe move this gpt>signature check to "load-from-gpt-partition" where
this block gets loaded anyway?

>  ;
>  
>  : pc-extended-partition? ( part-entry-addr -- true|false )
> @@ -411,6 +420,10 @@ B9E5                CONSTANT GPT-BASIC-DATA-PARTITION-2
>     1 read-sector block gpt>part-entry-lba x@-le
>     block-size * to seek-pos
>     block gpt>part-entry-size l@-le to gpt-part-size
> +   gpt-part-size block-array-size > IF
> +       cr ." GPT part size exceeds buffer allocated " cr

Isn't there this "addr" parameter on the stack which you might need to
drop here?

> +       FALSE EXIT
> +   THEN
>     block gpt>num-part-entry l@-le dup 0= IF FALSE EXIT THEN
>     1+ 1 ?DO
>        seek-pos 0 seek drop
> @@ -646,7 +659,7 @@ B9E5                CONSTANT GPT-BASIC-DATA-PARTITION-2
>  
>  : close ( -- )
>     debug-disk-label? IF ." Closing disk-label: block=0x" block u. ." block-size=" block-size .d cr THEN
> -   block d# 4096 free-mem
> +   block block-array-size free-mem
>  ;

 Thomas

Re: [PATCH SLOF 5/5] disk-label: make gpt detection code more robust

[Nikunj A Dadhania]

Thomas Huth <thuth@redhat.com> writes:

> On Mon, 22 Jun 2015 13:29:47 +0530
> Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> wrote:
>
>> * Check for Protective MBR Magic
>> * Check for valid GPT Signature
>> * Boundary check for allocated block size before reading into the
>>   buffer
>> 
>> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
>> ---
>>  slof/fs/packages/disk-label.fs | 21 +++++++++++++++++----
>>  1 file changed, 17 insertions(+), 4 deletions(-)
>> 
>> diff --git a/slof/fs/packages/disk-label.fs b/slof/fs/packages/disk-label.fs
>> index 821e959..d9c3a8d 100644
>> --- a/slof/fs/packages/disk-label.fs
>> +++ b/slof/fs/packages/disk-label.fs
>> @@ -20,6 +20,7 @@ false VALUE debug-disk-label?
>>  \ If we ever want to put a large kernel with initramfs from a PREP partition
>>  \ we might need to increase this value. The default value is 65536 blocks (32MB)
>>  d# 65536 value max-prep-partition-blocks
>> +d# 4096 value block-array-size
>>  
>>  s" disk-label" device-name
>>  
>> @@ -152,8 +153,8 @@ CONSTANT /gpt-part-entry
>>  : init-block ( -- )
>>     s" block-size" ['] $call-parent CATCH IF ABORT" parent has no block-size." THEN
>>     to block-size
>> -   d# 4096 alloc-mem
>> -   dup d# 4096 erase
>> +   block-array-size alloc-mem
>> +   dup block-array-size erase
>>     to block
>>     debug-disk-label? IF
>>        ." init-block: block-size=" block-size .d ." block=0x" block u. cr
>> @@ -175,10 +176,18 @@ CONSTANT /gpt-part-entry
>>     block mbr>magic w@-le aa55 <>
>>  ;
>>  
>> +\
>> +\ GPT Signature
>> +\ ("EFI PART", 45h 46h 49h 20h 50h 41h 52h 54h)
>> +\
>> +4546492050415254 CONSTANT GPT-SIGNATURE
>> +
>>  \ This word returns true if the currently loaded block has _NO_ GPT partition id
>>  : no-gpt? ( -- true|false )
>>     0 read-sector
>> -   1 partition>part-entry part-entry>id c@ ee <>
>> +   1 partition>part-entry part-entry>id c@ ee <> IF TRUE EXIT THEN
>> +   block mbr>magic w@-le aa55 <> IF TRUE EXIT THEN
>> +   1 read-sector block gpt>signature x@ GPT-SIGNATURE <>
>
> The comment above the function talks about the "currently loaded
> block", so I'd maybe avoid to load another sector here.
> Maybe move this gpt>signature check to "load-from-gpt-partition" where
> this block gets loaded anyway?

Sure.

>
>>  ;
>>  
>>  : pc-extended-partition? ( part-entry-addr -- true|false )
>> @@ -411,6 +420,10 @@ B9E5                CONSTANT GPT-BASIC-DATA-PARTITION-2
>>     1 read-sector block gpt>part-entry-lba x@-le
>>     block-size * to seek-pos
>>     block gpt>part-entry-size l@-le to gpt-part-size
>> +   gpt-part-size block-array-size > IF
>> +       cr ." GPT part size exceeds buffer allocated " cr
>
> Isn't there this "addr" parameter on the stack which you might need to
> drop here?

Will check

>
>> +       FALSE EXIT
>> +   THEN
>>     block gpt>num-part-entry l@-le dup 0= IF FALSE EXIT THEN
>>     1+ 1 ?DO
>>        seek-pos 0 seek drop
>> @@ -646,7 +659,7 @@ B9E5                CONSTANT GPT-BASIC-DATA-PARTITION-2
>>  
>>  : close ( -- )
>>     debug-disk-label? IF ." Closing disk-label: block=0x" block u. ." block-size=" block-size .d cr THEN
>> -   block d# 4096 free-mem
>> +   block block-array-size free-mem
>>  ;
>
>  Thomas