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

[Laszlo Ersek]

On 08/07/18 14:19, Laszlo Ersek wrote:
> On 08/07/18 09:04, Jing Liu wrote:
>> Add hint to firmware (e.g. SeaBIOS) to reserve addtional
>> IO/MEM/PREF spaces for legacy pci-pci bridge, to enable
>> some pci devices hotplugging whose IO/MEM/PREF spaces
>> requests are larger than the ones in pci-pci bridge set
>> by firmware.
>>
>> Signed-off-by: Jing Liu <address@hidden>
>> ---
>>  hw/pci-bridge/pci_bridge_dev.c | 20 ++++++++++++++++++++
>>  1 file changed, 20 insertions(+)

>> +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().
> 
>>  msi_error:
>>      slotid_cap_cleanup(dev);
>>  slotid_error:

I tried to understand the error handling a bit better. I'm confused.

First, under the label "shpc_error", we call pci_bridge_exitfn(), which
seems to clean up everything (checking individually for each thing to
clean up). Given this, I wonder why we introduced the "slotid_error" and
"msi_error" labels at all. Cascading teardown on the error path, and
invkoing a function that checks everything individually and then tears
it all down, are usually mutually exclusive.

Second, msi_uninit() and shpc_cleanup() are internally inconsistent
between each other. The former removes the respective capability from
config space -- with pci_del_capability() --, but the latter only has a
comment, "/* TODO: cleanup config space changes? */". The same comment
is present in the slotid_cap_cleanup() function. Given this
inconsistency, I don't know what to suggest for the new capability's
teardown, in pci_bridge_dev_exitfn()  -- should we just ignore it (as
suggested by this patch)?

Thanks
Laszlo