Re: [Qemu-devel] [PATCH v2 2/2] PCI-e device multi-function hot-add support

[Cao jin]

Hi Alex

On 09/22/2015 02:00 AM, Alex Williamson wrote:
> Please use different subjects that uniquely identify what each patch
> does, don't simply re-use the subject for the cover patch on each.

OK, will change it in next version.
> On Wed, 2015-09-16 at 10:02 +0800, Cao jin wrote:
> > In case user regret when hot-add multi-function, we should roll back,
> > device_del the function added but still not worked.
> >
> > Signed-off-by: Cao jin <address@hidden>
> > ---
> >   hw/pci/pcie.c | 18 ++++++++++++++++++
> >   1 file changed, 18 insertions(+)
> >
> > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> > index 89bf61b..497f390 100644
> > --- a/hw/pci/pcie.c
> > +++ b/hw/pci/pcie.c
> > @@ -265,9 +265,27 @@ 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);
> >
> > +    /* handle the condition: user hot-add multi function, but regret before
> > +     * finish it, and want to delete the added but not worked function. Fake
> > +     * the condition: the slot is polulated, power indicator is off and power
> > +     * controller is off, so device can be detached when OS write config space.
> > +     */
> > +    if (PCI_FUNC(pci_dev->devfn) > 0 &&
> > +            bus->devices[PCI_DEVFN(0, 0)] == NULL) {
> > +        pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTSTA,
> > +                PCI_EXP_SLTSTA_PDS);
>
> AFAICT, we're only setting this to make pcie_cap_slot_write_config()
> consider this device for being unplugged.  Would it not be cleaner to
> flag the device as unexposed to the guest and also use that flag to
> prevent config reads and writes to the device until function 0 is
> populated, so we know that the guest hasn't interacted with the device?
Yes, set PDS bit here, for the purpose that fake the unplug condition in 
pcie_cap_slot_write_config(), which means, let guest decide when to 
unplug device. So I think setting PDS bit here is necessary, am I right?

I am not quite clear about "flag device as unexposed", does the flag 
means PCI_EXP_SLTSTA_PDS bit, or anything else? Could you give more 
hints about it?
> > +
> > +        pcie_cap_slot_event(PCI_DEVICE(hotplug_dev),
> > +                PCI_EXP_HP_EV_PDC | PCI_EXP_HP_EV_ABP);
>
> Why do we need to test both vs just ABP, which is signaled in the
> existing patch below?

Test the two hotplug event, yes, ABP is enough for device_del. will 
remove PDC in next version.

> > +
> > +        return;
> > +    }
> > +
> >       pcie_cap_slot_push_attention_button(PCI_DEVICE(hotplug_dev));
> >   }
>
>
> .

--
Yours Sincerely,

Cao Jin