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

[Qemu-devel] [PATCH 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 the a function of
the 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.

With an SR/IOV device this becomes even more important, as the whole
purpose with SR/IOV 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_v9

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/ioh3420.c            |  1 -
 hw/pci-bridge/pcie_root_port.c     |  3 +++
 hw/pci/pcie.c                      | 14 ++++++++++++++
 include/hw/pci/pcie.h              |  1 +
 include/hw/pci/pcie_port.h         |  1 +
 include/hw/pci/pcie_regs.h         |  4 ++++
 7 files changed, 25 insertions(+), 1 deletion(-)

base-commit: a8d2b0685681e2f291faaa501efbbd76875f8ec8
-- 
git-series 0.9.1

[Qemu-devel] [PATCH 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 useful
passthrough with VFIO without Alex Williamson's pcie_acs_override kernel patch.

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

diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index 230478f..18feff5 100644
--- a/hw/pci/pcie.c
+++ b/hw/pci/pcie.c
@@ -906,3 +906,17 @@ void pcie_ats_init(PCIDevice *dev, uint16_t offset)
 
     pci_set_word(dev->wmask + dev->exp.ats_cap + PCI_ATS_CTRL, 0x800f);
 }
+
+/* Add an ACS (Access Control Services) capability */
+void pcie_acs_init(PCIDevice *dev, uint16_t offset, uint8_t egress_ctrl_vec_sz)
+{
+    int ectrl_words = (egress_ctrl_vec_sz + 31) & ~31;
+    pcie_add_capability(dev, PCI_EXT_CAP_ID_ACS, PCI_ACS_VER,
+                        offset, PCI_ACS_SIZEOF + ectrl_words);
+    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,
+                 PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_UF);
+    /* Make CTRL register writable */
+    memset(dev->wmask + offset + PCI_ACS_CTRL, 0xff, 2);
+}
diff --git a/include/hw/pci/pcie.h b/include/hw/pci/pcie.h
index 5b82a0d..c2da148 100644
--- a/include/hw/pci/pcie.h
+++ b/include/hw/pci/pcie.h
@@ -129,6 +129,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, uint8_t egress_ctrl_vec_sz);
 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..5e7409c 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 2/2] gen_pcie_root_port: Add ACS (Access Control Services) capability

[Knut Omang]

Claiming ACS support allows passthrough of an emulated device
(in a nested virt.setting) with VFIO without Alex Williamson's patch
for the pcie_acs_override kernel parameter.
A similar need appears on Windows with Hyper-V

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

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/ioh3420.c b/hw/pci-bridge/ioh3420.c
index 81f2de6..2064939 100644
--- a/hw/pci-bridge/ioh3420.c
+++ b/hw/pci-bridge/ioh3420.c
@@ -71,7 +71,6 @@ static int ioh3420_interrupts_init(PCIDevice *d, Error **errp)
     if (rc < 0) {
         assert(rc == -ENOTSUP);
     }
-
     return rc;
 }
 
diff --git a/hw/pci-bridge/pcie_root_port.c b/hw/pci-bridge/pcie_root_port.c
index 34ad767..c33a493 100644
--- a/hw/pci-bridge/pcie_root_port.c
+++ b/hw/pci-bridge/pcie_root_port.c
@@ -106,6 +106,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, 0);
+    }
     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 1/2] pcie: Add a simple PCIe ACS (Access Control Services) helper function

[Alex Williamson]

On Wed, 23 Jan 2019 19:27:59 +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 useful
> passthrough with VFIO without Alex Williamson's pcie_acs_override kernel patch.

Define "useful".  We can certainly still assign single function PFs to
an L2 guest, or multi-function so long as all the functions are
assigned.  I won't deny that it's problematic, but it's a virtual
topology that can be adjusted, so I think this is overstating things a
bit.

