Patchwork [09/10] pci: set PCI multi-function bit appropriately.

login
register
mail settings
Submitter Isaku Yamahata
Date June 17, 2010, 6:15 a.m.
Message ID <81d134b49ddbd54e82648618c0ce4e4d11fb77c3.1276755023.git.yamahata@valinux.co.jp>
Download mbox | patch
Permalink /patch/55973/
State New
Headers show

Comments

Isaku Yamahata - June 17, 2010, 6:15 a.m.
set PCI multi-function bit appropriately.

Signed-off-by: Isaku Yamahata <yamahata@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(-)
Michael S. Tsirkin - June 17, 2010, 9:37 a.m.
On Thu, Jun 17, 2010 at 03:15:51PM +0900, Isaku Yamahata wrote:
> set PCI multi-function bit appropriately.
> 
> Signed-off-by: Isaku Yamahata <yamahata@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

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

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

> +    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
Isaku Yamahata - June 18, 2010, 2:40 a.m.
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 <yamahata@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.
But I prefer more flexible and more user friendly way.


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

> 
> > +    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
>
Michael S. Tsirkin - June 18, 2010, 12:44 p.m.
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 <yamahata@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
Isaku Yamahata - June 18, 2010, 1:38 p.m.
On Fri, Jun 18, 2010 at 03:44:04PM +0300, Michael S. Tsirkin wrote:
> > > 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.

Yes, multi function hot plug is also on my wish list.


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

I think "unrelated" is policy. There is no obvious way to determine
which functions can be in a same device.
For example, popular chipset contains isa bridge, ide controller,
usb controller, sound and modem in a single device as functions.
It's up to hardware designer policy which functions are grouped into
a device.

So qemu should be able to populate any function in a device,
and leave such policy check to higher level tool like libvirt/virt-manager.
Michael S. Tsirkin - June 18, 2010, 2:59 p.m.
On Fri, Jun 18, 2010 at 10:38:20PM +0900, Isaku Yamahata wrote:
> On Fri, Jun 18, 2010 at 03:44:04PM +0300, Michael S. Tsirkin wrote:
> > > > 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.
> 
> Yes, multi function hot plug is also on my wish list.
> 
> 
> > 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.
> 
> I think "unrelated" is policy. There is no obvious way to determine
> which functions can be in a same device.
> For example, popular chipset contains isa bridge, ide controller,
> usb controller, sound and modem in a single device as functions.
> It's up to hardware designer policy which functions are grouped into
> a device.
> 
> So qemu should be able to populate any function in a device,
> and leave such policy check to higher level tool like libvirt/virt-manager.

Fine. But tweaking one device's config when another one is added does not
lead to robust code.  Let's just require that users declare devices as
multifunction, and verify this.
Jamie Lokier - June 18, 2010, 3:22 p.m.
Isaku Yamahata wrote:
> On Fri, Jun 18, 2010 at 03:44:04PM +0300, Michael S. Tsirkin wrote:
> > 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.
> 
> I think "unrelated" is policy. There is no obvious way to determine
> which functions can be in a same device.
> For example, popular chipset contains isa bridge, ide controller,
> usb controller, sound and modem in a single device as functions.
> It's up to hardware designer policy which functions are grouped into
> a device.

In hardware terms, quad-port ethernet controllers often present
themselves as a PCI bridge with four independent PCI ethernet
controllers, so they work with standard drivers.  Even though all four
are in a single chip.  Some USB devices do the same, presenting a
small bulk storage device to ship windows drivers [:roll-eyes: ;-)]
alongside the actual device.

-- Jamie
Michael S. Tsirkin - June 20, 2010, 10:03 a.m.
On Fri, Jun 18, 2010 at 10:38:20PM +0900, Isaku Yamahata wrote:
> On Fri, Jun 18, 2010 at 03:44:04PM +0300, Michael S. Tsirkin wrote:
> > > > 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.
> 
> Yes, multi function hot plug is also on my wish list.
> 
> 
> > 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.
> 
> I think "unrelated" is policy. There is no obvious way to determine
> which functions can be in a same device.
> For example, popular chipset contains isa bridge, ide controller,
> usb controller, sound and modem in a single device as functions.
> It's up to hardware designer policy which functions are grouped into
> a device.
> 
> So qemu should be able to populate any function in a device,
> and leave such policy check to higher level tool like libvirt/virt-manager.

Fine. But let's keep it simple: ask user what multifunction bit is,
and not tweak header for device A when device B is added.

> -- 
> yamahata

Patch

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;
+    uint8_t func;
+
+    for (func = 0; func < func_max; ++func) {
+        if (bus->devices[PCI_DEVFN(slot, func)]) {
+            break;
+        }
+    }
+    if (func == func_max) {
+        return;
+    }
+
+    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;
+}
+
 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;