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

[Alex Williamson]

On Tue, 2015-09-22 at 18:08 +0800, Cao jin wrote:
> 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 would consider it a hack.  You're setting up the device a certain way
to make it appear as if the guest has configured it that way, then
effectively sending the guest a spurious hotplug request for a device
that it theoretically doesn't know about.  If we were to prevent access
to the device, couldn't we remove it directly?

> 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?

If function 0 doesn't exist in the slot, should the guest be able to
perform PCI config accesses to the device?  If the guest cannot do
config cycle accesses to the device, then we know the device is unused
and we don't need to involve the guest in removing 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));
> >>   }
> >>
> >
> >
> > .
> >
>