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

[Qemu-devel] [PATCH v4 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_v11

The patches' basic operation with VFIO and iommu groups have
been tested with the above patch set and a rebased version of an
in progress igb ethernet device, which needs some more care
before I can let it go out.

Changes from v3:
----------------
- rebased to the latest qemu master
- Revised commit message and comments for patch #1 to make it
  clearer that VFIO works for single function devices even without ACS.
- Improved checking for valid endpoints for ACS.
- Fixed comment style issue
- Co-locate the pci_acs_init and _reset functions and
  rename pci_cap_acs_reset to pci_acs_reset to adhere to the naming
  conventions that _cap_ functions in pcie is for changing state
  in the main pcie capability and not the individual extended
  capabilities.
- Added Marcel's r-b to patch 2, which did not change

Changes from v2:
----------------
- rebased to the latest qemu master

Incorporated further feedback from Alex:
- Make sure slot/downstream capability bits are only set for slots.
- Make acs reset callback do nothing if no acs capability exists
- Set correct acs version
- div simplification

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 |  4 +++-
 hw/pci-bridge/pcie_root_port.c     |  4 +++-
 hw/pci/pcie.c                      | 39 +++++++++++++++++++++++++++++++-
 include/hw/pci/pcie.h              |  6 +++++-
 include/hw/pci/pcie_port.h         |  1 +-
 include/hw/pci/pcie_regs.h         |  4 +++-
 6 files changed, 58 insertions(+)

base-commit: 0b5e750bea635b167eb03d86c3d9a09bbd43bc06
-- 
git-series 0.9.1

[Qemu-devel] [PATCH v4 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 needed to pass
through individual functions of a device to different VMs with VFIO
without Alex Williamson's pcie_acs_override kernel patch or similar
in the guest.

Single function devices, or multifunction devices
that all goes to the same VM works fine even without ACS, as VFIO
will avoid putting the root port itself into the IOMMU group
even without ACS support in the port.

Multifunction endpoints may also implement an ACS capability,
only on function 0, and with more limited features.

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

diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index 230478f..6e87994 100644
--- a/hw/pci/pcie.c
+++ b/hw/pci/pcie.c
@@ -906,3 +906,42 @@ 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)
+{
+    bool is_pcie_slot = !!object_dynamic_cast(OBJECT(dev), TYPE_PCIE_SLOT);
+    uint16_t cap_bits = 0;
+
+    /*
+     * For endpoints, only multifunction devices may have an
+     * ACS capability, and only on function 0:
+     */
+    assert(is_pcie_slot ||
+           ((dev->cap_present & QEMU_PCI_CAP_MULTIFUNCTION) &&
+            PCI_FUNC(dev->devfn)));
+
+    pcie_add_capability(dev, PCI_EXT_CAP_ID_ACS, PCI_ACS_VER, offset,
+                        PCI_ACS_SIZEOF);
+    dev->exp.acs_cap = offset;
+
+    if (is_pcie_slot) {
+        /*
+         * Endpoints may also implement ACS, and optionally RR and CR,
+         * if they want to support p2p, but only slots may
+         * implement SV, TB or UF:
+         */
+        cap_bits =
+            PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_UF;
+    }
+
+    pci_set_word(dev->config + offset + PCI_ACS_CAP, cap_bits);
+    pci_set_word(dev->wmask + offset + PCI_ACS_CTRL, cap_bits);
+}
+
+void pcie_acs_reset(PCIDevice *dev)
+{
+    if (dev->exp.acs_cap) {
+        pci_set_word(dev->config + dev->exp.acs_cap + PCI_ACS_CTRL, 0);
+    }
+}
diff --git a/include/hw/pci/pcie.h b/include/hw/pci/pcie.h
index 5b82a0d..e30334d 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"
@@ -128,6 +131,9 @@ void pcie_add_capability(PCIDevice *dev,
                          uint16_t offset, uint16_t size);
 void pcie_sync_bridge_lnk(PCIDevice *dev);
 
+void pcie_acs_init(PCIDevice *dev, uint16_t offset);
+void pcie_acs_reset(PCIDevice *dev);
+
 void pcie_ari_init(PCIDevice *dev, uint16_t offset, uint16_t nextfn);
 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..1db86b0 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                     0x1
+#define PCI_ACS_SIZEOF                  8
+
 #endif /* QEMU_PCIE_REGS_H */
-- 
git-series 0.9.1

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

[Alex Williamson]

On Wed, 13 Feb 2019 10:29:58 +0100
Knut Omang <knut.omang@oracle.com> wrote:

> Add a helper function to add PCIe capability for Access Control Services (ACS)

This is redundant to the commit title.

> ACS support in the associated root port is needed to pass
> through individual functions of a device to different VMs with VFIO
> without Alex Williamson's pcie_acs_override kernel patch or similar
> in the guest.

This is overly subtle, to work backwards that individual functions
(plural!) of a device (singular!) must imply a multifunction endpoint
in the same hierarchy split to different L2 VMs.  Perhaps I only
finally realized this subtly on v4.

> Single function devices, or multifunction devices
> that all goes to the same VM works fine even without ACS, as VFIO
> will avoid putting the root port itself into the IOMMU group
> even without ACS support in the port.

Also confusing and incorrectly states that a) VFIO is responsible for
IOMMU grouping, it's not, and b) that the root port would not be
included in such a group, it would.  The latter was I thought the
impetus for this series.

