[Qemu-devel] [PATCH v2 0/2] pcie: Add simple ACS "support" to the generic PCIe root port

[Qemu-devel] [PATCH v2 0/2] pcie: Add simple ACS "support" to the generic PCIe root port

[Knut Omang]

These two patches together implements a PCIe capability
config space header for Access Control Services (ACS) for the
new Qemu specific generic root port. ACS support in the
associated root port is a prerequisite to be able to pass
individual functions of a device populating the port through to
an L2 guest from an unmodified kernel.

Without this, the IOMMU group the device belongs to will also
include the root port itself, and all functions the device provides.

It is necessary to support SR/IOV where the primary
purpose is to be able to share out individual VFs to different
guests, which will not be permitted by VFIO or the Windows Hyper-V equivalent
unless ACS is supported by the root port.

These patches can also be found as part of an updated version of
my SR/IOV emulation patch set at

  https://github.com/knuto/qemu/tree/sriov_patches_v10

Changes from v1:
----------------
Incorporated feedback from Alex Williamson:

- Make commit messages reflect a more correct understanding of how this
  affects VFIO operation.
- Implemented the CTRL register properly (reset callback + making non-implemented
  capabilities RO, default value 0)
- removed the egress ctrl vector parameter to the init function
- Fixed some whitespace issues

Knut Omang (2):
  pcie: Add a simple PCIe ACS (Access Control Services) helper function
  gen_pcie_root_port: Add ACS (Access Control Services) capability

 hw/pci-bridge/gen_pcie_root_port.c |  2 ++
 hw/pci-bridge/pcie_root_port.c     |  4 ++++
 hw/pci/pcie.c                      | 21 +++++++++++++++++++++
 include/hw/pci/pcie.h              |  6 ++++++
 include/hw/pci/pcie_port.h         |  1 +
 include/hw/pci/pcie_regs.h         |  4 ++++
 6 files changed, 38 insertions(+)

base-commit: a8d2b0685681e2f291faaa501efbbd76875f8ec8
-- 
git-series 0.9.1

[Qemu-devel] [PATCH v2 1/2] pcie: Add a simple PCIe ACS (Access Control Services) helper function

[Knut Omang]

Add a helper function to add PCIe capability for Access Control Services (ACS)
ACS support in the associated root port is a prerequisite to be able to do
passthrough of individual functions of a device with VFIO
without Alex Williamson's pcie_acs_override kernel patch or similar
in the guest.

Signed-off-by: Knut Omang <knut.omang@oracle.com>
---
 hw/pci/pcie.c              | 21 +++++++++++++++++++++
 include/hw/pci/pcie.h      |  6 ++++++
 include/hw/pci/pcie_regs.h |  4 ++++
 3 files changed, 31 insertions(+)

diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index 230478f..5ab3d1d 100644
--- a/hw/pci/pcie.c
+++ b/hw/pci/pcie.c
@@ -742,6 +742,13 @@ bool pcie_cap_is_arifwd_enabled(const PCIDevice *dev)
         PCI_EXP_DEVCTL2_ARI;
 }
 
+/* Access Control Services (ACS)
+ */
+void pcie_cap_acs_reset(PCIDevice *dev)
+{
+    pci_set_word(dev->config + dev->exp.acs_cap + PCI_ACS_CTRL, 0);
+}
+
 /**************************************************************************
  * pci express extended capability list management functions
  * uint16_t ext_cap_id (16 bit)
@@ -906,3 +913,17 @@ void pcie_ats_init(PCIDevice *dev, uint16_t offset)
 
     pci_set_word(dev->wmask + dev->exp.ats_cap + PCI_ATS_CTRL, 0x800f);
 }
+
+/* ACS (Access Control Services) */
+void pcie_acs_init(PCIDevice *dev, uint16_t offset)
+{
+    pcie_add_capability(dev, PCI_EXT_CAP_ID_ACS, PCI_ACS_VER,
+                        offset, PCI_ACS_SIZEOF);
+    dev->exp.acs_cap = offset;
+    pci_set_word(dev->config + offset + PCI_ACS_CAP,
+                 PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_UF);
+
+    pci_set_word(dev->config + offset + PCI_ACS_CTRL, 0);
+    pci_set_word(dev->wmask + offset + PCI_ACS_CTRL,
+                 PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_UF);
+}
diff --git a/include/hw/pci/pcie.h b/include/hw/pci/pcie.h
index 5b82a0d..4c40711 100644
--- a/include/hw/pci/pcie.h
+++ b/include/hw/pci/pcie.h
@@ -79,6 +79,9 @@ struct PCIExpressDevice {
 
     /* Offset of ATS capability in config space */
     uint16_t ats_cap;
+
+    /* ACS */
+    uint16_t acs_cap;
 };
 
 #define COMPAT_PROP_PCP "power_controller_present"
