Re: [Qemu-devel] [PATCH] hw/pci: add pci capability to pci-pci bridge

[Laszlo Ersek]

On 08/14/18 10:39, Liu, Jing2 wrote:
> Hi Laszlo,
> Sorry for late reply. I missed these mails because of wrong filter.
> And thanks very much for comments. My reply as belows.
> 
> On 8/7/2018 8:19 PM, Laszlo Ersek wrote:
>> On 08/07/18 09:04, Jing Liu wrote:
> [...]
>>> @@ -46,6 +46,12 @@ struct PCIBridgeDev {
>>>       uint32_t flags;
>>>         OnOffAuto msi;
>>> +
>>> +    /* additional resources to reserve on firmware init */
>>> +    uint64_t io_reserve;
>>> +    uint64_t mem_reserve;
>>> +    uint64_t pref32_reserve;
>>> +    uint64_t pref64_reserve;
>>>   };
>>>   typedef struct PCIBridgeDev PCIBridgeDev;
>>
>> (1) Please evaluate the following idea:
>>
> This is really good and I'm only not sure if DEFINE_PROP_ like,
> DEFINE_PROP_UINT32("bus-reserve", GenPCIERootPort,
> res_reserve.bus_reserve, -1), is a right/good way?
> I mean the third parameter "_f".

Yes, I think you can refer to members within structure fields like that.

> 
> [...]
>>
>> - gen_rp_props should be updated accordingly. The property names should
>> stay the same, but they should reference fields of the "res_reserve"
>> field.
>>
>> (2) Then, in a separate patch, you can add another "res_reserve" field
>> to "PCIBridgeDev". (As a consequence, "bus_reserve" will also be added,
>> which I think is the right thing to do.)
>>
> I consider whether we need limit the bus_reserve of pci-pci bridge. For
> old codes, we could add "unlimited" numbers of devices (but less than
> than max) onto pci-pci bridge.

In theory the bus_reserve hint makes sense too. There likely isn't a
great use case for it in practice, but that's not reason enough IMO to
diverge in the implementation (the factored-out structure and the
properties). It's certainly not wrong to offer the property, IMO.


> 
>> (3) There is a class that is derived from TYPE_PCI_BRIDGE_DEV:
>> TYPE_PCI_BRIDGE_SEAT_DEV. Is it correct to add the new properties /
>> fields to the latter as well?
>>
> Umm, I looked into the codes that doesn't have hotplug property?
> So do we need add resource reserve for it?

No clue. I don't know anything about TYPE_PCI_BRIDGE_SEAT_DEV; I'll have
to defer to Marcel and others on that.

> 
>>>   @@ -95,6 +101,13 @@ static void pci_bridge_dev_realize(PCIDevice
>>> *dev, Error **errp)
>>>           error_free(local_err);
>>>       }
>>>   +    err = pci_bridge_qemu_reserve_cap_init(dev, 0, 0,
>>> +          bridge_dev->io_reserve, bridge_dev->mem_reserve,
>>> +          bridge_dev->pref32_reserve, bridge_dev->pref64_reserve,
>>> errp);
>>> +    if (err) {
>>> +        goto cap_error;
>>> +    }
>>> +
>>>       if (shpc_present(dev)) {
>>>           /* TODO: spec recommends using 64 bit prefetcheable BAR.
>>>            * Check whether that works well. */
>>> @@ -103,6 +116,8 @@ static void pci_bridge_dev_realize(PCIDevice
>>> *dev, Error **errp)
>>>       }
>>>       return;
>>>   +cap_error:
>>> +    msi_uninit(dev);
>>
>> (4) This error handler doesn't look entirely correct; we can reach it
>> without having initialized MSI. (MSI setup is conditional; and even if
>> we attempt it, it is permitted to fail with "msi=auto".) Therefore,
>> msi_uninit() should be wrapped into "if (msi_present())", same as you
>> see in pci_bridge_dev_exitfn().
> 
> sure, can add a pre-check. But I don't understand why we need that,
> msi_uninit() will check msi_present()?

Thanks for pointing that out.

So, I grepped the source code for msi_uninit(). It seems that only one
location gates it with msi_present() -- in pci_bridge_dev_exitfn() --,
while 13 other locations just call it unconditionally.

I don't know what the consistent approach is here. I'll have to defer to
the PCI maintainers; their taste will decide. Technically your code
looks correct, then.

Thanks
Laszlo