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

[Isaku Yamahata]

On Mon, Jun 21, 2010 at 03:36:00PM +0300, Michael S. Tsirkin wrote:
> 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

Done.

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

Unfortunately there are two ways to set the bit.
- set the bit of all the function.
  Example: Intel X58(north bridge.)
- set the bit of only function = 0.
  Example: PIIX3, PIIX4, ... ICH10.

lspci -x would help to see what your pc has.


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

If the device tree was fully created via qdev from configuration file,
that would be correct.
However the  conversion isn't completed, so convenient functions
are exported to the code which creates the device tree.
Please notice that devfn here is also property.

While I could add one more parameter to pci_create_simple(),
I would have to touch many callers.

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

-- 
yamahata