From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56172) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZmEoP-0006ID-P8 for qemu-devel@nongnu.org; Wed, 14 Oct 2015 01:41:18 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZmEoM-0003fR-IC for qemu-devel@nongnu.org; Wed, 14 Oct 2015 01:41:17 -0400 Received: from [59.151.112.132] (port=53712 helo=heian.cn.fujitsu.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZmEoL-0003dV-Ik for qemu-devel@nongnu.org; Wed, 14 Oct 2015 01:41:14 -0400 References: <1444725695-27517-1-git-send-email-caoj.fnst@cn.fujitsu.com> <1444725695-27517-3-git-send-email-caoj.fnst@cn.fujitsu.com> <1444750027.4059.429.camel@redhat.com> From: Cao jin Message-ID: <561DEC4C.3090802@cn.fujitsu.com> Date: Wed, 14 Oct 2015 13:46:52 +0800 MIME-Version: 1.0 In-Reply-To: <1444750027.4059.429.camel@redhat.com> Content-Type: text/plain; charset="utf-8"; format=flowed 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: Alex Williamson Cc: pbonzini@redhat.com, izumi.taku@jp.fujitsu.com, qemu-devel@nongnu.org, mst@redhat.com Hi, Alex On 10/13/2015 11:27 PM, Alex Williamson wrote: > 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. > OK. >> 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)]) { > Ok. variables is intended to make the line shorter. > 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. > Yup, agree. I missed the consideration of bad behavior. I thought anyone use the device should read the vendor ID first(good behavior), then do anything he/she want. Thanks for reminding > 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. > OK > 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. Ok, will consider the "function access window" condition, to see what I can do with it >> 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)]) { > Ok >> + 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) >> { > > > > . > -- Yours Sincerely, Cao Jin