[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 2/2] remove function during multi-function ho
From: |
Alex Williamson |
Subject: |
Re: [Qemu-devel] [PATCH v3 2/2] remove function during multi-function hot-add |
Date: |
Tue, 13 Oct 2015 09:27:07 -0600 |
On Tue, 2015-10-13 at 16:41 +0800, Cao jin wrote:
> In case user regret when hot-adding multi-function, should roll back,
> device_del the function added but not exposed to the guest.
As Michael suggests, this patch should come first, before we actually
enable multi-function hot-add.
> Signed-off-by: Cao jin <address@hidden>
> ---
> hw/pci/pci_host.c | 6 +++++-
> hw/pci/pcie.c | 22 +++++++++++++++++-----
> 2 files changed, 22 insertions(+), 6 deletions(-)
>
> diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c
> index 3e26f92..35e5cf3 100644
> --- a/hw/pci/pci_host.c
> +++ b/hw/pci/pci_host.c
> @@ -20,6 +20,7 @@
>
> #include "hw/pci/pci.h"
> #include "hw/pci/pci_host.h"
> +#include "hw/pci/pci_bus.h"
> #include "trace.h"
>
> /* debug PCI */
> @@ -88,10 +89,13 @@ void pci_data_write(PCIBus *s, uint32_t addr, uint32_t
> val, int len)
> uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len)
> {
> PCIDevice *pci_dev = pci_dev_find_by_addr(s, addr);
> + PCIDevice *f0 = NULL;
> uint32_t config_addr = addr & (PCI_CONFIG_SPACE_SIZE - 1);
> uint32_t val;
> + uint8_t slot = (addr >> 11) & 0x1F;
>
> - if (!pci_dev) {
> + f0 = s->devices[PCI_DEVFN(slot, 0)];
> + if (!pci_dev || (!f0 && pci_dev)) {
This uses a lot more variables and operations than it needs to:
if (!pci_dev || !s->devices[PCI_DEVFN(PCI_SLOT(pci_dev->devfn), 0)]) {
Shouldn't we do the same on pci_data_write()? A well behaved guest
won't blindly write to config space, but not all guests are well
behaved.
Comments in the code would be nice here to explain that non-zero
functions are only exposed when function zero is present, allowing
direct removal of unexposed devices.
I imagine that due to qemu locking that we don't have a race here, but
note that devices[] is populated early in the core pci realize function,
prior to the device initialize function, and there are any number of
reasons that failure could still occur, which would create a window
where the function is accessible. I doubt this is an issue, but simply
note it for completeness.
> return ~0x0;
> }
>
> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> index 89bf61b..58d2153 100644
> --- a/hw/pci/pcie.c
> +++ b/hw/pci/pcie.c
> @@ -261,13 +261,30 @@ void pcie_cap_slot_hotplug_cb(HotplugHandler
> *hotplug_dev, DeviceState *dev,
> }
> }
>
> +static void pcie_unplug_device(PCIBus *bus, PCIDevice *dev, void *opaque)
> +{
> + object_unparent(OBJECT(dev));
> +}
> +
> void pcie_cap_slot_hot_unplug_request_cb(HotplugHandler *hotplug_dev,
> DeviceState *dev, Error **errp)
> {
> uint8_t *exp_cap;
> + PCIDevice *pci_dev = PCI_DEVICE(dev);
> + PCIBus *bus = pci_dev->bus;
>
> pcie_cap_slot_hotplug_common(PCI_DEVICE(hotplug_dev), dev, &exp_cap,
> errp);
>
> + /* In case user regret when hot-adding multi function, remove the
> function
> + * that is unexposed to guest individually, without interaction with
> guest.
> + */
> + if (PCI_FUNC(pci_dev->devfn) > 0 &&
> + bus->devices[PCI_DEVFN(PCI_SLOT(pci_dev->devfn), 0)] == NULL) {
Similarly,
if (PCI_FUNC(pci_dev->devfn) &&
!bus->devices[PCI_DEVFN(PCI_SLOT(pci_dev->devfn), 0)]) {
> + pcie_unplug_device(bus, pci_dev, NULL);
> +
> + return;
> + }
> +
> pcie_cap_slot_push_attention_button(PCI_DEVICE(hotplug_dev));
> }
>
> @@ -378,11 +395,6 @@ void pcie_cap_slot_reset(PCIDevice *dev)
> hotplug_event_update_event_status(dev);
> }
>
> -static void pcie_unplug_device(PCIBus *bus, PCIDevice *dev, void *opaque)
> -{
> - object_unparent(OBJECT(dev));
> -}
> -
> void pcie_cap_slot_write_config(PCIDevice *dev,
> uint32_t addr, uint32_t val, int len)
> {
Re: [Qemu-devel] [PATCH v3 0/2] PCI-e device multi-function hot-add support, Michael S. Tsirkin, 2015/10/15