[Qemu-devel] [PATCH v2 0/4] q35: Minor fixes/enhancements to improve usability of root ports

[Qemu-devel] [PATCH v2 0/4] q35: Minor fixes/enhancements to improve usability of root ports

[Knut Omang]

These are some minor fixes necessary to be able to operate ARI capable devices
in PCIe root ports on a q35 machine, using command line options like this:

 -device ioh3420,slot=0,id=pcie_port.0
 -device ioh3420,slot=1,id=pcie_port.1
 -device <ari_capable_device1>,bus=pcie_port.0
 -device <ari_capable_device2>,bus=pcie_port.1

Changes since v1:
  Removed "ioh3420: Provide a unique bus name and an interrupt mapping function"
    as it is no longer necessary.
  Added rename of ari -> arifwd for clarity and removal of obsolete init function
  (separate patches)
  Use the renamed functions in ioh3420.

Knut Omang (4):
  pcie: Fix incorrect write to the ari capability next function field
  pcie: Rename the pcie_cap_ari_* functions to pcie_cap_arifwd_*
  ioh3420: Remove obsoleted, unused ioh3420_init function
  ioh3420: Enable ARI forwarding

 hw/pci-bridge/ioh3420.c            | 27 +++------------------------
 hw/pci-bridge/xio3130_downstream.c |  4 ++--
 hw/pci/pcie.c                      | 13 +++++++------
 include/hw/pci/pcie.h              |  7 ++++---
 4 files changed, 16 insertions(+), 35 deletions(-)

-- 
1.9.0

[Qemu-devel] [PATCH v2 1/4] pcie: Fix incorrect write to the ari capability next function field

[Knut Omang]

PCI_ARI_CAP_NFN, a macro for reading next function was used instead of
the intended write.

Signed-off-by: Knut Omang <knut.omang@oracle.com>
---
 hw/pci/pcie.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index a123c01..de0e967 100644
--- a/hw/pci/pcie.c
+++ b/hw/pci/pcie.c
@@ -630,5 +630,5 @@ void pcie_ari_init(PCIDevice *dev, uint16_t offset, uint16_t nextfn)
 {
     pcie_add_capability(dev, PCI_EXT_CAP_ID_ARI, PCI_ARI_VER,
                         offset, PCI_ARI_SIZEOF);
-    pci_set_long(dev->config + offset + PCI_ARI_CAP, PCI_ARI_CAP_NFN(nextfn));
+    pci_set_long(dev->config + offset + PCI_ARI_CAP, (nextfn & 0xff) << 8);
 }
-- 
1.9.0

[Qemu-devel] [PATCH v2 2/4] pcie: Rename the pcie_cap_ari_* functions to pcie_cap_arifwd_*

[Knut Omang]

Rename helper functions to make a clearer distinction between
the PCIe capability/control register feature ARI forwarding and a
device that supports the ARI feature via an ARI extended PCIe capability.

Signed-off-by: Knut Omang <knut.omang@oracle.com>
---
 hw/pci-bridge/ioh3420.c            |  1 +
 hw/pci-bridge/xio3130_downstream.c |  4 ++--
 hw/pci/pcie.c                      | 11 ++++++-----
 include/hw/pci/pcie.h              |  7 ++++---
 4 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/hw/pci-bridge/ioh3420.c b/hw/pci-bridge/ioh3420.c
index 7cd87fc..aed2bf1 100644
--- a/hw/pci-bridge/ioh3420.c
+++ b/hw/pci-bridge/ioh3420.c
@@ -118,6 +118,7 @@ static int ioh3420_initfn(PCIDevice *d)
     if (rc < 0) {
         goto err_msi;
     }
+
     pcie_cap_deverr_init(d);
     pcie_cap_slot_init(d, s->slot);
     pcie_chassis_create(s->chassis);
diff --git a/hw/pci-bridge/xio3130_downstream.c b/hw/pci-bridge/xio3130_downstream.c
index 51f20d7..b3a6479 100644
--- a/hw/pci-bridge/xio3130_downstream.c
+++ b/hw/pci-bridge/xio3130_downstream.c
@@ -50,7 +50,7 @@ static void xio3130_downstream_reset(DeviceState *qdev)
 
     pcie_cap_deverr_reset(d);
     pcie_cap_slot_reset(d);
-    pcie_cap_ari_reset(d);
+    pcie_cap_arifwd_reset(d);
     pci_bridge_reset(qdev);
 }
 
