Kyösti Mälkki has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/34707 )
Change subject: x86/smm: Rename SMM_SUBREGION_CHIPSET to _IED ......................................................................
x86/smm: Rename SMM_SUBREGION_CHIPSET to _IED
Make it explicit what the region will be used for.
Change-Id: I82a09878a6adedb33fdaf4289c1299136140d1f6 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/include/cpu/x86/smm.h M src/soc/intel/cannonlake/memmap.c M src/soc/intel/cannonlake/smmrelocate.c M src/soc/intel/icelake/memmap.c M src/soc/intel/icelake/smmrelocate.c M src/soc/intel/skylake/memmap.c M src/soc/intel/skylake/smmrelocate.c 7 files changed, 7 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/07/34707/1
diff --git a/src/include/cpu/x86/smm.h b/src/include/cpu/x86/smm.h index b8b99ec..3b9e168 100644 --- a/src/include/cpu/x86/smm.h +++ b/src/include/cpu/x86/smm.h @@ -595,7 +595,7 @@ /* SMM cache region. */ SMM_SUBREGION_CACHE, /* Chipset specific area. */ - SMM_SUBREGION_CHIPSET, + SMM_SUBREGION_IED, /* Total sub regions supported. */ SMM_SUBREGION_NUM, }; diff --git a/src/soc/intel/cannonlake/memmap.c b/src/soc/intel/cannonlake/memmap.c index b5b538c..a80ff58 100644 --- a/src/soc/intel/cannonlake/memmap.c +++ b/src/soc/intel/cannonlake/memmap.c @@ -66,7 +66,7 @@ sub_base += sub_size - (ied_size + cache_size); sub_size = cache_size; break; - case SMM_SUBREGION_CHIPSET: + case SMM_SUBREGION_IED: /* IED is at the top. */ sub_base += sub_size - ied_size; sub_size = ied_size; diff --git a/src/soc/intel/cannonlake/smmrelocate.c b/src/soc/intel/cannonlake/smmrelocate.c index 67c603a..a9c300f 100644 --- a/src/soc/intel/cannonlake/smmrelocate.c +++ b/src/soc/intel/cannonlake/smmrelocate.c @@ -189,7 +189,7 @@
smm_region(&tseg_base, &tseg_size); smm_subregion(SMM_SUBREGION_HANDLER, ¶ms->smram_base, ¶ms->smram_size); - smm_subregion(SMM_SUBREGION_CHIPSET, ¶ms->ied_base, ¶ms->ied_size); + smm_subregion(SMM_SUBREGION_IED, ¶ms->ied_base, ¶ms->ied_size);
/* SMRR has 32-bits of valid address aligned to 4KiB. */ params->smrr_base.lo = (params->smram_base & rmask) | MTRR_TYPE_WRBACK; diff --git a/src/soc/intel/icelake/memmap.c b/src/soc/intel/icelake/memmap.c index 046774f..f8121b8 100644 --- a/src/soc/intel/icelake/memmap.c +++ b/src/soc/intel/icelake/memmap.c @@ -64,7 +64,7 @@ sub_base += sub_size - (ied_size + cache_size); sub_size = cache_size; break; - case SMM_SUBREGION_CHIPSET: + case SMM_SUBREGION_IED: /* IED is at the top. */ sub_base += sub_size - ied_size; sub_size = ied_size; diff --git a/src/soc/intel/icelake/smmrelocate.c b/src/soc/intel/icelake/smmrelocate.c index a93fa97..3770b24 100644 --- a/src/soc/intel/icelake/smmrelocate.c +++ b/src/soc/intel/icelake/smmrelocate.c @@ -188,7 +188,7 @@
smm_region(&tseg_base, &tseg_size); smm_subregion(SMM_SUBREGION_HANDLER, ¶ms->smram_base, ¶ms->smram_size); - smm_subregion(SMM_SUBREGION_CHIPSET, ¶ms->ied_base, ¶ms->ied_size); + smm_subregion(SMM_SUBREGION_IED, ¶ms->ied_base, ¶ms->ied_size);
/* SMRR has 32-bits of valid address aligned to 4KiB. */ params->smrr_base.lo = (params->smram_base & rmask) | MTRR_TYPE_WRBACK; diff --git a/src/soc/intel/skylake/memmap.c b/src/soc/intel/skylake/memmap.c index d6ab908..5008571 100644 --- a/src/soc/intel/skylake/memmap.c +++ b/src/soc/intel/skylake/memmap.c @@ -68,7 +68,7 @@ sub_base += sub_size - (ied_size + cache_size); sub_size = cache_size; break; - case SMM_SUBREGION_CHIPSET: + case SMM_SUBREGION_IED: /* IED is at the top. */ sub_base += sub_size - ied_size; sub_size = ied_size; diff --git a/src/soc/intel/skylake/smmrelocate.c b/src/soc/intel/skylake/smmrelocate.c index 828ea4f..0825eae 100644 --- a/src/soc/intel/skylake/smmrelocate.c +++ b/src/soc/intel/skylake/smmrelocate.c @@ -198,7 +198,7 @@
smm_region(&tseg_base, &tseg_size); smm_subregion(SMM_SUBREGION_HANDLER, ¶ms->smram_base, ¶ms->smram_size); - smm_subregion(SMM_SUBREGION_CHIPSET, ¶ms->ied_base, ¶ms->ied_size); + smm_subregion(SMM_SUBREGION_IED, ¶ms->ied_base, ¶ms->ied_size);
/* SMRR has 32-bits of valid address aligned to 4KiB. */ params->smrr_base.lo = (params->smram_base & rmask) | MTRR_TYPE_WRBACK;
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34707 )
Change subject: x86/smm: Rename SMM_SUBREGION_CHIPSET to _IED ......................................................................
Patch Set 4: Code-Review+2
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34707 )
Change subject: x86/smm: Rename SMM_SUBREGION_CHIPSET to _IED ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34707/5/src/include/cpu/x86/smm.h File src/include/cpu/x86/smm.h:
https://review.coreboot.org/c/coreboot/+/34707/5/src/include/cpu/x86/smm.h@5... PS5, Line 597: Chipset specific area Update comment too?
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34707 )
Change subject: x86/smm: Rename SMM_SUBREGION_CHIPSET to _IED ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34707/5/src/include/cpu/x86/smm.h File src/include/cpu/x86/smm.h:
https://review.coreboot.org/c/coreboot/+/34707/5/src/include/cpu/x86/smm.h@5... PS5, Line 597: Chipset specific area
Update comment too?
FWIW, it was originally named chipset because IED is a intel specific name. cpu/x86/ is supposed to generic. Though, I'm not sure it matters much in practice. One could do something like the following:
SMM_SUBREGION_CHIPSET, SMM_SUBREGION_IED = SMM_SUBREGION_IED,
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34707 )
Change subject: x86/smm: Rename SMM_SUBREGION_CHIPSET to _IED ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34707/5/src/include/cpu/x86/smm.h File src/include/cpu/x86/smm.h:
https://review.coreboot.org/c/coreboot/+/34707/5/src/include/cpu/x86/smm.h@5... PS5, Line 597: Chipset specific area
FWIW, it was originally named chipset because IED is a intel specific name. […]
Is it possible chipset declares more than one region we need to deal with? Also I kind of like to see this:
smm_subregion(SMM_SUBREGION_IED, ¶ms->ied_base, ¶ms->ied_size);
Hello Aaron Durbin, Patrick Rudolph, Arthur Heymans, build bot (Jenkins), Nico Huber, Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34707
to look at the new patch set (#6).
Change subject: x86/smm: Rename SMM_SUBREGION_CHIPSET to _IED ......................................................................
x86/smm: Rename SMM_SUBREGION_CHIPSET to _IED
Make it explicit what the region will be used for.
Change-Id: I82a09878a6adedb33fdaf4289c1299136140d1f6 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/include/cpu/x86/smm.h M src/soc/intel/cannonlake/memmap.c M src/soc/intel/cannonlake/smmrelocate.c M src/soc/intel/icelake/memmap.c M src/soc/intel/icelake/smmrelocate.c M src/soc/intel/skylake/memmap.c M src/soc/intel/skylake/smmrelocate.c 7 files changed, 8 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/07/34707/6
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34707 )
Change subject: x86/smm: Rename SMM_SUBREGION_CHIPSET to _IED ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34707/5/src/include/cpu/x86/smm.h File src/include/cpu/x86/smm.h:
https://review.coreboot.org/c/coreboot/+/34707/5/src/include/cpu/x86/smm.h@5... PS5, Line 597: Chipset specific area
Is it possible chipset declares more than one region we need to deal with? Also I kind of like to see this:
smm_subregion(SMM_SUBREGION_IED, ¶ms->ied_base, ¶ms->ied_size);
That should still be possible with: SMM_SUBREGION_IGD = SMM_SUBREGION_CHIPSET, right?
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34707 )
Change subject: x86/smm: Rename SMM_SUBREGION_CHIPSET to _IED ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34707/5/src/include/cpu/x86/smm.h File src/include/cpu/x86/smm.h:
https://review.coreboot.org/c/coreboot/+/34707/5/src/include/cpu/x86/smm.h@5... PS5, Line 597: Chipset specific area
Is it possible chipset declares more than one region we need to deal with? […]
I just don't see the value in this. If a platform has two custom block to setup, why does _IED get to be the one aliased _CHIPSET but not the other?
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34707 )
Change subject: x86/smm: Rename SMM_SUBREGION_CHIPSET to _IED ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34707/5/src/include/cpu/x86/smm.h File src/include/cpu/x86/smm.h:
https://review.coreboot.org/c/coreboot/+/34707/5/src/include/cpu/x86/smm.h@5... PS5, Line 597: Chipset specific area
I just don't see the value in this. […]
Can we postpone discussion of this 'clean' API at least to CB:34776 and/or CB:34758 ?
There is the unsolved question how to best consolidate IED in general, looks like there is couple variants of setting it up. Will we need IEDv1 IEDv2 somehow?
Also, I would like imdb to cover the complete coreboot-owned region of TSEG.
Hello Aaron Durbin, Patrick Rudolph, Arthur Heymans, build bot (Jenkins), Nico Huber, Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34707
to look at the new patch set (#7).
Change subject: x86/smm: Rename SMM_SUBREGION_CHIPSET to _IED ......................................................................
x86/smm: Rename SMM_SUBREGION_CHIPSET to _IED
Make it explicit what the region will be used for.
Change-Id: I82a09878a6adedb33fdaf4289c1299136140d1f6 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/include/cpu/x86/smm.h M src/soc/intel/cannonlake/memmap.c M src/soc/intel/cannonlake/smmrelocate.c M src/soc/intel/icelake/memmap.c M src/soc/intel/icelake/smmrelocate.c M src/soc/intel/skylake/memmap.c M src/soc/intel/skylake/smmrelocate.c 7 files changed, 8 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/07/34707/7
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34707 )
Change subject: x86/smm: Rename SMM_SUBREGION_CHIPSET to _IED ......................................................................
Patch Set 7: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/34707/5/src/include/cpu/x86/smm.h File src/include/cpu/x86/smm.h:
https://review.coreboot.org/c/coreboot/+/34707/5/src/include/cpu/x86/smm.h@5... PS5, Line 597: Chipset specific area
Can we postpone discussion of this 'clean' API at least to CB:34776 and/or CB:34758 ? […]
sure
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34707 )
Change subject: x86/smm: Rename SMM_SUBREGION_CHIPSET to _IED ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34707/5/src/include/cpu/x86/smm.h File src/include/cpu/x86/smm.h:
https://review.coreboot.org/c/coreboot/+/34707/5/src/include/cpu/x86/smm.h@5... PS5, Line 597: Chipset specific area
sure
Ack
Kyösti Mälkki has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/34707 )
Change subject: x86/smm: Rename SMM_SUBREGION_CHIPSET to _IED ......................................................................
Abandoned
not renaming after discussion
-
Aaron Durbin (Code Review) -
Arthur Heymans (Code Review)
-
Furquan Shaikh (Code Review)
-
Kyösti Mälkki (Code Review)