Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41155 )
Change subject: soc/intel/common/block/systemagent: Use TOUUD as base for MMIO above 4G ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41155/2/src/soc/intel/common/block/... File src/soc/intel/common/block/systemagent/systemagent.c:
https://review.coreboot.org/c/coreboot/+/41155/2/src/soc/intel/common/block/... PS2, Line 118: if (!get_enable_above_4GB_mmio())
And causing such issue when Above 4GB base and limit size is incorrect.
What were the MMIO addresses that were advertised in ACPI tables before and after your change? Do you have dumps of the tables?
It can be calculated easily
Before https://review.coreboot.org/c/coreboot/+/38125/10/src/soc/intel/common/block... #define BASE_32GB 0x800000000 #define SIZE_16GB 0x400000000
Store (BASE_32GB, MMIN) Store (SIZE_16GB, MLEN)
After if we have enable_above_4GB_mmio = 0 then didn't broadcast anything for upper 4GB as size is 0
If (LEqual (A4GS, 0)) { CreateQwordField (MCRS, PM02._LEN, MSEN) Store (0, MSEN) }
Else when enable_above_4GB_mmio = 1
#define ABOVE_4GB_MEM_BASE_ADDRESS (256ULL * GiB) #define ABOVE_4GB_MEM_BASE_SIZE (256ULL * GiB)
above address will get broadcast ed. Align with Intel RC code
After making sure we have right base and limit like below CL, we don't see such issue as mentioned above.
If there was an error in reporting and now it is fixed, why not always advertise the correct MMIO window available above 4G? Why is it made conditional?
I was making this conditional to remain parity with FSP/RC implementation.
Also i could see an FSP UPD which does some additional programming if we like to enable memory above 4GB.
That seems to be used only for ACPI table generation. I didn't really see any different programming happening with that UPD?
I will check this in more details, i think there were some programming required to board case this range as well at host bridge side thats the reason this UPD exist inside RC code else won't include any UPD if just meant for ACPI table generation as ACPI table is not part of RC ocde