@@ -91,7 +91,7 @@ static int xio3130_downstream_initfn(PCIDevice *d)
     if (rc < 0) {
         goto err_pcie_cap;
     }
-    pcie_cap_ari_init(d);
+    pcie_cap_arifwd_init(d);
     rc = pcie_aer_init(d, XIO3130_AER_OFFSET);
     if (rc < 0) {
         goto err;
diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index de0e967..6cb6e0c 100644
--- a/hw/pci/pcie.c
+++ b/hw/pci/pcie.c
@@ -497,9 +497,10 @@ void pcie_cap_flr_write_config(PCIDevice *dev,
     }
 }
 
-/* Alternative Routing-ID Interpretation (ARI) */
-/* ari forwarding support for down stream port */
-void pcie_cap_ari_init(PCIDevice *dev)
+/* Alternative Routing-ID Interpretation (ARI)
+ * forwarding support for root and downstream ports 
+ */
+void pcie_cap_arifwd_init(PCIDevice *dev)
 {
     uint32_t pos = dev->exp.exp_cap;
     pci_long_test_and_set_mask(dev->config + pos + PCI_EXP_DEVCAP2,
@@ -508,13 +509,13 @@ void pcie_cap_ari_init(PCIDevice *dev)
                                PCI_EXP_DEVCTL2_ARI);
 }
 
-void pcie_cap_ari_reset(PCIDevice *dev)
+void pcie_cap_arifwd_reset(PCIDevice *dev)
 {
     uint8_t *devctl2 = dev->config + dev->exp.exp_cap + PCI_EXP_DEVCTL2;
     pci_long_test_and_clear_mask(devctl2, PCI_EXP_DEVCTL2_ARI);
 }
 
