Change in coreboot[master]: soc/intel: common,skl,cnl,icl: drop reserved mmio memory size calcula...
Michael Niewöhner has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/36216 )
Change subject: soc/intel: common,skl,cnl,icl: drop reserved mmio memory size calculation ......................................................................
soc/intel: common,skl,cnl,icl: drop reserved mmio memory size calculation
Remove the calculation of the Reserved Intel MMIO Memory size from systemagent and memmap for two reasons:
1. It is not needed. The size is used in SA to calculate the space between cbmem_top and TSEG without DPR and Chipset Reserved Memory. Since this will always be equal to 0, the reservation will be skipped and TSEG, DPR and Chipset Reserved Memory will get reserved alltogether.
2. The reserved memory size is influenced by multiple other factors like PTT, TraceHub, PRMRR etc. Thus it is not trivial to do the calculation right. It fails, for example, when using the TOLUM returned by FSP with a PRMRR size of 0. In this case the size is calculated wrong. This makes it difficult to simply use the TOLUM offset returned by FSP (follow-up change) without adding more weird calculations.
Tested successfully on X11SSM-F
Change-Id: I0cc730551eb3a79c78a971b40056de8d029f4b82 Signed-off-by: Michael Niewöhner foss@mniewoehner.de --- M src/soc/intel/cannonlake/memmap.c M src/soc/intel/common/block/include/intelblocks/systemagent.h M src/soc/intel/common/block/systemagent/systemagent.c M src/soc/intel/icelake/memmap.c M src/soc/intel/skylake/memmap.c 5 files changed, 4 insertions(+), 63 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/16/36216/1
diff --git a/src/soc/intel/cannonlake/memmap.c b/src/soc/intel/cannonlake/memmap.c index f3286cc..8ba69e5 100644 --- a/src/soc/intel/cannonlake/memmap.c +++ b/src/soc/intel/cannonlake/memmap.c @@ -192,21 +192,6 @@ return dram_base; }
-/* - * SoC implementation - * - * SoC call to summarize all Intel Reserve MMIO size and report to SA - */ -size_t soc_reserved_mmio_size(void) -{ - struct ebda_config cfg; - - retrieve_ebda_object(&cfg); - - /* Get Intel Reserved Memory Range Size */ - return cfg.reserved_mem_size; -} - /* Fill up memory layout information */ void fill_soc_memmap_ebda(struct ebda_config *cfg) { diff --git a/src/soc/intel/common/block/include/intelblocks/systemagent.h b/src/soc/intel/common/block/include/intelblocks/systemagent.h index 133047c..ae9213c 100644 --- a/src/soc/intel/common/block/include/intelblocks/systemagent.h +++ b/src/soc/intel/common/block/include/intelblocks/systemagent.h @@ -106,8 +106,4 @@ /* SoC specific APIs to get UNCORE PRMRR base and mask values * returns 0, if able to get base and mask values; otherwise returns -1 */ int soc_get_uncore_prmmr_base_and_mask(uint64_t *base, uint64_t *mask); - -/* SoC call to summarize all Intel Reserve MMIO size and report to SA */ -size_t soc_reserved_mmio_size(void); - #endif /* SOC_INTEL_COMMON_BLOCK_SA_H */ diff --git a/src/soc/intel/common/block/systemagent/systemagent.c b/src/soc/intel/common/block/systemagent/systemagent.c index 0312cac..bd925b3 100644 --- a/src/soc/intel/common/block/systemagent/systemagent.c +++ b/src/soc/intel/common/block/systemagent/systemagent.c @@ -46,11 +46,6 @@ return -1; }
-__weak size_t soc_reserved_mmio_size(void) -{ - return 0; -} - __weak unsigned long sa_write_acpi_tables(struct device *dev, unsigned long current, struct acpi_rsdp *rsdp) @@ -156,7 +151,6 @@ { uintptr_t base_k, touud_k; size_t dpr_size = 0, size_k; - size_t reserved_mmio_size; uint64_t sa_map_values[MAX_MAP_ENTRIES]; uintptr_t top_of_ram; int index = *resource_count; @@ -164,9 +158,6 @@ if (CONFIG(SA_ENABLE_DPR)) dpr_size = sa_get_dpr_size();
- /* Get SoC reserve memory size as per user selection */ - reserved_mmio_size = soc_reserved_mmio_size(); - top_of_ram = (uintptr_t)cbmem_top();
/* 0 - > 0xa0000 */ @@ -181,14 +172,13 @@
sa_get_mem_map(dev, &sa_map_values[0]);
- /* top_of_ram -> TSEG - DPR - Intel Reserve Memory Size*/ + /* top_of_ram -> TSEG - DPR */ base_k = top_of_ram; - size_k = sa_map_values[SA_TSEG_REG] - dpr_size - base_k - - reserved_mmio_size; + size_k = sa_map_values[SA_TSEG_REG] - dpr_size - base_k; mmio_resource(dev, index++, base_k / KiB, size_k / KiB);
- /* TSEG - DPR - Intel Reserve Memory Size -> BGSM */ - base_k = sa_map_values[SA_TSEG_REG] - dpr_size - reserved_mmio_size; + /* TSEG - DPR -> BGSM */ + base_k = sa_map_values[SA_TSEG_REG] - dpr_size; size_k = sa_map_values[SA_BGSM_REG] - base_k; reserved_ram_resource(dev, index++, base_k / KiB, size_k / KiB);
diff --git a/src/soc/intel/icelake/memmap.c b/src/soc/intel/icelake/memmap.c index 20c4e6f..122cb1a 100644 --- a/src/soc/intel/icelake/memmap.c +++ b/src/soc/intel/icelake/memmap.c @@ -190,21 +190,6 @@ return dram_base; }
-/* - * SoC implementation - * - * SoC call to summarize all Intel Reserve MMIO size and report to SA - */ -size_t soc_reserved_mmio_size(void) -{ - struct ebda_config cfg; - - retrieve_ebda_object(&cfg); - - /* Get Intel Reserved Memory Range Size */ - return cfg.reserved_mem_size; -} - /* Fill up memory layout information */ void fill_soc_memmap_ebda(struct ebda_config *cfg) { diff --git a/src/soc/intel/skylake/memmap.c b/src/soc/intel/skylake/memmap.c index 29f2517..12446de 100644 --- a/src/soc/intel/skylake/memmap.c +++ b/src/soc/intel/skylake/memmap.c @@ -217,21 +217,6 @@ return dram_base; }
-/* - * SoC implementation - * - * SoC call to summarize all Intel Reserve MMIO size and report to SA - */ -size_t soc_reserved_mmio_size(void) -{ - struct ebda_config cfg; - - retrieve_ebda_object(&cfg); - - /* Get Intel Reserved Memory Range Size */ - return cfg.reserved_mem_size; -} - /* Fill up memory layout information */ void fill_soc_memmap_ebda(struct ebda_config *cfg) {
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36216 )
Change subject: soc/intel: common,skl,cnl,icl: drop reserved mmio memory size calculation ......................................................................
Uploaded patch set 2: Patch Set 1 was rebased.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36216 )
Change subject: soc/intel: common,skl,cnl,icl: drop reserved mmio memory size calculation ......................................................................
Uploaded patch set 3: Patch Set 2 was rebased.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36216 )
Change subject: soc/intel: common,skl,cnl,icl: drop reserved mmio memory size calculation ......................................................................
Uploaded patch set 4: Patch Set 3 was rebased.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36216 )
Change subject: soc/intel: common,skl,cnl,icl: drop reserved mmio memory size calculation ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/c/coreboot/+/36216/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36216/5//COMMIT_MSG@14 PS5, Line 14: Since this will always be equal to 0 I'm not sure I understand or follow. what is 'this'? What this change is doing is actually manipulating the reserved mmio vs ram split which impacts mtrr usage.
https://review.coreboot.org/c/coreboot/+/36216/5/src/soc/intel/common/block/... File src/soc/intel/common/block/systemagent/systemagent.c:
https://review.coreboot.org/c/coreboot/+/36216/5/src/soc/intel/common/block/... PS5, Line 178: mmio_resource(dev, index++, base_k / KiB, size_k / KiB); I'm not sure it matters too much, but this change implicitly marks the PRMRR, trace, and ptt memory as mmio where before it was reserved ram. There will be implications to MTRR usage depending on alignment and size.
i.e. mmio reservation increased in size and reserved ram decreased.
Do you have logs from before and after this change?
Hello Kyösti Mälkki, Aaron Durbin, Patrick Rudolph, Subrata Banik, Arthur Heymans, Paul Menzel, build bot (Jenkins), Nico Huber, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36216
to look at the new patch set (#6).
Change subject: soc/intel: common,skl,cnl,icl: drop reserved mmio memory size calculation ......................................................................
soc/intel: common,skl,cnl,icl: drop reserved mmio memory size calculation
Remove the calculation of the Reserved Intel MMIO Memory size from systemagent and memmap for two reasons:
1. It is not needed. The size is used in SA to calculate the space between cbmem_top and TSEG without DPR and Chipset Reserved Memory. Since this will always be equal to 0, the reservation will be skipped and TSEG, DPR and Chipset Reserved Memory will get reserved alltogether.
2. The reserved memory size is influenced by multiple other factors like PTT, TraceHub, PRMRR etc. Thus it is not trivial to do the calculation right. It fails, for example, when using the TOLUM returned by FSP with a PRMRR size of 0. In this case the size is calculated wrong. This makes it difficult to simply use the TOLUM offset returned by FSP (follow-up change) without adding more weird calculations.
Tested successfully on X11SSM-F
Change-Id: I0cc730551eb3a79c78a971b40056de8d029f4b82 Signed-off-by: Michael Niewöhner foss@mniewoehner.de --- M src/soc/intel/cannonlake/memmap.c M src/soc/intel/common/block/include/intelblocks/systemagent.h M src/soc/intel/common/block/systemagent/systemagent.c M src/soc/intel/icelake/memmap.c M src/soc/intel/skylake/memmap.c 5 files changed, 4 insertions(+), 63 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/16/36216/6
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36216 )
Change subject: soc/intel: common,skl,cnl,icl: drop reserved mmio memory size calculation ......................................................................
Uploaded patch set 6: New patch set was added with same tree, parent, and commit message as Patch Set 5.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36216 )
Change subject: soc/intel: common,skl,cnl,icl: drop reserved mmio memory size calculation ......................................................................
Patch Set 6:
(4 comments)
https://review.coreboot.org/c/coreboot/+/36216/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36216/5//COMMIT_MSG@14 PS5, Line 14: Since this will always be equal to 0
I'm not sure I understand or follow. […]
Oh, thanks for pointing this out. There was some misunderstanding because CB:21539 which introduced this reserved memory handling wasn't clear about the intention.
I'll point out where in the code we have this 0.
https://review.coreboot.org/c/coreboot/+/36216/5/src/soc/intel/common/block/... File src/soc/intel/common/block/systemagent/systemagent.c:
https://review.coreboot.org/c/coreboot/+/36216/5/src/soc/intel/common/block/... PS5, Line 187: - reserved_mmio_size; So, this is 0 by definition.
So originally, before CB:21539, the range cbmem_top()..TSEG-DPR was marked as MMIO. Currently, we reserved an empty range here (which led to some confusion).
https://review.coreboot.org/c/coreboot/+/36216/5/src/soc/intel/common/block/... PS5, Line 191: base_k = sa_map_values[SA_TSEG_REG] - dpr_size - reserved_mmio_size; This should be cbmem_top() by definition.
So, I guess we can get rid of the reserved size calculation (what this change suggests), but should keep the current behaviour (mark everything as reserved RAM; not MMIO).
Michael, please compare logs with the current upstream code and this patch altered so that it says `base_k = top_of_ram;` here. I would expect no difference. The above mmio_resource() could be completely dropped then. (meh, this will not be fun to put into a reasonable commit message :-/)
https://review.coreboot.org/c/coreboot/+/36216/5/src/soc/intel/common/block/... PS5, Line 180: /* TSEG - DPR -> BGSM */ While we are at it. Could we update these comments to state something useful? e.g. what is being reserved (and not how it's calculated; that is already obvious from the code).
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36216 )
Change subject: soc/intel: common,skl,cnl,icl: drop reserved mmio memory size calculation ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36216/5/src/soc/intel/common/block/... File src/soc/intel/common/block/systemagent/systemagent.c:
https://review.coreboot.org/c/coreboot/+/36216/5/src/soc/intel/common/block/... PS5, Line 187: - reserved_mmio_size;
So, this is 0 by definition. […]
Yes, but it's not necessarily 0 by definition. It's 0 understand platform configurations. This code was here to handle the various configurations. When features that would use that area are turned off then, yes, it does collapse to 0. Anyway, let's look at the logs and go from there.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36216 )
Change subject: soc/intel: common,skl,cnl,icl: drop reserved mmio memory size calculation ......................................................................
Patch Set 6:
sorry for being late in review.. what is the reason of dropping reverse API call ? what we are really wish to achieve here.
I do doubt ur test coverage, i mean did u really enable any reversed range like PRMRR, ME Stolen Memory, GDXC etc...the reason we wrote this function to know if FSP has reserved something without CB knowledge. existing code should hold good.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36216 )
Change subject: soc/intel: common,skl,cnl,icl: drop reserved mmio memory size calculation ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36216/5/src/soc/intel/common/block/... File src/soc/intel/common/block/systemagent/systemagent.c:
https://review.coreboot.org/c/coreboot/+/36216/5/src/soc/intel/common/block/... PS5, Line 187: - reserved_mmio_size;
Yes, but it's not necessarily 0 by definition. It's 0 understand platform configurations. […]
I just went through all the affected memmap implementations again, still don't see it.
Small core has no DPR and no reserved range, and `cbmem_top = TSEG base`. Big cores all define `cbmem_top = TSEG base - DPR - reserved range`.
So what we effectively have here is an API that allows us to decide, at the SoC level, what portion of the range cbmem_top..DPR is MMIO and what portion is RAM. But we never make use of that flexibility. We always mark everything as RAM.
Which leaves us with a design choice, but nothing to look at in logs. Do we want to keep this API? It wouldn't be hard: big core would simply have to define `soc_reserved_mmio_size = TSEG base - DPR - cbmem_top` (with the `cbmem_top` provided by FSP). But I don't see why we'd ever need it.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36216 )
Change subject: soc/intel: common,skl,cnl,icl: drop reserved mmio memory size calculation ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36216/5/src/soc/intel/common/block/... File src/soc/intel/common/block/systemagent/systemagent.c:
https://review.coreboot.org/c/coreboot/+/36216/5/src/soc/intel/common/block/... PS5, Line 187: - reserved_mmio_size;
I just went through all the affected memmap implementations again, still […]
going by the ascii art memory map those carve outs should be taken into account. I think that if we tested all the potential combinations that affect the memory map, I bet the cbmem_top() implementations are wrong. What we have now is a snapshot in coreboot of the things we've collectively seen/tested. As more people are retrofitting coreboot onto windows devices that have a larger set of unique combinations I bet we'll run into issues.
Marking areas as RAM is orthogonal to how those areas are actually used. My understanding was that the intent of this patchset is to remove the tight coupling to FSP implementation based on the various options (either passed into FSP or statically configured in CSE/soft straps). I personally like knowing the full memory map, but if it makes things easier for people to honor what is returned from FSP I'm not going to battle against it.
In short, I'm fine with the current patchset as long as we keep the region types marked consistently from how it was previously.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36216 )
Change subject: soc/intel: common,skl,cnl,icl: drop reserved mmio memory size calculation ......................................................................
Patch Set 6:
Patch Set 6:
sorry for being late in review.. what is the reason of dropping reverse API call ? what we are really wish to achieve here.
I do doubt ur test coverage, i mean did u really enable any reversed range like PRMRR, ME Stolen Memory, GDXC etc...the reason we wrote this function to know if FSP has reserved something without CB knowledge. existing code should hold good.
I do doubt ur test coverage, i mean did u really enable any reversed range like PRMRR
Yes, and it broke. See discussion, please
existing code should hold good.
No. See discussion.
Subrata, check the whole patch series, please. There are multiple problems that need to be solved by a) relying on FSP or b) Intel making FSP finally open source or c) Intel doing it's homework and documenting their stuff ;-) I prefer b, but I guess this won't happen, even though Raja Koduri claimed working on making it open which I suspect to be some marketing move :/
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36216 )
Change subject: soc/intel: common,skl,cnl,icl: drop reserved mmio memory size calculation ......................................................................
Patch Set 6:
Patch Set 6:
Patch Set 6:
sorry for being late in review.. what is the reason of dropping reverse API call ? what we are really wish to achieve here.
I do doubt ur test coverage, i mean did u really enable any reversed range like PRMRR, ME Stolen Memory, GDXC etc...the reason we wrote this function to know if FSP has reserved something without CB knowledge. existing code should hold good.
I do doubt ur test coverage, i mean did u really enable any reversed range like PRMRR
Yes, and it broke. See discussion, please
The reserved memory size is influenced by multiple other factors like PTT, TraceHub, PRMRR etc.
i don't sure if i understood why its broken, i have enabled all those config several time on RVP for any issue debug. And don't see any issue since last 3 year. And its not new that this code is only used by chrome team, we have IOT and DCG team using the same code with those configs and never seen such issue.
existing code should hold good.
No. See discussion.
Subrata, check the whole patch series,
i believe your main agenda is to get rid of memmap design and follow FSP Hob to know the memory start. if yes, then don't bother with my concern. i'm only concern that if i do enable those reversed ranges using FSP UPD then i'm expecting do see that size in API.
please. There are multiple problems that need to be solved by a) relying on FSP or b) Intel making FSP finally open source or c) Intel doing it's homework and documenting their stuff ;-) I prefer b, but I guess this won't happen, even though Raja Koduri claimed working on making it open which I suspect to be some marketing move :/
I don't think i do get copied in such big management ladder. may be you can consider adding Vincent.
But this design of Memmap.c is something that intel-google agreed upto in past and so far it holds good.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36216 )
Change subject: soc/intel: common,skl,cnl,icl: drop reserved mmio memory size calculation ......................................................................
Uploaded patch set 7.
Hello Kyösti Mälkki, Aaron Durbin, Patrick Rudolph, Subrata Banik, Arthur Heymans, Paul Menzel, build bot (Jenkins), Nico Huber, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36216
to look at the new patch set (#7).
Change subject: soc/intel: common,skl,cnl,icl: drop reserved mmio memory size calculation ......................................................................
soc/intel: common,skl,cnl,icl: drop reserved mmio memory size calculation
Remove the calculation of the Reserved Intel MMIO Memory size from systemagent and memmap for two reasons:
1. It is not needed. The size is used in SA to calculate the space between cbmem_top and TSEG without DPR and Chipset Reserved Memory. Since this will always be equal to 0, the reservation will be skipped and TSEG, DPR and Chipset Reserved Memory will get reserved alltogether.
2. The reserved memory size is influenced by multiple other factors like PTT, TraceHub, PRMRR etc. Thus it is not trivial to do the calculation right. It fails, for example, when using the TOLUM returned by FSP with a PRMRR size of 0. In this case the size is calculated wrong. This makes it difficult to simply use the TOLUM offset returned by FSP (follow-up change) without adding more weird calculations.
Tested successfully on X11SSM-F
Change-Id: I0cc730551eb3a79c78a971b40056de8d029f4b82 Signed-off-by: Michael Niewöhner foss@mniewoehner.de --- M src/soc/intel/cannonlake/memmap.c M src/soc/intel/common/block/include/intelblocks/systemagent.h M src/soc/intel/common/block/systemagent/systemagent.c M src/soc/intel/icelake/memmap.c M src/soc/intel/skylake/memmap.c 5 files changed, 3 insertions(+), 71 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/16/36216/7
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36216 )
Change subject: soc/intel: common,skl,cnl,icl: drop reserved mmio memory size calculation ......................................................................
Patch Set 7:
Patch Set 6:
Patch Set 6:
Patch Set 6:
sorry for being late in review.. what is the reason of dropping reverse API call ? what we are really wish to achieve here.
I do doubt ur test coverage, i mean did u really enable any reversed range like PRMRR, ME Stolen Memory, GDXC etc...the reason we wrote this function to know if FSP has reserved something without CB knowledge. existing code should hold good.
I do doubt ur test coverage, i mean did u really enable any reversed range like PRMRR
Yes, and it broke. See discussion, please
The reserved memory size is influenced by multiple other factors like PTT, TraceHub, PRMRR etc.
i don't sure if i understood why its broken, i have enabled all those config several time on RVP for any issue debug. And don't see any issue since last 3 year. And its not new that this code is only used by chrome team, we have IOT and DCG team using the same code with those configs and never seen such issue.
existing code should hold good.
No. See discussion.
Subrata, check the whole patch series,
i believe your main agenda is to get rid of memmap design and follow FSP Hob to know the memory start. if yes, then don't bother with my concern. i'm only concern that if i do enable those reversed ranges using FSP UPD then i'm expecting do see that size in API.
please. There are multiple problems that need to be solved by a) relying on FSP or b) Intel making FSP finally open source or c) Intel doing it's homework and documenting their stuff ;-) I prefer b, but I guess this won't happen, even though Raja Koduri claimed working on making it open which I suspect to be some marketing move :/
I don't think i do get copied in such big management ladder. may be you can consider adding Vincent.
I am aware that this is not your decision, that's why I mentioned Raja.
Sorry, I'm not familiar with all those people, yet - which Vincent exactly?
But this design of Memmap.c is something that intel-google agreed upto in past and so far it holds good.
Well, as already stated we found multiple problems. For example disabling SGX and setting PRMRR=0 does not work on master. This was the reason to look deeper into memmap and systemagent and finally raised the problems there.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36216 )
Change subject: soc/intel: common,skl,cnl,icl: drop reserved mmio memory size calculation ......................................................................
Patch Set 7:
But this design of Memmap.c is something that intel-google agreed upto in past and so far it holds good.
Well, as already stated we found multiple problems. For example disabling SGX and setting PRMRR=0 does not work on master. This was the reason to look deeper into memmap and systemagent and finally raised the problems there.
Wasn't that because of a revert?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36216 )
Change subject: soc/intel: common,skl,cnl,icl: drop reserved mmio memory size calculation ......................................................................
Patch Set 7:
Patch Set 7:
Patch Set 6:
Patch Set 6:
Patch Set 6:
sorry for being late in review.. what is the reason of dropping reverse API call ? what we are really wish to achieve here.
I do doubt ur test coverage, i mean did u really enable any reversed range like PRMRR, ME Stolen Memory, GDXC etc...the reason we wrote this function to know if FSP has reserved something without CB knowledge. existing code should hold good.
I do doubt ur test coverage, i mean did u really enable any reversed range like PRMRR
Yes, and it broke. See discussion, please
The reserved memory size is influenced by multiple other factors like PTT, TraceHub, PRMRR etc.
i don't sure if i understood why its broken, i have enabled all those config several time on RVP for any issue debug. And don't see any issue since last 3 year. And its not new that this code is only used by chrome team, we have IOT and DCG team using the same code with those configs and never seen such issue.
existing code should hold good.
No. See discussion.
Subrata, check the whole patch series,
i believe your main agenda is to get rid of memmap design and follow FSP Hob to know the memory start. if yes, then don't bother with my concern. i'm only concern that if i do enable those reversed ranges using FSP UPD then i'm expecting do see that size in API.
please. There are multiple problems that need to be solved by a) relying on FSP or b) Intel making FSP finally open source or c) Intel doing it's homework and documenting their stuff ;-) I prefer b, but I guess this won't happen, even though Raja Koduri claimed working on making it open which I suspect to be some marketing move :/
I don't think i do get copied in such big management ladder. may be you can consider adding Vincent.
I am aware that this is not your decision, that's why I mentioned Raja.
Sorry, I'm not familiar with all those people, yet - which Vincent exactly?
But this design of Memmap.c is something that intel-google agreed upto in past and so far it holds good.
Well, as already stated we found multiple problems. For example disabling SGX and setting PRMRR=0 does not work on master. This was the reason to look deeper into memmap and systemagent and finally raised the problems there.
Make SGX disable its okay, you don't need to handle PRMRR size, let its be default. its will be default taken care. i doubt that some misconfiguration in the reason that you are ran into
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36216 )
Change subject: soc/intel: common,skl,cnl,icl: drop reserved mmio memory size calculation ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36216/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36216/7//COMMIT_MSG@24 PS7, Line 24: Tested successfully on X11SSM-F needs retest
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36216 )
Change subject: soc/intel: common,skl,cnl,icl: drop reserved mmio memory size calculation ......................................................................
Uploaded patch set 8: Patch Set 7 was rebased.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36216 )
Change subject: soc/intel: common,skl,cnl,icl: drop reserved mmio memory size calculation ......................................................................
Patch Set 8: Code-Review+1
(4 comments)
There is no issue wrt. this change on master. The memmap calculations in general probably get some corner cases wrong. SGX disabled is not a corner case though, `sgx_enable = 0` and `PrmrrSize = 0` are the default.
Beside my latest comment on the commit message, I think this change is ready.
https://review.coreboot.org/c/coreboot/+/36216/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36216/7//COMMIT_MSG@22 PS7, Line 22: change) without adding more weird calculations. The second point seems to be just noise. And I'm not convinced by some of the details. Probably something that was tested in between the other patches?
Let's focus on the calculations, we know that: * TSEG - DPR - reserved - top_of_memory is 0 * TSEG - DPR - reserved is top_of_memory.
And hence can simplify the code here. This was confirmed by reading the code and testing.
https://review.coreboot.org/c/coreboot/+/36216/5/src/soc/intel/common/block/... File src/soc/intel/common/block/systemagent/systemagent.c:
https://review.coreboot.org/c/coreboot/+/36216/5/src/soc/intel/common/block/... PS5, Line 187: - reserved_mmio_size;
going by the ascii art memory map those carve outs should be taken into account. […]
Done?
https://review.coreboot.org/c/coreboot/+/36216/5/src/soc/intel/common/block/... PS5, Line 191: base_k = sa_map_values[SA_TSEG_REG] - dpr_size - reserved_mmio_size;
This should be cbmem_top() by definition. […]
Done
https://review.coreboot.org/c/coreboot/+/36216/5/src/soc/intel/common/block/... PS5, Line 180: /* TSEG - DPR -> BGSM */
While we are at it. Could we update these comments to state something […]
Done
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36216 )
Change subject: soc/intel: common,skl,cnl,icl: drop reserved mmio memory size calculation ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36216/5/src/soc/intel/common/block/... File src/soc/intel/common/block/systemagent/systemagent.c:
https://review.coreboot.org/c/coreboot/+/36216/5/src/soc/intel/common/block/... PS5, Line 187: - reserved_mmio_size;
Done?
Where did we land on the reserved ram vs reserved mmio differences? It's no difference for many of the cases because reserved_mmio_size was 0?
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36216 )
Change subject: soc/intel: common,skl,cnl,icl: drop reserved mmio memory size calculation ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36216/5/src/soc/intel/common/block/... File src/soc/intel/common/block/systemagent/systemagent.c:
https://review.coreboot.org/c/coreboot/+/36216/5/src/soc/intel/common/block/... PS5, Line 187: - reserved_mmio_size;
Where did we land on the reserved ram vs reserved mmio differences? It's no difference for many of t […]
There is no difference. `reserved_mmio_size` always fills the gap between cbmem_top() and DPR exactly. I have confirmed that theory by reading all the `memmap.c` over again and again. And Michael confirmed it by testing.
The code in the memmaps isn't hard to follow btw. we always have
+-----------------------------+ TOLUD | traditional (including DPR) | +-----------------------------+ DPR base | reserved_mmio_size | +-----------------------------+ cbmem_top() | CBMEM |
I don't see anything between the reserved part and cbmem_top() in any combinations of options and platforms.
When the calculation here was introduced it made a difference. Platforms had soc_reserved_mmio_size() implemented step by step. But that wasn't necessary. The result is the same, everything from cbmem_top() to TSEG (inclusive) is marked as reserved RAM, everything above TSEG to TOLUD is marked as MMIO.
I really doubt there is anything to miss, so this change shouldn't alter anything. It just gets rid of the unnecessary boilerplate.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36216 )
Change subject: soc/intel: common,skl,cnl,icl: drop reserved mmio memory size calculation ......................................................................
Patch Set 8: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/36216/5/src/soc/intel/common/block/... File src/soc/intel/common/block/systemagent/systemagent.c:
https://review.coreboot.org/c/coreboot/+/36216/5/src/soc/intel/common/block/... PS5, Line 187: - reserved_mmio_size;
There is no difference. `reserved_mmio_size` always fills the gap between […]
ok
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36216 )
Change subject: soc/intel: common,skl,cnl,icl: drop reserved mmio memory size calculation ......................................................................
Uploaded patch set 9: Commit message was updated.
(3 comments)
https://review.coreboot.org/c/coreboot/+/36216/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36216/5//COMMIT_MSG@14 PS5, Line 14: Since this will always be equal to 0
Oh, thanks for pointing this out. There was some misunderstanding because […]
Done
https://review.coreboot.org/c/coreboot/+/36216/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36216/7//COMMIT_MSG@22 PS7, Line 22: change) without adding more weird calculations.
The second point seems to be just noise. And I'm not convinced by […]
Done
https://review.coreboot.org/c/coreboot/+/36216/5/src/soc/intel/common/block/... File src/soc/intel/common/block/systemagent/systemagent.c:
https://review.coreboot.org/c/coreboot/+/36216/5/src/soc/intel/common/block/... PS5, Line 178: mmio_resource(dev, index++, base_k / KiB, size_k / KiB);
I'm not sure it matters too much, but this change implicitly marks the PRMRR, trace, and ptt memory […]
I guess this is answered by Nico on the left, isn't it?
Hello Kyösti Mälkki, Aaron Durbin, Patrick Rudolph, Subrata Banik, Arthur Heymans, Paul Menzel, build bot (Jenkins), Nico Huber, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36216
to look at the new patch set (#9).
Change subject: soc/intel: common,skl,cnl,icl: drop reserved mmio memory size calculation ......................................................................
soc/intel: common,skl,cnl,icl: drop reserved mmio memory size calculation
Remove the calculation of the Reserved Intel MMIO Memory size from systemagent and memmap, since it is not needed.
The size is used in SA to calculate the space between cbmem_top and TSEG without DPR and Chipset Reserved Memory. Since this will always be equal to 0, the reservation will be skipped and TSEG, DPR and Chipset Reserved Memory will get reserved alltogether.
By reading the code and pratical testing we figured out that: - TSEG - DPR - reserved - top_of_memory == 0 - TSEG - DPR - reserved == top_of_memory
This means the whole block will never reserve anything because it is always 0. Hence the code can be removed for simplification.
Tested successfully on X11SSM-F
Change-Id: I0cc730551eb3a79c78a971b40056de8d029f4b82 Signed-off-by: Michael Niewöhner foss@mniewoehner.de --- M src/soc/intel/cannonlake/memmap.c M src/soc/intel/common/block/include/intelblocks/systemagent.h M src/soc/intel/common/block/systemagent/systemagent.c M src/soc/intel/icelake/memmap.c M src/soc/intel/skylake/memmap.c 5 files changed, 3 insertions(+), 71 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/16/36216/9
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36216 )
Change subject: soc/intel: common,skl,cnl,icl: drop reserved mmio memory size calculation ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36216/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36216/7//COMMIT_MSG@24 PS7, Line 24: Tested successfully on X11SSM-F
needs retest
Done
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36216 )
Change subject: soc/intel: common,skl,cnl,icl: drop reserved mmio memory size calculation ......................................................................
Patch Set 9: Code-Review+1
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36216 )
Change subject: soc/intel: common,skl,cnl,icl: drop reserved mmio memory size calculation ......................................................................
Patch Set 9: Code-Review+2
Nico Huber has submitted this change. ( https://review.coreboot.org/c/coreboot/+/36216 )
Change subject: soc/intel: common,skl,cnl,icl: drop reserved mmio memory size calculation ......................................................................
soc/intel: common,skl,cnl,icl: drop reserved mmio memory size calculation
Remove the calculation of the Reserved Intel MMIO Memory size from systemagent and memmap, since it is not needed.
The size is used in SA to calculate the space between cbmem_top and TSEG without DPR and Chipset Reserved Memory. Since this will always be equal to 0, the reservation will be skipped and TSEG, DPR and Chipset Reserved Memory will get reserved alltogether.
By reading the code and pratical testing we figured out that: - TSEG - DPR - reserved - top_of_memory == 0 - TSEG - DPR - reserved == top_of_memory
This means the whole block will never reserve anything because it is always 0. Hence the code can be removed for simplification.
Tested successfully on X11SSM-F
Change-Id: I0cc730551eb3a79c78a971b40056de8d029f4b82 Signed-off-by: Michael Niewöhner foss@mniewoehner.de Reviewed-on: https://review.coreboot.org/c/coreboot/+/36216 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Michael Niewöhner Reviewed-by: Nico Huber nico.h@gmx.de Reviewed-by: Aaron Durbin adurbin@chromium.org --- M src/soc/intel/cannonlake/memmap.c M src/soc/intel/common/block/include/intelblocks/systemagent.h M src/soc/intel/common/block/systemagent/systemagent.c M src/soc/intel/icelake/memmap.c M src/soc/intel/skylake/memmap.c 5 files changed, 3 insertions(+), 71 deletions(-)
Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved Aaron Durbin: Looks good to me, but someone else must approve Michael Niewöhner: Looks good to me, but someone else must approve
diff --git a/src/soc/intel/cannonlake/memmap.c b/src/soc/intel/cannonlake/memmap.c index 7adaa30..2239f13 100644 --- a/src/soc/intel/cannonlake/memmap.c +++ b/src/soc/intel/cannonlake/memmap.c @@ -211,21 +211,6 @@ return dram_base; }
-/* - * SoC implementation - * - * SoC call to summarize all Intel Reserve MMIO size and report to SA - */ -size_t soc_reserved_mmio_size(void) -{ - struct ebda_config cfg; - - retrieve_ebda_object(&cfg); - - /* Get Intel Reserved Memory Range Size */ - return cfg.reserved_mem_size; -} - /* Fill up memory layout information */ void fill_soc_memmap_ebda(struct ebda_config *cfg) { diff --git a/src/soc/intel/common/block/include/intelblocks/systemagent.h b/src/soc/intel/common/block/include/intelblocks/systemagent.h index 133047c..ae9213c 100644 --- a/src/soc/intel/common/block/include/intelblocks/systemagent.h +++ b/src/soc/intel/common/block/include/intelblocks/systemagent.h @@ -106,8 +106,4 @@ /* SoC specific APIs to get UNCORE PRMRR base and mask values * returns 0, if able to get base and mask values; otherwise returns -1 */ int soc_get_uncore_prmmr_base_and_mask(uint64_t *base, uint64_t *mask); - -/* SoC call to summarize all Intel Reserve MMIO size and report to SA */ -size_t soc_reserved_mmio_size(void); - #endif /* SOC_INTEL_COMMON_BLOCK_SA_H */ diff --git a/src/soc/intel/common/block/systemagent/systemagent.c b/src/soc/intel/common/block/systemagent/systemagent.c index 0312cac..e03942f 100644 --- a/src/soc/intel/common/block/systemagent/systemagent.c +++ b/src/soc/intel/common/block/systemagent/systemagent.c @@ -46,11 +46,6 @@ return -1; }
-__weak size_t soc_reserved_mmio_size(void) -{ - return 0; -} - __weak unsigned long sa_write_acpi_tables(struct device *dev, unsigned long current, struct acpi_rsdp *rsdp) @@ -125,8 +120,7 @@ * These are the host memory ranges that should be added: * - 0 -> 0xa0000: cacheable * - 0xc0000 -> top_of_ram : cacheable - * - top_of_ram -> TSEG - DPR: uncacheable - * - TESG - DPR -> BGSM: cacheable with standard MTRRs and reserved + * - top_of_ram -> BGSM: cacheable with standard MTRRs and reserved * - BGSM -> TOLUD: not cacheable with standard MTRRs and reserved * - 4GiB -> TOUUD: cacheable * @@ -155,18 +149,11 @@ static void sa_add_dram_resources(struct device *dev, int *resource_count) { uintptr_t base_k, touud_k; - size_t dpr_size = 0, size_k; - size_t reserved_mmio_size; + size_t size_k; uint64_t sa_map_values[MAX_MAP_ENTRIES]; uintptr_t top_of_ram; int index = *resource_count;
- if (CONFIG(SA_ENABLE_DPR)) - dpr_size = sa_get_dpr_size(); - - /* Get SoC reserve memory size as per user selection */ - reserved_mmio_size = soc_reserved_mmio_size(); - top_of_ram = (uintptr_t)cbmem_top();
/* 0 - > 0xa0000 */ @@ -181,14 +168,8 @@
sa_get_mem_map(dev, &sa_map_values[0]);
- /* top_of_ram -> TSEG - DPR - Intel Reserve Memory Size*/ + /* top_of_ram -> BGSM */ base_k = top_of_ram; - size_k = sa_map_values[SA_TSEG_REG] - dpr_size - base_k - - reserved_mmio_size; - mmio_resource(dev, index++, base_k / KiB, size_k / KiB); - - /* TSEG - DPR - Intel Reserve Memory Size -> BGSM */ - base_k = sa_map_values[SA_TSEG_REG] - dpr_size - reserved_mmio_size; size_k = sa_map_values[SA_BGSM_REG] - base_k; reserved_ram_resource(dev, index++, base_k / KiB, size_k / KiB);
diff --git a/src/soc/intel/icelake/memmap.c b/src/soc/intel/icelake/memmap.c index 20c4e6f..122cb1a 100644 --- a/src/soc/intel/icelake/memmap.c +++ b/src/soc/intel/icelake/memmap.c @@ -190,21 +190,6 @@ return dram_base; }
-/* - * SoC implementation - * - * SoC call to summarize all Intel Reserve MMIO size and report to SA - */ -size_t soc_reserved_mmio_size(void) -{ - struct ebda_config cfg; - - retrieve_ebda_object(&cfg); - - /* Get Intel Reserved Memory Range Size */ - return cfg.reserved_mem_size; -} - /* Fill up memory layout information */ void fill_soc_memmap_ebda(struct ebda_config *cfg) { diff --git a/src/soc/intel/skylake/memmap.c b/src/soc/intel/skylake/memmap.c index f2790ef..780c73c 100644 --- a/src/soc/intel/skylake/memmap.c +++ b/src/soc/intel/skylake/memmap.c @@ -212,21 +212,6 @@ return dram_base; }
-/* - * SoC implementation - * - * SoC call to summarize all Intel Reserve MMIO size and report to SA - */ -size_t soc_reserved_mmio_size(void) -{ - struct ebda_config cfg; - - retrieve_ebda_object(&cfg); - - /* Get Intel Reserved Memory Range Size */ - return cfg.reserved_mem_size; -} - /* Fill up memory layout information */ void fill_soc_memmap_ebda(struct ebda_config *cfg) {
-
Aaron Durbin (Code Review) -
Michael Niewöhner (Code Review)
-
Nico Huber (Code Review)
-
Subrata Banik (Code Review)