> Signed-off-by: Knut Omang <knut.omang@oracle.com>
> ---
>  hw/pci/pcie.c              | 14 ++++++++++++++
>  include/hw/pci/pcie.h      |  1 +
>  include/hw/pci/pcie_regs.h |  4 ++++
>  3 files changed, 19 insertions(+)
> 
> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> index 230478f..18feff5 100644
> --- a/hw/pci/pcie.c
> +++ b/hw/pci/pcie.c
> @@ -906,3 +906,17 @@ void pcie_ats_init(PCIDevice *dev, uint16_t offset)
>  
>      pci_set_word(dev->wmask + dev->exp.ats_cap + PCI_ATS_CTRL, 0x800f);
>  }
> +
> +/* Add an ACS (Access Control Services) capability */
> +void pcie_acs_init(PCIDevice *dev, uint16_t offset, uint8_t egress_ctrl_vec_sz)
> +{
> +    int ectrl_words = (egress_ctrl_vec_sz + 31) & ~31;
> +    pcie_add_capability(dev, PCI_EXT_CAP_ID_ACS, PCI_ACS_VER,
> +                        offset, PCI_ACS_SIZEOF + ectrl_words);

The egress control vector is only valid if the egress control
capability is enabled, which is not set below, so this just seems to
waste config space and introduces a meaningless function arg.

> +    pci_set_word(dev->config + offset + PCI_ACS_CAP,
> +                 PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_UF);

Some of these bits are only valid for downstream ports, it would
violate the spec to set them on and endpoint.

> +    pci_set_word(dev->config + offset + PCI_ACS_CTRL,
> +                 PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_UF);

The default values of the control register bits is zero, so we
shouldn't be setting it here and we should have a reset hook to clear
it.

> +    /* Make CTRL register writable */
> +    memset(dev->wmask + offset + PCI_ACS_CTRL, 0xff, 2);
> +}
> diff --git a/include/hw/pci/pcie.h b/include/hw/pci/pcie.h
> index 5b82a0d..c2da148 100644
> --- a/include/hw/pci/pcie.h
> +++ b/include/hw/pci/pcie.h
> @@ -129,6 +129,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, uint8_t egress_ctrl_vec_sz);
>  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..5e7409c 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 */

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

[Alex Williamson]

On Wed, 23 Jan 2019 19:28:00 +0100
Knut Omang <knut.omang@oracle.com> wrote:

> Claiming ACS support allows passthrough of an emulated device
> (in a nested virt.setting) with VFIO without Alex Williamson's patch
> for the pcie_acs_override kernel parameter.
> A similar need appears on Windows with Hyper-V
> 
> Signed-off-by: Knut Omang <knut.omang@oracle.com>
> ---
>  hw/pci-bridge/gen_pcie_root_port.c | 2 ++
>  hw/pci-bridge/ioh3420.c            | 1 -
>  hw/pci-bridge/pcie_root_port.c     | 3 +++
>  include/hw/pci/pcie_port.h         | 1 +
>  4 files changed, 6 insertions(+), 1 deletion(-)
> 
> 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

Perhaps (GEN_PCIE_ROOT_PORT_AER_OFFSET + PCI_ERR_SIZEOF)

>  #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/ioh3420.c b/hw/pci-bridge/ioh3420.c
> index 81f2de6..2064939 100644
> --- a/hw/pci-bridge/ioh3420.c
> +++ b/hw/pci-bridge/ioh3420.c
> @@ -71,7 +71,6 @@ static int ioh3420_interrupts_init(PCIDevice *d, Error **errp)
>      if (rc < 0) {
>          assert(rc == -ENOTSUP);
>      }
> -

Unrelated

>      return rc;
>  }
>  
> diff --git a/hw/pci-bridge/pcie_root_port.c b/hw/pci-bridge/pcie_root_port.c
> index 34ad767..c33a493 100644
> --- a/hw/pci-bridge/pcie_root_port.c
> +++ b/hw/pci-bridge/pcie_root_port.c
> @@ -106,6 +106,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, 0);
> +    }
>      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 1/2] pcie: Add a simple PCIe ACS (Access Control Services) helper function

[Knut Omang]

On Wed, 2019-01-23 at 12:04 -0700, Alex Williamson wrote:
> On Wed, 23 Jan 2019 19:27:59 +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 useful
> > passthrough with VFIO without Alex Williamson's pcie_acs_override kernel patch.
> 
> Define "useful".  

Hmm - just that without the patches, the root port itself 
also gets assigned to the same group, which seemed problematic to me
(without any further testing than just binding/unbinding to VFIO)

Knut