> Multifunction endpoints may also implement an ACS capability,
> only on function 0, and with more limited features.

"only on function 0" is incorrect, each function of a multifunction
device should (not must) implement an ACS capability if any of them do.

Can't we just say something like:

"Implementing an ACS capability on downstream ports and multifuction
endpoints indicates isolation and IOMMU visibility to a finer
granularity thereby creating smaller IOMMU groups in the guest and thus
more flexibility in assigning endpoints to guest userspace or an L2
guest."

(I avoided including SR-IOV with multifunction since that's not
implemented here)

> Signed-off-by: Knut Omang <knut.omang@oracle.com>
> ---
>  hw/pci/pcie.c              | 39 +++++++++++++++++++++++++++++++++++++++-
>  include/hw/pci/pcie.h      |  6 ++++++-
>  include/hw/pci/pcie_regs.h |  4 ++++-
>  3 files changed, 49 insertions(+)
> 
> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> index 230478f..6e87994 100644
> --- a/hw/pci/pcie.c
> +++ b/hw/pci/pcie.c
> @@ -906,3 +906,42 @@ 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)
> +{
> +    bool is_pcie_slot = !!object_dynamic_cast(OBJECT(dev), TYPE_PCIE_SLOT);

Perhaps we should be using pci_is_express_downstream_port().

> +    uint16_t cap_bits = 0;
> +
> +    /*
> +     * For endpoints, only multifunction devices may have an
> +     * ACS capability, and only on function 0:

Incorrect

> +     */
> +    assert(is_pcie_slot ||
> +           ((dev->cap_present & QEMU_PCI_CAP_MULTIFUNCTION) &&
> +            PCI_FUNC(dev->devfn)));

The second test should be:

((dev->cap_present & QEMU_PCI_CAP_MULTIFUNCTION) ||
 PCI_FUNC(dev->devfn))

If the function number is non-zero, then it's clearly a multifunction
device, the multifunction capability is only required on function
zero.  Just as in my previous example, an ACS capability can only
describe/control the DMA flow of the function implementing it, nothing
in the spec that I can see imposes function zero's DMA flow on the
other functions.

There's also a gap here that function zero can set the multifunction
capability, but there may be no secondary devices defined.  Not that
we necessarily need to resolve this, but it's a nuance of allowing
arbitrary multifunction configurations as QEMU does.

> +
> +    pcie_add_capability(dev, PCI_EXT_CAP_ID_ACS, PCI_ACS_VER, offset,
> +                        PCI_ACS_SIZEOF);
> +    dev->exp.acs_cap = offset;
> +
> +    if (is_pcie_slot) {
> +        /*
> +         * Endpoints may also implement ACS, and optionally RR and CR,
> +         * if they want to support p2p, but only slots may
> +         * implement SV, TB or UF:

Careful using "may" with spec references.

"Downstream ports must implement SV, TB, RR, CR, and UF (with caveats
on the latter three that we ignore for simplicity).  Endpoints may also
implement a subset of ACS capabilities, but these are optional if the
endpoint does not support peer-to-peer between functions and thus
omitted here."

> +         */
> +        cap_bits =
> +            PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_UF;

PCI_ACS_DT has similar "must be implemented" requirements to RR, RC,
and UF, why is it not included?  For all of these "caveat" ones there
are conditions which can make it optional for root ports, but required
for switch downstream ports, so it seems reasonable that we include
both since that's what our is_pci_slot() test covers.  Thanks,

Alex

> +    }
> +
> +    pci_set_word(dev->config + offset + PCI_ACS_CAP, cap_bits);
> +    pci_set_word(dev->wmask + offset + PCI_ACS_CTRL, cap_bits);
> +}
> +
> +void pcie_acs_reset(PCIDevice *dev)
> +{
> +    if (dev->exp.acs_cap) {
> +        pci_set_word(dev->config + dev->exp.acs_cap + PCI_ACS_CTRL, 0);
> +    }
> +}
> diff --git a/include/hw/pci/pcie.h b/include/hw/pci/pcie.h
> index 5b82a0d..e30334d 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"
> @@ -128,6 +131,9 @@ void pcie_add_capability(PCIDevice *dev,
>                           uint16_t offset, uint16_t size);
>  void pcie_sync_bridge_lnk(PCIDevice *dev);
>  
> +void pcie_acs_init(PCIDevice *dev, uint16_t offset);
> +void pcie_acs_reset(PCIDevice *dev);
> +
>  void pcie_ari_init(PCIDevice *dev, uint16_t offset, uint16_t nextfn);
>  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..1db86b0 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                     0x1
> +#define PCI_ACS_SIZEOF                  8
> +
>  #endif /* QEMU_PCIE_REGS_H */

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

