John Zhao has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/45608 )
Change subject: soc/intel/xeon_sp: Improve performance efficiencies ......................................................................
soc/intel/xeon_sp: Improve performance efficiencies
Coverity detects performance inefficiencies as IIO_RESOUCE_INSTANCE structure (size 623 bytes) is PASS_BY_VALUE. Fix it with PASS_BY_REFERENCE.
Found-by: Coverity CID 1432759 TEST=None
Signed-off-by: John Zhao john.zhao@intel.com Change-Id: I9ae9ae38fe2c13c5433aa5e1dcbb30ebd30622ab --- M src/soc/intel/xeon_sp/cpx/acpi.c 1 file changed, 6 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/08/45608/1
diff --git a/src/soc/intel/xeon_sp/cpx/acpi.c b/src/soc/intel/xeon_sp/cpx/acpi.c index cd497c5..21ca119 100644 --- a/src/soc/intel/xeon_sp/cpx/acpi.c +++ b/src/soc/intel/xeon_sp/cpx/acpi.c @@ -605,16 +605,16 @@ * in the context of ATSR subtable, it adds ATSR subtable when it is first called. */ static unsigned long acpi_create_dmar_ds_pci_br_for_port(unsigned long current, - int port, int stack, IIO_RESOURCE_INSTANCE iio_resource, uint32_t pcie_seg, + int port, int stack, IIO_RESOURCE_INSTANCE *iio_resource, uint32_t pcie_seg, bool is_atsr, bool *first) {
if (get_stack_for_port(port) != stack) return 0;
- const uint32_t bus = iio_resource.StackRes[stack].BusBase; - const uint32_t dev = iio_resource.PcieInfo.PortInfo[port].Device; - const uint32_t func = iio_resource.PcieInfo.PortInfo[port].Function; + const uint32_t bus = iio_resource->StackRes[stack].BusBase; + const uint32_t dev = iio_resource->PcieInfo.PortInfo[port].Device; + const uint32_t func = iio_resource->PcieInfo.PortInfo[port].Function;
const uint32_t id = pci_mmio_read_config32(PCI_DEV(bus, dev, func), PCI_VENDOR_ID); @@ -707,7 +707,7 @@ hob->PlatformData.IIO_resource[socket]; for (int p = PORT_0; p < MAX_PORTS; ++p) current += acpi_create_dmar_ds_pci_br_for_port(current, p, stack, - iio_resource, pcie_seg, false, NULL); + &iio_resource, pcie_seg, false, NULL);
// Add VMD if (hob->PlatformData.VMDStackEnable[socket][stack] && @@ -771,7 +771,7 @@ if (socket == 0 && p == PORT_0) continue; current += acpi_create_dmar_ds_pci_br_for_port(current, p, - stack, iio_resource, pcie_seg, true, &first); + stack, &iio_resource, pcie_seg, true, &first); } } if (tmp != current)
Marc Jones has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45608 )
Change subject: soc/intel/xeon_sp: Improve performance efficiencies ......................................................................
Patch Set 1: Code-Review+1
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45608 )
Change subject: soc/intel/xeon_sp: Improve performance efficiencies ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45608/1/src/soc/intel/xeon_sp/cpx/a... File src/soc/intel/xeon_sp/cpx/acpi.c:
https://review.coreboot.org/c/coreboot/+/45608/1/src/soc/intel/xeon_sp/cpx/a... PS1, Line 608: IIO_RESOURCE_INSTANCE iio_resource isn't modified, it should be const
Hello Marc Jones, build bot (Jenkins), Tim Wawrzynczak, Angel Pons, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45608
to look at the new patch set (#2).
Change subject: soc/intel/xeon_sp: Improve performance efficiencies ......................................................................
soc/intel/xeon_sp: Improve performance efficiencies
Coverity detects performance inefficiencies as IIO_RESOUCE_INSTANCE structure (size 623 bytes) is PASS_BY_VALUE. Fix it with PASS_BY_REFERENCE.
Found-by: Coverity CID 1432759 TEST=None
Signed-off-by: John Zhao john.zhao@intel.com Change-Id: I9ae9ae38fe2c13c5433aa5e1dcbb30ebd30622ab --- M src/soc/intel/xeon_sp/cpx/acpi.c 1 file changed, 8 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/08/45608/2
John Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45608 )
Change subject: soc/intel/xeon_sp: Improve performance efficiencies ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45608/1/src/soc/intel/xeon_sp/cpx/a... File src/soc/intel/xeon_sp/cpx/acpi.c:
https://review.coreboot.org/c/coreboot/+/45608/1/src/soc/intel/xeon_sp/cpx/a... PS1, Line 608: IIO_RESOURCE_INSTANCE
iio_resource isn't modified, it should be const
Ack
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45608 )
Change subject: soc/intel/xeon_sp: Improve performance efficiencies ......................................................................
Patch Set 2: Code-Review+2
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45608 )
Change subject: soc/intel/xeon_sp: Improve performance efficiencies ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/45608/2/src/soc/intel/xeon_sp/cpx/a... File src/soc/intel/xeon_sp/cpx/acpi.c:
https://review.coreboot.org/c/coreboot/+/45608/2/src/soc/intel/xeon_sp/cpx/a... PS2, Line 706: iio_resource then this can be a pointer, and you can just pass `iio_resource` on line 710
https://review.coreboot.org/c/coreboot/+/45608/2/src/soc/intel/xeon_sp/cpx/a... PS2, Line 751: const IIO_RESOURCE_INSTANCE iio_resource = same here, create a pointer (this still makes a copy), then lines 755/756 change too
John Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45608 )
Change subject: soc/intel/xeon_sp: Improve performance efficiencies ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45608/2/src/soc/intel/xeon_sp/cpx/a... File src/soc/intel/xeon_sp/cpx/acpi.c:
https://review.coreboot.org/c/coreboot/+/45608/2/src/soc/intel/xeon_sp/cpx/a... PS2, Line 706: iio_resource
then this can be a pointer, and you can just pass `iio_resource` on line 710
It seems not necessary since it would need to allocate and free resource for iio_resource.
John Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45608 )
Change subject: soc/intel/xeon_sp: Improve performance efficiencies ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45608/2/src/soc/intel/xeon_sp/cpx/a... File src/soc/intel/xeon_sp/cpx/acpi.c:
https://review.coreboot.org/c/coreboot/+/45608/2/src/soc/intel/xeon_sp/cpx/a... PS2, Line 751: const IIO_RESOURCE_INSTANCE iio_resource =
same here, create a pointer (this still makes a copy), then lines 755/756 change too
same reason as above statement.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45608 )
Change subject: soc/intel/xeon_sp: Improve performance efficiencies ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45608/2/src/soc/intel/xeon_sp/cpx/a... File src/soc/intel/xeon_sp/cpx/acpi.c:
https://review.coreboot.org/c/coreboot/+/45608/2/src/soc/intel/xeon_sp/cpx/a... PS2, Line 706: iio_resource
It seems not necessary since it would need to allocate and free resource for iio_resource.
Right now, `iio_resource` is copied from `hob->PlatformData.IIO_resource[socket]`. There are 2 ways to avoid the copy: ``` const IIO_RESOURCE_INSTANCE *iio_resource = &hob->PlatformData.IIO_resource[socket]; for (int p = PORT_0; p < MAX_PORTS; ++p) current += acpi_create_dmar_ds_pci_br_for_port(current, p, stack, iio_resource, pcie_seg, false, NULL); ``` or you could do this: ``` for (int p = PORT_0; p < MAX_PORTS; ++p) current += acpi_create_dmar_ds_pci_br_for_port(current, p, stack, &hob->PlatformData.IIO_resource[socket], pcie_seg, false, NULL); ```
Hello build bot (Jenkins), Marc Jones, Jonathan Zhang, Tim Wawrzynczak, Angel Pons, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45608
to look at the new patch set (#3).
Change subject: soc/intel/xeon_sp: Improve performance efficiencies ......................................................................
soc/intel/xeon_sp: Improve performance efficiencies
Coverity detects performance inefficiencies as IIO_RESOUCE_INSTANCE structure (size 623 bytes) is PASS_BY_VALUE. Fix it with PASS_BY_REFERENCE.
Found-by: Coverity CID 1432759 TEST=None
Signed-off-by: John Zhao john.zhao@intel.com Change-Id: I9ae9ae38fe2c13c5433aa5e1dcbb30ebd30622ab --- M src/soc/intel/xeon_sp/cpx/acpi.c 1 file changed, 13 insertions(+), 13 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/08/45608/3
John Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45608 )
Change subject: soc/intel/xeon_sp: Improve performance efficiencies ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45608/2/src/soc/intel/xeon_sp/cpx/a... File src/soc/intel/xeon_sp/cpx/acpi.c:
https://review.coreboot.org/c/coreboot/+/45608/2/src/soc/intel/xeon_sp/cpx/a... PS2, Line 706: iio_resource
Right now, `iio_resource` is copied from `hob->PlatformData.IIO_resource[socket]`. […]
done.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45608 )
Change subject: soc/intel/xeon_sp: Improve performance efficiencies ......................................................................
Patch Set 3: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/45608 )
Change subject: soc/intel/xeon_sp: Improve performance efficiencies ......................................................................
soc/intel/xeon_sp: Improve performance efficiencies
Coverity detects performance inefficiencies as IIO_RESOUCE_INSTANCE structure (size 623 bytes) is PASS_BY_VALUE. Fix it with PASS_BY_REFERENCE.
Found-by: Coverity CID 1432759 TEST=None
Signed-off-by: John Zhao john.zhao@intel.com Change-Id: I9ae9ae38fe2c13c5433aa5e1dcbb30ebd30622ab Reviewed-on: https://review.coreboot.org/c/coreboot/+/45608 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/soc/intel/xeon_sp/cpx/acpi.c 1 file changed, 13 insertions(+), 13 deletions(-)
Approvals: build bot (Jenkins): Verified Tim Wawrzynczak: Looks good to me, approved
diff --git a/src/soc/intel/xeon_sp/cpx/acpi.c b/src/soc/intel/xeon_sp/cpx/acpi.c index cd497c5..1b6f1a3 100644 --- a/src/soc/intel/xeon_sp/cpx/acpi.c +++ b/src/soc/intel/xeon_sp/cpx/acpi.c @@ -605,16 +605,16 @@ * in the context of ATSR subtable, it adds ATSR subtable when it is first called. */ static unsigned long acpi_create_dmar_ds_pci_br_for_port(unsigned long current, - int port, int stack, IIO_RESOURCE_INSTANCE iio_resource, uint32_t pcie_seg, + int port, int stack, const IIO_RESOURCE_INSTANCE *iio_resource, uint32_t pcie_seg, bool is_atsr, bool *first) {
if (get_stack_for_port(port) != stack) return 0;
- const uint32_t bus = iio_resource.StackRes[stack].BusBase; - const uint32_t dev = iio_resource.PcieInfo.PortInfo[port].Device; - const uint32_t func = iio_resource.PcieInfo.PortInfo[port].Function; + const uint32_t bus = iio_resource->StackRes[stack].BusBase; + const uint32_t dev = iio_resource->PcieInfo.PortInfo[port].Device; + const uint32_t func = iio_resource->PcieInfo.PortInfo[port].Function;
const uint32_t id = pci_mmio_read_config32(PCI_DEV(bus, dev, func), PCI_VENDOR_ID); @@ -703,8 +703,8 @@
// Add PCIe Ports if (socket != 0 || stack != CSTACK) { - IIO_RESOURCE_INSTANCE iio_resource = - hob->PlatformData.IIO_resource[socket]; + const IIO_RESOURCE_INSTANCE *iio_resource = + &hob->PlatformData.IIO_resource[socket]; for (int p = PORT_0; p < MAX_PORTS; ++p) current += acpi_create_dmar_ds_pci_br_for_port(current, p, stack, iio_resource, pcie_seg, false, NULL); @@ -748,12 +748,12 @@ uint32_t pcie_seg = hob->PlatformData.CpuQpiInfo[socket].PcieSegment; unsigned long tmp = current; bool first = true; - IIO_RESOURCE_INSTANCE iio_resource = - hob->PlatformData.IIO_resource[socket]; + const IIO_RESOURCE_INSTANCE *iio_resource = + &hob->PlatformData.IIO_resource[socket];
for (int stack = 0; stack <= PSTACK2; ++stack) { - uint32_t bus = iio_resource.StackRes[stack].BusBase; - uint32_t vtd_base = iio_resource.StackRes[stack].VtdBarAddress; + uint32_t bus = iio_resource->StackRes[stack].BusBase; + uint32_t vtd_base = iio_resource->StackRes[stack].VtdBarAddress; if (!vtd_base) continue; uint64_t vtd_mmio_cap = read64((void *)(vtd_base + VTD_EXT_CAP_LOW)); @@ -821,10 +821,10 @@ assert(hob != NULL && hob_size != 0);
for (int socket = 0; socket < hob->PlatformData.numofIIO; ++socket) { - IIO_RESOURCE_INSTANCE iio_resource = - hob->PlatformData.IIO_resource[socket]; + IIO_RESOURCE_INSTANCE *iio_resource = + &hob->PlatformData.IIO_resource[socket]; for (int stack = 0; stack <= PSTACK2; ++stack) { - uint32_t vtd_base = iio_resource.StackRes[stack].VtdBarAddress; + uint32_t vtd_base = iio_resource->StackRes[stack].VtdBarAddress; if (!vtd_base) continue;
Subrata Banik has created a revert of this change. ( https://review.coreboot.org/c/coreboot/+/45608 )
Change subject: soc/intel/xeon_sp: Improve performance efficiencies ......................................................................
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45608 )
Change subject: soc/intel/xeon_sp: Improve performance efficiencies ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/c/coreboot/+/45608/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45608/4//COMMIT_MSG@9 PS4, Line 9: IIO_RESOUCE_INSTANCE RESOURCE
https://review.coreboot.org/c/coreboot/+/45608/4//COMMIT_MSG@11 PS4, Line 11: PASS_BY_REFERENCE. Maybe mention, that the performance difference probably does not matter for coreboot.