> We can certainly still assign single function PFs to
> an L2 guest, or multi-function so long as all the functions are
> assigned.  I won't deny that it's problematic, but it's a virtual
> topology that can be adjusted, so I think this is overstating things a
> bit.
> 
> > Signed-off-by: Knut Omang <knut.omang@oracle.com>
> > ---
> >  hw/pci/pcie.c              | 14 ++++++++++++++
> >  include/hw/pci/pcie.h      |  1 +
> >  include/hw/pci/pcie_regs.h |  4 ++++
> >  3 files changed, 19 insertions(+)
> > 
> > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> > index 230478f..18feff5 100644
> > --- a/hw/pci/pcie.c
> > +++ b/hw/pci/pcie.c
> > @@ -906,3 +906,17 @@ void pcie_ats_init(PCIDevice *dev, uint16_t offset)
> >  
> >      pci_set_word(dev->wmask + dev->exp.ats_cap + PCI_ATS_CTRL, 0x800f);
> >  }
> > +
> > +/* Add an ACS (Access Control Services) capability */
> > +void pcie_acs_init(PCIDevice *dev, uint16_t offset, uint8_t egress_ctrl_vec_sz)
> > +{
> > +    int ectrl_words = (egress_ctrl_vec_sz + 31) & ~31;
> > +    pcie_add_capability(dev, PCI_EXT_CAP_ID_ACS, PCI_ACS_VER,
> > +                        offset, PCI_ACS_SIZEOF + ectrl_words);
> 
> The egress control vector is only valid if the egress control
> capability is enabled, which is not set below, so this just seems to
> waste config space and introduces a meaningless function arg.
> 
> > +    pci_set_word(dev->config + offset + PCI_ACS_CAP,
> > +                 PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_UF);
> 
> Some of these bits are only valid for downstream ports, it would
> violate the spec to set them on and endpoint.
> 
> > +    pci_set_word(dev->config + offset + PCI_ACS_CTRL,
> > +                 PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_UF);
> 
> The default values of the control register bits is zero, so we
> shouldn't be setting it here and we should have a reset hook to clear
> it.
> 
> > +    /* Make CTRL register writable */
> > +    memset(dev->wmask + offset + PCI_ACS_CTRL, 0xff, 2);
> > +}
> > diff --git a/include/hw/pci/pcie.h b/include/hw/pci/pcie.h
> > index 5b82a0d..c2da148 100644
> > --- a/include/hw/pci/pcie.h
> > +++ b/include/hw/pci/pcie.h
> > @@ -129,6 +129,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, uint8_t egress_ctrl_vec_sz);
> >  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..5e7409c 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 */
> 

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

[Knut Omang]

On Wed, 2019-01-23 at 12:04 -0700, Alex Williamson wrote:
> On Wed, 23 Jan 2019 19:28:00 +0100
> Knut Omang <knut.omang@oracle.com> wrote:
> 
> > Claiming ACS support allows passthrough of an emulated device
> > (in a nested virt.setting) with VFIO without Alex Williamson's patch
> > for the pcie_acs_override kernel parameter.
> > A similar need appears on Windows with Hyper-V
> > 
> > Signed-off-by: Knut Omang <knut.omang@oracle.com>
> > ---
> >  hw/pci-bridge/gen_pcie_root_port.c | 2 ++
> >  hw/pci-bridge/ioh3420.c            | 1 -
> >  hw/pci-bridge/pcie_root_port.c     | 3 +++
> >  include/hw/pci/pcie_port.h         | 1 +
> >  4 files changed, 6 insertions(+), 1 deletion(-)
> > 
> > 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
> 
> Perhaps (GEN_PCIE_ROOT_PORT_AER_OFFSET + PCI_ERR_SIZEOF)
> 
> >  #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/ioh3420.c b/hw/pci-bridge/ioh3420.c
> > index 81f2de6..2064939 100644
> > --- a/hw/pci-bridge/ioh3420.c
> > +++ b/hw/pci-bridge/ioh3420.c
> > @@ -71,7 +71,6 @@ static int ioh3420_interrupts_init(PCIDevice *d, Error **errp)
> >      if (rc < 0) {
> >          assert(rc == -ENOTSUP);
> >      }
> > -
> 
> Unrelated

Oops, will fix, thanks.

Knut 

> 
> >      return rc;
> >  }
> >  
> > diff --git a/hw/pci-bridge/pcie_root_port.c b/hw/pci-bridge/pcie_root_port.c
> > index 34ad767..c33a493 100644
> > --- a/hw/pci-bridge/pcie_root_port.c
> > +++ b/hw/pci-bridge/pcie_root_port.c
> > @@ -106,6 +106,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, 0);
> > +    }
> >      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 1/2] pcie: Add a simple PCIe ACS (Access Control Services) helper function

