[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] hw/pci: add pci capability to pci-pci bridge
From: |
Laszlo Ersek |
Subject: |
Re: [Qemu-devel] [PATCH] hw/pci: add pci capability to pci-pci bridge |
Date: |
Tue, 7 Aug 2018 17:59:20 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 |
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
Re: [Qemu-devel] [PATCH] hw/pci: add pci capability to pci-pci bridge, Marcel Apfelbaum, 2018/08/11
Re: [Qemu-devel] [PATCH] hw/pci: add pci capability to pci-pci bridge, Liu, Jing2, 2018/08/14