[Qemu-devel] Re: [PATCH v4 3/6] pci: set PCI multi-function bit appropriately.

[Michael S. Tsirkin]

On Mon, Jun 21, 2010 at 03:03:58PM +0900, Isaku Yamahata wrote:
> Set PCI multi-function bit according to multifunction property.
> PCI address, devfn ,is exported to users as addr property,
> so users can populate pci function(PCIDevice in qemu)
> at arbitrary devfn.
> It means each function(PCIDevice) don't know whether pci device
> (PCIDevice[8]) is multi function or not.
> So this patch allows user to set multifunction bit via property
> and checks whether multifunction bit is set correctly.
> 
> Signed-off-by: Isaku Yamahata <address@hidden>

Applying it this way will break bisect.
We also need to handle migration compatibility.
I propose we split it this way:
- patch to add multifunction property (ignored)
- set property in builtin devices where appropriate
- patch to look at property and set bit in header

> ---
> changes v3 -> v4:
> - introduce multifunction property.
> 
> changes v2 -> v3:
> - introduce PCI_FUNC_MAX
> - more commit log
> 
> changes v1 -> v2:
> ---
>  hw/pci.c |   60 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
>  hw/pci.h |    4 ++++
>  2 files changed, 61 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/pci.c b/hw/pci.c
> index b6c0a10..abc3c1d 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -67,6 +67,7 @@ static struct BusInfo pci_bus_info = {
>          DEFINE_PROP_PCI_DEVFN("addr", PCIDevice, devfn, -1),
>          DEFINE_PROP_STRING("romfile", PCIDevice, romfile),
>          DEFINE_PROP_UINT32("rombar",  PCIDevice, rom_bar, 1),
> +        DEFINE_PROP_UINT8("multifunction",  PCIDevice, mf, 0),

Please make this a bit property, not UINT8. It can be stored in
cap_present.

>          DEFINE_PROP_END_OF_LIST()
>      }
>  };
> @@ -575,6 +576,44 @@ static void pci_init_wmask_bridge(PCIDevice *d)
>      pci_set_word(d->wmask + PCI_BRIDGE_CONTROL, 0xffff);
>  }
>  
> +static int pci_init_multifunction(PCIBus *bus, PCIDevice *dev)
> +{

IMO we should just add in pci_register_device:

        if (d->cap_resent & QEMU_PCI_CAP_MULTIFUNCTION) {
                dev->config[PCI_HEADER_TYPE] |= PCI_HEADER_TYPE_MULTI_FUNCTION;
        } else if (PCI_FUNC(dev->devfn)) {
                error_report("PCI: single function device can't be populated 
%x.%x",
                             PCI_SLOT(dev->devfn), PCI_FUNC(dev->devfn));
                return -1;
        }

And be done with it.

> +    uint8_t slot = PCI_SLOT(dev->devfn);
> +    uint8_t func = PCI_FUNC(dev->devfn);
> +
> +    /* we are here before bus->devices[dev->devfn] = dev */
> +    assert(!bus->devices[dev->devfn]);

Can users trigger this?
If yes, this needs and error, not an assert.

> +
> +    if (dev->mf) {
> +        dev->config[PCI_HEADER_TYPE] |= PCI_HEADER_TYPE_MULTI_FUNCTION;
> +    }
> +
> +    if (func) {

Please open-code func above.

> +        PCIDevice *d = bus->devices[PCI_DEVFN(slot, 0)];
> +        if (d && !d->mf) {
> +            /* function 0 should set multifunction bit */
> +            error_report("PCI: single function device can't be populated "
> +                         "in function %x.%x", slot, func);
> +            return -1;
> +        }
> +        return 0;
> +    }
> +
> +    if (dev->mf) {
> +        return 0;
> +    }
> +    /* function 0 indicates single function, so function > 0 must be NULL */


We don't need the below test: each function will be checked
when it is added.

> +    for (func = 1; func < PCI_FUNC_MAX; ++func) {
> +        if (bus->devices[PCI_DEVFN(slot, func)]) {
> +            error_report("PCI: %x.0 indicates single function, "
> +                         "but %x.%x is already populated.",
> +                         slot, slot, func);
> +            return -1;
> +        }
> +    }
> +    return 0;
> +}
> +
>  static void pci_config_alloc(PCIDevice *pci_dev)
>  {
>      int config_size = pci_config_size(pci_dev);
> @@ -629,6 +668,9 @@ static PCIDevice *do_pci_register_device(PCIDevice 
> *pci_dev, PCIBus *bus,
>      if (is_bridge) {
>          pci_init_wmask_bridge(pci_dev);
>      }
> +    if (pci_init_multifunction(bus, pci_dev)) {
> +        return NULL;
> +    }
>  
>      if (!config_read)
>          config_read = pci_default_read_config;
> @@ -1652,22 +1694,34 @@ void pci_qdev_register_many(PCIDeviceInfo *info)
>      }
>  }
>  
> -PCIDevice *pci_create(PCIBus *bus, int devfn, const char *name)
> +PCIDevice *pci_create_mf(PCIBus *bus, int devfn, uint8_t mf, const char 
> *name)
>  {
>      DeviceState *dev;
>  
>      dev = qdev_create(&bus->qbus, name);
>      qdev_prop_set_uint32(dev, "addr", devfn);
> +    qdev_prop_set_uint8(dev, "multifunction", mf);
>      return DO_UPCAST(PCIDevice, qdev, dev);
>  }
>  
> -PCIDevice *pci_create_simple(PCIBus *bus, int devfn, const char *name)
> +PCIDevice *pci_create_simple_mf(PCIBus *bus, int devfn, uint8_t mf,
> +                                const char *name)
>  {
> -    PCIDevice *dev = pci_create(bus, devfn, name);
> +    PCIDevice *dev = pci_create_mf(bus, devfn, mf, name);
>      qdev_init_nofail(&dev->qdev);
>      return dev;
>  }
>  
> +PCIDevice *pci_create(PCIBus *bus, int devfn, const char *name)
> +{
> +    return pci_create_mf(bus, devfn, 0, name);
> +}
> +
> +PCIDevice *pci_create_simple(PCIBus *bus, int devfn, const char *name)
> +{
> +    return pci_create_simple_mf(bus, devfn, 0, name);
> +}
> +
>  static int pci_find_space(PCIDevice *pdev, uint8_t size)
>  {
>      int config_size = pci_config_size(pdev);
> diff --git a/hw/pci.h b/hw/pci.h
> index 76adc66..685fd44 100644
> --- a/hw/pci.h
> +++ b/hw/pci.h
> @@ -131,6 +131,7 @@ struct PCIDevice {
>      /* the following fields are read only */
>      PCIBus *bus;
>      uint32_t devfn;
> +    uint8_t mf;         /* multi function capabile device */

Add a bit in cap_present please.

>      char name[64];
>      PCIIORegion io_regions[PCI_NUM_REGIONS];
>  
> @@ -343,6 +344,9 @@ typedef struct {
>  void pci_qdev_register(PCIDeviceInfo *info);
>  void pci_qdev_register_many(PCIDeviceInfo *info);
>  
> +PCIDevice *pci_create_mf(PCIBus *bus, int devfn, uint8_t mf, const char 
> *name);
> +PCIDevice *pci_create_simple_mf(PCIBus *bus, int devfn, uint8_t mf,
> +                                const char *name);

mf->multifunction

But do we need the extra functions? I thought qdev can handle
the flag?

>  PCIDevice *pci_create(PCIBus *bus, int devfn, const char *name);
>  PCIDevice *pci_create_simple(PCIBus *bus, int devfn, const char *name);
>  
> -- 
> 1.6.6.1