[Alex Williamson]

On Wed, 23 Jan 2019 20:14:07 +0100
Knut Omang <knut.omang@oracle.com> wrote:

> On Wed, 2019-01-23 at 12:04 -0700, Alex Williamson wrote:
> > On Wed, 23 Jan 2019 19:27:59 +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 useful
> > > passthrough with VFIO without Alex Williamson's pcie_acs_override kernel patch.  
> > 
> > Define "useful".    
> 
> Hmm - just that without the patches, the root port itself 
> also gets assigned to the same group, which seemed problematic to me
> (without any further testing than just binding/unbinding to VFIO)

vfio-pci binding rules only apply to endpoints.  A root port lacking
ACS will include all devices downstream of it in the IOMMU group, and
potentially sibling functions, and devices downstream of those, but it
doesn't absolutely preclude L2 assignment, or L1 userspace usage,
which is already widely used.  It simply means that all the endpoints
within that group need to be bound to vfio-pci and can only have a
single owner.  Thanks,

Alex

> > We can certainly still assign single function PFs to
> > an L2 guest, or multi-function so long as all the functions are
> > assigned.  I won't deny that it's problematic, but it's a virtual
> > topology that can be adjusted, so I think this is overstating things a
> > bit.
> >   
> > > Signed-off-by: Knut Omang <knut.omang@oracle.com>
> > > ---
> > >  hw/pci/pcie.c              | 14 ++++++++++++++
> > >  include/hw/pci/pcie.h      |  1 +
> > >  include/hw/pci/pcie_regs.h |  4 ++++
> > >  3 files changed, 19 insertions(+)
> > > 
> > > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> > > index 230478f..18feff5 100644
> > > --- a/hw/pci/pcie.c
> > > +++ b/hw/pci/pcie.c
> > > @@ -906,3 +906,17 @@ void pcie_ats_init(PCIDevice *dev, uint16_t offset)
> > >  
> > >      pci_set_word(dev->wmask + dev->exp.ats_cap + PCI_ATS_CTRL, 0x800f);
> > >  }
> > > +
> > > +/* Add an ACS (Access Control Services) capability */
> > > +void pcie_acs_init(PCIDevice *dev, uint16_t offset, uint8_t egress_ctrl_vec_sz)
> > > +{
> > > +    int ectrl_words = (egress_ctrl_vec_sz + 31) & ~31;
> > > +    pcie_add_capability(dev, PCI_EXT_CAP_ID_ACS, PCI_ACS_VER,
> > > +                        offset, PCI_ACS_SIZEOF + ectrl_words);  
> > 
> > The egress control vector is only valid if the egress control
> > capability is enabled, which is not set below, so this just seems to
> > waste config space and introduces a meaningless function arg.
> >   
> > > +    pci_set_word(dev->config + offset + PCI_ACS_CAP,
> > > +                 PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_UF);  
> > 
> > Some of these bits are only valid for downstream ports, it would
> > violate the spec to set them on and endpoint.
> >   
> > > +    pci_set_word(dev->config + offset + PCI_ACS_CTRL,
> > > +                 PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_UF);  
> > 
> > The default values of the control register bits is zero, so we
> > shouldn't be setting it here and we should have a reset hook to clear
> > it.
> >   
> > > +    /* Make CTRL register writable */
> > > +    memset(dev->wmask + offset + PCI_ACS_CTRL, 0xff, 2);
> > > +}
> > > diff --git a/include/hw/pci/pcie.h b/include/hw/pci/pcie.h
> > > index 5b82a0d..c2da148 100644
> > > --- a/include/hw/pci/pcie.h
> > > +++ b/include/hw/pci/pcie.h
> > > @@ -129,6 +129,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, uint8_t egress_ctrl_vec_sz);
> > >  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..5e7409c 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 */  
> >   
> 

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

[Knut Omang]

