[Qemu-devel] [PATCH] pci/pcie: make pci_find_device() ARI aware.

[Qemu-devel] [PATCH] pci/pcie: make pci_find_device() ARI aware.

[Isaku Yamahata]

make pci_find_device() ARI aware.

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
 hw/pci.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/hw/pci.c b/hw/pci.c
index 8d0e3df..851f350 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -1596,11 +1596,18 @@ PCIBus *pci_find_bus(PCIBus *bus, int bus_num)
 
 PCIDevice *pci_find_device(PCIBus *bus, int bus_num, int slot, int function)
 {
+    PCIDevice *d;
     bus = pci_find_bus(bus, bus_num);
 
     if (!bus)
         return NULL;
 
+    d = bus->parent_dev;
+    if (d && pci_is_express(d) &&
+        pcie_cap_get_type(d) == PCI_EXP_TYPE_DOWNSTREAM &&
+        !pcie_cap_is_ari_enabled(d) && slot > 0) {
+        return NULL;
+    }
     return bus->devices[PCI_DEVFN(slot, function)];
 }
 
-- 
1.7.1.1

[Qemu-devel] Re: [PATCH] pci/pcie: make pci_find_device() ARI aware.

[Michael S. Tsirkin]

On Thu, Jan 20, 2011 at 03:57:39PM +0900, Isaku Yamahata wrote:
> make pci_find_device() ARI aware.
> 
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> ---
>  hw/pci.c |    7 +++++++
>  1 files changed, 7 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/pci.c b/hw/pci.c
> index 8d0e3df..851f350 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -1596,11 +1596,18 @@ PCIBus *pci_find_bus(PCIBus *bus, int bus_num)
>  
>  PCIDevice *pci_find_device(PCIBus *bus, int bus_num, int slot, int function)
>  {
> +    PCIDevice *d;
>      bus = pci_find_bus(bus, bus_num);
>  
>      if (!bus)
>          return NULL;
>  
> +    d = bus->parent_dev;
> +    if (d && pci_is_express(d) &&
> +        pcie_cap_get_type(d) == PCI_EXP_TYPE_DOWNSTREAM &&
> +        !pcie_cap_is_ari_enabled(d) && slot > 0) {
> +        return NULL;
> +    }
>      return bus->devices[PCI_DEVFN(slot, function)];
>  }

I'd like to split this express-specific code out in some way.
Further, the downstream port always has a single slot.
Maybe it shouldn't be a bus at all, but this needs some thought.
How about we put flag in PCIBus that says that a single
slot is supported? Downstream ports would just set it.

> -- 
> 1.7.1.1

[Qemu-devel] Re: [PATCH] pci/pcie: make pci_find_device() ARI aware.

[Isaku Yamahata]

On Thu, Jan 20, 2011 at 04:15:48PM +0200, Michael S. Tsirkin wrote:
> On Thu, Jan 20, 2011 at 03:57:39PM +0900, Isaku Yamahata wrote:
> > make pci_find_device() ARI aware.
> > 
> > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> > ---
> >  hw/pci.c |    7 +++++++
> >  1 files changed, 7 insertions(+), 0 deletions(-)
> > 
> > diff --git a/hw/pci.c b/hw/pci.c
> > index 8d0e3df..851f350 100644
> > --- a/hw/pci.c
> > +++ b/hw/pci.c
> > @@ -1596,11 +1596,18 @@ PCIBus *pci_find_bus(PCIBus *bus, int bus_num)
> >  
> >  PCIDevice *pci_find_device(PCIBus *bus, int bus_num, int slot, int function)
> >  {
> > +    PCIDevice *d;
> >      bus = pci_find_bus(bus, bus_num);
> >  
> >      if (!bus)
> >          return NULL;
> >  
> > +    d = bus->parent_dev;
> > +    if (d && pci_is_express(d) &&
> > +        pcie_cap_get_type(d) == PCI_EXP_TYPE_DOWNSTREAM &&
> > +        !pcie_cap_is_ari_enabled(d) && slot > 0) {
> > +        return NULL;
> > +    }
> >      return bus->devices[PCI_DEVFN(slot, function)];
> >  }
> 
> I'd like to split this express-specific code out in some way.
> Further, the downstream port always has a single slot.
> Maybe it shouldn't be a bus at all, but this needs some thought.

Yes, it needs consideration.


