Change in ...coreboot[master]: soc/intel/braswell/acpi/lpc.asl: Allocate used ROM size only

[Frans Hendriks (Code Review)]

Frans Hendriks has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31822

Change subject: soc/intel/braswell/acpi/lpc.asl: Allocate used ROM size only
......................................................................

soc/intel/braswell/acpi/lpc.asl: Allocate used ROM size only

Fixed ROM area is allocated.
Reduce the ROM size using CONFIG_COREBOOT_ROMSIZE.

BUG=N/A
TEST=Facebook FBG-1701 booting Embedded Linux

Change-Id: I7a47bf2600f546271c5a65641d29f868ff2748bf
Signed-off-by: Frans Hendriks 
---
M src/soc/intel/braswell/acpi/lpc.asl
1 file changed, 5 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/22/31822/1

diff --git a/src/soc/intel/braswell/acpi/lpc.asl b/src/soc/intel/braswell/acpi/lpc.asl
index 6b2ecec..a28eb38 100644
--- a/src/soc/intel/braswell/acpi/lpc.asl
+++ b/src/soc/intel/braswell/acpi/lpc.asl
@@ -3,7 +3,7 @@
  *
  * Copyright (C) 2007-2009 coresystems GmbH
  * Copyright (C) 2013 Google Inc.
- * Copyright (C) 2018 Eltan B.V.
+ * Copyright (C) 2018-2019 Eltan B.V.
  *
  * This program is free software; you can redistribute it and/or
  * modify it under the terms of the GNU General Public License as
@@ -44,7 +44,10 @@
    	Name (_HID, EISAID("INT0800"))
    	Name (_CRS, ResourceTemplate()
    	{
-			Memory32Fixed(ReadOnly, 0xff000000, 0x01000000)
+			Memory32Fixed(ReadOnly, 0xffffffff -
+				      (CONFIG_COREBOOT_ROMSIZE_KB*1024) + 1,
+				      CONFIG_COREBOOT_ROMSIZE_KB*1024)
+
    	})
    }

[Frans Hendriks (Code Review)]

Hello Patrick Rudolph, build bot (Jenkins),

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/31822

to look at the new patch set (#2).

Change subject: soc/intel/braswell/acpi/lpc.asl: Allocate used ROM size only
......................................................................

soc/intel/braswell/acpi/lpc.asl: Allocate used ROM size only

Fixed ROM area is allocated.
Reduce the ROM size using CONFIG_COREBOOT_ROMSIZE.

BUG=N/A
TEST=Facebook FBG-1701 booting Embedded Linux

Change-Id: I7a47bf2600f546271c5a65641d29f868ff2748bf
Signed-off-by: Frans Hendriks 
---
M src/soc/intel/braswell/acpi/lpc.asl
1 file changed, 5 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/22/31822/2

[Frans Hendriks (Code Review)]

Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31822 )

Change subject: soc/intel/braswell/acpi/lpc.asl: Allocate used ROM size only
......................................................................

Patch Set 2:

Can you review/approve this patch?

[Michał Żygowski (Code Review)]

Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31822 )

Change subject: soc/intel/braswell/acpi/lpc.asl: Allocate used ROM size only
......................................................................

Patch Set 2: Code-Review+1

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31822 )

Change subject: soc/intel/braswell/acpi/lpc.asl: Allocate used ROM size only
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/#/c/31822/2/src/soc/intel/braswell/acpi/lpc.asl 
File src/soc/intel/braswell/acpi/lpc.asl:

https://review.coreboot.org/#/c/31822/2/src/soc/intel/braswell/acpi/lpc.asl@... 
PS2, Line 48: 				      ((CONFIG_COREBOOT_ROMSIZE_KB*1024) + 1),
was this really tested? the parentheses look wrong

[Frans Hendriks (Code Review)]

Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31822 )

Change subject: soc/intel/braswell/acpi/lpc.asl: Allocate used ROM size only
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/#/c/31822/2/src/soc/intel/braswell/acpi/lpc.asl 
File src/soc/intel/braswell/acpi/lpc.asl:

https://review.coreboot.org/#/c/31822/2/src/soc/intel/braswell/acpi/lpc.asl@... 
PS2, Line 48: 				      ((CONFIG_COREBOOT_ROMSIZE_KB*1024) + 1),
> was this really tested? the parentheses look wrong

Yes, tested and working.
Please not that this is part of second argement which is 
0xffffffff - ((CONFIG_COREBOOT_ROMSIZE_KB*1024) + 1). This contains twice '(' and twice ')'

[Michał Żygowski (Code Review)]

Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31822 )

Change subject: soc/intel/braswell/acpi/lpc.asl: Allocate used ROM size only
......................................................................