On Wed, 2019-01-23 at 12:32 -0700, Alex Williamson wrote:
> On Wed, 23 Jan 2019 20:14:07 +0100
> Knut Omang <knut.omang@oracle.com> wrote:
> 
> > On Wed, 2019-01-23 at 12:04 -0700, Alex Williamson wrote:
> > > On Wed, 23 Jan 2019 19:27:59 +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 useful
> > > > passthrough with VFIO without Alex Williamson's pcie_acs_override kernel patch.  
> > > 
> > > Define "useful".    
> > 
> > Hmm - just that without the patches, the root port itself 
> > also gets assigned to the same group, which seemed problematic to me
> > (without any further testing than just binding/unbinding to VFIO)
> 
> vfio-pci binding rules only apply to endpoints.  A root port lacking
> ACS will include all devices downstream of it in the IOMMU group, and
> potentially sibling functions, and devices downstream of those, but it
> doesn't absolutely preclude L2 assignment, or L1 userspace usage,
> which is already widely used.  It simply means that all the endpoints
> within that group need to be bound to vfio-pci and can only have a
> single owner.  Thanks,

I see, that makes sense - I'll moderate my language!

Thanks,
Knut
> 
> Alex
> 
> > > We can certainly still assign single function PFs to
> > > an L2 guest, or multi-function so long as all the functions are
> > > assigned.  I won't deny that it's problematic, but it's a virtual
> > > topology that can be adjusted, so I think this is overstating things a
> > > bit.
> > >   
> > > > Signed-off-by: Knut Omang <knut.omang@oracle.com>
> > > > ---
> > > >  hw/pci/pcie.c              | 14 ++++++++++++++
> > > >  include/hw/pci/pcie.h      |  1 +
> > > >  include/hw/pci/pcie_regs.h |  4 ++++
> > > >  3 files changed, 19 insertions(+)
> > > > 
> > > > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> > > > index 230478f..18feff5 100644
> > > > --- a/hw/pci/pcie.c
> > > > +++ b/hw/pci/pcie.c
> > > > @@ -906,3 +906,17 @@ void pcie_ats_init(PCIDevice *dev, uint16_t offset)
> > > >  
> > > >      pci_set_word(dev->wmask + dev->exp.ats_cap + PCI_ATS_CTRL, 0x800f);
> > > >  }
> > > > +
> > > > +/* Add an ACS (Access Control Services) capability */
> > > > +void pcie_acs_init(PCIDevice *dev, uint16_t offset, uint8_t egress_ctrl_vec_sz)
> > > > +{
> > > > +    int ectrl_words = (egress_ctrl_vec_sz + 31) & ~31;
> > > > +    pcie_add_capability(dev, PCI_EXT_CAP_ID_ACS, PCI_ACS_VER,
> > > > +                        offset, PCI_ACS_SIZEOF + ectrl_words);  
> > > 
> > > The egress control vector is only valid if the egress control
> > > capability is enabled, which is not set below, so this just seems to
> > > waste config space and introduces a meaningless function arg.
> > >   
> > > > +    pci_set_word(dev->config + offset + PCI_ACS_CAP,
> > > > +                 PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR | PCI_ACS_CR |
> PCI_ACS_UF);  
> > > 
> > > Some of these bits are only valid for downstream ports, it would
> > > violate the spec to set them on and endpoint.
> > >   
> > > > +    pci_set_word(dev->config + offset + PCI_ACS_CTRL,
> > > > +                 PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR | PCI_ACS_CR |
> PCI_ACS_UF);  
> > > 
> > > The default values of the control register bits is zero, so we
> > > shouldn't be setting it here and we should have a reset hook to clear
> > > it.
> > >   
> > > > +    /* Make CTRL register writable */
> > > > +    memset(dev->wmask + offset + PCI_ACS_CTRL, 0xff, 2);
> > > > +}
> > > > diff --git a/include/hw/pci/pcie.h b/include/hw/pci/pcie.h
> > > > index 5b82a0d..c2da148 100644
> > > > --- a/include/hw/pci/pcie.h
> > > > +++ b/include/hw/pci/pcie.h
> > > > @@ -129,6 +129,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, uint8_t egress_ctrl_vec_sz);
> > > >  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..5e7409c 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 */  
> > >   
> > 
> 

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

[Knut Omang]

