diff mbox series

[v3,2/2] gen_pcie_root_port: Add ACS (Access Control Services) capability

Message ID 04c99e3c0b56dcbfe494f668bbe11777adbadba9.1549779509.git-series.knut.omang@oracle.com (mailing list archive)
State New, archived
Headers show
Series pcie: Add simple ACS "support" to the generic PCIe root port | expand

Commit Message

Knut Omang Feb. 10, 2019, 6:53 a.m. UTC
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 | 4 ++++
 hw/pci-bridge/pcie_root_port.c     | 4 ++++
 include/hw/pci/pcie_port.h         | 1 +
 3 files changed, 9 insertions(+)

Comments

Marcel Apfelbaum Feb. 11, 2019, 8:07 a.m. UTC | #1
On 2/10/19 8:53 AM, Knut Omang 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 | 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..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;
>   

Reviewed-by: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>

Thanks,
Marcel
Knut Omang Feb. 13, 2019, 9:34 a.m. UTC | #2
On Mon, 2019-02-11 at 10:07 +0200, Marcel Apfelbaum wrote:
> 
> On 2/10/19 8:53 AM, Knut Omang 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 | 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..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;
> >   
> 
> Reviewed-by: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>

Thanks! I added this r-b to v4, but left out the one for patch 1 as it got some
more changes based on Alex's feedback and our fruitful discussion afterwards,
so you might want to recheck it.

Knut
> 
> Thanks,
> Marcel
diff mbox series

Patch

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