> How about we put flag in PCIBus that says that a single
> slot is supported? Downstream ports would just set it.

So such a flag must be set/clear by something like pcie_cap_ari_write_config()
depending on ARI bit at runtime.
pcie device can have 256 functions instead of 8.

Maybe we'd like to emulate how p2p bridge transfers pci transaction
to child pci bus somehow.
So how about introducing new method? something like the following.

Which is your preference? new flag or new method?

PCIBus
	PCIDevice (*get_device)(bus, slot, function)

pci_get_device_default()
	return bus->devices[PCI_DEVFN(slot, function)];

pcie_downstream_get_device()
	d = bus->parent_dev;
	if (!pcie_cap_is_ari_enabled(d) && slot > 0) {
		return NULL
	}
	return bus->devices[PCI_DEVFN(slot, function)];

pci_find_device()
	...	
	return bus->get_device(bus, slot, function)
-- 
yamahata

[Qemu-devel] Re: [PATCH] pci/pcie: make pci_find_device() ARI aware.

[Michael S. Tsirkin]

On Fri, Jan 21, 2011 at 07:44:16PM +0900, Isaku Yamahata wrote:
> On Thu, Jan 20, 2011 at 04:15:48PM +0200, Michael S. Tsirkin wrote:
> > On Thu, Jan 20, 2011 at 03:57:39PM +0900, Isaku Yamahata wrote:
> > > make pci_find_device() ARI aware.
> > > 
> > > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> > > ---
> > >  hw/pci.c |    7 +++++++
> > >  1 files changed, 7 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/hw/pci.c b/hw/pci.c
> > > index 8d0e3df..851f350 100644
> > > --- a/hw/pci.c
> > > +++ b/hw/pci.c
> > > @@ -1596,11 +1596,18 @@ PCIBus *pci_find_bus(PCIBus *bus, int bus_num)
> > >  
> > >  PCIDevice *pci_find_device(PCIBus *bus, int bus_num, int slot, int function)
> > >  {
> > > +    PCIDevice *d;
> > >      bus = pci_find_bus(bus, bus_num);
> > >  
> > >      if (!bus)
> > >          return NULL;
> > >  
> > > +    d = bus->parent_dev;
> > > +    if (d && pci_is_express(d) &&
> > > +        pcie_cap_get_type(d) == PCI_EXP_TYPE_DOWNSTREAM &&
> > > +        !pcie_cap_is_ari_enabled(d) && slot > 0) {
> > > +        return NULL;
> > > +    }
> > >      return bus->devices[PCI_DEVFN(slot, function)];
> > >  }
> > 
> > I'd like to split this express-specific code out in some way.
> > Further, the downstream port always has a single slot.
> > Maybe it shouldn't be a bus at all, but this needs some thought.
> 
> Yes, it needs consideration.
> 
> 
> > How about we put flag in PCIBus that says that a single
> > slot is supported? Downstream ports would just set it.
> 
> So such a flag must be set/clear by something like pcie_cap_ari_write_config()
> depending on ARI bit at runtime.

Well, to figure it out, could you please describe what is the situation
your patch tries to fix? I would generally would like a reason for the
change to be given in commit logs, please try to avoid just restating
what the patch does.

Are you trying to create a device with > 8 functions?
If that is the case I suspect this is not the best way
to do this at all.

> pcie device can have 256 functions instead of 8.

Only if it's an ARI device. And note that if you have a device with
256 functions and disable ARI in the port, it appears as
multiple devices.

> Maybe we'd like to emulate how p2p bridge transfers pci transaction
> to child pci bus somehow.

To support > 8 functions per device, some refactoring would be needed:
you can not figure out slot and function from the root bus,
it depends on ARI along the way. So APIs that pass in
decoded slot/function do not make sense anymore,
you must pass in devfn all the way.

But since everyone decodes and encodes them in the same way,
many things will work even without decoding.



-- 
MST

[Qemu-devel] Re: [PATCH] pci/pcie: make pci_find_device() ARI aware.

[Isaku Yamahata]

