fmaptool: fmap_config.h: Base offset missing

[Rolf Evers-Fischer]

Hello,
my ApolloLake board was not able to find the "romstage" in the SPI memory, 
because it searched in the wrong part of the memory.

___FMAP__COREBOOT_BASE was set to the relative offset within the parent 
section, but rdev_readat needs the *absolute* address: There was a 
mismatch.
However, I was able to fix this problem locally by adding an additional 
fixed offset to fmap_top, like

diff --git a/src/lib/cbfs.c b/src/lib/cbfs.c
index aa652c2..f36a499 100644
--- a/src/lib/cbfs.c
+++ b/src/lib/cbfs.c
@@ -230,7 +230,7 @@ static int cbfs_master_header_props(struct cbfs_props *props
    if (bdev == NULL)
    	return -1;

-	size_t fmap_top = ___FMAP__COREBOOT_BASE + ___FMAP__COREBOOT_SIZE;
+	size_t fmap_top = ___FMAP__COREBOOT_BASE + ___FMAP__COREBOOT_SIZE + 0x300000;

/* Find location of header using signed 32-bit offset from
     * end of CBFS region. */

But imho this shouldn't be a permanent solution!

Therefore I suggest to modify the 'fmaptool' in such way that it outputs 
the absolute address for (at least) ___FMAP__COREBOOT_BASE, because the 
'rdev_readat()' function  of the 'boot_device_ro()' needs it in this form.

What is your opinion?

Kind regards,
 Rolf

[Aaron Durbin]

On Thu, Jun 16, 2016 at 8:55 AM, Rolf Evers-Fischer
 wrote:
> Hello,
> my ApolloLake board was not able to find the "romstage" in the SPI memory,
> because it searched in the wrong part of the memory.
>
> ___FMAP__COREBOOT_BASE was set to the relative offset within the parent
> section, but rdev_readat needs the *absolute* address: There was a
> mismatch.
> However, I was able to fix this problem locally by adding an additional
> fixed offset to fmap_top, like

Could you please provide an example fmd and the generated header file?
> diff --git a/src/lib/cbfs.c b/src/lib/cbfs.c
> index aa652c2..f36a499 100644
> --- a/src/lib/cbfs.c
> +++ b/src/lib/cbfs.c
> @@ -230,7 +230,7 @@ static int cbfs_master_header_props(struct cbfs_props *props
>         if (bdev == NULL)
>                 return -1;
>   size_t fmap_top = ___FMAP__COREBOOT_BASE + ___FMAP__COREBOOT_SIZE;
>   size_t fmap_top = ___FMAP__COREBOOT_BASE + ___FMAP__COREBOOT_SIZE + 0x300000;
>
>   /* Find location of header using signed 32-bit offset from
>    * end of CBFS region. */
>
>
> But imho this shouldn't be a permanent solution!
>
> Therefore I suggest to modify the 'fmaptool' in such way that it outputs
> the absolute address for (at least) ___FMAP__COREBOOT_BASE, because the
> 'rdev_readat()' function  of the 'boot_device_ro()' needs it in this form.
>
> What is your opinion?
>
> Kind regards,
>  Rolf

[Julius Werner]

Yes, this definitely looks like a bug... thanks a lot for reporting
it. The fix is simple and I've uploaded it here:
https://review.coreboot.org/15273

I'm curious why this hasn't been noticed anywhere before, though...
(on some boards this just happens to work out because the parent
section of COREBOOT starts at offset 0 anyway, but on others as you
found out it doesn't).

On Thu, Jun 16, 2016 at 6:55 AM, Rolf Evers-Fischer
 wrote:
> Hello,
> my ApolloLake board was not able to find the "romstage" in the SPI memory,
> because it searched in the wrong part of the memory.
>
> ___FMAP__COREBOOT_BASE was set to the relative offset within the parent
> section, but rdev_readat needs the *absolute* address: There was a
> mismatch.
> However, I was able to fix this problem locally by adding an additional
> fixed offset to fmap_top, like
>
> diff --git a/src/lib/cbfs.c b/src/lib/cbfs.c
> index aa652c2..f36a499 100644
> --- a/src/lib/cbfs.c
> +++ b/src/lib/cbfs.c
> @@ -230,7 +230,7 @@ static int cbfs_master_header_props(struct cbfs_props *props
>         if (bdev == NULL)
>                 return -1;
>   size_t fmap_top = ___FMAP__COREBOOT_BASE + ___FMAP__COREBOOT_SIZE;
>   size_t fmap_top = ___FMAP__COREBOOT_BASE + ___FMAP__COREBOOT_SIZE + 0x300000;
>
>   /* Find location of header using signed 32-bit offset from
>    * end of CBFS region. */
>
>
> But imho this shouldn't be a permanent solution!
>
> Therefore I suggest to modify the 'fmaptool' in such way that it outputs
> the absolute address for (at least) ___FMAP__COREBOOT_BASE, because the
> 'rdev_readat()' function  of the 'boot_device_ro()' needs it in this form.
>
> What is your opinion?
>
> Kind regards,
>  Rolf