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

[Knut Omang]

On Thu, 2019-01-24 at 10:33 -0700, Alex Williamson wrote:
> On Thu, 24 Jan 2019 11:12:53 +0100
> Knut Omang <address@hidden> 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 <address@hidden>
> > ---
> >  hw/pci-bridge/gen_pcie_root_port.c | 2 ++
> >  hw/pci-bridge/pcie_root_port.c     | 4 ++++
> >  include/hw/pci/pcie_port.h         | 1 +
> >  3 files changed, 7 insertions(+)
> > 
> > diff --git a/hw/pci-bridge/gen_pcie_root_port.c 
> > b/hw/pci-bridge/gen_pcie_root_port.c
> > index 9766edb..b5a5ecc 100644
> > --- a/hw/pci-bridge/gen_pcie_root_port.c
> > +++ b/hw/pci-bridge/gen_pcie_root_port.c
> > @@ -20,6 +20,7 @@
> >          OBJECT_CHECK(GenPCIERootPort, (obj), TYPE_GEN_PCIE_ROOT_PORT)
> >  
> >  #define GEN_PCIE_ROOT_PORT_AER_OFFSET           0x100
> > +#define GEN_PCIE_ROOT_PORT_ACS_OFFSET           0x148
> 
> So you prefer that everyone passing through here decode these to figure
> out that ACS_OFFSET is (AER_OFFSET + ERR_SIZEOF) since my comment on v1
> was ignored?

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

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

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

Thanks!
Knut

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