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