On Wed, 2019-01-23 at 12:04 -0700, Alex Williamson wrote:
> On Wed, 23 Jan 2019 19:27:59 +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 useful
> > passthrough with VFIO without Alex Williamson's pcie_acs_override kernel patch.
> 
> Define "useful".  We can certainly still assign single function PFs to
> an L2 guest, or multi-function so long as all the functions are
> assigned.  I won't deny that it's problematic, but it's a virtual
> topology that can be adjusted, so I think this is overstating things a
> bit.
> 
> > Signed-off-by: Knut Omang <knut.omang@oracle.com>
> > ---
> >  hw/pci/pcie.c              | 14 ++++++++++++++
> >  include/hw/pci/pcie.h      |  1 +
> >  include/hw/pci/pcie_regs.h |  4 ++++
> >  3 files changed, 19 insertions(+)
> > 
> > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> > index 230478f..18feff5 100644
> > --- a/hw/pci/pcie.c
> > +++ b/hw/pci/pcie.c
> > @@ -906,3 +906,17 @@ void pcie_ats_init(PCIDevice *dev, uint16_t offset)
> >  
> >      pci_set_word(dev->wmask + dev->exp.ats_cap + PCI_ATS_CTRL, 0x800f);
> >  }
> > +
> > +/* Add an ACS (Access Control Services) capability */
> > +void pcie_acs_init(PCIDevice *dev, uint16_t offset, uint8_t egress_ctrl_vec_sz)
> > +{
> > +    int ectrl_words = (egress_ctrl_vec_sz + 31) & ~31;
> > +    pcie_add_capability(dev, PCI_EXT_CAP_ID_ACS, PCI_ACS_VER,
> > +                        offset, PCI_ACS_SIZEOF + ectrl_words);
> 
> The egress control vector is only valid if the egress control
> capability is enabled, which is not set below, so this just seems to
> waste config space and introduces a meaningless function arg.

I think my intention way back when I implemented this was to provide it as a skeleton
for further detailing later, I use it with ectrl_words = 0 in the root port.

> > +    pci_set_word(dev->config + offset + PCI_ACS_CAP,
> > +                 PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_UF);
> 
> Some of these bits are only valid for downstream ports, it would
> violate the spec to set them on and endpoint.

I must admit I haven't really dived deep here - I just set the cap bits that one of my
servers sets for it's root ports - this is the one I used as model:

00:02.0 PCI bridge: Intel Corporation 5520/5500/X58 I/O Hub PCI Express Root Port 1 (rev
22) (prog-if 00 [Normal decode])
00:02.0 0604: 8086:3409 (rev 22)

> > +    pci_set_word(dev->config + offset + PCI_ACS_CTRL,
> > +                 PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_UF);
> 
> The default values of the control register bits is zero, so we
> shouldn't be setting it here and we should have a reset hook to clear
> it.

I agree - I'll implement it,
thanks!

Knut

> 
> > +    /* Make CTRL register writable */
> > +    memset(dev->wmask + offset + PCI_ACS_CTRL, 0xff, 2);
> > +}
> > diff --git a/include/hw/pci/pcie.h b/include/hw/pci/pcie.h
> > index 5b82a0d..c2da148 100644
> > --- a/include/hw/pci/pcie.h
> > +++ b/include/hw/pci/pcie.h
> > @@ -129,6 +129,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, uint8_t egress_ctrl_vec_sz);
> >  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..5e7409c 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 */
> 

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

[Alex Williamson]

On Wed, 23 Jan 2019 20:46:14 +0100
Knut Omang <knut.omang@oracle.com> wrote:

