Re: [Qemu-devel] [PATCH v2 4/4] ioh3420: Enable ARI forwarding

[Knut Omang <knut.omang@oracle.com>]

On Mon, 2014-08-25 at 06:09 +0000, Gonglei (Arei) wrote:
> > From: Michael S. Tsirkin [mailto:mst@redhat.com]
> > Subject: Re: [PATCH v2 4/4] ioh3420: Enable ARI forwarding
> > 
> > On Sun, Aug 24, 2014 at 03:32:20PM +0200, Knut Omang wrote:
> > > Signed-off-by: Knut Omang <knut.omang@oracle.com>
> > 
> > BTW pcie_cap_is_arifwd_enabled is still unused.
> 
> Not yet, but I have posted a patch series:
> 
> [PATCH v2 0/2] add check for PCIe root ports and downstream ports
> 
> Which we used this function to check ARI Forwarding is enabled or not.
> I hope you can help me review it, thanks a lot!
> 
> BTW, I will rebase my patches on Kunt's patch series. And will do some fixes 
> about Hu Tao's and Marcel Apfelbaum's comments.
> 
> > We really should use it to make ARI work properly, right?
> > 
> Yes. I think we should.

Modelling the real world, one way to go with multifunction devices would
be to introduce a separate concept of a "PCIFunction" object in the
current Qemu model, and let each device have 1 or more such function
objects associated with them. 

The simpler way to implement a true multifunction device would just be
to add one PCIDevice object at the right bus, devfn for each additional
function that are associated with the master device. 

I have implemented the latter approach in a simple SR/IOV emulation
implementation - will post a patch for that shortly, as it seems to be
relevant for this discussion.

With that approach (which came out pleasantly simple wrt changes in the
code) the role of the ARI forwarding bit would be to do nothing when
enabled, as ARI forwarding works with no changes, but make ARI
forwarding, when disabled or not available, makes the additional
functions beyond #8 not addressable from the outside (which in my view
is what a modified version of Gonglei's patch should do)

Knut

> Best regards,
> -Gonglei
> 
> > > ---
> > >  hw/pci-bridge/ioh3420.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > >
> > > diff --git a/hw/pci-bridge/ioh3420.c b/hw/pci-bridge/ioh3420.c
> > > index e6674a1..cce2fdd 100644
> > > --- a/hw/pci-bridge/ioh3420.c
> > > +++ b/hw/pci-bridge/ioh3420.c
> > > @@ -85,6 +85,7 @@ static void ioh3420_reset(DeviceState *qdev)
> > >      pcie_cap_root_reset(d);
> > >      pcie_cap_deverr_reset(d);
> > >      pcie_cap_slot_reset(d);
> > > +    pcie_cap_arifwd_reset(d);
> > >      pcie_aer_root_reset(d);
> > >      pci_bridge_reset(qdev);
> > >      pci_bridge_disable_base_limit(d);
> > > @@ -119,6 +120,7 @@ static int ioh3420_initfn(PCIDevice *d)
> > >          goto err_msi;
> > >      }
> > >
> > > +    pcie_cap_arifwd_init(d);
> > >      pcie_cap_deverr_init(d);
> > >      pcie_cap_slot_init(d, s->slot);
> > >      pcie_chassis_create(s->chassis);
> > > --
> > > 1.9.0
>