On Fri, Jan 21, 2011 at 04:29:41PM +0200, Michael S. Tsirkin wrote:
> On Fri, Jan 21, 2011 at 07:44:16PM +0900, Isaku Yamahata wrote:
> > On Thu, Jan 20, 2011 at 04:15:48PM +0200, Michael S. Tsirkin wrote:
> > > On Thu, Jan 20, 2011 at 03:57:39PM +0900, Isaku Yamahata wrote:
> > > > make pci_find_device() ARI aware.
> > > > 
> > > > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> > > > ---
> > > >  hw/pci.c |    7 +++++++
> > > >  1 files changed, 7 insertions(+), 0 deletions(-)
> > > > 
> > > > diff --git a/hw/pci.c b/hw/pci.c
> > > > index 8d0e3df..851f350 100644
> > > > --- a/hw/pci.c
> > > > +++ b/hw/pci.c
> > > > @@ -1596,11 +1596,18 @@ PCIBus *pci_find_bus(PCIBus *bus, int bus_num)
> > > >  
> > > >  PCIDevice *pci_find_device(PCIBus *bus, int bus_num, int slot, int function)
> > > >  {
> > > > +    PCIDevice *d;
> > > >      bus = pci_find_bus(bus, bus_num);
> > > >  
> > > >      if (!bus)
> > > >          return NULL;
> > > >  
> > > > +    d = bus->parent_dev;
> > > > +    if (d && pci_is_express(d) &&
> > > > +        pcie_cap_get_type(d) == PCI_EXP_TYPE_DOWNSTREAM &&
> > > > +        !pcie_cap_is_ari_enabled(d) && slot > 0) {
> > > > +        return NULL;
> > > > +    }
> > > >      return bus->devices[PCI_DEVFN(slot, function)];
> > > >  }
> > > 
> > > I'd like to split this express-specific code out in some way.
> > > Further, the downstream port always has a single slot.
> > > Maybe it shouldn't be a bus at all, but this needs some thought.
> > 
> > Yes, it needs consideration.
> > 
> > 
> > > How about we put flag in PCIBus that says that a single
> > > slot is supported? Downstream ports would just set it.
> > 
> > So such a flag must be set/clear by something like pcie_cap_ari_write_config()
> > depending on ARI bit at runtime.
> 
> Well, to figure it out, could you please describe what is the situation
> your patch tries to fix? I would generally would like a reason for the
> change to be given in commit logs, please try to avoid just restating
> what the patch does.

It seems that I should have added the comment to refer the spec.
I'd like to implement ARI enable bit correctly.

Downstream port(and root port) doesn't forward pci transaction for
function > 7 by default for compatibility, 
Only when ARI forwarding enable bit of downstream/root port is set,
the virtual p2p bridge forwards pci transaction for
function > 7 (i.e. slot > 0).

  6.13 Alternative routing-ID Interpretation(ARI)
  7.8.15 Device capabilites 2 register
  ARI forwarding supproted
  7.8.16 Device control 2 register
  ARI forwarding Enable
    5 ARI Forwarding Enable When set, the Downstream Port
    disables its traditional Device Number field being 0 enforcement
    when turning a Type 1 Configuration Request into a Type 0
    Configuration Request, permitting access to Extended Functions
    in an ARI Device immediately below the Port. See Section 6.13.
    Default value of this bit is 0b. Must be hardwired to 0b if the ARI
    Forwarding Supported bit is 0b.

Oh, the patch should check root port in addition to downstream port.

> Are you trying to create a device with > 8 functions?
> If that is the case I suspect this is not the best way
> to do this at all.
>
> > pcie device can have 256 functions instead of 8.
> 
> Only if it's an ARI device. And note that if you have a device with
> 256 functions and disable ARI in the port, it appears as
> multiple devices.
> 
> > Maybe we'd like to emulate how p2p bridge transfers pci transaction
> > to child pci bus somehow.
> 
> To support > 8 functions per device, some refactoring would be needed:
> you can not figure out slot and function from the root bus,
> it depends on ARI along the way. So APIs that pass in
> decoded slot/function do not make sense anymore,
> you must pass in devfn all the way.
> 
> But since everyone decodes and encodes them in the same way,
> many things will work even without decoding.

I think there are only two issues to address.
Configuration space and hot plug.
As you already described it, ARI is defined such that APIs can stay same,
just interpret slot bits as part of function number.
This patch addresses configuration space access.

Multifunction hot plug hasn't been addressed even for conventional pci case.
Some kind of refactoring would be necessary for it, I think.
-- 
yamahata

[Qemu-devel] Re: [PATCH] pci/pcie: make pci_find_device() ARI aware.

[Michael S. Tsirkin]

