[Top][All Lists]

[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: Marcel Apfelbaum
Subject: Re: [Qemu-devel] [PATCH] hw/pci: add pci capability to pci-pci bridge
Date: Sun, 12 Aug 2018 10:11:07 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1

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(+)
+    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.

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


  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

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.



reply via email to

[Prev in Thread] Current Thread [Next in Thread]