Patch Set 2: -Code-Review

(1 comment)

https://review.coreboot.org/#/c/31822/2/src/soc/intel/braswell/acpi/lpc.asl 
File src/soc/intel/braswell/acpi/lpc.asl:

https://review.coreboot.org/#/c/31822/2/src/soc/intel/braswell/acpi/lpc.asl@... 
PS2, Line 48: 				      ((CONFIG_COREBOOT_ROMSIZE_KB*1024) + 1),
> Yes, tested and working. […]

I guess this is not what Nico meant. Let's take a CONFIG_COREBOOT_ROMSIZE_KB as 8192, then:

Memory32Fixed(ReadOnly, 0xffffffff - ((CONFIG_COREBOOT_ROMSIZE_KB*1024) + 1), CONFIG_COREBOOT_ROMSIZE_KB*1024)

Will be

Memory32Fixed(ReadOnly, 0xffffffff - (8MiB + 1), 8MiB) which is not correct. Why?
because 0xffffffff - (8MiB +1) will be 0xff7fffff, but should be 0xff800000.

0xffffffff - (8MiB + 1) == 0xffffffff - 8MiB - 1 != 0xffffffff - 8MiB + 1

The '+1' must be  outside parentheses.

[Frans Hendriks (Code Review)]

Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31822 )

Change subject: soc/intel/braswell/acpi/lpc.asl: Allocate used ROM size only
......................................................................

Patch Set 2:
> Patch Set 2: -Code-Review
>
> (1 comment)

I did the +1 outside parentheses in patchset 1, but gerrit reported error. (I can't check the real error gerrit reported). 
For this reason I had modified it to patchset 2, but might introduce this issue.

I'll check.

[Frans Hendriks (Code Review)]

Hello Patrick Rudolph, Piotr Król, build bot (Jenkins), Nico Huber, Michał Żygowski, Patrick Georgi,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/31822

to look at the new patch set (#3).

Change subject: soc/intel/braswell/acpi/lpc.asl: Allocate used ROM size only
......................................................................

soc/intel/braswell/acpi/lpc.asl: Allocate used ROM size only

Fixed ROM area is allocated.
Reduce the ROM size using CONFIG_COREBOOT_ROMSIZE.

BUG=N/A
TEST=Facebook FBG-1701 booting Embedded Linux

Change-Id: I7a47bf2600f546271c5a65641d29f868ff2748bf
Signed-off-by: Frans Hendriks 
---
M src/soc/intel/braswell/acpi/lpc.asl
1 file changed, 4 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/22/31822/3

[Michał Żygowski (Code Review)]

Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31822 )

Change subject: soc/intel/braswell/acpi/lpc.asl: Allocate used ROM size only
......................................................................

Patch Set 3:
> Patch Set 2:
> > Patch Set 2: -Code-Review
> >
> > (1 comment)
>
> I did the +1 outside parentheses in patchset 1, but gerrit reported error. (I can't check the real error gerrit reported). 
> For this reason I had modified it to patchset 2, but might introduce this issue.
>
> I'll check.

Here are the example results: https://qa.coreboot.org/job/coreboot-gerrit/92939/testReport/junit/board/chr...
You have to just click the Jenkins links and check the logs.
If the `+1` makes mess, let's try

0xffffffff - ((CONFIG_COREBOOT_ROMSIZE_KB*1024) - 1)

It should make the trick I think.

[Frans Hendriks (Code Review)]

Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31822 )

Change subject: soc/intel/braswell/acpi/lpc.asl: Allocate used ROM size only
......................................................................

Patch Set 3:
> Patch Set 3:
> > Patch Set 2:
> > > Patch Set 2: -Code-Review
> > >
> > > (1 comment)
> >
> > I did the +1 outside parentheses in patchset 1, but gerrit reported error. (I can't check the real error gerrit reported). 
> > For this reason I had modified it to patchset 2, but might introduce this issue.
> >
> > I'll check.
>
> Here are the example results: https://qa.coreboot.org/job/coreboot-gerrit/92939/testReport/junit/board/chr...
> You have to just click the Jenkins links and check the logs.
> If the `+1` makes mess, let's try
>
> 0xffffffff - ((CONFIG_COREBOOT_ROMSIZE_KB*1024) - 1)
>
> It should make the trick I think.

I mean that I could not find these results of patchset 1. 
I'll check.

[Frans Hendriks (Code Review)]

Hello Patrick Rudolph, Piotr Król, build bot (Jenkins), Nico Huber, Michał Żygowski, Patrick Georgi,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/31822

to look at the new patch set (#4).

Change subject: soc/intel/braswell/acpi/lpc.asl: Allocate used ROM size only
......................................................................

soc/intel/braswell/acpi/lpc.asl: Allocate used ROM size only

Fixed ROM area is allocated.
Reduce the ROM size using CONFIG_COREBOOT_ROMSIZE.

BUG=N/A
TEST=Facebook FBG-1701 booting Embedded Linux

Change-Id: I7a47bf2600f546271c5a65641d29f868ff2748bf
Signed-off-by: Frans Hendriks 
---
M src/soc/intel/braswell/acpi/lpc.asl
1 file changed, 13 insertions(+), 3 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/22/31822/4

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31822 )

Change subject: soc/intel/braswell/acpi/lpc.asl: Allocate used ROM size only
......................................................................

Patch Set 4:

This looks like a severe compiler bug, please don't
just work around it. It seems the compiler precalcu-
lates the correct numbers, but places 0s into the
Memory32Fixed resource and the numbers after it,
amidst the bytestream:

Buffer that contains 14B:
  11 11 0a 0e
  The memory range filled with 0s:
  86 09 00 00 00 00 00 00 00 00 00 00
  End Tag:
  79 00
  After the buffer our numbers:
  00 00 80 ff 00 00 80 00

Please let me know if you don't want to check if the
issue persists upstream and report this.

[Frans Hendriks (Code Review)]

Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31822 )

Change subject: soc/intel/braswell/acpi/lpc.asl: Allocate used ROM size only
......................................................................

Patch Set 4:
> Patch Set 4:
>
> This looks like a severe compiler bug, please don't
> just work around it. It seems the compiler precalcu-
> lates the correct numbers, but places 0s into the
> Memory32Fixed resource and the numbers after it,
> amidst the bytestream:
>
> Buffer that contains 14B:
>   11 11 0a 0e
>   The memory range filled with 0s:
>   86 09 00 00 00 00 00 00 00 00 00 00
>   End Tag:
>   79 00
>   After the buffer our numbers:
>   00 00 80 ff 00 00 80 00
>
> Please let me know if you don't want to check if the
> issue persists upstream and report this.

(How did you generate this bytestream?)

I would expect this behavoir. 
Compiler will create RBUF with 0s. Run-time this buffer will be filled.

Can you clarify why it looks like a bug?

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31822 )