[Knut Omang]

On Wed, 2019-02-13 at 12:13 -0700, Alex Williamson wrote:
> On Wed, 13 Feb 2019 10:29:58 +0100
> Knut Omang <knut.omang@oracle.com> wrote:
> 
> > Add a helper function to add PCIe capability for Access Control Services
> > (ACS)
> 
> This is redundant to the commit title.
> 
> > ACS support in the associated root port is needed to pass
> > through individual functions of a device to different VMs with VFIO
> > without Alex Williamson's pcie_acs_override kernel patch or similar
> > in the guest.
> 
> This is overly subtle, to work backwards that individual functions
> (plural!) of a device (singular!) must imply a multifunction endpoint
> in the same hierarchy split to different L2 VMs.  Perhaps I only
> finally realized this subtly on v4.
> 
> > Single function devices, or multifunction devices
> > that all goes to the same VM works fine even without ACS, as VFIO
> > will avoid putting the root port itself into the IOMMU group
> > even without ACS support in the port.
> 
> Also confusing and incorrectly states that a) VFIO is responsible for
> IOMMU grouping, it's not, and b) that the root port would not be
> included in such a group, it would.  The latter was I thought the
> impetus for this series.

that wasn't the intention but I can see that it looks that way now

> > Multifunction endpoints may also implement an ACS capability,
> > only on function 0, and with more limited features.
> 
> "only on function 0" is incorrect, each function of a multifunction
> device should (not must) implement an ACS capability if any of them do.
> 
> Can't we just say something like:
> 
> "Implementing an ACS capability on downstream ports and multifuction
> endpoints indicates isolation and IOMMU visibility to a finer
> granularity thereby creating smaller IOMMU groups in the guest and thus
> more flexibility in assigning endpoints to guest userspace or an L2
> guest."

sure - will use this - and remove my confusing attempt to 
credit to your override patch and VFIO :)

> (I avoided including SR-IOV with multifunction since that's not
> implemented here)

I agree