> On Wed, 2019-01-23 at 12:04 -0700, Alex Williamson wrote:
> > On Wed, 23 Jan 2019 19:27:59 +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 useful
> > > passthrough with VFIO without Alex Williamson's pcie_acs_override kernel patch.  
> > 
> > Define "useful".  We can certainly still assign single function PFs to
> > an L2 guest, or multi-function so long as all the functions are
> > assigned.  I won't deny that it's problematic, but it's a virtual
> > topology that can be adjusted, so I think this is overstating things a
> > bit.
> >   
> > > Signed-off-by: Knut Omang <knut.omang@oracle.com>
> > > ---
> > >  hw/pci/pcie.c              | 14 ++++++++++++++
> > >  include/hw/pci/pcie.h      |  1 +
> > >  include/hw/pci/pcie_regs.h |  4 ++++
> > >  3 files changed, 19 insertions(+)
> > > 
> > > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> > > index 230478f..18feff5 100644
> > > --- a/hw/pci/pcie.c
> > > +++ b/hw/pci/pcie.c
> > > @@ -906,3 +906,17 @@ void pcie_ats_init(PCIDevice *dev, uint16_t offset)
> > >  
> > >      pci_set_word(dev->wmask + dev->exp.ats_cap + PCI_ATS_CTRL, 0x800f);
> > >  }
> > > +
> > > +/* Add an ACS (Access Control Services) capability */
> > > +void pcie_acs_init(PCIDevice *dev, uint16_t offset, uint8_t egress_ctrl_vec_sz)
> > > +{
> > > +    int ectrl_words = (egress_ctrl_vec_sz + 31) & ~31;
> > > +    pcie_add_capability(dev, PCI_EXT_CAP_ID_ACS, PCI_ACS_VER,
> > > +                        offset, PCI_ACS_SIZEOF + ectrl_words);  
> > 
> > The egress control vector is only valid if the egress control
> > capability is enabled, which is not set below, so this just seems to
> > waste config space and introduces a meaningless function arg.  
> 
> I think my intention way back when I implemented this was to provide it as a skeleton
> for further detailing later, I use it with ectrl_words = 0 in the root port.
> 
> > > +    pci_set_word(dev->config + offset + PCI_ACS_CAP,
> > > +                 PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_UF);  
> > 
> > Some of these bits are only valid for downstream ports, it would
> > violate the spec to set them on and endpoint.  
> 
> I must admit I haven't really dived deep here - I just set the cap bits that one of my
> servers sets for it's root ports - this is the one I used as model:
> 
> 00:02.0 PCI bridge: Intel Corporation 5520/5500/X58 I/O Hub PCI Express Root Port 1 (rev
> 22) (prog-if 00 [Normal decode])
> 00:02.0 0604: 8086:3409 (rev 22)
> 
> > > +    pci_set_word(dev->config + offset + PCI_ACS_CTRL,
> > > +                 PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_UF);  
> > 
> > The default values of the control register bits is zero, so we
> > shouldn't be setting it here and we should have a reset hook to clear
> > it.  
> 
> I agree - I'll implement it,
> thanks!
> 
> Knut
> 
> >   
> > > +    /* Make CTRL register writable */
> > > +    memset(dev->wmask + offset + PCI_ACS_CTRL, 0xff, 2);

While you're at it, it doesn't make sense to set unimplemented control
bits as writable, this should match the bits set in the capabilities
register.  Thanks,

Alex

> > > +}
> > > diff --git a/include/hw/pci/pcie.h b/include/hw/pci/pcie.h
> > > index 5b82a0d..c2da148 100644
> > > --- a/include/hw/pci/pcie.h
> > > +++ b/include/hw/pci/pcie.h
> > > @@ -129,6 +129,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, uint8_t egress_ctrl_vec_sz);
> > >  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..5e7409c 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 */  
> >   
> 
> 

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

[Knut Omang]