On Sat, Jan 22, 2011 at 01:39:57AM +0900, Isaku Yamahata wrote:
> On Fri, Jan 21, 2011 at 04:29:41PM +0200, Michael S. Tsirkin wrote:
> > On Fri, Jan 21, 2011 at 07:44:16PM +0900, Isaku Yamahata wrote:
> > > On Thu, Jan 20, 2011 at 04:15:48PM +0200, Michael S. Tsirkin wrote:
> > > > On Thu, Jan 20, 2011 at 03:57:39PM +0900, Isaku Yamahata wrote:
> > > > > make pci_find_device() ARI aware.
> > > > > 
> > > > > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> > > > > ---
> > > > >  hw/pci.c |    7 +++++++
> > > > >  1 files changed, 7 insertions(+), 0 deletions(-)
> > > > > 
> > > > > diff --git a/hw/pci.c b/hw/pci.c
> > > > > index 8d0e3df..851f350 100644
> > > > > --- a/hw/pci.c
> > > > > +++ b/hw/pci.c
> > > > > @@ -1596,11 +1596,18 @@ PCIBus *pci_find_bus(PCIBus *bus, int bus_num)
> > > > >  
> > > > >  PCIDevice *pci_find_device(PCIBus *bus, int bus_num, int slot, int function)
> > > > >  {
> > > > > +    PCIDevice *d;
> > > > >      bus = pci_find_bus(bus, bus_num);
> > > > >  
> > > > >      if (!bus)
> > > > >          return NULL;
> > > > >  
> > > > > +    d = bus->parent_dev;
> > > > > +    if (d && pci_is_express(d) &&
> > > > > +        pcie_cap_get_type(d) == PCI_EXP_TYPE_DOWNSTREAM &&
> > > > > +        !pcie_cap_is_ari_enabled(d) && slot > 0) {
> > > > > +        return NULL;
> > > > > +    }
> > > > >      return bus->devices[PCI_DEVFN(slot, function)];
> > > > >  }
> > > > 
> > > > I'd like to split this express-specific code out in some way.
> > > > Further, the downstream port always has a single slot.
> > > > Maybe it shouldn't be a bus at all, but this needs some thought.
> > > 
> > > Yes, it needs consideration.
> > > 
> > > 
> > > > How about we put flag in PCIBus that says that a single
> > > > slot is supported? Downstream ports would just set it.
> > > 
> > > So such a flag must be set/clear by something like pcie_cap_ari_write_config()
> > > depending on ARI bit at runtime.
> > 
> > Well, to figure it out, could you please describe what is the situation
> > your patch tries to fix? I would generally would like a reason for the
> > change to be given in commit logs, please try to avoid just restating
> > what the patch does.
> 
> It seems that I should have added the comment to refer the spec.
> I'd like to implement ARI enable bit correctly.
> 
> Downstream port(and root port) doesn't forward pci transaction for
> function > 7 by default for compatibility, 
> Only when ARI forwarding enable bit of downstream/root port is set,
> the virtual p2p bridge forwards pci transaction for
> function > 7 (i.e. slot > 0).

Oh, I see, yes, function > 7 gets described as slot >0.
I think this is what I missed.
Hmm, it'd pretty confusing. Should we fix this,
pass devfn all over?

I now understand what the code does, it just needs
a good comment that explains that at the moment
slot encodes the high bits of the device id.

Also, let's replace pcie_cap_is_ari_enabled
with an inline that does all the relevant logic
E.g.

/* With PCI Express Endpoints, there's a single device behind
   each downstream port bus, and bits 3:7 of the function number get
   encoded in the slot number (the Express spec calls it the Device
   Number). This allows > 8 functions, but
   these extended functions are only accessible when the
   Alternative routing-ID Interpretation (ARI)
   capability is enabled in the downstream port. With that capability
   disabled the port enforces the Device Number field being 0.*/
static inline
bool pcie_check_slot(PCIDevice *dev)
{
	return !pci_is_express(dev) || !slot ||
		pcie_cap_get_type(dev) != PCI_EXP_TYPE_DOWNSTREAM ||
		(pci_get_long(dev->config + dev->exp.exp_cap + PCI_EXP_DEVCTL2) &
		PCI_EXP_DEVCTL2_ARI);
}




