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

[Isaku Yamahata]

On Wed, Jun 23, 2010 at 12:52:10PM +0300, Michael S. Tsirkin wrote:
> > > > @@ -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.

Personally I'm okay with either way as long as you accept the patch series.

In fact the existing qemu PIIX3/4 sets the bit of only function 0
and doesn't set the bit of function > 0.
- It would be better not to change the existing behavior.
- If all functions in a device are required to set multifunction bit,
  pci ide and ochi usb initialization code must be touched
  for pc and mips malta.

Said that, which way do you want to go?
- The current patches.(v5 9/9)
  My preference.

- require all functions in a device to set multi function bit.
  patch pci ide, ochi usb
  It will result in qemu behavior change.

- require all functions in a device to set multi function bit.
  patch pci ide, ochi usb.
  But try not to chage the existing qemu behavior by using
  pci_clear_multifunction()

-- 
yamahata