On Wed, Jun 23, 2010 at 04:25:20PM +0900, Isaku Yamahata wrote:
> 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 <yamah...@valinux.co.jp>
> >
> > 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.
This is correct:
The order in which configuration software probes devices residing on a
bus segment is not specified. Typically, configuration software either
starts with Device Number 0 and works up or starts at Device Number 31
and works down. If a single function device is detected (i.e., bit 7 in
the Header Type register of function 0 is 0), no more functions for that
Device Number will be checked. If a multi-function device is detected
(i.e., bit 7 in the Header Type register of function 0 is 1), then all
remaining Function Numbers will be checked.
So what my proposal would do is set the bit for all functions.
I don't think it matters - do you?
If you want to try and match the behaviour you observe
in actual hardware exactly, we can add
/* Some devices only set multifunction status bit in function 0. */
static void pci_clear_multifunction(...) {
if (PCI_FUNC(dev->devfn))
dev->config[PCI_HEADER_TYPE] &=
~PCI_HEADER_TYPE_MULTI_FUNCTION;
}
and devices can call this in their init routine.
> >
> > > + 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