>   6.13 Alternative routing-ID Interpretation(ARI)
>   7.8.15 Device capabilites 2 register
>   ARI forwarding supproted
>   7.8.16 Device control 2 register
>   ARI forwarding Enable
>     5 ARI Forwarding Enable When set, the Downstream Port
>     disables its traditional Device Number field being 0 enforcement
>     when turning a Type 1 Configuration Request into a Type 0
>     Configuration Request, permitting access to Extended Functions
>     in an ARI Device immediately below the Port. See Section 6.13.
>     Default value of this bit is 0b. Must be hardwired to 0b if the ARI
>     Forwarding Supported bit is 0b.
> Oh, the patch should check root port in addition to downstream port.

It should? Where does it say so?

> > Are you trying to create a device with > 8 functions?
> > If that is the case I suspect this is not the best way
> > to do this at all.
> >
> > > pcie device can have 256 functions instead of 8.
> > 
> > Only if it's an ARI device. And note that if you have a device with
> > 256 functions and disable ARI in the port, it appears as
> > multiple devices.
> > 
> > > Maybe we'd like to emulate how p2p bridge transfers pci transaction
> > > to child pci bus somehow.
> > 
> > To support > 8 functions per device, some refactoring would be needed:
> > you can not figure out slot and function from the root bus,
> > it depends on ARI along the way. So APIs that pass in
> > decoded slot/function do not make sense anymore,
> > you must pass in devfn all the way.
> > 
> > But since everyone decodes and encodes them in the same way,
> > many things will work even without decoding.
> 
> I think there are only two issues to address.
> Configuration space and hot plug.

info pci, maybe others (firmware path?)

> As you already described it, ARI is defined such that APIs can stay same,
> just interpret slot bits as part of function number.
> This patch addresses configuration space access.
> 
> Multifunction hot plug hasn't been addressed even for conventional pci case.

But at least we learned to deny hotplug attempts for functions > 0...

> Some kind of refactoring would be necessary for it, I think.

It's probably just a matter of figuring out what works with acpi.
Express has a native hotplug which likely works ok already ...

> -- 
> yamahata

[Qemu-devel] Re: [PATCH] pci/pcie: make pci_find_device() ARI aware.

[Isaku Yamahata]

On Sun, Jan 23, 2011 at 05:57:53PM +0200, Michael S. Tsirkin wrote:
> On Sat, Jan 22, 2011 at 01:39:57AM +0900, Isaku Yamahata wrote:
> > On Fri, Jan 21, 2011 at 04:29:41PM +0200, Michael S. Tsirkin wrote:
> > > On Fri, Jan 21, 2011 at 07:44:16PM +0900, Isaku Yamahata wrote:
> > > > On Thu, Jan 20, 2011 at 04:15:48PM +0200, Michael S. Tsirkin wrote:
> > > > > On Thu, Jan 20, 2011 at 03:57:39PM +0900, Isaku Yamahata wrote:
> > > > > > make pci_find_device() ARI aware.
> > > > > > 
> > > > > > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> > > > > > ---
> > > > > >  hw/pci.c |    7 +++++++
> > > > > >  1 files changed, 7 insertions(+), 0 deletions(-)
> > > > > > 
> > > > > > diff --git a/hw/pci.c b/hw/pci.c
> > > > > > index 8d0e3df..851f350 100644
> > > > > > --- a/hw/pci.c
> > > > > > +++ b/hw/pci.c
> > > > > > @@ -1596,11 +1596,18 @@ PCIBus *pci_find_bus(PCIBus *bus, int bus_num)
> > > > > >  
> > > > > >  PCIDevice *pci_find_device(PCIBus *bus, int bus_num, int slot, int function)
> > > > > >  {
> > > > > > +    PCIDevice *d;
> > > > > >      bus = pci_find_bus(bus, bus_num);
> > > > > >  
> > > > > >      if (!bus)
> > > > > >          return NULL;
> > > > > >  
> > > > > > +    d = bus->parent_dev;
> > > > > > +    if (d && pci_is_express(d) &&
> > > > > > +        pcie_cap_get_type(d) == PCI_EXP_TYPE_DOWNSTREAM &&
> > > > > > +        !pcie_cap_is_ari_enabled(d) && slot > 0) {
> > > > > > +        return NULL;
> > > > > > +    }
> > > > > >      return bus->devices[PCI_DEVFN(slot, function)];
> > > > > >  }
> > > > > 
> > > > > I'd like to split this express-specific code out in some way.
> > > > > Further, the downstream port always has a single slot.
> > > > > Maybe it shouldn't be a bus at all, but this needs some thought.
> > > > 
> > > > Yes, it needs consideration.
> > > > 
> > > > 
> > > > > How about we put flag in PCIBus that says that a single
> > > > > slot is supported? Downstream ports would just set it.
> > > > 
> > > > So such a flag must be set/clear by something like pcie_cap_ari_write_config()
> > > > depending on ARI bit at runtime.
> > > 
> > > Well, to figure it out, could you please describe what is the situation
> > > your patch tries to fix? I would generally would like a reason for the
> > > change to be given in commit logs, please try to avoid just restating
> > > what the patch does.
> > 
> > It seems that I should have added the comment to refer the spec.
> > I'd like to implement ARI enable bit correctly.
> > 
> > Downstream port(and root port) doesn't forward pci transaction for
> > function > 7 by default for compatibility, 
> > Only when ARI forwarding enable bit of downstream/root port is set,
> > the virtual p2p bridge forwards pci transaction for
> > function > 7 (i.e. slot > 0).
> 
> Oh, I see, yes, function > 7 gets described as slot >0.
> I think this is what I missed.
> Hmm, it'd pretty confusing. Should we fix this,
> pass devfn all over?

