[Qemu-devel] Re: [PATCH 09/10] pci: set PCI multi-function bit appropriately.

[Michael S. Tsirkin]

On Fri, Jun 18, 2010 at 11:40:57AM +0900, Isaku Yamahata wrote:
> On Thu, Jun 17, 2010 at 12:37:21PM +0300, Michael S. Tsirkin wrote:
> > On Thu, Jun 17, 2010 at 03:15:51PM +0900, Isaku Yamahata wrote:
> > > set PCI multi-function bit appropriately.
> > > 
> > > Signed-off-by: Isaku Yamahata <address@hidden>
> > > 
> > > ---
> > > changes v1 -> v2:
> > > don't set header type register in configuration space.
> > > ---
> > >  hw/pci.c |   25 +++++++++++++++++++++++++
> > >  1 files changed, 25 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/hw/pci.c b/hw/pci.c
> > > index 5316aa5..ee391dc 100644
> > > --- a/hw/pci.c
> > > +++ b/hw/pci.c
> > > @@ -607,6 +607,30 @@ static void pci_init_wmask_bridge(PCIDevice *d)
> > >      pci_set_word(d->wmask + PCI_BRIDGE_CONTROL, 0xffff);
> > >  }
> > >  
> > > +static void pci_init_multifunction(PCIBus *bus, PCIDevice *dev)
> > > +{
> > > +    uint8_t slot = PCI_SLOT(dev->devfn);
> > > +    uint8_t func_max = 8;
> > 
> > enum or define would be better
> 
> Will fix.
> 
> 
> > > +    uint8_t func;
> > 
> > If I understand correctly what this does, it goes over
> > other functions of the same device, and sets the MULTI_FUNCTION bit
> > for them if there's more than one function.
> > Instead, why don't we just set PCI_HEADER_TYPE_MULTI_FUNCTION
> > in relevant devices?
> 
> 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 I chose to handle it in pci generic layer.
> 
> It can be argued that it's user operation fault and that
> the missing part is validation checks to catch such user errors.

Exactly. Another part that is missing is a way to hotplug
a multifunction device.

OTOH I think that hotplug of separate functions has no chance to work,
so users are better off getting an error.

> But I prefer more flexible and more user friendly way.

I think that most users would only add many functions
to a device as a result of an error.

If we really want the ability to put unrelated devices
as functions in a single one, let's just add
a 'multifunction' qdev property, and validate that
it is set appropriately.

> 
> > > +
> > > +    for (func = 0; func < func_max; ++func) {
> > > +        if (bus->devices[PCI_DEVFN(slot, func)]) {
> > > +            break;
> > > +        }
> > > +    }
> > > +    if (func == func_max) {
> > > +        return;
> > > +    }
> > > +
> > 
> > The above only works if the function is called before
> > device is added to bus.
> 
> That's right. This function is called before bus->devices[devfn] = dev.

This needs a comment.

> > 
> > > +    for (func = 0; func < func_max; ++func) {
> > > +        if (bus->devices[PCI_DEVFN(slot, func)]) {
> > > +            bus->devices[PCI_DEVFN(slot, func)]->config[PCI_HEADER_TYPE] 
> > > |=
> > > +                PCI_HEADER_TYPE_MULTI_FUNCTION;
> > > +        }
> > > +    }
> > > +    dev->config[PCI_HEADER_TYPE] |= PCI_HEADER_TYPE_MULTI_FUNCTION;
> > 
> > Isn't the bit set above already?
> > 
> > > +}
> > > +
> > >  static void pci_config_alloc(PCIDevice *pci_dev)
> > >  {
> > >      int config_size = pci_config_size(pci_dev);
> > > @@ -660,6 +684,7 @@ static PCIDevice *do_pci_register_device(PCIDevice 
> > > *pci_dev, PCIBus *bus,
> > >      if (is_bridge) {
> > >          pci_init_wmask_bridge(pci_dev);
> > >      }
> > > +    pci_init_multifunction(bus, pci_dev);
> > >  
> > >      if (!config_read)
> > >          config_read = pci_default_read_config;
> > > -- 
> > > 1.6.6.1
> > 
> 
> -- 
> yamahata