Hi,

On Fri, Dec 02, 2022 at 08:38:37PM +0100, sean@geanix.com wrote:

Quoting Pierre-Clément Tosi ptosi@google.com:

...

Add a check for calloc() failing to allocate the requested memory.

Make decode_regions() return an error code.

Cc: Bin Meng bmeng.cn@gmail.com Cc: Simon Glass sjg@chromium.org Cc: Stefan Roese sr@denx.de Signed-off-by: Pierre-Clément Tosi ptosi@google.com

---

Hi,

We upgraded from v2022.04 -> v2022.10, and this PATCH breaks PCI (scsi/sata) support for x86_64. I have seen this in qemu, i havn't had the time to test this on real hardware.

Steps to reproduce: $ make efi-x86_payload64_defconfig && make -j32 && scripts/build-efi.sh -sPrp

Decompiling the resulting u-boot.dtb shows

pci { compatible = "pci-x86"; u-boot,dm-pre-reloc; };

which isn't supported by the patch as we return -EINVAL when missing "ranges". The rationale here was that the property seemed mandatory (see [1] and [2]); was this a misunderstanding of potential corner cases?

OTOH, I see that most DTS using "pci-x86" (a pseudo-device intended to act as a PCI bus) actually provide "ranges". In particular, a comment added by 0ced70a0bb7a ("x86: tpl: Add a fake PCI bus") states that

The BARs are allocated statically in the device tree.

So I'll let others comment on this but either:

- arch/x86/dts/efi-x86_payload.dts (and a few other DTS) is missing "ranges"; or - pci_uclass_pre_probe() should treat UCLASS_SIMPLE_BUS differently.

[1]: https://www.kernel.org/doc/Documentation/devicetree/bindings/pci/host-generi... [2]: IEEE Std 1275-1994

Br, /Sean

...

drivers/pci/pci-uclass.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c index 970ee1adf1..2c85e78a13 100644 --- a/drivers/pci/pci-uclass.c +++ b/drivers/pci/pci-uclass.c @@ -954,7 +954,7 @@ int pci_bind_bus_devices(struct udevice *bus) return 0; }