Sounds to make sense.
Although it seems only pci_find_device() will be affected at a glance, 
I'll look into it more closely.


> I now understand what the code does, it just needs
> a good comment that explains that at the moment
> slot encodes the high bits of the device id.
> 
> Also, let's replace pcie_cap_is_ari_enabled
> with an inline that does all the relevant logic
> E.g.
> 
> /* With PCI Express Endpoints, there's a single device behind
>    each downstream port bus, and bits 3:7 of the function number get
>    encoded in the slot number (the Express spec calls it the Device
>    Number). This allows > 8 functions, but
>    these extended functions are only accessible when the
>    Alternative routing-ID Interpretation (ARI)
>    capability is enabled in the downstream port. With that capability
>    disabled the port enforces the Device Number field being 0.*/
> static inline
> bool pcie_check_slot(PCIDevice *dev)
> {
> 	return !pci_is_express(dev) || !slot ||
> 		pcie_cap_get_type(dev) != PCI_EXP_TYPE_DOWNSTREAM ||
> 		(pci_get_long(dev->config + dev->exp.exp_cap + PCI_EXP_DEVCTL2) &
> 		PCI_EXP_DEVCTL2_ARI);
> }

Okay.


> >   6.13 Alternative routing-ID Interpretation(ARI)
> >   7.8.15 Device capabilites 2 register
> >   ARI forwarding supproted
> >   7.8.16 Device control 2 register
> >   ARI forwarding Enable
> >     5 ARI Forwarding Enable When set, the Downstream Port
> >     disables its traditional Device Number field being 0 enforcement
> >     when turning a Type 1 Configuration Request into a Type 0
> >     Configuration Request, permitting access to Extended Functions
> >     in an ARI Device immediately below the Port. See Section 6.13.
> >     Default value of this bit is 0b. Must be hardwired to 0b if the ARI
> >     Forwarding Supported bit is 0b.
> > Oh, the patch should check root port in addition to downstream port.
> 
> It should? Where does it say so?

I wasn't clear enough.
pcie_check_slot() above should include something like
  !((type == downstream) ||
    (type == root && the below is endpoint))


> > > Are you trying to create a device with > 8 functions?
> > > If that is the case I suspect this is not the best way
> > > to do this at all.
> > >
> > > > pcie device can have 256 functions instead of 8.
> > > 
> > > Only if it's an ARI device. And note that if you have a device with
> > > 256 functions and disable ARI in the port, it appears as
> > > multiple devices.
> > > 
> > > > Maybe we'd like to emulate how p2p bridge transfers pci transaction
> > > > to child pci bus somehow.
> > > 
> > > To support > 8 functions per device, some refactoring would be needed:
> > > you can not figure out slot and function from the root bus,
> > > it depends on ARI along the way. So APIs that pass in
> > > decoded slot/function do not make sense anymore,
> > > you must pass in devfn all the way.
> > > 
> > > But since everyone decodes and encodes them in the same way,
> > > many things will work even without decoding.
> > 
> > I think there are only two issues to address.
> > Configuration space and hot plug.
> 
> info pci, maybe others (firmware path?)

Oh yes, maybe.


