From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46392) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zm1Tu-00082r-Cz for qemu-devel@nongnu.org; Tue, 13 Oct 2015 11:27:15 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zm1Tr-0005OY-5h for qemu-devel@nongnu.org; Tue, 13 Oct 2015 11:27:14 -0400 Received: from mx1.redhat.com ([209.132.183.28]:46637) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zm1Tq-0005OK-Ue for qemu-devel@nongnu.org; Tue, 13 Oct 2015 11:27:11 -0400 Message-ID: <1444750027.4059.429.camel@redhat.com> From: Alex Williamson Date: Tue, 13 Oct 2015 09:27:07 -0600 In-Reply-To: <1444725695-27517-3-git-send-email-caoj.fnst@cn.fujitsu.com> References: <1444725695-27517-1-git-send-email-caoj.fnst@cn.fujitsu.com> <1444725695-27517-3-git-send-email-caoj.fnst@cn.fujitsu.com> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v3 2/2] remove function during multi-function hot-add List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Cao jin Cc: pbonzini@redhat.com, izumi.taku@jp.fujitsu.com, qemu-devel@nongnu.org, mst@redhat.com 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 > --- > 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) > {