On Wed, 2019-01-23 at 12:56 -0700, Alex Williamson wrote:
> On Wed, 23 Jan 2019 20:46:14 +0100
> Knut Omang <knut.omang@oracle.com> wrote:
> 
> > On Wed, 2019-01-23 at 12:04 -0700, Alex Williamson wrote:
> > > On Wed, 23 Jan 2019 19:27:59 +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 useful
> > > > passthrough with VFIO without Alex Williamson's pcie_acs_override kernel patch.  
> > > 
> > > Define "useful".  We can certainly still assign single function PFs to
> > > an L2 guest, or multi-function so long as all the functions are
> > > assigned.  I won't deny that it's problematic, but it's a virtual
> > > topology that can be adjusted, so I think this is overstating things a
> > > bit.
> > >   
> > > > Signed-off-by: Knut Omang <knut.omang@oracle.com>
> > > > ---
> > > >  hw/pci/pcie.c              | 14 ++++++++++++++
> > > >  include/hw/pci/pcie.h      |  1 +
> > > >  include/hw/pci/pcie_regs.h |  4 ++++
> > > >  3 files changed, 19 insertions(+)
> > > > 
> > > > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> > > > index 230478f..18feff5 100644
> > > > --- a/hw/pci/pcie.c
> > > > +++ b/hw/pci/pcie.c
> > > > @@ -906,3 +906,17 @@ void pcie_ats_init(PCIDevice *dev, uint16_t offset)
> > > >  
> > > >      pci_set_word(dev->wmask + dev->exp.ats_cap + PCI_ATS_CTRL, 0x800f);
> > > >  }
> > > > +
> > > > +/* Add an ACS (Access Control Services) capability */
> > > > +void pcie_acs_init(PCIDevice *dev, uint16_t offset, uint8_t egress_ctrl_vec_sz)
> > > > +{
> > > > +    int ectrl_words = (egress_ctrl_vec_sz + 31) & ~31;
> > > > +    pcie_add_capability(dev, PCI_EXT_CAP_ID_ACS, PCI_ACS_VER,
> > > > +                        offset, PCI_ACS_SIZEOF + ectrl_words);  
> > > 
> > > The egress control vector is only valid if the egress control
> > > capability is enabled, which is not set below, so this just seems to
> > > waste config space and introduces a meaningless function arg.  
> > 
> > I think my intention way back when I implemented this was to provide it as a skeleton
> > for further detailing later, I use it with ectrl_words = 0 in the root port.
> > 
> > > > +    pci_set_word(dev->config + offset + PCI_ACS_CAP,
> > > > +                 PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR | PCI_ACS_CR |
> PCI_ACS_UF);  
> > > 
> > > Some of these bits are only valid for downstream ports, it would
> > > violate the spec to set them on and endpoint.  
> > 
> > I must admit I haven't really dived deep here - I just set the cap bits that one of my
> > servers sets for it's root ports - this is the one I used as model:
> > 
> > 00:02.0 PCI bridge: Intel Corporation 5520/5500/X58 I/O Hub PCI Express Root Port 1
> (rev
> > 22) (prog-if 00 [Normal decode])
> > 00:02.0 0604: 8086:3409 (rev 22)
> > 
> > > > +    pci_set_word(dev->config + offset + PCI_ACS_CTRL,
> > > > +                 PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR | PCI_ACS_CR |
> PCI_ACS_UF);  
> > > 
> > > The default values of the control register bits is zero, so we
> > > shouldn't be setting it here and we should have a reset hook to clear
> > > it.  
> > 
> > I agree - I'll implement it,
> > thanks!
> > 
> > Knut
> > 
> > >   
> > > > +    /* Make CTRL register writable */
> > > > +    memset(dev->wmask + offset + PCI_ACS_CTRL, 0xff, 2);
> 
> While you're at it, it doesn't make sense to set unimplemented control
> bits as writable, this should match the bits set in the capabilities
> register.  Thanks,

Good point, I'll fix that as well,

Thanks!
Knut

> 
> Alex
> 
> > > > +}
> > > > diff --git a/include/hw/pci/pcie.h b/include/hw/pci/pcie.h
> > > > index 5b82a0d..c2da148 100644
> > > > --- a/include/hw/pci/pcie.h
> > > > +++ b/include/hw/pci/pcie.h
> > > > @@ -129,6 +129,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, uint8_t egress_ctrl_vec_sz);
> > > >  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..5e7409c 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 */  
> > >   
> > 
> > 
> 

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

[no-reply]

Patchew URL: https://patchew.org/QEMU/cover.c7f64d9a8f7ff9259c785560765645151c85dc75.1548266832.git-series.knut.omang@oracle.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Subject: [Qemu-devel] [PATCH 0/2] pcie: Add simple ACS "support" to the generic PCIe root port
Type: series
Message-id: cover.c7f64d9a8f7ff9259c785560765645151c85dc75.1548266832.git-series.knut.omang@oracle.com

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
d758b99be0 gen_pcie_root_port: Add ACS (Access Control Services) capability
f09b14c145 pcie: Add a simple PCIe ACS (Access Control Services) helper function

=== OUTPUT BEGIN ===
1/2 Checking commit f09b14c145d7 (pcie: Add a simple PCIe ACS (Access Control Services) helper function)
WARNING: line over 80 characters
#30: FILE: hw/pci/pcie.c:917:
+                 PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_UF);

WARNING: line over 80 characters
#32: FILE: hw/pci/pcie.c:919:
+                 PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_UF);

ERROR: code indent should never use tabs
#57: FILE: include/hw/pci/pcie_regs.h:179:
+#define PCI_ACS_VER^I^I^I0x2$

total: 1 errors, 2 warnings, 32 lines checked

Patch 1/2 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

2/2 Checking commit d758b99be0f8 (gen_pcie_root_port: Add ACS (Access Control Services) capability)
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/cover.c7f64d9a8f7ff9259c785560765645151c85dc75.1548266832.git-series.knut.omang@oracle.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com