> > As you already described it, ARI is defined such that APIs can stay same,
> > just interpret slot bits as part of function number.
> > This patch addresses configuration space access.
> > 
> > Multifunction hot plug hasn't been addressed even for conventional pci case.
> 
> But at least we learned to deny hotplug attempts for functions > 0...
> 
> > Some kind of refactoring would be necessary for it, I think.
> 
> It's probably just a matter of figuring out what works with acpi.
> Express has a native hotplug which likely works ok already ...
> 
> > -- 
> > yamahata
> 

-- 
yamahata

[Qemu-devel] Re: [PATCH] pci/pcie: make pci_find_device() ARI aware.

[Michael S. Tsirkin]

On Mon, Jan 24, 2011 at 08:39:57PM +0900, Isaku Yamahata wrote:
> On Sun, Jan 23, 2011 at 05:57:53PM +0200, Michael S. Tsirkin wrote:
> > On Sat, Jan 22, 2011 at 01:39:57AM +0900, Isaku Yamahata wrote:
> > > On Fri, Jan 21, 2011 at 04:29:41PM +0200, Michael S. Tsirkin wrote:
> > > > On Fri, Jan 21, 2011 at 07:44:16PM +0900, Isaku Yamahata wrote:
> > > > > On Thu, Jan 20, 2011 at 04:15:48PM +0200, Michael S. Tsirkin wrote:
> > > > > > On Thu, Jan 20, 2011 at 03:57:39PM +0900, Isaku Yamahata wrote:
> > > > > > > make pci_find_device() ARI aware.
> > > > > > > 
> > > > > > > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> > > > > > > ---
> > > > > > >  hw/pci.c |    7 +++++++
> > > > > > >  1 files changed, 7 insertions(+), 0 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/hw/pci.c b/hw/pci.c
> > > > > > > index 8d0e3df..851f350 100644
> > > > > > > --- a/hw/pci.c
> > > > > > > +++ b/hw/pci.c
> > > > > > > @@ -1596,11 +1596,18 @@ PCIBus *pci_find_bus(PCIBus *bus, int bus_num)
> > > > > > >  
> > > > > > >  PCIDevice *pci_find_device(PCIBus *bus, int bus_num, int slot, int function)
> > > > > > >  {
> > > > > > > +    PCIDevice *d;
> > > > > > >      bus = pci_find_bus(bus, bus_num);
> > > > > > >  
> > > > > > >      if (!bus)
> > > > > > >          return NULL;
> > > > > > >  
> > > > > > > +    d = bus->parent_dev;
> > > > > > > +    if (d && pci_is_express(d) &&
> > > > > > > +        pcie_cap_get_type(d) == PCI_EXP_TYPE_DOWNSTREAM &&
> > > > > > > +        !pcie_cap_is_ari_enabled(d) && slot > 0) {
> > > > > > > +        return NULL;
> > > > > > > +    }
> > > > > > >      return bus->devices[PCI_DEVFN(slot, function)];
> > > > > > >  }
> > > > > > 
> > > > > > I'd like to split this express-specific code out in some way.
> > > > > > Further, the downstream port always has a single slot.
> > > > > > Maybe it shouldn't be a bus at all, but this needs some thought.
> > > > > 
> > > > > Yes, it needs consideration.
> > > > > 
> > > > > 
> > > > > > How about we put flag in PCIBus that says that a single
> > > > > > slot is supported? Downstream ports would just set it.
> > > > > 
> > > > > So such a flag must be set/clear by something like pcie_cap_ari_write_config()
> > > > > depending on ARI bit at runtime.
> > > > 
> > > > Well, to figure it out, could you please describe what is the situation
> > > > your patch tries to fix? I would generally would like a reason for the
> > > > change to be given in commit logs, please try to avoid just restating
> > > > what the patch does.
> > > 
> > > It seems that I should have added the comment to refer the spec.
> > > I'd like to implement ARI enable bit correctly.
> > > 
> > > Downstream port(and root port) doesn't forward pci transaction for
> > > function > 7 by default for compatibility, 
> > > Only when ARI forwarding enable bit of downstream/root port is set,
> > > the virtual p2p bridge forwards pci transaction for
> > > function > 7 (i.e. slot > 0).
> > 
> > Oh, I see, yes, function > 7 gets described as slot >0.
> > I think this is what I missed.
> > Hmm, it'd pretty confusing. Should we fix this,
> > pass devfn all over?
> 
> Sounds to make sense.
> Although it seems only pci_find_device() will be affected at a glance, 
> I'll look into it more closely.
> 
> 
> > I now understand what the code does, it just needs
> > a good comment that explains that at the moment
> > slot encodes the high bits of the device id.
> > 
> > Also, let's replace pcie_cap_is_ari_enabled
> > with an inline that does all the relevant logic
> > E.g.
> > 
> > /* With PCI Express Endpoints, there's a single device behind
> >    each downstream port bus, and bits 3:7 of the function number get
> >    encoded in the slot number (the Express spec calls it the Device
> >    Number). This allows > 8 functions, but
> >    these extended functions are only accessible when the
> >    Alternative routing-ID Interpretation (ARI)
> >    capability is enabled in the downstream port. With that capability
> >    disabled the port enforces the Device Number field being 0.*/
> > static inline
> > bool pcie_check_slot(PCIDevice *dev)
> > {
> > 	return !pci_is_express(dev) || !slot ||
> > 		pcie_cap_get_type(dev) != PCI_EXP_TYPE_DOWNSTREAM ||
> > 		(pci_get_long(dev->config + dev->exp.exp_cap + PCI_EXP_DEVCTL2) &
> > 		PCI_EXP_DEVCTL2_ARI);
> > }
> 
> Okay.
> 
> 
> > >   6.13 Alternative routing-ID Interpretation(ARI)
> > >   7.8.15 Device capabilites 2 register
> > >   ARI forwarding supproted
> > >   7.8.16 Device control 2 register
> > >   ARI forwarding Enable
> > >     5 ARI Forwarding Enable When set, the Downstream Port
> > >     disables its traditional Device Number field being 0 enforcement
> > >     when turning a Type 1 Configuration Request into a Type 0
> > >     Configuration Request, permitting access to Extended Functions
> > >     in an ARI Device immediately below the Port. See Section 6.13.
> > >     Default value of this bit is 0b. Must be hardwired to 0b if the ARI
> > >     Forwarding Supported bit is 0b.
> > > Oh, the patch should check root port in addition to downstream port.
> > 
> > It should? Where does it say so?
> 
> I wasn't clear enough.
> pcie_check_slot() above should include something like
>   !((type == downstream) ||
>     (type == root && the below is endpoint))

