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 <yamah...@valinux.co.jp>
> > >
> > > ---
> > > 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
