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

[Alex Williamson <alex.williamson@redhat.com>]

Please use different subjects that uniquely identify what each patch
does, don't simply re-use the subject for the cover patch on each.


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 <caoj.fnst@cn.fujitsu.com>
> ---
>  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?

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

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