> > Signed-off-by: Knut Omang <knut.omang@oracle.com>
> > ---
> >  hw/pci/pcie.c              | 39 +++++++++++++++++++++++++++++++++++++++-
> >  include/hw/pci/pcie.h      |  6 ++++++-
> >  include/hw/pci/pcie_regs.h |  4 ++++-
> >  3 files changed, 49 insertions(+)
> > 
> > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> > index 230478f..6e87994 100644
> > --- a/hw/pci/pcie.c
> > +++ b/hw/pci/pcie.c
> > @@ -906,3 +906,42 @@ 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)
> > +{
> > +    bool is_pcie_slot = !!object_dynamic_cast(OBJECT(dev), TYPE_PCIE_SLOT);
> 
> Perhaps we should be using pci_is_express_downstream_port().

oh - yes - I forgot that we need to look in pci.h for those kind of 
helpers..

> > +    uint16_t cap_bits = 0;
> > +
> > +    /*
> > +     * For endpoints, only multifunction devices may have an
> > +     * ACS capability, and only on function 0:
> 
> Incorrect
> 
> > +     */
> > +    assert(is_pcie_slot ||
> > +           ((dev->cap_present & QEMU_PCI_CAP_MULTIFUNCTION) &&
> > +            PCI_FUNC(dev->devfn)));
> 
> The second test should be:
> 
> ((dev->cap_present & QEMU_PCI_CAP_MULTIFUNCTION) ||
>  PCI_FUNC(dev->devfn))
> 
> If the function number is non-zero, then it's clearly a multifunction
> device, the multifunction capability is only required on function
> zero.  Just as in my previous example, an ACS capability can only
> describe/control the DMA flow of the function implementing it, nothing
> in the spec that I can see imposes function zero's DMA flow on the
> other functions.

Ah - of course - that makes sense - 
was thinking too complicated here, and also my comment didn't match
the code at all..

> There's also a gap here that function zero can set the multifunction
> capability, but there may be no secondary devices defined.  Not that
> we necessarily need to resolve this, but it's a nuance of allowing
> arbitrary multifunction configurations as QEMU does.

Yes, in the SR/IOV case, at least as I have implemented it in QEMU, 
with one PF that would be the default - as no VFs are defined at reset, 
there's only one function, but it still need to be multifunction 
for QEMU to accept more functions appearing later.

> > +
> > +    pcie_add_capability(dev, PCI_EXT_CAP_ID_ACS, PCI_ACS_VER, offset,
> > +                        PCI_ACS_SIZEOF);
> > +    dev->exp.acs_cap = offset;
> > +
> > +    if (is_pcie_slot) {
> > +        /*
> > +         * Endpoints may also implement ACS, and optionally RR and CR,
> > +         * if they want to support p2p, but only slots may
> > +         * implement SV, TB or UF:
> 
> Careful using "may" with spec references.

:-D

> "Downstream ports must implement SV, TB, RR, CR, and UF (with caveats
> on the latter three that we ignore for simplicity).  Endpoints may also
> implement a subset of ACS capabilities, but these are optional if the
> endpoint does not support peer-to-peer between functions and thus
> omitted here."

Thanks - I'll put that in instead

> > +         */
> > +        cap_bits =
> > +            PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_UF;
> 
> PCI_ACS_DT has similar "must be implemented" requirements to RR, RC,
> and UF, why is it not included?  For all of these "caveat" ones there
> are conditions which can make it optional for root ports, but required
> for switch downstream ports, so it seems reasonable that we include
> both since that's what our is_pci_slot() test covers.  Thanks,

That was because my "reference" root ports don't not implement it, taking the
conservative approach:

00:07.0 PCI bridge: Intel Corporation 5520/5500/X58 I/O Hub PCI Express Root Port 7 (rev 22) (prog-if 00 [Normal decode])
       ...
       Capabilities: [150 v1] Access Control Services
                ACSCap: SrcValid+ TransBlk+ ReqRedir+ CmpltRedir+ UpstreamFwd+ EgressCtrl- DirectTrans-
                ACSCtl: SrcValid+ TransBlk- ReqRedir+ CmpltRedir+ UpstreamFwd+ EgressCtrl- DirectTrans-

In fact, all gens of servers I have access to supports just the same cap bits in
their root ports, in order of release date. The latest gen even have some root
ports without an ACS capability.

00:02.0 PCI bridge: Intel Corporation Xeon E5/Core i7 IIO PCI Express Root Port 2a (rev 07)
00:01.0 PCI bridge: Intel Corporation Xeon E7 v4/Xeon E5 v4/Xeon E3 v4/Xeon D PCI Express Root Port 1 (rev 01) (prog-if 00 [Normal decode])
00:03.0 PCI bridge: Intel Corporation Xeon E7 v3/Xeon E5 v3/Core i7 PCI Express Root Port 3 (rev 02) (prog-if 00 [Normal decode])
00:02.0 PCI bridge: Intel Corporation Xeon E7 v2/Xeon E5 v2/Core i7 PCI Express Root Port 2a (rev 04) (prog-if 00 [Normal decode])
17:00.0 PCI bridge: Intel Corporation Sky Lake-E PCI Express Root Port A (rev 04) (prog-if 00 [Normal decode])

All of these have DirectTrans- in their ACSCap.

[For reference, the one without ACS cap, in the same server as 17:00.0 above:

00:1c.0 PCI bridge: Intel Corporation Lewisburg PCI Express Root Port #1 (rev f9) (prog-if 00 [Normal decode])
]

Do you prefer that we set DT as default anyway, or should we stay within "known
territory" for the OSes, at least for now?

Knut

> Alex
> 
> > +    }
> > +
> > +    pci_set_word(dev->config + offset + PCI_ACS_CAP, cap_bits);
> > +    pci_set_word(dev->wmask + offset + PCI_ACS_CTRL, cap_bits);
> > +}
> > +
> > +void pcie_acs_reset(PCIDevice *dev)
> > +{
> > +    if (dev->exp.acs_cap) {
> > +        pci_set_word(dev->config + dev->exp.acs_cap + PCI_ACS_CTRL, 0);
> > +    }
> > +}
> > diff --git a/include/hw/pci/pcie.h b/include/hw/pci/pcie.h
> > index 5b82a0d..e30334d 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"
> > @@ -128,6 +131,9 @@ void pcie_add_capability(PCIDevice *dev,
> >                           uint16_t offset, uint16_t size);
> >  void pcie_sync_bridge_lnk(PCIDevice *dev);
> >  
> > +void pcie_acs_init(PCIDevice *dev, uint16_t offset);
> > +void pcie_acs_reset(PCIDevice *dev);
> > +
> >  void pcie_ari_init(PCIDevice *dev, uint16_t offset, uint16_t nextfn);
> >  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..1db86b0 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                     0x1
> > +#define PCI_ACS_SIZEOF                  8
> > +
> >  #endif /* QEMU_PCIE_REGS_H */

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

[Alex Williamson]

On Thu, 14 Feb 2019 08:07:33 +0100
Knut Omang <knut.omang@oracle.com> wrote:

