Attention is currently required from: Tim Wawrzynczak.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/75012?usp=email )
The change is no longer submittable: All-Comments-Resolved is unsatisfied now.
Change subject: allocator_v4: Treat above 4G resources more natively
......................................................................
Patch Set 4:
(1 comment)
Patchset:
PS4:
Just learned more AMD details. It looks like we should lower
DOMAIN_RESOURCE_32BIT_LIMIT to ECAM_MMCONF_BASE_ADDRESS
or wait for CB:74712 etc.
--
To view, visit https://review.coreboot.org/c/coreboot/+/75012?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ia822f0ce648c7f7afc801d9cb00b6459fe7cebea
Gerrit-Change-Number: 75012
Gerrit-PatchSet: 4
Gerrit-Owner: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Lean Sheng Tan <sheng.tan(a)9elements.com>
Gerrit-Reviewer: Tim Wawrzynczak <inforichland(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Tim Wawrzynczak <inforichland(a)gmail.com>
Gerrit-Comment-Date: Fri, 02 Jun 2023 14:50:57 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Nico Huber, Tim Wawrzynczak.
Lean Sheng Tan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/75012?usp=email )
Change subject: allocator_v4: Treat above 4G resources more natively
......................................................................
Patch Set 4: Code-Review+1
--
To view, visit https://review.coreboot.org/c/coreboot/+/75012?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ia822f0ce648c7f7afc801d9cb00b6459fe7cebea
Gerrit-Change-Number: 75012
Gerrit-PatchSet: 4
Gerrit-Owner: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Lean Sheng Tan <sheng.tan(a)9elements.com>
Gerrit-Reviewer: Tim Wawrzynczak <inforichland(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Tim Wawrzynczak <inforichland(a)gmail.com>
Gerrit-Comment-Date: Fri, 02 Jun 2023 14:46:35 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Arthur Heymans, Bill XIE, Eric Lai, Jonathan Zhang, Kyösti Mälkki, Nico Huber, Werner Zeh.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/65420?usp=email )
Change subject: allocator_v4: Use memranges only for toplevel
......................................................................
Patch Set 18: Code-Review+2
--
To view, visit https://review.coreboot.org/c/coreboot/+/65420?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I70c700318a85f6760f27597730bc9c9a86dbe6b3
Gerrit-Change-Number: 65420
Gerrit-PatchSet: 18
Gerrit-Owner: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Arthur Heymans <arthur.heymans(a)9elements.com>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Bill XIE <persmule(a)hardenedlinux.org>
Gerrit-Reviewer: Eric Lai <eric_lai(a)quanta.corp-partner.google.com>
Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki(a)gmail.com>
Gerrit-Reviewer: Lean Sheng Tan <sheng.tan(a)9elements.com>
Gerrit-Reviewer: Tim Wawrzynczak <inforichland(a)gmail.com>
Gerrit-Reviewer: Werner Zeh <werner.zeh(a)siemens.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Jonathan Zhang <jon.zhixiong.zhang(a)gmail.com>
Gerrit-Attention: Arthur Heymans <arthur.heymans(a)9elements.com>
Gerrit-Attention: Bill XIE <persmule(a)hardenedlinux.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Jonathan Zhang <jon.zhixiong.zhang(a)gmail.com>
Gerrit-Attention: Eric Lai <eric_lai(a)quanta.corp-partner.google.com>
Gerrit-Attention: Werner Zeh <werner.zeh(a)siemens.com>
Gerrit-Attention: Kyösti Mälkki <kyosti.malkki(a)gmail.com>
Gerrit-Comment-Date: Fri, 02 Jun 2023 14:43:55 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Nico Huber, Tim Wawrzynczak.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/75012?usp=email )
Change subject: allocator_v4: Treat above 4G resources more natively
......................................................................
Patch Set 4: Code-Review+2
--
To view, visit https://review.coreboot.org/c/coreboot/+/75012?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ia822f0ce648c7f7afc801d9cb00b6459fe7cebea
Gerrit-Change-Number: 75012
Gerrit-PatchSet: 4
Gerrit-Owner: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Tim Wawrzynczak <inforichland(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Tim Wawrzynczak <inforichland(a)gmail.com>
Gerrit-Comment-Date: Fri, 02 Jun 2023 14:43:22 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Arthur Heymans, Felix Held, Fred Reitberger, Jason Glenesk, Matt DeVillier, Raul Rangel.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/74843?usp=email )
Change subject: soc/amd/common/data_fabric/domain: provide amd_pci_domain_fill_ssdt
......................................................................
Patch Set 11: Code-Review+1
(3 comments)
File src/soc/amd/common/block/data_fabric/domain.c:
https://review.coreboot.org/c/coreboot/+/74843/comment/cf3cbd38_2d7bd701 :
PS11, Line 169: if (base > PCI_IO_CONFIG_LAST_PORT || limit < PCI_IO_CONFIG_INDEX) {
I wonder what ACPI says about having a resource producer that covers this
range and a consumer for cf8..cff both in the same device node. If it would
do the right thing, we wouldn't need special handling for it in coreboot, I
guess.
https://review.coreboot.org/c/coreboot/+/74843/comment/17d02c81_f43c30aa :
PS11, Line 176: /*spit IO range to not cover PCI config IO ports*/
Whitspace around comment marks and *split*.
https://review.coreboot.org/c/coreboot/+/74843/comment/66db6e8f_179244ca :
PS11, Line 168:
: if (base > PCI_IO_CONFIG_LAST_PORT || limit < PCI_IO_CONFIG_INDEX) {
: /* no overlap with PCI config IO ports */
: write_ssdt_domain_io_range_helper(base, limit);
: } else {/* overlap with PCI config IO ports */
: if (base == PCI_IO_CONFIG_INDEX && limit == PCI_IO_CONFIG_LAST_PORT)
: return; /* IO range exactly covers the PCI config IO ports */
: if (base < PCI_IO_CONFIG_INDEX && limit > PCI_IO_CONFIG_LAST_PORT) {
: /*spit IO range to not cover PCI config IO ports*/
: write_ssdt_domain_io_range_helper(base, PCI_IO_CONFIG_INDEX - 1);
: write_ssdt_domain_io_range_helper(PCI_IO_CONFIG_LAST_PORT + 1, limit);
: } else if (limit <= PCI_IO_CONFIG_LAST_PORT) {
: write_ssdt_domain_io_range_helper(base, PCI_IO_CONFIG_INDEX - 1);
: } else { /* base >= PCI_IO_CONFIG_INDEX */
: write_ssdt_domain_io_range_helper(PCI_IO_CONFIG_LAST_PORT + 1, limit);
: }
: }
Much simpler and should work too AFAICT:
```
if (base < PCI_IO_CONFIG_INDEX)
write_ssdt_domain_io_range_helper(base, MIN(limit, PCI_IO_CONFIG_INDEX - 1));
if (limit > PCI_IO_CONFIG_LAST_PORT)
write_ssdt_domain_io_range_helper(MAX(base, PCI_IO_CONFIG_LAST_PORT + 1), limit);
```
(or similar code without MIN/MAX on the original else branch)
i.e. add each of the split ranges that isn't empty.
--
To view, visit https://review.coreboot.org/c/coreboot/+/74843?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Iaf6d38a8ef5bb0163c4d1c021bf892c323d9a448
Gerrit-Change-Number: 74843
Gerrit-PatchSet: 11
Gerrit-Owner: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Fred Reitberger <reitbergerfred(a)gmail.com>
Gerrit-Reviewer: Jason Glenesk <jason.glenesk(a)gmail.com>
Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Jason Glenesk <jason.glenesk(a)gmail.com>
Gerrit-Attention: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Attention: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Attention: Fred Reitberger <reitbergerfred(a)gmail.com>
Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Comment-Date: Fri, 02 Jun 2023 14:39:57 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Felix Held, Fred Reitberger, Jason Glenesk, Matt DeVillier, Raul Rangel.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/74992?usp=email )
Change subject: soc/amd/common/data_fabric/domain: write _BBN method in SSDT
......................................................................
Patch Set 8: Code-Review+1
--
To view, visit https://review.coreboot.org/c/coreboot/+/74992?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I8badeb0064b498d3f18217ea24bff73676913b02
Gerrit-Change-Number: 74992
Gerrit-PatchSet: 8
Gerrit-Owner: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Reviewer: Fred Reitberger <reitbergerfred(a)gmail.com>
Gerrit-Reviewer: Jason Glenesk <jason.glenesk(a)gmail.com>
Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Jason Glenesk <jason.glenesk(a)gmail.com>
Gerrit-Attention: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Attention: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com>
Gerrit-Attention: Fred Reitberger <reitbergerfred(a)gmail.com>
Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Comment-Date: Fri, 02 Jun 2023 14:07:22 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Arthur Heymans, Tim Wawrzynczak.
Hello Arthur Heymans, Tim Wawrzynczak, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/75012?usp=email
to look at the new patch set (#4).
Change subject: allocator_v4: Treat above 4G resources more natively
......................................................................
allocator_v4: Treat above 4G resources more natively
We currently have two competing mechanisms to limit the placement of
resources:
1. the explicit `.limit` field of a resource, and
2. the IORESOURCE_ABOVE_4G flag.
This makes the resource allocator unnecessarily complex. Ideally, we
would always reduce the `.limit` field if we want to "pin" a specific
resource below 4G. However, as that's not done across the tree yet,
we will use the _absence_ of the IORESOURCE_ABOVE_4G flag as a hint
to implicitly lower the `limit` of a resource. In this patch, this
is done inside the effective_limit() function that hides the flag
from the rest of the allocator.
To automatically place resources above 4G if their limit allows it,
we have to allocate from top down. Hence, we disable the prompt for
RESOURCE_ALLOCATION_TOP_DOWN.
One implication of the changes is that we act differently when a
cold-plugged device reports a prefetchable resource with 32-bit
limit. Before this change, we would fail to allocate the resource.
After this change, it forces everything on the same root port below
the 4G line.
A possible solution to get completely rid of the IORESOURCE_ABOVE_4G
flag would be rules to place resources of certain devices below 4G.
For instance, the primary VGA device and storage and HID devices
could be made available to a payload that can only address 32 bits.
For now, effective_limit() provides us enough abstraction as if the
`limit` would be the only variable to consider. With this, we get
rid of all the special handling of above 4G resources during phase 2
of the allocator. Which saves us about 20% of the code :D
Change-Id: Ia822f0ce648c7f7afc801d9cb00b6459fe7cebea
Signed-off-by: Nico Huber <nico.h(a)gmx.de>
Original-reviewed-on: https://review.coreboot.org/c/coreboot/+/65413
Original-reviewed-by: Arthur Heymans <arthur(a)aheymans.xyz>
Original-reviewed-by: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
---
M src/device/Kconfig
M src/device/resource_allocator_v4.c
2 files changed, 51 insertions(+), 189 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/12/75012/4
--
To view, visit https://review.coreboot.org/c/coreboot/+/75012?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ia822f0ce648c7f7afc801d9cb00b6459fe7cebea
Gerrit-Change-Number: 75012
Gerrit-PatchSet: 4
Gerrit-Owner: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Tim Wawrzynczak <inforichland(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Tim Wawrzynczak <inforichland(a)gmail.com>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-MessageType: newpatchset
Felix Held has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/75611?usp=email )
Change subject: soc/amd/picasso/root_complex: reserve IOMMU MMIO area
......................................................................
soc/amd/picasso/root_complex: reserve IOMMU MMIO area
This makes sure that the resource allocator won't use this address range
for anything else. This should be right below the above 4GB PCI BAR MMIO
region, but better reserve it here so nothing else will get allocated
there.
Signed-off-by: Felix Held <felix-coreboot(a)felixheld.de>
Change-Id: I5a8150873cb019ca1d903ed269e18d6f9fabb871
---
M src/soc/amd/picasso/root_complex.c
1 file changed, 3 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/11/75611/1
diff --git a/src/soc/amd/picasso/root_complex.c b/src/soc/amd/picasso/root_complex.c
index 7a83197..996cd31 100644
--- a/src/soc/amd/picasso/root_complex.c
+++ b/src/soc/amd/picasso/root_complex.c
@@ -147,6 +147,9 @@
gnb_apic->size = 0x00001000;
gnb_apic->flags = IORESOURCE_MEM | IORESOURCE_ASSIGNED | IORESOURCE_FIXED;
+ /* Reserve fixed IOMMU MMIO region */
+ mmio_from_to(dev, idx++, 0xfd00000000, 0xfdffffffff);
+
if (fsp_hob_iterator_init(&hob_iterator) != CB_SUCCESS) {
printk(BIOS_ERR, "%s incomplete because no HOB list was found\n", __func__);
return;
--
To view, visit https://review.coreboot.org/c/coreboot/+/75611?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I5a8150873cb019ca1d903ed269e18d6f9fabb871
Gerrit-Change-Number: 75611
Gerrit-PatchSet: 1
Gerrit-Owner: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-MessageType: newchange