Change subject: soc/intel/braswell/acpi/lpc.asl: Allocate used ROM size only
......................................................................

Patch Set 4:
> Patch Set 4:
> > Patch Set 4:
> >
> > This looks like a severe compiler bug, please don't
> > just work around it. It seems the compiler precalcu-
> > lates the correct numbers, but places 0s into the
> > Memory32Fixed resource and the numbers after it,
> > amidst the bytestream:
> >
> > Buffer that contains 14B:
> >   11 11 0a 0e
> >   The memory range filled with 0s:
> >   86 09 00 00 00 00 00 00 00 00 00 00
> >   End Tag:
> >   79 00
> >   After the buffer our numbers:
> >   00 00 80 ff 00 00 80 00
> >
> > Please let me know if you don't want to check if the
> > issue persists upstream and report this.
>
> (How did you generate this bytestream?)

Make deletes the file after the failed build, but you
should be able to get it back with:

cd build/; /path/to/your/xgcc/bin/iasl -vw 3150 -we -p dsdt.aml dsdt.asl
> I would expect this behavoir. 
> Compiler will create RBUF with 0s. Run-time this buffer will be filled.

There is no runtime code and Memory32Fixed only contains
32-bit numbers and never code by definition.
> Can you clarify why it looks like a bug?

After the buffer ends, _CRS is done and we are back at
the device scope. And there it puts these numbers. Not
even encoded as AML integers but plain 32-bit words. If
the compiler wanted to do it at runtime, it would have
to emit code like yours in patch set 4.

Now that I think about it again, somehow you managed to
build patch set 1 and also Jenkins to build patch set 2.
So maybe it is fixed already in a newer version of the
compiler and some Jenkins node still runs an old version?

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31822 )

Change subject: soc/intel/braswell/acpi/lpc.asl: Allocate used ROM size only
......................................................................

Patch Set 4:
> Now that I think about it again, somehow you managed to
> build patch set 1 and also Jenkins to build patch set 2.
> So maybe it is fixed already in a newer version of the
> compiler and some Jenkins node still runs an old version?

Turned out that depending on what spurious numbers end up
in the byte stream, it sometimes results in a decompilation
warning, sometimes in an error. And we only checked for
warnings ^^ cf. CB:32469. This explains why we got different
results for the differently placed parentheses.

[Frans Hendriks (Code Review)]

Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31822 )