> On Wed, 2019-02-13 at 12:13 -0700, Alex Williamson wrote:
> > On Wed, 13 Feb 2019 10:29:58 +0100
> > Knut Omang <knut.omang@oracle.com> wrote:
> >   
> > > Add a helper function to add PCIe capability for Access Control Services
> > > (ACS)  
> > 
> > This is redundant to the commit title.
> >   
> > > ACS support in the associated root port is needed to pass
> > > through individual functions of a device to different VMs with VFIO
> > > without Alex Williamson's pcie_acs_override kernel patch or similar
> > > in the guest.  
> > 
> > This is overly subtle, to work backwards that individual functions
> > (plural!) of a device (singular!) must imply a multifunction endpoint
> > in the same hierarchy split to different L2 VMs.  Perhaps I only
> > finally realized this subtly on v4.
> >   
> > > Single function devices, or multifunction devices
> > > that all goes to the same VM works fine even without ACS, as VFIO
> > > will avoid putting the root port itself into the IOMMU group
> > > even without ACS support in the port.  
> > 
> > Also confusing and incorrectly states that a) VFIO is responsible for
> > IOMMU grouping, it's not, and b) that the root port would not be
> > included in such a group, it would.  The latter was I thought the
> > impetus for this series.  
> 
> that wasn't the intention but I can see that it looks that way now
> 
> > > Multifunction endpoints may also implement an ACS capability,
> > > only on function 0, and with more limited features.  
> > 
> > "only on function 0" is incorrect, each function of a multifunction
> > device should (not must) implement an ACS capability if any of them do.
> > 
> > Can't we just say something like:
> > 
> > "Implementing an ACS capability on downstream ports and multifuction
> > endpoints indicates isolation and IOMMU visibility to a finer
> > granularity thereby creating smaller IOMMU groups in the guest and thus
> > more flexibility in assigning endpoints to guest userspace or an L2
> > guest."  
> 
> sure - will use this - and remove my confusing attempt to 
> credit to your override patch and VFIO :)
> 
> > (I avoided including SR-IOV with multifunction since that's not
> > implemented here)  
> 
> I agree
> 
> > > Signed-off-by: Knut Omang <knut.omang@oracle.com>
> > > ---
> > >  hw/pci/pcie.c              | 39 +++++++++++++++++++++++++++++++++++++++-
> > >  include/hw/pci/pcie.h      |  6 ++++++-
> > >  include/hw/pci/pcie_regs.h |  4 ++++-
> > >  3 files changed, 49 insertions(+)
> > > 
> > > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> > > index 230478f..6e87994 100644
> > > --- a/hw/pci/pcie.c
> > > +++ b/hw/pci/pcie.c
> > > @@ -906,3 +906,42 @@ 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)
> > > +{
> > > +    bool is_pcie_slot = !!object_dynamic_cast(OBJECT(dev), TYPE_PCIE_SLOT);  
> > 
> > Perhaps we should be using pci_is_express_downstream_port().  
> 
> oh - yes - I forgot that we need to look in pci.h for those kind of 
> helpers..
> 
> > > +    uint16_t cap_bits = 0;
> > > +
> > > +    /*
> > > +     * For endpoints, only multifunction devices may have an
> > > +     * ACS capability, and only on function 0:  
> > 
> > Incorrect
> >   
> > > +     */
> > > +    assert(is_pcie_slot ||
> > > +           ((dev->cap_present & QEMU_PCI_CAP_MULTIFUNCTION) &&
> > > +            PCI_FUNC(dev->devfn)));  
> > 
> > The second test should be:
> > 
> > ((dev->cap_present & QEMU_PCI_CAP_MULTIFUNCTION) ||
> >  PCI_FUNC(dev->devfn))
> > 
> > If the function number is non-zero, then it's clearly a multifunction
> > device, the multifunction capability is only required on function
> > zero.  Just as in my previous example, an ACS capability can only
> > describe/control the DMA flow of the function implementing it, nothing
> > in the spec that I can see imposes function zero's DMA flow on the
> > other functions.  
> 
> Ah - of course - that makes sense - 
> was thinking too complicated here, and also my comment didn't match
> the code at all..
> 
> > There's also a gap here that function zero can set the multifunction
> > capability, but there may be no secondary devices defined.  Not that
> > we necessarily need to resolve this, but it's a nuance of allowing
> > arbitrary multifunction configurations as QEMU does.  
> 
> Yes, in the SR/IOV case, at least as I have implemented it in QEMU, 
> with one PF that would be the default - as no VFs are defined at reset, 
> there's only one function, but it still need to be multifunction 
> for QEMU to accept more functions appearing later.
> 
> > > +
> > > +    pcie_add_capability(dev, PCI_EXT_CAP_ID_ACS, PCI_ACS_VER, offset,
> > > +                        PCI_ACS_SIZEOF);
> > > +    dev->exp.acs_cap = offset;
> > > +
> > > +    if (is_pcie_slot) {
> > > +        /*
> > > +         * Endpoints may also implement ACS, and optionally RR and CR,
> > > +         * if they want to support p2p, but only slots may
> > > +         * implement SV, TB or UF:  
> > 
> > Careful using "may" with spec references.  
> 
> :-D
> 
> > "Downstream ports must implement SV, TB, RR, CR, and UF (with caveats
> > on the latter three that we ignore for simplicity).  Endpoints may also
> > implement a subset of ACS capabilities, but these are optional if the
> > endpoint does not support peer-to-peer between functions and thus
> > omitted here."  
> 
> Thanks - I'll put that in instead
> 
> > > +         */
> > > +        cap_bits =
> > > +            PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_UF;  
> > 
> > PCI_ACS_DT has similar "must be implemented" requirements to RR, RC,
> > and UF, why is it not included?  For all of these "caveat" ones there
> > are conditions which can make it optional for root ports, but required
> > for switch downstream ports, so it seems reasonable that we include
> > both since that's what our is_pci_slot() test covers.  Thanks,  
> 
> That was because my "reference" root ports don't not implement it, taking the
> conservative approach:
> 
> 00:07.0 PCI bridge: Intel Corporation 5520/5500/X58 I/O Hub PCI Express Root Port 7 (rev 22) (prog-if 00 [Normal decode])
>        ...
>        Capabilities: [150 v1] Access Control Services
>                 ACSCap: SrcValid+ TransBlk+ ReqRedir+ CmpltRedir+ UpstreamFwd+ EgressCtrl- DirectTrans-
>                 ACSCtl: SrcValid+ TransBlk- ReqRedir+ CmpltRedir+ UpstreamFwd+ EgressCtrl- DirectTrans-
> 
> In fact, all gens of servers I have access to supports just the same cap bits in
> their root ports, in order of release date. The latest gen even have some root
> ports without an ACS capability.
> 
> 00:02.0 PCI bridge: Intel Corporation Xeon E5/Core i7 IIO PCI Express Root Port 2a (rev 07)
> 00:01.0 PCI bridge: Intel Corporation Xeon E7 v4/Xeon E5 v4/Xeon E3 v4/Xeon D PCI Express Root Port 1 (rev 01) (prog-if 00 [Normal decode])
> 00:03.0 PCI bridge: Intel Corporation Xeon E7 v3/Xeon E5 v3/Core i7 PCI Express Root Port 3 (rev 02) (prog-if 00 [Normal decode])
> 00:02.0 PCI bridge: Intel Corporation Xeon E7 v2/Xeon E5 v2/Core i7 PCI Express Root Port 2a (rev 04) (prog-if 00 [Normal decode])
> 17:00.0 PCI bridge: Intel Corporation Sky Lake-E PCI Express Root Port A (rev 04) (prog-if 00 [Normal decode])
> 
> All of these have DirectTrans- in their ACSCap.
> 
> [For reference, the one without ACS cap, in the same server as 17:00.0 above:
> 
> 00:1c.0 PCI bridge: Intel Corporation Lewisburg PCI Express Root Port #1 (rev f9) (prog-if 00 [Normal decode])
> ]
> 
> Do you prefer that we set DT as default anyway, or should we stay within "known
> territory" for the OSes, at least for now?

Per the spec:

  ACS Direct Translated P2P: must be implemented by Root Ports that
  support Address Translation Services (ATS) and also support
  peer-to-peer traffic with other Root Ports; must be implemented by
  Switch Downstream Ports.

The caveats for root ports here are why your hardware is potentially
spec compliant without supporting DT.  There are no caveats for switch
downstream ports, therefore it would not be spec compliant to exclude
it.  I think your options are either to exclude switch downstream ports
from the function or to set DT.  Thanks,

Alex

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

[Knut Omang]

On Thu, 2019-02-14 at 08:51 -0700, Alex Williamson wrote:
> On Thu, 14 Feb 2019 08:07:33 +0100
> Knut Omang <knut.omang@oracle.com> wrote:
> 
> > On Wed, 2019-02-13 at 12:13 -0700, Alex Williamson wrote:
> > > On Wed, 13 Feb 2019 10:29:58 +0100
> > > Knut Omang <knut.omang@oracle.com> wrote:
> > >   
> > > > Add a helper function to add PCIe capability for Access Control Services
> > > > (ACS)  
> > > 
> > > This is redundant to the commit title.
> > >   
> > > > ACS support in the associated root port is needed to pass
> > > > through individual functions of a device to different VMs with VFIO
> > > > without Alex Williamson's pcie_acs_override kernel patch or similar
> > > > in the guest.  
> > > 
> > > This is overly subtle, to work backwards that individual functions
> > > (plural!) of a device (singular!) must imply a multifunction endpoint
> > > in the same hierarchy split to different L2 VMs.  Perhaps I only
> > > finally realized this subtly on v4.
> > >   
> > > > Single function devices, or multifunction devices
> > > > that all goes to the same VM works fine even without ACS, as VFIO
> > > > will avoid putting the root port itself into the IOMMU group
> > > > even without ACS support in the port.  
> > > 
> > > Also confusing and incorrectly states that a) VFIO is responsible for
> > > IOMMU grouping, it's not, and b) that the root port would not be
> > > included in such a group, it would.  The latter was I thought the
> > > impetus for this series.  
> > 
> > that wasn't the intention but I can see that it looks that way now
> > 
> > > > Multifunction endpoints may also implement an ACS capability,
> > > > only on function 0, and with more limited features.  
> > > 
> > > "only on function 0" is incorrect, each function of a multifunction
> > > device should (not must) implement an ACS capability if any of them do.
> > > 
> > > Can't we just say something like:
> > > 
> > > "Implementing an ACS capability on downstream ports and multifuction
> > > endpoints indicates isolation and IOMMU visibility to a finer
> > > granularity thereby creating smaller IOMMU groups in the guest and thus
> > > more flexibility in assigning endpoints to guest userspace or an L2
> > > guest."  
> > 
> > sure - will use this - and remove my confusing attempt to 
> > credit to your override patch and VFIO :)
> > 
> > > (I avoided including SR-IOV with multifunction since that's not
> > > implemented here)  
> > 
> > I agree
> > 
> > > > Signed-off-by: Knut Omang <knut.omang@oracle.com>
> > > > ---
> > > >  hw/pci/pcie.c              | 39
> > > > +++++++++++++++++++++++++++++++++++++++-
> > > >  include/hw/pci/pcie.h      |  6 ++++++-
> > > >  include/hw/pci/pcie_regs.h |  4 ++++-
> > > >  3 files changed, 49 insertions(+)
> > > > 
> > > > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> > > > index 230478f..6e87994 100644
> > > > --- a/hw/pci/pcie.c
> > > > +++ b/hw/pci/pcie.c
> > > > @@ -906,3 +906,42 @@ 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)
> > > > +{
> > > > +    bool is_pcie_slot = !!object_dynamic_cast(OBJECT(dev),
> > > > TYPE_PCIE_SLOT);  
> > > 
> > > Perhaps we should be using pci_is_express_downstream_port().  
> > 
> > oh - yes - I forgot that we need to look in pci.h for those kind of 
> > helpers..
> > 
> > > > +    uint16_t cap_bits = 0;
> > > > +
> > > > +    /*
> > > > +     * For endpoints, only multifunction devices may have an
> > > > +     * ACS capability, and only on function 0:  
> > > 
> > > Incorrect
> > >   
> > > > +     */
> > > > +    assert(is_pcie_slot ||
> > > > +           ((dev->cap_present & QEMU_PCI_CAP_MULTIFUNCTION) &&
> > > > +            PCI_FUNC(dev->devfn)));  
> > > 
> > > The second test should be:
> > > 
> > > ((dev->cap_present & QEMU_PCI_CAP_MULTIFUNCTION) ||
> > >  PCI_FUNC(dev->devfn))
> > > 
> > > If the function number is non-zero, then it's clearly a multifunction
> > > device, the multifunction capability is only required on function
> > > zero.  Just as in my previous example, an ACS capability can only
> > > describe/control the DMA flow of the function implementing it, nothing
> > > in the spec that I can see imposes function zero's DMA flow on the
> > > other functions.  
> > 
> > Ah - of course - that makes sense - 
> > was thinking too complicated here, and also my comment didn't match
> > the code at all..
> > 
> > > There's also a gap here that function zero can set the multifunction
> > > capability, but there may be no secondary devices defined.  Not that
> > > we necessarily need to resolve this, but it's a nuance of allowing
> > > arbitrary multifunction configurations as QEMU does.  
> > 
> > Yes, in the SR/IOV case, at least as I have implemented it in QEMU, 
> > with one PF that would be the default - as no VFs are defined at reset, 
> > there's only one function, but it still need to be multifunction 
> > for QEMU to accept more functions appearing later.
> > 
> > > > +
> > > > +    pcie_add_capability(dev, PCI_EXT_CAP_ID_ACS, PCI_ACS_VER, offset,
> > > > +                        PCI_ACS_SIZEOF);
> > > > +    dev->exp.acs_cap = offset;
> > > > +
> > > > +    if (is_pcie_slot) {
> > > > +        /*
> > > > +         * Endpoints may also implement ACS, and optionally RR and CR,
> > > > +         * if they want to support p2p, but only slots may
> > > > +         * implement SV, TB or UF:  
> > > 
> > > Careful using "may" with spec references.  
> > 
> > :-D
> > 
> > > "Downstream ports must implement SV, TB, RR, CR, and UF (with caveats
> > > on the latter three that we ignore for simplicity).  Endpoints may also
> > > implement a subset of ACS capabilities, but these are optional if the
> > > endpoint does not support peer-to-peer between functions and thus
> > > omitted here."  
> > 
> > Thanks - I'll put that in instead
> > 
> > > > +         */
> > > > +        cap_bits =
> > > > +            PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR | PCI_ACS_CR |
> > > > PCI_ACS_UF;  
> > > 
> > > PCI_ACS_DT has similar "must be implemented" requirements to RR, RC,
> > > and UF, why is it not included?  For all of these "caveat" ones there
> > > are conditions which can make it optional for root ports, but required
> > > for switch downstream ports, so it seems reasonable that we include
> > > both since that's what our is_pci_slot() test covers.  Thanks,  
> > 
> > That was because my "reference" root ports don't not implement it, taking
> > the
> > conservative approach:
> > 
> > 00:07.0 PCI bridge: Intel Corporation 5520/5500/X58 I/O Hub PCI Express Root
> > Port 7 (rev 22) (prog-if 00 [Normal decode])
> >        ...
> >        Capabilities: [150 v1] Access Control Services
> >                 ACSCap: SrcValid+ TransBlk+ ReqRedir+ CmpltRedir+
> > UpstreamFwd+ EgressCtrl- DirectTrans-
> >                 ACSCtl: SrcValid+ TransBlk- ReqRedir+ CmpltRedir+
> > UpstreamFwd+ EgressCtrl- DirectTrans-
> > 
> > In fact, all gens of servers I have access to supports just the same cap
> > bits in
> > their root ports, in order of release date. The latest gen even have some
> > root
> > ports without an ACS capability.
> > 
> > 00:02.0 PCI bridge: Intel Corporation Xeon E5/Core i7 IIO PCI Express Root
> > Port 2a (rev 07)
> > 00:01.0 PCI bridge: Intel Corporation Xeon E7 v4/Xeon E5 v4/Xeon E3 v4/Xeon
> > D PCI Express Root Port 1 (rev 01) (prog-if 00 [Normal decode])
> > 00:03.0 PCI bridge: Intel Corporation Xeon E7 v3/Xeon E5 v3/Core i7 PCI
> > Express Root Port 3 (rev 02) (prog-if 00 [Normal decode])
> > 00:02.0 PCI bridge: Intel Corporation Xeon E7 v2/Xeon E5 v2/Core i7 PCI
> > Express Root Port 2a (rev 04) (prog-if 00 [Normal decode])
> > 17:00.0 PCI bridge: Intel Corporation Sky Lake-E PCI Express Root Port A
> > (rev 04) (prog-if 00 [Normal decode])
> > 
> > All of these have DirectTrans- in their ACSCap.
> > 
> > [For reference, the one without ACS cap, in the same server as 17:00.0
> > above:
> > 
> > 00:1c.0 PCI bridge: Intel Corporation Lewisburg PCI Express Root Port #1
> > (rev f9) (prog-if 00 [Normal decode])
> > ]
> > 
> > Do you prefer that we set DT as default anyway, or should we stay within
> > "known
> > territory" for the OSes, at least for now?
> 
> Per the spec:
> 
>   ACS Direct Translated P2P: must be implemented by Root Ports that
>   support Address Translation Services (ATS) and also support
>   peer-to-peer traffic with other Root Ports; must be implemented by
>   Switch Downstream Ports.
> 
> The caveats for root ports here are why your hardware is potentially
> spec compliant without supporting DT.  There are no caveats for switch
> downstream ports, therefore it would not be spec compliant to exclude
> it.  I think your options are either to exclude switch downstream ports
> from the function or to set DT.  Thanks,

Better to set DT then - a future generic switch downstream port would want to
have ACS too.

Knut

> Alex

[Qemu-devel] [PATCH v4 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>
Reviewed-by: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
---
 hw/pci-bridge/gen_pcie_root_port.c | 4 ++++
 hw/pci-bridge/pcie_root_port.c     | 4 ++++
 include/hw/pci/pcie_port.h         | 1 +
 3 files changed, 9 insertions(+)

diff --git a/hw/pci-bridge/gen_pcie_root_port.c b/hw/pci-bridge/gen_pcie_root_port.c
index 9766edb..26bda73 100644
--- a/hw/pci-bridge/gen_pcie_root_port.c
+++ b/hw/pci-bridge/gen_pcie_root_port.c
@@ -20,6 +20,9 @@
         OBJECT_CHECK(GenPCIERootPort, (obj), TYPE_GEN_PCIE_ROOT_PORT)
 
 #define GEN_PCIE_ROOT_PORT_AER_OFFSET           0x100
+#define GEN_PCIE_ROOT_PORT_ACS_OFFSET \
+        (GEN_PCIE_ROOT_PORT_AER_OFFSET + PCI_ERR_SIZEOF)
+
 #define GEN_PCIE_ROOT_PORT_MSIX_NR_VECTOR       1
 
 typedef struct GenPCIERootPort {
@@ -149,6 +152,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..e94d918 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_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