@@ -116,6 +119,8 @@ 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_acs_reset(PCIDevice *dev);
+
 /* ARI forwarding capability and control */
 void pcie_cap_arifwd_init(PCIDevice *dev);
 void pcie_cap_arifwd_reset(PCIDevice *dev);
@@ -129,6 +134,7 @@ void pcie_add_capability(PCIDevice *dev,
 void pcie_sync_bridge_lnk(PCIDevice *dev);
 
 void pcie_ari_init(PCIDevice *dev, uint16_t offset, uint16_t nextfn);
+void pcie_acs_init(PCIDevice *dev, uint16_t offset);
 void pcie_dev_ser_num_init(PCIDevice *dev, uint16_t offset, uint64_t ser_num);
 void pcie_ats_init(PCIDevice *dev, uint16_t offset);
 
diff --git a/include/hw/pci/pcie_regs.h b/include/hw/pci/pcie_regs.h
index ad4e780..3fc9aca 100644
--- a/include/hw/pci/pcie_regs.h
+++ b/include/hw/pci/pcie_regs.h
@@ -175,4 +175,8 @@ typedef enum PCIExpLinkWidth {
                                          PCI_ERR_COR_INTERNAL |         \
                                          PCI_ERR_COR_HL_OVERFLOW)
 
+/* ACS */
+#define PCI_ACS_VER                     0x2
+#define PCI_ACS_SIZEOF                  8
+
 #endif /* QEMU_PCIE_REGS_H */
-- 
git-series 0.9.1

[Qemu-devel] [PATCH v2 2/2] gen_pcie_root_port: Add ACS (Access Control Services) capability

[Knut Omang]

Claim ACS support in the generic PCIe root port to allow
passthrough of individual functions of a device to different
guests (in a nested virt.setting) with VFIO.
Without this patch, all functions of a device, such as all VFs of
an SR/IOV device, will end up in the same IOMMU group.
A similar situation occurs on Windows with Hyper-V.

In the single function device case, it also has a small cosmetic
benefit in that the root port itself is not grouped with
the device. VFIO handles that situation in that binding rules
only apply to endpoints, so it does not limit passthrough in
those cases.

Signed-off-by: Knut Omang <knut.omang@oracle.com>
---
 hw/pci-bridge/gen_pcie_root_port.c | 2 ++
 hw/pci-bridge/pcie_root_port.c     | 4 ++++
 include/hw/pci/pcie_port.h         | 1 +
 3 files changed, 7 insertions(+)

diff --git a/hw/pci-bridge/gen_pcie_root_port.c b/hw/pci-bridge/gen_pcie_root_port.c
index 9766edb..b5a5ecc 100644
--- a/hw/pci-bridge/gen_pcie_root_port.c
+++ b/hw/pci-bridge/gen_pcie_root_port.c
@@ -20,6 +20,7 @@
         OBJECT_CHECK(GenPCIERootPort, (obj), TYPE_GEN_PCIE_ROOT_PORT)
 
 #define GEN_PCIE_ROOT_PORT_AER_OFFSET           0x100
+#define GEN_PCIE_ROOT_PORT_ACS_OFFSET           0x148
 #define GEN_PCIE_ROOT_PORT_MSIX_NR_VECTOR       1
 
 typedef struct GenPCIERootPort {
@@ -149,6 +150,7 @@ static void gen_rp_dev_class_init(ObjectClass *klass, void *data)
     rpc->interrupts_init = gen_rp_interrupts_init;
     rpc->interrupts_uninit = gen_rp_interrupts_uninit;
     rpc->aer_offset = GEN_PCIE_ROOT_PORT_AER_OFFSET;
+    rpc->acs_offset = GEN_PCIE_ROOT_PORT_ACS_OFFSET;
 }
 
 static const TypeInfo gen_rp_dev_info = {
diff --git a/hw/pci-bridge/pcie_root_port.c b/hw/pci-bridge/pcie_root_port.c
index 34ad767..a0b4cf7 100644
--- a/hw/pci-bridge/pcie_root_port.c
+++ b/hw/pci-bridge/pcie_root_port.c
@@ -47,6 +47,7 @@ static void rp_reset(DeviceState *qdev)
     pcie_cap_deverr_reset(d);
     pcie_cap_slot_reset(d);
     pcie_cap_arifwd_reset(d);
+    pcie_cap_acs_reset(d);
     pcie_aer_root_reset(d);
     pci_bridge_reset(qdev);
     pci_bridge_disable_base_limit(d);
@@ -106,6 +107,9 @@ static void rp_realize(PCIDevice *d, Error **errp)
     pcie_aer_root_init(d);
     rp_aer_vector_update(d);
 
+    if (rpc->acs_offset) {
+        pcie_acs_init(d, rpc->acs_offset);
+    }
     return;
 
 err:
diff --git a/include/hw/pci/pcie_port.h b/include/hw/pci/pcie_port.h
index df242a0..09586f4 100644
--- a/include/hw/pci/pcie_port.h
+++ b/include/hw/pci/pcie_port.h
@@ -78,6 +78,7 @@ typedef struct PCIERootPortClass {
     int exp_offset;
     int aer_offset;
     int ssvid_offset;
+    int acs_offset;    /* If nonzero, optional ACS capability offset */
     int ssid;
 } PCIERootPortClass;
 
-- 
git-series 0.9.1

Re: [Qemu-devel] [PATCH v2 1/2] pcie: Add a simple PCIe ACS (Access Control Services) helper function

[Alex Williamson]

On Thu, 24 Jan 2019 11:12:52 +0100
Knut Omang <knut.omang@oracle.com> wrote:

> Add a helper function to add PCIe capability for Access Control Services (ACS)
> ACS support in the associated root port is a prerequisite to be able to do
> passthrough of individual functions of a device with VFIO
> without Alex Williamson's pcie_acs_override kernel patch or similar
> in the guest.
> 
> Signed-off-by: Knut Omang <knut.omang@oracle.com>
> ---
>  hw/pci/pcie.c              | 21 +++++++++++++++++++++
>  include/hw/pci/pcie.h      |  6 ++++++
>  include/hw/pci/pcie_regs.h |  4 ++++
>  3 files changed, 31 insertions(+)
> 
> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> index 230478f..5ab3d1d 100644
> --- a/hw/pci/pcie.c
> +++ b/hw/pci/pcie.c
> @@ -742,6 +742,13 @@ bool pcie_cap_is_arifwd_enabled(const PCIDevice *dev)
>          PCI_EXP_DEVCTL2_ARI;
>  }
>  
> +/* Access Control Services (ACS)
> + */

Comment style

> +void pcie_cap_acs_reset(PCIDevice *dev)
> +{
> +    pci_set_word(dev->config + dev->exp.acs_cap + PCI_ACS_CTRL, 0);
> +}
> +
>  /**************************************************************************
>   * pci express extended capability list management functions
>   * uint16_t ext_cap_id (16 bit)
> @@ -906,3 +913,17 @@ void pcie_ats_init(PCIDevice *dev, uint16_t offset)
>  
>      pci_set_word(dev->wmask + dev->exp.ats_cap + PCI_ATS_CTRL, 0x800f);
>  }
> +
> +/* ACS (Access Control Services) */
> +void pcie_acs_init(PCIDevice *dev, uint16_t offset)
> +{
> +    pcie_add_capability(dev, PCI_EXT_CAP_ID_ACS, PCI_ACS_VER,
> +                        offset, PCI_ACS_SIZEOF);
> +    dev->exp.acs_cap = offset;
> +    pci_set_word(dev->config + offset + PCI_ACS_CAP,
> +                 PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_UF);

This is still only valid for downstream ports yet neither restricted
nor commented do indicate that.  You could use an object_dynamic_cast
to triggger an assert should someone use it for an invalid type of
device, ex:

assert(object_dynamic_cast(OBJECT(dev), TYPE_PCIE_SLOT));

> +
> +    pci_set_word(dev->config + offset + PCI_ACS_CTRL, 0);

Suspect this is unnecessary given the reset callback.

> +    pci_set_word(dev->wmask + offset + PCI_ACS_CTRL,
> +                 PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_UF);
> +}
> diff --git a/include/hw/pci/pcie.h b/include/hw/pci/pcie.h
> index 5b82a0d..4c40711 100644
> --- a/include/hw/pci/pcie.h
> +++ b/include/hw/pci/pcie.h
> @@ -79,6 +79,9 @@ struct PCIExpressDevice {
>  
>      /* Offset of ATS capability in config space */
>      uint16_t ats_cap;
> +
> +    /* ACS */
> +    uint16_t acs_cap;
>  };
>  
>  #define COMPAT_PROP_PCP "power_controller_present"
> @@ -116,6 +119,8 @@ 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_acs_reset(PCIDevice *dev);
> +
>  /* ARI forwarding capability and control */
>  void pcie_cap_arifwd_init(PCIDevice *dev);
>  void pcie_cap_arifwd_reset(PCIDevice *dev);
> @@ -129,6 +134,7 @@ void pcie_add_capability(PCIDevice *dev,
>  void pcie_sync_bridge_lnk(PCIDevice *dev);
>  
>  void pcie_ari_init(PCIDevice *dev, uint16_t offset, uint16_t nextfn);
> +void pcie_acs_init(PCIDevice *dev, uint16_t offset);
>  void pcie_dev_ser_num_init(PCIDevice *dev, uint16_t offset, uint64_t ser_num);
>  void pcie_ats_init(PCIDevice *dev, uint16_t offset);
>  
> diff --git a/include/hw/pci/pcie_regs.h b/include/hw/pci/pcie_regs.h
> index ad4e780..3fc9aca 100644
> --- a/include/hw/pci/pcie_regs.h
> +++ b/include/hw/pci/pcie_regs.h
> @@ -175,4 +175,8 @@ typedef enum PCIExpLinkWidth {
>                                           PCI_ERR_COR_INTERNAL |         \
>                                           PCI_ERR_COR_HL_OVERFLOW)
>  
> +/* ACS */
> +#define PCI_ACS_VER                     0x2

There's no such version, even the PCIe 5.0 drafts only define version 1.

> +#define PCI_ACS_SIZEOF                  8
> +
>  #endif /* QEMU_PCIE_REGS_H */

Re: [Qemu-devel] [PATCH v2 2/2] gen_pcie_root_port: Add ACS (Access Control Services) capability

[Alex Williamson]

On Thu, 24 Jan 2019 11:12:53 +0100
Knut Omang <knut.omang@oracle.com> wrote:

> Claim ACS support in the generic PCIe root port to allow
> passthrough of individual functions of a device to different
> guests (in a nested virt.setting) with VFIO.
> Without this patch, all functions of a device, such as all VFs of
> an SR/IOV device, will end up in the same IOMMU group.
> A similar situation occurs on Windows with Hyper-V.
> 
> In the single function device case, it also has a small cosmetic
> benefit in that the root port itself is not grouped with
> the device. VFIO handles that situation in that binding rules
> only apply to endpoints, so it does not limit passthrough in
> those cases.
> 
> Signed-off-by: Knut Omang <knut.omang@oracle.com>
> ---
>  hw/pci-bridge/gen_pcie_root_port.c | 2 ++
>  hw/pci-bridge/pcie_root_port.c     | 4 ++++
>  include/hw/pci/pcie_port.h         | 1 +
>  3 files changed, 7 insertions(+)
> 
> diff --git a/hw/pci-bridge/gen_pcie_root_port.c b/hw/pci-bridge/gen_pcie_root_port.c
> index 9766edb..b5a5ecc 100644
> --- a/hw/pci-bridge/gen_pcie_root_port.c
> +++ b/hw/pci-bridge/gen_pcie_root_port.c
> @@ -20,6 +20,7 @@
>          OBJECT_CHECK(GenPCIERootPort, (obj), TYPE_GEN_PCIE_ROOT_PORT)
>  
>  #define GEN_PCIE_ROOT_PORT_AER_OFFSET           0x100
> +#define GEN_PCIE_ROOT_PORT_ACS_OFFSET           0x148

So you prefer that everyone passing through here decode these to figure
out that ACS_OFFSET is (AER_OFFSET + ERR_SIZEOF) since my comment on v1
was ignored?

>  #define GEN_PCIE_ROOT_PORT_MSIX_NR_VECTOR       1
>  
>  typedef struct GenPCIERootPort {
> @@ -149,6 +150,7 @@ static void gen_rp_dev_class_init(ObjectClass *klass, void *data)
>      rpc->interrupts_init = gen_rp_interrupts_init;
>      rpc->interrupts_uninit = gen_rp_interrupts_uninit;
>      rpc->aer_offset = GEN_PCIE_ROOT_PORT_AER_OFFSET;
> +    rpc->acs_offset = GEN_PCIE_ROOT_PORT_ACS_OFFSET;
>  }
>  
>  static const TypeInfo gen_rp_dev_info = {
> diff --git a/hw/pci-bridge/pcie_root_port.c b/hw/pci-bridge/pcie_root_port.c
> index 34ad767..a0b4cf7 100644
> --- a/hw/pci-bridge/pcie_root_port.c
> +++ b/hw/pci-bridge/pcie_root_port.c
> @@ -47,6 +47,7 @@ static void rp_reset(DeviceState *qdev)
>      pcie_cap_deverr_reset(d);
>      pcie_cap_slot_reset(d);
>      pcie_cap_arifwd_reset(d);
> +    pcie_cap_acs_reset(d);

Only the generic root port initializes acs_offset to enable an ACS
capability, but all members of the device class call the reset function
which does no checking that an ACS capability exists.  We've just
corrupted config space for the device.

>      pcie_aer_root_reset(d);
>      pci_bridge_reset(qdev);
>      pci_bridge_disable_base_limit(d);
> @@ -106,6 +107,9 @@ static void rp_realize(PCIDevice *d, Error **errp)
>      pcie_aer_root_init(d);
>      rp_aer_vector_update(d);
>  
> +    if (rpc->acs_offset) {
> +        pcie_acs_init(d, rpc->acs_offset);
> +    }
>      return;
>  
>  err:
> diff --git a/include/hw/pci/pcie_port.h b/include/hw/pci/pcie_port.h
> index df242a0..09586f4 100644
> --- a/include/hw/pci/pcie_port.h
> +++ b/include/hw/pci/pcie_port.h
> @@ -78,6 +78,7 @@ typedef struct PCIERootPortClass {
>      int exp_offset;
>      int aer_offset;
>      int ssvid_offset;
> +    int acs_offset;    /* If nonzero, optional ACS capability offset */
>      int ssid;
>  } PCIERootPortClass;
>  

Re: [Qemu-devel] [PATCH v2 1/2] pcie: Add a simple PCIe ACS (Access Control Services) helper function

[Knut Omang]

On Thu, 2019-01-24 at 10:22 -0700, Alex Williamson wrote:
> On Thu, 24 Jan 2019 11:12:52 +0100
> Knut Omang <knut.omang@oracle.com> wrote:
> 
> > Add a helper function to add PCIe capability for Access Control Services (ACS)
> > ACS support in the associated root port is a prerequisite to be able to do
> > passthrough of individual functions of a device with VFIO
> > without Alex Williamson's pcie_acs_override kernel patch or similar
> > in the guest.
> > 
> > Signed-off-by: Knut Omang <knut.omang@oracle.com>
> > ---
> >  hw/pci/pcie.c              | 21 +++++++++++++++++++++
> >  include/hw/pci/pcie.h      |  6 ++++++
> >  include/hw/pci/pcie_regs.h |  4 ++++
> >  3 files changed, 31 insertions(+)
> > 
> > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> > index 230478f..5ab3d1d 100644
> > --- a/hw/pci/pcie.c
> > +++ b/hw/pci/pcie.c
> > @@ -742,6 +742,13 @@ bool pcie_cap_is_arifwd_enabled(const PCIDevice *dev)
> >          PCI_EXP_DEVCTL2_ARI;
> >  }
> >  
> > +/* Access Control Services (ACS)
> > + */
> 
> Comment style
>
> > +void pcie_cap_acs_reset(PCIDevice *dev)
> > +{
> > +    pci_set_word(dev->config + dev->exp.acs_cap + PCI_ACS_CTRL, 0);
> > +}
> > +
> >  /**************************************************************************
> >   * pci express extended capability list management functions
> >   * uint16_t ext_cap_id (16 bit)
> > @@ -906,3 +913,17 @@ void pcie_ats_init(PCIDevice *dev, uint16_t offset)
> >  
> >      pci_set_word(dev->wmask + dev->exp.ats_cap + PCI_ATS_CTRL, 0x800f);
> >  }
> > +
> > +/* ACS (Access Control Services) */
> > +void pcie_acs_init(PCIDevice *dev, uint16_t offset)
> > +{
> > +    pcie_add_capability(dev, PCI_EXT_CAP_ID_ACS, PCI_ACS_VER,
> > +                        offset, PCI_ACS_SIZEOF);
> > +    dev->exp.acs_cap = offset;
> > +    pci_set_word(dev->config + offset + PCI_ACS_CAP,
> > +                 PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_UF);
> 
> This is still only valid for downstream ports yet neither restricted
> nor commented do indicate that.  You could use an object_dynamic_cast
> to triggger an assert should someone use it for an invalid type of
> device, ex:
> 
> assert(object_dynamic_cast(OBJECT(dev), TYPE_PCIE_SLOT));

Sorry, I didn't realize what you meant with v1 - this evolved from 
just a fix in the implementation of ioh3420 to a fix in the generic code, 
which I now realize of course is also used for downstream ports...

> > +
> > +    pci_set_word(dev->config + offset + PCI_ACS_CTRL, 0);
> 
> Suspect this is unnecessary given the reset callback.

ok

> > +    pci_set_word(dev->wmask + offset + PCI_ACS_CTRL,
> > +                 PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_UF);
> > +}
> > diff --git a/include/hw/pci/pcie.h b/include/hw/pci/pcie.h
> > index 5b82a0d..4c40711 100644
> > --- a/include/hw/pci/pcie.h
> > +++ b/include/hw/pci/pcie.h
> > @@ -79,6 +79,9 @@ struct PCIExpressDevice {
> >  
> >      /* Offset of ATS capability in config space */
> >      uint16_t ats_cap;
> > +
> > +    /* ACS */
> > +    uint16_t acs_cap;
> >  };
> >  
> >  #define COMPAT_PROP_PCP "power_controller_present"
> > @@ -116,6 +119,8 @@ 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_acs_reset(PCIDevice *dev);
> > +
> >  /* ARI forwarding capability and control */
> >  void pcie_cap_arifwd_init(PCIDevice *dev);
> >  void pcie_cap_arifwd_reset(PCIDevice *dev);
> > @@ -129,6 +134,7 @@ void pcie_add_capability(PCIDevice *dev,
> >  void pcie_sync_bridge_lnk(PCIDevice *dev);
> >  
> >  void pcie_ari_init(PCIDevice *dev, uint16_t offset, uint16_t nextfn);
> > +void pcie_acs_init(PCIDevice *dev, uint16_t offset);
> >  void pcie_dev_ser_num_init(PCIDevice *dev, uint16_t offset, uint64_t ser_num);
> >  void pcie_ats_init(PCIDevice *dev, uint16_t offset);
> >  
> > diff --git a/include/hw/pci/pcie_regs.h b/include/hw/pci/pcie_regs.h
> > index ad4e780..3fc9aca 100644
> > --- a/include/hw/pci/pcie_regs.h
> > +++ b/include/hw/pci/pcie_regs.h
> > @@ -175,4 +175,8 @@ typedef enum PCIExpLinkWidth {
> >                                           PCI_ERR_COR_INTERNAL |         \
> >                                           PCI_ERR_COR_HL_OVERFLOW)
> >  
> > +/* ACS */
> > +#define PCI_ACS_VER                     0x2
> 
> There's no such version, even the PCIe 5.0 drafts only define version 1.

Hmm - I have no idea how it ended up as 2 in the first place - my model device is of
course also v1 - will fix it.

Thanks!
Knut

> > +#define PCI_ACS_SIZEOF                  8
> > +
> >  #endif /* QEMU_PCIE_REGS_H */
> 

Re: [Qemu-devel] [PATCH v2 2/2] gen_pcie_root_port: Add ACS (Access Control Services) capability

[Knut Omang]

On Thu, 2019-01-24 at 10:33 -0700, Alex Williamson wrote:
> On Thu, 24 Jan 2019 11:12:53 +0100
> Knut Omang <knut.omang@oracle.com> wrote:
> 
> > Claim ACS support in the generic PCIe root port to allow
> > passthrough of individual functions of a device to different
> > guests (in a nested virt.setting) with VFIO.
> > Without this patch, all functions of a device, such as all VFs of
> > an SR/IOV device, will end up in the same IOMMU group.
> > A similar situation occurs on Windows with Hyper-V.
> > 
> > In the single function device case, it also has a small cosmetic
> > benefit in that the root port itself is not grouped with
> > the device. VFIO handles that situation in that binding rules
> > only apply to endpoints, so it does not limit passthrough in
> > those cases.
> > 
> > Signed-off-by: Knut Omang <knut.omang@oracle.com>
> > ---
> >  hw/pci-bridge/gen_pcie_root_port.c | 2 ++
> >  hw/pci-bridge/pcie_root_port.c     | 4 ++++
> >  include/hw/pci/pcie_port.h         | 1 +
> >  3 files changed, 7 insertions(+)
> > 
> > diff --git a/hw/pci-bridge/gen_pcie_root_port.c b/hw/pci-bridge/gen_pcie_root_port.c
> > index 9766edb..b5a5ecc 100644
> > --- a/hw/pci-bridge/gen_pcie_root_port.c
> > +++ b/hw/pci-bridge/gen_pcie_root_port.c
> > @@ -20,6 +20,7 @@
> >          OBJECT_CHECK(GenPCIERootPort, (obj), TYPE_GEN_PCIE_ROOT_PORT)
> >  
> >  #define GEN_PCIE_ROOT_PORT_AER_OFFSET           0x100
> > +#define GEN_PCIE_ROOT_PORT_ACS_OFFSET           0x148
> 
> So you prefer that everyone passing through here decode these to figure
> out that ACS_OFFSET is (AER_OFFSET + ERR_SIZEOF) since my comment on v1
> was ignored?

Sorry, not at all - I managed to overlook your comment - will fix it,

> >  #define GEN_PCIE_ROOT_PORT_MSIX_NR_VECTOR       1
> >  
> >  typedef struct GenPCIERootPort {
> > @@ -149,6 +150,7 @@ static void gen_rp_dev_class_init(ObjectClass *klass, void *data)
> >      rpc->interrupts_init = gen_rp_interrupts_init;
> >      rpc->interrupts_uninit = gen_rp_interrupts_uninit;
> >      rpc->aer_offset = GEN_PCIE_ROOT_PORT_AER_OFFSET;
> > +    rpc->acs_offset = GEN_PCIE_ROOT_PORT_ACS_OFFSET;
> >  }
> >  
> >  static const TypeInfo gen_rp_dev_info = {
> > diff --git a/hw/pci-bridge/pcie_root_port.c b/hw/pci-bridge/pcie_root_port.c
> > index 34ad767..a0b4cf7 100644
> > --- a/hw/pci-bridge/pcie_root_port.c
> > +++ b/hw/pci-bridge/pcie_root_port.c
> > @@ -47,6 +47,7 @@ static void rp_reset(DeviceState *qdev)
> >      pcie_cap_deverr_reset(d);
> >      pcie_cap_slot_reset(d);
> >      pcie_cap_arifwd_reset(d);
> > +    pcie_cap_acs_reset(d);
> 
> Only the generic root port initializes acs_offset to enable an ACS
> capability, but all members of the device class call the reset function
> which does no checking that an ACS capability exists.  We've just
> corrupted config space for the device.

Ouch! Not good at all, sorry!
Will look at it (after a good night's sleep this time..)

Thanks!
Knut

> >      pcie_aer_root_reset(d);
> >      pci_bridge_reset(qdev);
> >      pci_bridge_disable_base_limit(d);
> > @@ -106,6 +107,9 @@ static void rp_realize(PCIDevice *d, Error **errp)
> >      pcie_aer_root_init(d);
> >      rp_aer_vector_update(d);
> >  
> > +    if (rpc->acs_offset) {
> > +        pcie_acs_init(d, rpc->acs_offset);
> > +    }
> >      return;
> >  
> >  err:
> > diff --git a/include/hw/pci/pcie_port.h b/include/hw/pci/pcie_port.h
> > index df242a0..09586f4 100644
> > --- a/include/hw/pci/pcie_port.h
> > +++ b/include/hw/pci/pcie_port.h
> > @@ -78,6 +78,7 @@ typedef struct PCIERootPortClass {
> >      int exp_offset;
> >      int aer_offset;
> >      int ssvid_offset;
> > +    int acs_offset;    /* If nonzero, optional ACS capability offset */
> >      int ssid;
> >  } PCIERootPortClass;
> >  
>