Re: [PATCH 01/17] pnv_phb3.c: add unique chassis and slot for pnv_phb3_root_port

From: Daniel Henrique Barboza <danielhb413@gmail.com>
To: "Cédric Le Goater" <clg@kaod.org>, qemu-devel@nongnu.org
Cc: qemu-ppc@nongnu.org, david@gibson.dropbear.id.au
Subject: Re: [PATCH 01/17] pnv_phb3.c: add unique chassis and slot for pnv_phb3_root_port
Date: Tue, 4 Jan 2022 16:33:04 -0300	[thread overview]
Message-ID: <2dd76203-e22f-9142-b94a-df94e1f9b797@gmail.com> (raw)
In-Reply-To: <2a8367ed-d23b-3716-77d0-911cba9ecb74@kaod.org>

On 1/3/22 05:24, Cédric Le Goater wrote:
> On 12/28/21 20:37, Daniel Henrique Barboza wrote:
>> When creating a pnv_phb3_root_port using the command line, the first
>> root port is created successfully, but the second fails with the
>> following error:
>>
>> qemu-system-ppc64: -device pnv-phb3-root-port,bus=phb3-root.0,id=pcie.3:
>> Can't add chassis slot, error -16
>>
>> This error comes from the realize() function of its parent type,
>> rp_realize() from TYPE_PCIE_ROOT_PORT. pcie_chassis_add_slot() fails
>> with -EBUSY if there's an existing PCIESlot that has the same
>> chassis/slot value, regardless of being in a different bus.
>>
>> One way to prevent this error is simply set chassis and slot values in
>> the command line. However, since phb3 root buses only supports a single
>> root port, we can just get an unique chassis/slot value by checking
>> which root bus the pnv_phb3_root_port is going to be attached, get the
>> equivalent phb3 device and use its chip-id and index values, which are
>> guaranteed to be unique.
> 
> I guess parent_realize() will fail if we add 2 root port devices under
> the same phb ?

If we change chassis/slot for each new pci root port the QEMU emulation will
allow it. The problem is with skiboot which, at least according to the commit
that introduced powernv9 support [1], does not support multiple PCIE devices in the
same PHB:

----
No default device layout is provided and PCI devices can be added on
any of the available PCIe Root Port (pcie.0 .. 2 of a Power9 chip)
with address 0x0 as the firwware (skiboot) only accepts a single
device per root port.
----

That said, I'm taking this information at face value. Perhaps this is a test
worth doing to at least document this restriction more explicitly in the
docs.

[1] https://github.com/qemu/qemu/commit/4f9924c4d4cf9c039e247c5cdbbf71bce4e573c3

Thanks,

Daniel

> 
> Thanks,
> 
> C.
> 
> 
>>
>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>> ---
>>   hw/pci-host/pnv_phb3.c | 16 ++++++++++++++++
>>   1 file changed, 16 insertions(+)
>>
>> diff --git a/hw/pci-host/pnv_phb3.c b/hw/pci-host/pnv_phb3.c
>> index 4e2d680d44..130d392b3e 100644
>> --- a/hw/pci-host/pnv_phb3.c
>> +++ b/hw/pci-host/pnv_phb3.c
>> @@ -1156,8 +1156,24 @@ static const TypeInfo pnv_phb3_root_bus_info = {
>>   static void pnv_phb3_root_port_realize(DeviceState *dev, Error **errp)
>>   {
>>       PCIERootPortClass *rpc = PCIE_ROOT_PORT_GET_CLASS(dev);
>> +    PCIDevice *pci = PCI_DEVICE(dev);
>> +    PCIBus *bus = pci_get_bus(pci);
>> +    PnvPHB3 *phb = NULL;
>>       Error *local_err = NULL;
>> +    phb = (PnvPHB3 *) object_dynamic_cast(OBJECT(bus->qbus.parent),
>> +                                          TYPE_PNV_PHB3);
>> +
>> +    if (!phb) {
>> +        error_setg(errp,
>> +"pnv_phb3_root_port devices must be connected to pnv-phb3 buses");
>> +        return;
>> +    }
>> +
>> +    /* Set unique chassis/slot values for the root port */
>> +    qdev_prop_set_uint8(&pci->qdev, "chassis", phb->chip_id);
>> +    qdev_prop_set_uint16(&pci->qdev, "slot", phb->phb_id);
>> +
>>       rpc->parent_realize(dev, &local_err);
>>       if (local_err) {
>>           error_propagate(errp, local_err);
>>
> 