Change subject: soc/intel/braswell/acpi/lpc.asl: Allocate used ROM size only
......................................................................

Patch Set 4:
> Patch Set 4:
> > Now that I think about it again, somehow you managed to
> > build patch set 1 and also Jenkins to build patch set 2.
> > So maybe it is fixed already in a newer version of the
> > compiler and some Jenkins node still runs an old version?
>
> Turned out that depending on what spurious numbers end up
> in the byte stream, it sometimes results in a decompilation
> warning, sometimes in an error. And we only checked for
> warnings ^^ cf. CB:32469. This explains why we got different
> results for the differently placed parentheses.

I checked results of the failing situation. 
build\dsdt.dsl is created only containing:
"Error: Unknown opcode 0xFE at table offset" and "Could not parse ACPI tables, AE_AML_BAD_OPCODE"

I did not check what IASL reporting.

[Frans Hendriks (Code Review)]

Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31822 )

Change subject: soc/intel/braswell/acpi/lpc.asl: Allocate used ROM size only
......................................................................

Patch Set 4:
> Patch Set 4:
> > Patch Set 4:
> > > Now that I think about it again, somehow you managed to
> > > build patch set 1 and also Jenkins to build patch set 2.
> > > So maybe it is fixed already in a newer version of the
> > > compiler and some Jenkins node still runs an old version?
> >
> > Turned out that depending on what spurious numbers end up
> > in the byte stream, it sometimes results in a decompilation
> > warning, sometimes in an error. And we only checked for
> > warnings ^^ cf. CB:32469. This explains why we got different
> > results for the differently placed parentheses.
>
> I checked results of the failing situation. 
> build\dsdt.dsl is created only containing:
> "Error: Unknown opcode 0xFE at table offset" and "Could not parse ACPI tables, AE_AML_BAD_OPCODE"
>
> I did not check what IASL reporting.

IMO this fix is ready to review/merge. 
Any comments?

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31822 )

Change subject: soc/intel/braswell/acpi/lpc.asl: Allocate used ROM size only
......................................................................

Patch Set 4: Code-Review+2

Frans, did you check if the issue persists in upstream
IASL? report a bug already?

Change in coreboot[master]: soc/intel/braswell/acpi/lpc.asl: Allocate used ROM size only

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/31822 )

Change subject: soc/intel/braswell/acpi/lpc.asl: Allocate used ROM size only
......................................................................

soc/intel/braswell/acpi/lpc.asl: Allocate used ROM size only

Fixed ROM area is allocated.
Reduce the ROM size using CONFIG_COREBOOT_ROMSIZE.

BUG=N/A
TEST=Facebook FBG-1701 booting Embedded Linux

Change-Id: I7a47bf2600f546271c5a65641d29f868ff2748bf
Signed-off-by: Frans Hendriks 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/31822
Tested-by: build bot (Jenkins) 
Reviewed-by: Nico Huber 
---
M src/soc/intel/braswell/acpi/lpc.asl
1 file changed, 13 insertions(+), 3 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Nico Huber: Looks good to me, approved

diff --git a/src/soc/intel/braswell/acpi/lpc.asl b/src/soc/intel/braswell/acpi/lpc.asl
index 6b2ecec..9caa8f1 100644
--- a/src/soc/intel/braswell/acpi/lpc.asl
+++ b/src/soc/intel/braswell/acpi/lpc.asl
@@ -3,7 +3,7 @@
  *
  * Copyright (C) 2007-2009 coresystems GmbH
  * Copyright (C) 2013 Google Inc.
- * Copyright (C) 2018 Eltan B.V.
+ * Copyright (C) 2018-2019 Eltan B.V.
  *
  * This program is free software; you can redistribute it and/or
  * modify it under the terms of the GNU General Public License as
@@ -42,10 +42,20 @@
    Device (FWH)		/* Firmware Hub */
    {
    	Name (_HID, EISAID("INT0800"))
-		Name (_CRS, ResourceTemplate()
+		Name (RBUF, ResourceTemplate()
    	{
-			Memory32Fixed(ReadOnly, 0xff000000, 0x01000000)
+			Memory32Fixed(ReadOnly, 0, 0, FBAR)
    	})
+
+		Method (_CRS)
+		{
+			CreateDwordField (^RBUF, ^FBAR._BAS, FBAS)
+			CreateDwordField (^RBUF, ^FBAR._LEN, FLEN)
+			Multiply(CONFIG_COREBOOT_ROMSIZE_KB, 1024, Local0)
+			Store(Local0, FLEN)
+			Add(Subtract(0xffffffff, Local0), 1, FBAS)
+			Return (^RBUF)
+		}
    }

#if !CONFIG(DISABLE_HPET)