-bool pcie_cap_is_ari_enabled(const PCIDevice *dev)
+bool pcie_cap_is_arifwd_enabled(const PCIDevice *dev)
 {
     if (!pci_is_express(dev)) {
         return false;
diff --git a/include/hw/pci/pcie.h b/include/hw/pci/pcie.h
index 7fe81f3..d139d58 100644
--- a/include/hw/pci/pcie.h
+++ b/include/hw/pci/pcie.h
@@ -103,9 +103,10 @@ void pcie_cap_flr_init(PCIDevice *dev);
 void pcie_cap_flr_write_config(PCIDevice *dev,
                            uint32_t addr, uint32_t val, int len);
 
-void pcie_cap_ari_init(PCIDevice *dev);
-void pcie_cap_ari_reset(PCIDevice *dev);
-bool pcie_cap_is_ari_enabled(const PCIDevice *dev);
+/* ARI forwarding capability and control */
+void pcie_cap_arifwd_init(PCIDevice *dev);
+void pcie_cap_arifwd_reset(PCIDevice *dev);
+bool pcie_cap_is_arifwd_enabled(const PCIDevice *dev);
 
 /* PCI express extended capability helper functions */
 uint16_t pcie_find_capability(PCIDevice *dev, uint16_t cap_id);
-- 
1.9.0

[Qemu-devel] [PATCH v2 3/4] ioh3420: Remove obsoleted, unused ioh3420_init function

[Knut Omang]

Signed-off-by: Knut Omang <knut.omang@oracle.com>
---
 hw/pci-bridge/ioh3420.c | 24 ------------------------
 1 file changed, 24 deletions(-)

diff --git a/hw/pci-bridge/ioh3420.c b/hw/pci-bridge/ioh3420.c
index aed2bf1..e6674a1 100644
--- a/hw/pci-bridge/ioh3420.c
+++ b/hw/pci-bridge/ioh3420.c
@@ -157,30 +157,6 @@ static void ioh3420_exitfn(PCIDevice *d)
     pci_bridge_exitfn(d);
 }
 
-PCIESlot *ioh3420_init(PCIBus *bus, int devfn, bool multifunction,
-                         const char *bus_name, pci_map_irq_fn map_irq,
-                         uint8_t port, uint8_t chassis, uint16_t slot)
-{
-    PCIDevice *d;
-    PCIBridge *br;
-    DeviceState *qdev;
-
-    d = pci_create_multifunction(bus, devfn, multifunction, "ioh3420");
-    if (!d) {
-        return NULL;
-    }
-    br = PCI_BRIDGE(d);
-
-    qdev = DEVICE(d);
-    pci_bridge_map_irq(br, bus_name, map_irq);
-    qdev_prop_set_uint8(qdev, "port", port);
-    qdev_prop_set_uint8(qdev, "chassis", chassis);
-    qdev_prop_set_uint16(qdev, "slot", slot);
-    qdev_init_nofail(qdev);
-
-    return PCIE_SLOT(d);
-}
-
 static Property ioh3420_props[] = {
     DEFINE_PROP_BIT(COMPAT_PROP_PCP, PCIDevice, cap_present,
                     QEMU_PCIE_SLTCAP_PCP_BITNR, true),
-- 
1.9.0

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

[Knut Omang]

Signed-off-by: Knut Omang <knut.omang@oracle.com>
---
 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

Re: [Qemu-devel] [PATCH v2 0/4] q35: Minor fixes/enhancements to improve usability of root ports

[Michael S. Tsirkin]

On Sun, Aug 24, 2014 at 03:32:16PM +0200, Knut Omang wrote:
> These are some minor fixes necessary to be able to operate ARI capable devices
> in PCIe root ports on a q35 machine, using command line options like this:
> 
>  -device ioh3420,slot=0,id=pcie_port.0
>  -device ioh3420,slot=1,id=pcie_port.1
>  -device <ari_capable_device1>,bus=pcie_port.0
>  -device <ari_capable_device2>,bus=pcie_port.1
> 
> Changes since v1:
>   Removed "ioh3420: Provide a unique bus name and an interrupt mapping function"
>     as it is no longer necessary.
>   Added rename of ari -> arifwd for clarity and removal of obsolete init function
>   (separate patches)
>   Use the renamed functions in ioh3420.


Applied, thanks!

> Knut Omang (4):
>   pcie: Fix incorrect write to the ari capability next function field
>   pcie: Rename the pcie_cap_ari_* functions to pcie_cap_arifwd_*
>   ioh3420: Remove obsoleted, unused ioh3420_init function
>   ioh3420: Enable ARI forwarding
> 
>  hw/pci-bridge/ioh3420.c            | 27 +++------------------------
>  hw/pci-bridge/xio3130_downstream.c |  4 ++--
>  hw/pci/pcie.c                      | 13 +++++++------
>  include/hw/pci/pcie.h              |  7 ++++---
>  4 files changed, 16 insertions(+), 35 deletions(-)
> 
> -- 
> 1.9.0

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

[Michael S. Tsirkin]

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.
We really should use it to make ARI work properly, right?

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

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

[Gonglei (Arei)]

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

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

Re: [Qemu-devel] [PATCH v2 3/4] ioh3420: Remove obsoleted, unused ioh3420_init function

[Gonglei (Arei)]

> -----Original Message-----
> From: Knut Omang [mailto:knut.omang@oracle.com]
> Sent: Sunday, August 24, 2014 9:32 PM
> To: qemu-devel@nongnu.org
> Cc: Michael S. Tsirkin; Alexey Kardashevskiy; Juan Quintela; Marcel Apfelbaum;
> Markus Armbruster; Paolo Bonzini; Gonglei (Arei); Igor Mammedov; Knut
> Omang
> Subject: [PATCH v2 3/4] ioh3420: Remove obsoleted, unused ioh3420_init
> function
> 
> Signed-off-by: Knut Omang <knut.omang@oracle.com>
> ---
>  hw/pci-bridge/ioh3420.c | 24 ------------------------
>  1 file changed, 24 deletions(-)
> 
> diff --git a/hw/pci-bridge/ioh3420.c b/hw/pci-bridge/ioh3420.c
> index aed2bf1..e6674a1 100644
> --- a/hw/pci-bridge/ioh3420.c
> +++ b/hw/pci-bridge/ioh3420.c
> @@ -157,30 +157,6 @@ static void ioh3420_exitfn(PCIDevice *d)
>      pci_bridge_exitfn(d);
>  }
> 
> -PCIESlot *ioh3420_init(PCIBus *bus, int devfn, bool multifunction,
> -                         const char *bus_name, pci_map_irq_fn
> map_irq,
> -                         uint8_t port, uint8_t chassis, uint16_t slot)
> -{
> -    PCIDevice *d;
> -    PCIBridge *br;
> -    DeviceState *qdev;
> -
> -    d = pci_create_multifunction(bus, devfn, multifunction, "ioh3420");
> -    if (!d) {
> -        return NULL;
> -    }
> -    br = PCI_BRIDGE(d);
> -
> -    qdev = DEVICE(d);
> -    pci_bridge_map_irq(br, bus_name, map_irq);
> -    qdev_prop_set_uint8(qdev, "port", port);
> -    qdev_prop_set_uint8(qdev, "chassis", chassis);
> -    qdev_prop_set_uint16(qdev, "slot", slot);
> -    qdev_init_nofail(qdev);
> -
> -    return PCIE_SLOT(d);
> -}
> -
>  static Property ioh3420_props[] = {
>      DEFINE_PROP_BIT(COMPAT_PROP_PCP, PCIDevice, cap_present,
>                      QEMU_PCIE_SLTCAP_PCP_BITNR, true),
> --
> 1.9.0

That's OK. But the declaration of header file should also be removed.
I noticed MST have pulled into master tree. So, maybe I can post
another patch to fix this trivial problem.

Best regards,
-Gonglei

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

[Knut Omang]

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
> 

Re: [Qemu-devel] [PATCH v2 3/4] ioh3420: Remove obsoleted, unused ioh3420_init function

[Knut Omang]

On Mon, 2014-08-25 at 07:47 +0000, Gonglei (Arei) wrote:
> > -----Original Message-----
> > From: Knut Omang [mailto:knut.omang@oracle.com]
> > Sent: Sunday, August 24, 2014 9:32 PM
> > To: qemu-devel@nongnu.org
> > Cc: Michael S. Tsirkin; Alexey Kardashevskiy; Juan Quintela; Marcel Apfelbaum;
> > Markus Armbruster; Paolo Bonzini; Gonglei (Arei); Igor Mammedov; Knut
> > Omang
> > Subject: [PATCH v2 3/4] ioh3420: Remove obsoleted, unused ioh3420_init
> > function
> > 
> > Signed-off-by: Knut Omang <knut.omang@oracle.com>
> > ---
> >  hw/pci-bridge/ioh3420.c | 24 ------------------------
> >  1 file changed, 24 deletions(-)
> > 
> > diff --git a/hw/pci-bridge/ioh3420.c b/hw/pci-bridge/ioh3420.c
> > index aed2bf1..e6674a1 100644
> > --- a/hw/pci-bridge/ioh3420.c
> > +++ b/hw/pci-bridge/ioh3420.c
> > @@ -157,30 +157,6 @@ static void ioh3420_exitfn(PCIDevice *d)
> >      pci_bridge_exitfn(d);
> >  }
> > 
> > -PCIESlot *ioh3420_init(PCIBus *bus, int devfn, bool multifunction,
> > -                         const char *bus_name, pci_map_irq_fn
> > map_irq,
> > -                         uint8_t port, uint8_t chassis, uint16_t slot)
> > -{
> > -    PCIDevice *d;
> > -    PCIBridge *br;
> > -    DeviceState *qdev;
> > -
> > -    d = pci_create_multifunction(bus, devfn, multifunction, "ioh3420");
> > -    if (!d) {
> > -        return NULL;
> > -    }
> > -    br = PCI_BRIDGE(d);
> > -
> > -    qdev = DEVICE(d);
> > -    pci_bridge_map_irq(br, bus_name, map_irq);
> > -    qdev_prop_set_uint8(qdev, "port", port);
> > -    qdev_prop_set_uint8(qdev, "chassis", chassis);
> > -    qdev_prop_set_uint16(qdev, "slot", slot);
> > -    qdev_init_nofail(qdev);
> > -
> > -    return PCIE_SLOT(d);
> > -}
> > -
> >  static Property ioh3420_props[] = {
> >      DEFINE_PROP_BIT(COMPAT_PROP_PCP, PCIDevice, cap_present,
> >                      QEMU_PCIE_SLTCAP_PCP_BITNR, true),
> > --
> > 1.9.0
> 
> That's OK. But the declaration of header file should also be removed.
> I noticed MST have pulled into master tree. So, maybe I can post
> another patch to fix this trivial problem.

Yes, definitely, an obvious mistake from me, sorry.
Don't understand how I could have overlooked that..

Knut

> Best regards,
> -Gonglei

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

[Michael S. Tsirkin]

On Mon, Aug 25, 2014 at 11:13:01AM +0200, Knut Omang wrote:
> 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

Makes sense to me.

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