Re: [Qemu-devel] [PATCH v3 2/2] remove function during multi-function hot-add

[Cao jin]

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 <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)]) {
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