So:
(pcie_cap_get_type(dev) != PCI_EXP_TYPE_DOWNSTREAM &&
pcie_cap_get_type(dev) != PCI_EXP_ROOT) 

I think the endpoint check is not needed: the rule really
applies to any device besides the donwstream port itself.
and that is never below root, is it?

> 
> > > > Are you trying to create a device with > 8 functions?
> > > > If that is the case I suspect this is not the best way
> > > > to do this at all.
> > > >
> > > > > pcie device can have 256 functions instead of 8.
> > > > 
> > > > Only if it's an ARI device. And note that if you have a device with
> > > > 256 functions and disable ARI in the port, it appears as
> > > > multiple devices.
> > > > 
> > > > > Maybe we'd like to emulate how p2p bridge transfers pci transaction
> > > > > to child pci bus somehow.
> > > > 
> > > > To support > 8 functions per device, some refactoring would be needed:
> > > > you can not figure out slot and function from the root bus,
> > > > it depends on ARI along the way. So APIs that pass in
> > > > decoded slot/function do not make sense anymore,
> > > > you must pass in devfn all the way.
> > > > 
> > > > But since everyone decodes and encodes them in the same way,
> > > > many things will work even without decoding.
> > > 
> > > I think there are only two issues to address.
> > > Configuration space and hot plug.
> > 
> > info pci, maybe others (firmware path?)
> 
> Oh yes, maybe.
> 
> 
> > > As you already described it, ARI is defined such that APIs can stay same,
> > > just interpret slot bits as part of function number.
> > > This patch addresses configuration space access.
> > > 
> > > Multifunction hot plug hasn't been addressed even for conventional pci case.
> > 
> > But at least we learned to deny hotplug attempts for functions > 0...
> > 
> > > Some kind of refactoring would be necessary for it, I think.
> > 
> > It's probably just a matter of figuring out what works with acpi.
> > Express has a native hotplug which likely works ok already ...
> > 
> > > -- 
> > > yamahata
> > 
> 
> -- 
> yamahata