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

[Michael S. Tsirkin]

On Wed, Jun 23, 2010 at 07:13:38PM +0900, Isaku Yamahata wrote:
> 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.

I think that your patchset is correct, I'll take it after a bit of review.
I will try to find a bit of time to rearrange the code in pci.c a bit,
but this can come afterwards.

I think it's unfortunate that we need to scan the bus to check
other devices in the same function, but I don't have better ideas.

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