-static void decode_regions(struct pci_controller *hose, ofnode parent_node, +static int decode_regions(struct pci_controller *hose, ofnode parent_node, ofnode node) { int pci_addr_cells, addr_cells, size_cells; @@ -968,7 +968,7 @@ static void decode_regions(struct pci_controller *hose, ofnode parent_node, prop = ofnode_get_property(node, "ranges", &len); if (!prop) { debug("%s: Cannot decode regions\n", __func__);

• return;
  

• return -EINVAL;
  

  }

  pci_addr_cells = ofnode_read_simple_addr_cells(node);

@@ -986,6 +986,8 @@ static void decode_regions(struct pci_controller *hose, ofnode parent_node, max_regions = len / cells_per_record + CONFIG_NR_DRAM_BANKS; hose->regions = (struct pci_region *) calloc(1, max_regions * sizeof(struct pci_region));

• if (!hose->regions)

• return -ENOMEM;
  
  

  for (i = 0; i < max_regions; i++, len -= cells_per_record) { u64 pci_addr, addr, size;

@@ -1053,7 +1055,7 @@ static void decode_regions(struct pci_controller *hose, ofnode parent_node, /* Add a region for our local memory */ bd = gd->bd; if (!bd)

• return;
  

• return 0;
  
  

  for (i = 0; i < CONFIG_NR_DRAM_BANKS; ++i) { if (bd->bi_dram[i].size) {

@@ -1068,7 +1070,7 @@ static void decode_regions(struct pci_controller *hose, ofnode parent_node, } }

• return;

• return 0;

}

static int pci_uclass_pre_probe(struct udevice *bus) @@ -1097,7 +1099,10 @@ static int pci_uclass_pre_probe(struct udevice *bus) /* For bridges, use the top-level PCI controller */ if (!device_is_on_pci_bus(bus)) { hose->ctlr = bus;

• decode_regions(hose, dev_ofnode(bus->parent), dev_ofnode(bus));
  

• ret = decode_regions(hose, dev_ofnode(bus->parent),
  

• 		     dev_ofnode(bus));
  

• if (ret)
  

• 	return ret;
  

  } else { struct pci_controller *parent_hose;

-- 2.36.1.124.g0e6072fb45-goog

Hi,

On Tue, Mar 21, 2023 at 08:57:18AM +0100, Christian Gmeiner wrote:

Am So., 4. Dez. 2022 um 22:22 Uhr schrieb Pierre-Clément Tosi ptosi@google.com:

...

Hi,

On Fri, Dec 02, 2022 at 08:38:37PM +0100, sean@geanix.com wrote:

...

Quoting Pierre-Clément Tosi ptosi@google.com:

...

Add a check for calloc() failing to allocate the requested memory.

Make decode_regions() return an error code.

Cc: Bin Meng bmeng.cn@gmail.com Cc: Simon Glass sjg@chromium.org Cc: Stefan Roese sr@denx.de Signed-off-by: Pierre-Clément Tosi ptosi@google.com

---

Hi,

We upgraded from v2022.04 -> v2022.10, and this PATCH breaks PCI (scsi/sata) support for x86_64. I have seen this in qemu, i havn't had the time to test this on real hardware.

Steps to reproduce: $ make efi-x86_payload64_defconfig && make -j32 && scripts/build-efi.sh -sPrp

Breaks my use case too and is basically a revert of https://source.denx.de/u-boot/u-boot/-/commit/f2825f6ec0bb50e7bd9376828a3221... In my use case we are using coreboot for different Intel SoCs with a generic U-Boot payload.

...

Decompiling the resulting u-boot.dtb shows

pci {
        compatible = "pci-x86";
        u-boot,dm-pre-reloc;
};

Yes.. that is what can be found here: https://source.denx.de/u-boot/u-boot/-/blob/master/arch/x86/dts/coreboot.dts...

...

which isn't supported by the patch as we return -EINVAL when missing "ranges". The rationale here was that the property seemed mandatory (see [1] and [2]); was this a misunderstanding of potential corner cases?

At the moment I would like to revert this change.

Thanks for sharing such a corner case.

IMO, normal operation shouldn't rely on errors being silently skipped because return values are being blindly ignored. Instead, we could limit EINVAL to libfdt errors that aren't FDT_ERR_NOTFOUND, which would address your use-case, where "ranges" is missing from the DT node.

Is your issue fixed by this patch:

https://patchwork.ozlabs.org/project/uboot/patch/20230220194927.476708-8-sjg...

?

...

OTOH, I see that most DTS using "pci-x86" (a pseudo-device intended to act as a PCI bus) actually provide "ranges". In particular, a comment added by 0ced70a0bb7a ("x86: tpl: Add a fake PCI bus") states that

The BARs are allocated statically in the device tree.

So I'll let others comment on this but either:

• arch/x86/dts/efi-x86_payload.dts (and a few other DTS) is missing "ranges"; or
• pci_uclass_pre_probe() should treat UCLASS_SIMPLE_BUS differently.

[2]: IEEE Std 1275-1994

...

Br, /Sean

...

drivers/pci/pci-uclass.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c index 970ee1adf1..2c85e78a13 100644 --- a/drivers/pci/pci-uclass.c +++ b/drivers/pci/pci-uclass.c @@ -954,7 +954,7 @@ int pci_bind_bus_devices(struct udevice *bus) return 0; }

-static void decode_regions(struct pci_controller *hose, ofnode parent_node, +static int decode_regions(struct pci_controller *hose, ofnode parent_node, ofnode node) { int pci_addr_cells, addr_cells, size_cells; @@ -968,7 +968,7 @@ static void decode_regions(struct pci_controller *hose, ofnode parent_node, prop = ofnode_get_property(node, "ranges", &len); if (!prop) { debug("%s: Cannot decode regions\n", __func__);

•       return;
  

•       return -EINVAL;
  

  }

  pci_addr_cells = ofnode_read_simple_addr_cells(node);

@@ -986,6 +986,8 @@ static void decode_regions(struct pci_controller *hose, ofnode parent_node, max_regions = len / cells_per_record + CONFIG_NR_DRAM_BANKS; hose->regions = (struct pci_region *) calloc(1, max_regions * sizeof(struct pci_region));

• if (!hose->regions)

•       return -ENOMEM;
  
  

  for (i = 0; i < max_regions; i++, len -= cells_per_record) { u64 pci_addr, addr, size;

@@ -1053,7 +1055,7 @@ static void decode_regions(struct pci_controller *hose, ofnode parent_node, /* Add a region for our local memory */ bd = gd->bd; if (!bd)

•       return;
  

•       return 0;
  
  

  for (i = 0; i < CONFIG_NR_DRAM_BANKS; ++i) { if (bd->bi_dram[i].size) {

@@ -1068,7 +1070,7 @@ static void decode_regions(struct pci_controller *hose, ofnode parent_node, } }

• return;

• return 0;

}

static int pci_uclass_pre_probe(struct udevice *bus) @@ -1097,7 +1099,10 @@ static int pci_uclass_pre_probe(struct udevice *bus) /* For bridges, use the top-level PCI controller */ if (!device_is_on_pci_bus(bus)) { hose->ctlr = bus;

•       decode_regions(hose, dev_ofnode(bus->parent), dev_ofnode(bus));
  

•       ret = decode_regions(hose, dev_ofnode(bus->parent),
  

•                            dev_ofnode(bus));
  

•       if (ret)
  

•               return ret;
  

  } else { struct pci_controller *parent_hose;

-- 2.36.1.124.g0e6072fb45-goog

-- Pierre

-- greets -- Christian Gmeiner, MSc

https://christian-gmeiner.info/privacypolicy

Thanks