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

[Marcel Apfelbaum]

Hi Laszlo,

On 08/07/2018 06:59 PM, Laszlo Ersek wrote:
> 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().

You are right.  msi_present should be checked.

> > >   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.

I think is possible you miss-interpreted pci_bridge_dev_exitfn
with pci_bridge_exitfn. The first one is the "catch all", the second
one that is used in the error path is for the bridge specific cleanup.

> 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() --,

Right

>   but the latter only has a
> comment, "/* TODO: cleanup config space changes? */".

But also disables the QEMU_PCI_CAP_SLOTID (but no code checks it anyway)
I agree it should call pci_del_capability to delete the SHPC capability,
maybe is some "debt" from early development stages.

>   The same comment
> is present in the slotid_cap_cleanup() function. Given this
> inconsistency,

Here we also miss a call to pci_del_capability.

> 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)?

No, we should remove it properly.

I think it is not considered a "big" issue since adding/removing PCI 
capabilities
is not an operation that deals with resources, we only edit an array  
(the config space)
that will not be used anyway if the device init sequence failed.

That does not mean the code should not be clean.

Thanks,
Marcel

> Thanks
> Laszlo