Re: [Qemu-devel] [PATCH v5 2/2] enable multi-function hot-add

["Michael S. Tsirkin" <mst@redhat.com>]

On Mon, Oct 26, 2015 at 11:29:18AM +0800, Cao jin wrote:
> Enable pcie device multifunction hot, just ensure the function 0
> added last, then driver will got the notification to scan all the
> function in the slot.
> 
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> ---
>  hw/pci/pci.c         | 31 ++++++++++++++++++++++++++++++-
>  hw/pci/pci_host.c    | 13 +++++++++++--
>  hw/pci/pcie.c        | 18 +++++++++---------
>  include/hw/pci/pci.h |  1 +
>  4 files changed, 51 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index b0bf540..6f43b12 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -847,6 +847,9 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
>      PCIConfigWriteFunc *config_write = pc->config_write;
>      Error *local_err = NULL;
>      AddressSpace *dma_as;
> +    DeviceState *dev = DEVICE(pci_dev);
> +
> +    pci_dev->bus = bus;
>  
>      if (devfn < 0) {
>          for(devfn = bus->devfn_min ; devfn < ARRAY_SIZE(bus->devices);
> @@ -864,9 +867,17 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
>                     PCI_SLOT(devfn), PCI_FUNC(devfn), name,
>                     bus->devices[devfn]->name);
>          return NULL;
> +    } else if (dev->hotplugged &&
> +               pci_get_function_0(pci_dev)) {
> +        error_setg(errp, "PCI: slot %d function 0 already ocuppied by %s,"
> +                   " new func %s cannot be exposed to guest.",
> +                   PCI_SLOT(devfn),
> +                   bus->devices[PCI_DEVFN(PCI_SLOT(devfn), 0)]->name,
> +                   name);
> +
> +       return NULL;
>      }
>  
> -    pci_dev->bus = bus;
>      pci_dev->devfn = devfn;
>      dma_as = pci_device_iommu_address_space(pci_dev);
>  
> @@ -2454,6 +2465,24 @@ void pci_bus_get_w64_range(PCIBus *bus, Range *range)
>      pci_for_each_device_under_bus(bus, pci_dev_get_w64, range);
>  }
>  
> +/* ARI device function number range is 0-255, means has only 1 function0;
> + * while non-ARI device has 1 function0 in each slot. non-ARI device could
> + * be PCI or PCIe, and there is up to 32 slots for PCI */
> +PCIDevice *pci_get_function_0(PCIDevice *pci_dev)
> +{
> +    PCIDevice *parent_dev;
> +
> +    parent_dev = pci_bridge_get_device(pci_dev->bus);
> +    if (pcie_cap_is_arifwd_enabled(parent_dev) &&
> +        pci_find_capability(pci_dev, PCI_EXT_CAP_ID_ARI)) {
> +        /* ARI enabled */
> +        return pci_dev->bus->devices[0];

That's wrong I think since software might enable ARI after hotplug.
And I'm not sure all functions must have the ARI capability.

I don't see why don't you just go by spec:

static
bool pcie_has_upstream_port(PCIDevice *dev)
{
    PCIDevice *parent_dev = pci_bridge_get_device(pci_dev->bus);

    /* 
     * Device associated with an upstream port.
     * As there are several types of these, it's easier to check the
     * parent device: upstream ports are always connected to
     * root or downstream ports.
     */
    return parent_dev &&
	    pci_is_express(parent_dev) &&
		parent_dev->exp.exp_cap &&
		(pcie_cap_get_type(parent_dev) == PCI_EXP_TYPE_ROOT_PORT ||
		pcie_cap_get_type(parent_dev) == PCI_EXP_TYPE_DOWNSTREAM);
}


PCIDevice *pci_get_function_0(PCIDevice *pci_dev)
{
	if (pcie_has_upstream_port(dev)) {
	    /* With an upstream PCIE port, we only support 1 device at slot 0 */
            return pci_dev->bus->devices[0];
	} else {
	    /* Other bus types might support multiple devices at slots 0-31 */
            return pci_dev->bus->devices[PCI_DEVFN(PCI_SLOT(pci_dev->devfn), 0)];
	}
}

> +    } else {
> +        /* no ARI */
> +        return pci_dev->bus->devices[PCI_DEVFN(PCI_SLOT(pci_dev->devfn), 0)];
> +    }
> +}
> +
>  static const TypeInfo pci_device_type_info = {
>      .name = TYPE_PCI_DEVICE,
>      .parent = TYPE_DEVICE,
> diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c
> index 3e26f92..63d7d2f 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 */
> @@ -75,7 +76,11 @@ void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, int len)
>      PCIDevice *pci_dev = pci_dev_find_by_addr(s, addr);
>      uint32_t config_addr = addr & (PCI_CONFIG_SPACE_SIZE - 1);
>  
> -    if (!pci_dev) {
> +    /* non-zero functions are only exposed when function 0 is present,
> +     * allowing direct removal of unexposed functions.
> +     */
> +    if (!pci_dev ||
> +        (pci_dev->qdev.hotplugged && !pci_get_function_0(pci_dev))) {
>          return;
>      }
>  
> @@ -91,7 +96,11 @@ uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len)
>      uint32_t config_addr = addr & (PCI_CONFIG_SPACE_SIZE - 1);
>      uint32_t val;
>  
> -    if (!pci_dev) {
> +    /* non-zero functions are only exposed when function 0 is present,
> +     * allowing direct removal of unexposed functions.
> +     */
> +    if (!pci_dev ||
> +        (pci_dev->qdev.hotplugged && !pci_get_function_0(pci_dev))) {
>          return ~0x0;
>      }
>  
> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> index b1adeaf..4ba9501 100644
> --- a/hw/pci/pcie.c
> +++ b/hw/pci/pcie.c
> @@ -249,16 +249,16 @@ void pcie_cap_slot_hotplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
>          return;
>      }
>  
> -    /* TODO: multifunction hot-plug.
> -     * Right now, only a device of function = 0 is allowed to be
> -     * hot plugged/unplugged.
> +    /* To enable multifunction hot-plug, we just ensure the function
> +     * 0 added last. When function 0 is added, we set the sltsta and
> +     * inform OS via event notification.
>       */
> -    assert(PCI_FUNC(pci_dev->devfn) == 0);
> -
> -    pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTSTA,
> -                               PCI_EXP_SLTSTA_PDS);
> -    pcie_cap_slot_event(PCI_DEVICE(hotplug_dev),
> -                        PCI_EXP_HP_EV_PDC | PCI_EXP_HP_EV_ABP);
> +    if (pci_get_function_0(pci_dev)) {
> +        pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTSTA,
> +                                   PCI_EXP_SLTSTA_PDS);
> +        pcie_cap_slot_event(PCI_DEVICE(hotplug_dev),
> +                            PCI_EXP_HP_EV_PDC | PCI_EXP_HP_EV_ABP);
> +    }
>  }
>  
>  static void pcie_unplug_device(PCIBus *bus, PCIDevice *dev, void *opaque)
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index f5e7fd8..379b6e1 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -397,6 +397,7 @@ void pci_for_each_bus_depth_first(PCIBus *bus,
>                                    void *(*begin)(PCIBus *bus, void *parent_state),
>                                    void (*end)(PCIBus *bus, void *state),
>                                    void *parent_state);
> +PCIDevice *pci_get_function_0(PCIDevice *pci_dev);
>  
>  /* Use this wrapper when specific scan order is not required. */
>  static inline
> -- 
> 2.1.0