* [Qemu-devel] [PATCH v2 0/2] pcie: Add simple ACS "support" to the generic PCIe root port @ 2019-01-24 10:12 Knut Omang 2019-01-24 10:12 ` [Qemu-devel] [PATCH v2 1/2] pcie: Add a simple PCIe ACS (Access Control Services) helper function Knut Omang 2019-01-24 10:12 ` [Qemu-devel] [PATCH v2 2/2] gen_pcie_root_port: Add ACS (Access Control Services) capability Knut Omang 0 siblings, 2 replies; 7+ messages in thread From: Knut Omang @ 2019-01-24 10:12 UTC (permalink / raw) To: qemu-devel Cc: Michael S . Tsirkin, Marcel Apfelbaum, Alex Williamson, Stefan Hajnoczi, Elijah Shakkour, Tal Attaly, Knut Omang These two patches together implements a PCIe capability config space header for Access Control Services (ACS) for the new Qemu specific generic root port. ACS support in the associated root port is a prerequisite to be able to pass individual functions of a device populating the port through to an L2 guest from an unmodified kernel. Without this, the IOMMU group the device belongs to will also include the root port itself, and all functions the device provides. It is necessary to support SR/IOV where the primary purpose is to be able to share out individual VFs to different guests, which will not be permitted by VFIO or the Windows Hyper-V equivalent unless ACS is supported by the root port. These patches can also be found as part of an updated version of my SR/IOV emulation patch set at https://github.com/knuto/qemu/tree/sriov_patches_v10 Changes from v1: ---------------- Incorporated feedback from Alex Williamson: - Make commit messages reflect a more correct understanding of how this affects VFIO operation. - Implemented the CTRL register properly (reset callback + making non-implemented capabilities RO, default value 0) - removed the egress ctrl vector parameter to the init function - Fixed some whitespace issues Knut Omang (2): pcie: Add a simple PCIe ACS (Access Control Services) helper function gen_pcie_root_port: Add ACS (Access Control Services) capability hw/pci-bridge/gen_pcie_root_port.c | 2 ++ hw/pci-bridge/pcie_root_port.c | 4 ++++ hw/pci/pcie.c | 21 +++++++++++++++++++++ include/hw/pci/pcie.h | 6 ++++++ include/hw/pci/pcie_port.h | 1 + include/hw/pci/pcie_regs.h | 4 ++++ 6 files changed, 38 insertions(+) base-commit: a8d2b0685681e2f291faaa501efbbd76875f8ec8 -- git-series 0.9.1 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [Qemu-devel] [PATCH v2 1/2] pcie: Add a simple PCIe ACS (Access Control Services) helper function 2019-01-24 10:12 [Qemu-devel] [PATCH v2 0/2] pcie: Add simple ACS "support" to the generic PCIe root port Knut Omang @ 2019-01-24 10:12 ` Knut Omang 2019-01-24 17:22 ` Alex Williamson 2019-01-24 10:12 ` [Qemu-devel] [PATCH v2 2/2] gen_pcie_root_port: Add ACS (Access Control Services) capability Knut Omang 1 sibling, 1 reply; 7+ messages in thread From: Knut Omang @ 2019-01-24 10:12 UTC (permalink / raw) To: qemu-devel Cc: Michael S . Tsirkin, Marcel Apfelbaum, Alex Williamson, Stefan Hajnoczi, Elijah Shakkour, Tal Attaly, Knut Omang Add a helper function to add PCIe capability for Access Control Services (ACS) ACS support in the associated root port is a prerequisite to be able to do passthrough of individual functions of a device with VFIO without Alex Williamson's pcie_acs_override kernel patch or similar in the guest. Signed-off-by: Knut Omang <knut.omang@oracle.com> --- hw/pci/pcie.c | 21 +++++++++++++++++++++ include/hw/pci/pcie.h | 6 ++++++ include/hw/pci/pcie_regs.h | 4 ++++ 3 files changed, 31 insertions(+) diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c index 230478f..5ab3d1d 100644 --- a/hw/pci/pcie.c +++ b/hw/pci/pcie.c @@ -742,6 +742,13 @@ bool pcie_cap_is_arifwd_enabled(const PCIDevice *dev) PCI_EXP_DEVCTL2_ARI; } +/* Access Control Services (ACS) + */ +void pcie_cap_acs_reset(PCIDevice *dev) +{ + pci_set_word(dev->config + dev->exp.acs_cap + PCI_ACS_CTRL, 0); +} + /************************************************************************** * pci express extended capability list management functions * uint16_t ext_cap_id (16 bit) @@ -906,3 +913,17 @@ void pcie_ats_init(PCIDevice *dev, uint16_t offset) pci_set_word(dev->wmask + dev->exp.ats_cap + PCI_ATS_CTRL, 0x800f); } + +/* ACS (Access Control Services) */ +void pcie_acs_init(PCIDevice *dev, uint16_t offset) +{ + pcie_add_capability(dev, PCI_EXT_CAP_ID_ACS, PCI_ACS_VER, + offset, PCI_ACS_SIZEOF); + dev->exp.acs_cap = offset; + pci_set_word(dev->config + offset + PCI_ACS_CAP, + PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_UF); + + pci_set_word(dev->config + offset + PCI_ACS_CTRL, 0); + pci_set_word(dev->wmask + offset + PCI_ACS_CTRL, + PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_UF); +} diff --git a/include/hw/pci/pcie.h b/include/hw/pci/pcie.h index 5b82a0d..4c40711 100644 --- a/include/hw/pci/pcie.h +++ b/include/hw/pci/pcie.h @@ -79,6 +79,9 @@ struct PCIExpressDevice { /* Offset of ATS capability in config space */ uint16_t ats_cap; + + /* ACS */ + uint16_t acs_cap; }; #define COMPAT_PROP_PCP "power_controller_present" @@ -116,6 +119,8 @@ void pcie_cap_flr_init(PCIDevice *dev); void pcie_cap_flr_write_config(PCIDevice *dev, uint32_t addr, uint32_t val, int len); +void pcie_cap_acs_reset(PCIDevice *dev); + /* ARI forwarding capability and control */ void pcie_cap_arifwd_init(PCIDevice *dev); void pcie_cap_arifwd_reset(PCIDevice *dev); @@ -129,6 +134,7 @@ void pcie_add_capability(PCIDevice *dev, void pcie_sync_bridge_lnk(PCIDevice *dev); void pcie_ari_init(PCIDevice *dev, uint16_t offset, uint16_t nextfn); +void pcie_acs_init(PCIDevice *dev, uint16_t offset); void pcie_dev_ser_num_init(PCIDevice *dev, uint16_t offset, uint64_t ser_num); void pcie_ats_init(PCIDevice *dev, uint16_t offset); diff --git a/include/hw/pci/pcie_regs.h b/include/hw/pci/pcie_regs.h index ad4e780..3fc9aca 100644 --- a/include/hw/pci/pcie_regs.h +++ b/include/hw/pci/pcie_regs.h @@ -175,4 +175,8 @@ typedef enum PCIExpLinkWidth { PCI_ERR_COR_INTERNAL | \ PCI_ERR_COR_HL_OVERFLOW) +/* ACS */ +#define PCI_ACS_VER 0x2 +#define PCI_ACS_SIZEOF 8 + #endif /* QEMU_PCIE_REGS_H */ -- git-series 0.9.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/2] pcie: Add a simple PCIe ACS (Access Control Services) helper function 2019-01-24 10:12 ` [Qemu-devel] [PATCH v2 1/2] pcie: Add a simple PCIe ACS (Access Control Services) helper function Knut Omang @ 2019-01-24 17:22 ` Alex Williamson 2019-01-24 20:14 ` Knut Omang 0 siblings, 1 reply; 7+ messages in thread From: Alex Williamson @ 2019-01-24 17:22 UTC (permalink / raw) To: Knut Omang Cc: qemu-devel, Michael S . Tsirkin, Marcel Apfelbaum, Stefan Hajnoczi, Elijah Shakkour, Tal Attaly On Thu, 24 Jan 2019 11:12:52 +0100 Knut Omang <knut.omang@oracle.com> wrote: > Add a helper function to add PCIe capability for Access Control Services (ACS) > ACS support in the associated root port is a prerequisite to be able to do > passthrough of individual functions of a device with VFIO > without Alex Williamson's pcie_acs_override kernel patch or similar > in the guest. > > Signed-off-by: Knut Omang <knut.omang@oracle.com> > --- > hw/pci/pcie.c | 21 +++++++++++++++++++++ > include/hw/pci/pcie.h | 6 ++++++ > include/hw/pci/pcie_regs.h | 4 ++++ > 3 files changed, 31 insertions(+) > > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c > index 230478f..5ab3d1d 100644 > --- a/hw/pci/pcie.c > +++ b/hw/pci/pcie.c > @@ -742,6 +742,13 @@ bool pcie_cap_is_arifwd_enabled(const PCIDevice *dev) > PCI_EXP_DEVCTL2_ARI; > } > > +/* Access Control Services (ACS) > + */ Comment style > +void pcie_cap_acs_reset(PCIDevice *dev) > +{ > + pci_set_word(dev->config + dev->exp.acs_cap + PCI_ACS_CTRL, 0); > +} > + > /************************************************************************** > * pci express extended capability list management functions > * uint16_t ext_cap_id (16 bit) > @@ -906,3 +913,17 @@ void pcie_ats_init(PCIDevice *dev, uint16_t offset) > > pci_set_word(dev->wmask + dev->exp.ats_cap + PCI_ATS_CTRL, 0x800f); > } > + > +/* ACS (Access Control Services) */ > +void pcie_acs_init(PCIDevice *dev, uint16_t offset) > +{ > + pcie_add_capability(dev, PCI_EXT_CAP_ID_ACS, PCI_ACS_VER, > + offset, PCI_ACS_SIZEOF); > + dev->exp.acs_cap = offset; > + pci_set_word(dev->config + offset + PCI_ACS_CAP, > + PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_UF); This is still only valid for downstream ports yet neither restricted nor commented do indicate that. You could use an object_dynamic_cast to triggger an assert should someone use it for an invalid type of device, ex: assert(object_dynamic_cast(OBJECT(dev), TYPE_PCIE_SLOT)); > + > + pci_set_word(dev->config + offset + PCI_ACS_CTRL, 0); Suspect this is unnecessary given the reset callback. > + pci_set_word(dev->wmask + offset + PCI_ACS_CTRL, > + PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_UF); > +} > diff --git a/include/hw/pci/pcie.h b/include/hw/pci/pcie.h > index 5b82a0d..4c40711 100644 > --- a/include/hw/pci/pcie.h > +++ b/include/hw/pci/pcie.h > @@ -79,6 +79,9 @@ struct PCIExpressDevice { > > /* Offset of ATS capability in config space */ > uint16_t ats_cap; > + > + /* ACS */ > + uint16_t acs_cap; > }; > > #define COMPAT_PROP_PCP "power_controller_present" > @@ -116,6 +119,8 @@ void pcie_cap_flr_init(PCIDevice *dev); > void pcie_cap_flr_write_config(PCIDevice *dev, > uint32_t addr, uint32_t val, int len); > > +void pcie_cap_acs_reset(PCIDevice *dev); > + > /* ARI forwarding capability and control */ > void pcie_cap_arifwd_init(PCIDevice *dev); > void pcie_cap_arifwd_reset(PCIDevice *dev); > @@ -129,6 +134,7 @@ void pcie_add_capability(PCIDevice *dev, > void pcie_sync_bridge_lnk(PCIDevice *dev); > > void pcie_ari_init(PCIDevice *dev, uint16_t offset, uint16_t nextfn); > +void pcie_acs_init(PCIDevice *dev, uint16_t offset); > void pcie_dev_ser_num_init(PCIDevice *dev, uint16_t offset, uint64_t ser_num); > void pcie_ats_init(PCIDevice *dev, uint16_t offset); > > diff --git a/include/hw/pci/pcie_regs.h b/include/hw/pci/pcie_regs.h > index ad4e780..3fc9aca 100644 > --- a/include/hw/pci/pcie_regs.h > +++ b/include/hw/pci/pcie_regs.h > @@ -175,4 +175,8 @@ typedef enum PCIExpLinkWidth { > PCI_ERR_COR_INTERNAL | \ > PCI_ERR_COR_HL_OVERFLOW) > > +/* ACS */ > +#define PCI_ACS_VER 0x2 There's no such version, even the PCIe 5.0 drafts only define version 1. > +#define PCI_ACS_SIZEOF 8 > + > #endif /* QEMU_PCIE_REGS_H */ ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/2] pcie: Add a simple PCIe ACS (Access Control Services) helper function 2019-01-24 17:22 ` Alex Williamson @ 2019-01-24 20:14 ` Knut Omang 0 siblings, 0 replies; 7+ messages in thread From: Knut Omang @ 2019-01-24 20:14 UTC (permalink / raw) To: Alex Williamson Cc: qemu-devel, Michael S . Tsirkin, Marcel Apfelbaum, Stefan Hajnoczi, Elijah Shakkour, Tal Attaly On Thu, 2019-01-24 at 10:22 -0700, Alex Williamson wrote: > On Thu, 24 Jan 2019 11:12:52 +0100 > Knut Omang <knut.omang@oracle.com> wrote: > > > Add a helper function to add PCIe capability for Access Control Services (ACS) > > ACS support in the associated root port is a prerequisite to be able to do > > passthrough of individual functions of a device with VFIO > > without Alex Williamson's pcie_acs_override kernel patch or similar > > in the guest. > > > > Signed-off-by: Knut Omang <knut.omang@oracle.com> > > --- > > hw/pci/pcie.c | 21 +++++++++++++++++++++ > > include/hw/pci/pcie.h | 6 ++++++ > > include/hw/pci/pcie_regs.h | 4 ++++ > > 3 files changed, 31 insertions(+) > > > > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c > > index 230478f..5ab3d1d 100644 > > --- a/hw/pci/pcie.c > > +++ b/hw/pci/pcie.c > > @@ -742,6 +742,13 @@ bool pcie_cap_is_arifwd_enabled(const PCIDevice *dev) > > PCI_EXP_DEVCTL2_ARI; > > } > > > > +/* Access Control Services (ACS) > > + */ > > Comment style > > > +void pcie_cap_acs_reset(PCIDevice *dev) > > +{ > > + pci_set_word(dev->config + dev->exp.acs_cap + PCI_ACS_CTRL, 0); > > +} > > + > > /************************************************************************** > > * pci express extended capability list management functions > > * uint16_t ext_cap_id (16 bit) > > @@ -906,3 +913,17 @@ void pcie_ats_init(PCIDevice *dev, uint16_t offset) > > > > pci_set_word(dev->wmask + dev->exp.ats_cap + PCI_ATS_CTRL, 0x800f); > > } > > + > > +/* ACS (Access Control Services) */ > > +void pcie_acs_init(PCIDevice *dev, uint16_t offset) > > +{ > > + pcie_add_capability(dev, PCI_EXT_CAP_ID_ACS, PCI_ACS_VER, > > + offset, PCI_ACS_SIZEOF); > > + dev->exp.acs_cap = offset; > > + pci_set_word(dev->config + offset + PCI_ACS_CAP, > > + PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_UF); > > This is still only valid for downstream ports yet neither restricted > nor commented do indicate that. You could use an object_dynamic_cast > to triggger an assert should someone use it for an invalid type of > device, ex: > > assert(object_dynamic_cast(OBJECT(dev), TYPE_PCIE_SLOT)); Sorry, I didn't realize what you meant with v1 - this evolved from just a fix in the implementation of ioh3420 to a fix in the generic code, which I now realize of course is also used for downstream ports... > > + > > + pci_set_word(dev->config + offset + PCI_ACS_CTRL, 0); > > Suspect this is unnecessary given the reset callback. ok > > + pci_set_word(dev->wmask + offset + PCI_ACS_CTRL, > > + PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_UF); > > +} > > diff --git a/include/hw/pci/pcie.h b/include/hw/pci/pcie.h > > index 5b82a0d..4c40711 100644 > > --- a/include/hw/pci/pcie.h > > +++ b/include/hw/pci/pcie.h > > @@ -79,6 +79,9 @@ struct PCIExpressDevice { > > > > /* Offset of ATS capability in config space */ > > uint16_t ats_cap; > > + > > + /* ACS */ > > + uint16_t acs_cap; > > }; > > > > #define COMPAT_PROP_PCP "power_controller_present" > > @@ -116,6 +119,8 @@ void pcie_cap_flr_init(PCIDevice *dev); > > void pcie_cap_flr_write_config(PCIDevice *dev, > > uint32_t addr, uint32_t val, int len); > > > > +void pcie_cap_acs_reset(PCIDevice *dev); > > + > > /* ARI forwarding capability and control */ > > void pcie_cap_arifwd_init(PCIDevice *dev); > > void pcie_cap_arifwd_reset(PCIDevice *dev); > > @@ -129,6 +134,7 @@ void pcie_add_capability(PCIDevice *dev, > > void pcie_sync_bridge_lnk(PCIDevice *dev); > > > > void pcie_ari_init(PCIDevice *dev, uint16_t offset, uint16_t nextfn); > > +void pcie_acs_init(PCIDevice *dev, uint16_t offset); > > void pcie_dev_ser_num_init(PCIDevice *dev, uint16_t offset, uint64_t ser_num); > > void pcie_ats_init(PCIDevice *dev, uint16_t offset); > > > > diff --git a/include/hw/pci/pcie_regs.h b/include/hw/pci/pcie_regs.h > > index ad4e780..3fc9aca 100644 > > --- a/include/hw/pci/pcie_regs.h > > +++ b/include/hw/pci/pcie_regs.h > > @@ -175,4 +175,8 @@ typedef enum PCIExpLinkWidth { > > PCI_ERR_COR_INTERNAL | \ > > PCI_ERR_COR_HL_OVERFLOW) > > > > +/* ACS */ > > +#define PCI_ACS_VER 0x2 > > There's no such version, even the PCIe 5.0 drafts only define version 1. Hmm - I have no idea how it ended up as 2 in the first place - my model device is of course also v1 - will fix it. Thanks! Knut > > +#define PCI_ACS_SIZEOF 8 > > + > > #endif /* QEMU_PCIE_REGS_H */ > ^ permalink raw reply [flat|nested] 7+ messages in thread
* [Qemu-devel] [PATCH v2 2/2] gen_pcie_root_port: Add ACS (Access Control Services) capability 2019-01-24 10:12 [Qemu-devel] [PATCH v2 0/2] pcie: Add simple ACS "support" to the generic PCIe root port Knut Omang 2019-01-24 10:12 ` [Qemu-devel] [PATCH v2 1/2] pcie: Add a simple PCIe ACS (Access Control Services) helper function Knut Omang @ 2019-01-24 10:12 ` Knut Omang 2019-01-24 17:33 ` Alex Williamson 1 sibling, 1 reply; 7+ messages in thread From: Knut Omang @ 2019-01-24 10:12 UTC (permalink / raw) To: qemu-devel Cc: Michael S . Tsirkin, Marcel Apfelbaum, Alex Williamson, Stefan Hajnoczi, Elijah Shakkour, Tal Attaly, Knut Omang Claim ACS support in the generic PCIe root port to allow passthrough of individual functions of a device to different guests (in a nested virt.setting) with VFIO. Without this patch, all functions of a device, such as all VFs of an SR/IOV device, will end up in the same IOMMU group. A similar situation occurs on Windows with Hyper-V. In the single function device case, it also has a small cosmetic benefit in that the root port itself is not grouped with the device. VFIO handles that situation in that binding rules only apply to endpoints, so it does not limit passthrough in those cases. Signed-off-by: Knut Omang <knut.omang@oracle.com> --- hw/pci-bridge/gen_pcie_root_port.c | 2 ++ hw/pci-bridge/pcie_root_port.c | 4 ++++ include/hw/pci/pcie_port.h | 1 + 3 files changed, 7 insertions(+) diff --git a/hw/pci-bridge/gen_pcie_root_port.c b/hw/pci-bridge/gen_pcie_root_port.c index 9766edb..b5a5ecc 100644 --- a/hw/pci-bridge/gen_pcie_root_port.c +++ b/hw/pci-bridge/gen_pcie_root_port.c @@ -20,6 +20,7 @@ OBJECT_CHECK(GenPCIERootPort, (obj), TYPE_GEN_PCIE_ROOT_PORT) #define GEN_PCIE_ROOT_PORT_AER_OFFSET 0x100 +#define GEN_PCIE_ROOT_PORT_ACS_OFFSET 0x148 #define GEN_PCIE_ROOT_PORT_MSIX_NR_VECTOR 1 typedef struct GenPCIERootPort { @@ -149,6 +150,7 @@ static void gen_rp_dev_class_init(ObjectClass *klass, void *data) rpc->interrupts_init = gen_rp_interrupts_init; rpc->interrupts_uninit = gen_rp_interrupts_uninit; rpc->aer_offset = GEN_PCIE_ROOT_PORT_AER_OFFSET; + rpc->acs_offset = GEN_PCIE_ROOT_PORT_ACS_OFFSET; } static const TypeInfo gen_rp_dev_info = { diff --git a/hw/pci-bridge/pcie_root_port.c b/hw/pci-bridge/pcie_root_port.c index 34ad767..a0b4cf7 100644 --- a/hw/pci-bridge/pcie_root_port.c +++ b/hw/pci-bridge/pcie_root_port.c @@ -47,6 +47,7 @@ static void rp_reset(DeviceState *qdev) pcie_cap_deverr_reset(d); pcie_cap_slot_reset(d); pcie_cap_arifwd_reset(d); + pcie_cap_acs_reset(d); pcie_aer_root_reset(d); pci_bridge_reset(qdev); pci_bridge_disable_base_limit(d); @@ -106,6 +107,9 @@ static void rp_realize(PCIDevice *d, Error **errp) pcie_aer_root_init(d); rp_aer_vector_update(d); + if (rpc->acs_offset) { + pcie_acs_init(d, rpc->acs_offset); + } return; err: diff --git a/include/hw/pci/pcie_port.h b/include/hw/pci/pcie_port.h index df242a0..09586f4 100644 --- a/include/hw/pci/pcie_port.h +++ b/include/hw/pci/pcie_port.h @@ -78,6 +78,7 @@ typedef struct PCIERootPortClass { int exp_offset; int aer_offset; int ssvid_offset; + int acs_offset; /* If nonzero, optional ACS capability offset */ int ssid; } PCIERootPortClass; -- git-series 0.9.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/2] gen_pcie_root_port: Add ACS (Access Control Services) capability 2019-01-24 10:12 ` [Qemu-devel] [PATCH v2 2/2] gen_pcie_root_port: Add ACS (Access Control Services) capability Knut Omang @ 2019-01-24 17:33 ` Alex Williamson 2019-01-24 20:36 ` Knut Omang 0 siblings, 1 reply; 7+ messages in thread From: Alex Williamson @ 2019-01-24 17:33 UTC (permalink / raw) To: Knut Omang Cc: qemu-devel, Michael S . Tsirkin, Marcel Apfelbaum, Stefan Hajnoczi, Elijah Shakkour, Tal Attaly On Thu, 24 Jan 2019 11:12:53 +0100 Knut Omang <knut.omang@oracle.com> wrote: > Claim ACS support in the generic PCIe root port to allow > passthrough of individual functions of a device to different > guests (in a nested virt.setting) with VFIO. > Without this patch, all functions of a device, such as all VFs of > an SR/IOV device, will end up in the same IOMMU group. > A similar situation occurs on Windows with Hyper-V. > > In the single function device case, it also has a small cosmetic > benefit in that the root port itself is not grouped with > the device. VFIO handles that situation in that binding rules > only apply to endpoints, so it does not limit passthrough in > those cases. > > Signed-off-by: Knut Omang <knut.omang@oracle.com> > --- > hw/pci-bridge/gen_pcie_root_port.c | 2 ++ > hw/pci-bridge/pcie_root_port.c | 4 ++++ > include/hw/pci/pcie_port.h | 1 + > 3 files changed, 7 insertions(+) > > diff --git a/hw/pci-bridge/gen_pcie_root_port.c b/hw/pci-bridge/gen_pcie_root_port.c > index 9766edb..b5a5ecc 100644 > --- a/hw/pci-bridge/gen_pcie_root_port.c > +++ b/hw/pci-bridge/gen_pcie_root_port.c > @@ -20,6 +20,7 @@ > OBJECT_CHECK(GenPCIERootPort, (obj), TYPE_GEN_PCIE_ROOT_PORT) > > #define GEN_PCIE_ROOT_PORT_AER_OFFSET 0x100 > +#define GEN_PCIE_ROOT_PORT_ACS_OFFSET 0x148 So you prefer that everyone passing through here decode these to figure out that ACS_OFFSET is (AER_OFFSET + ERR_SIZEOF) since my comment on v1 was ignored? > #define GEN_PCIE_ROOT_PORT_MSIX_NR_VECTOR 1 > > typedef struct GenPCIERootPort { > @@ -149,6 +150,7 @@ static void gen_rp_dev_class_init(ObjectClass *klass, void *data) > rpc->interrupts_init = gen_rp_interrupts_init; > rpc->interrupts_uninit = gen_rp_interrupts_uninit; > rpc->aer_offset = GEN_PCIE_ROOT_PORT_AER_OFFSET; > + rpc->acs_offset = GEN_PCIE_ROOT_PORT_ACS_OFFSET; > } > > static const TypeInfo gen_rp_dev_info = { > diff --git a/hw/pci-bridge/pcie_root_port.c b/hw/pci-bridge/pcie_root_port.c > index 34ad767..a0b4cf7 100644 > --- a/hw/pci-bridge/pcie_root_port.c > +++ b/hw/pci-bridge/pcie_root_port.c > @@ -47,6 +47,7 @@ static void rp_reset(DeviceState *qdev) > pcie_cap_deverr_reset(d); > pcie_cap_slot_reset(d); > pcie_cap_arifwd_reset(d); > + pcie_cap_acs_reset(d); Only the generic root port initializes acs_offset to enable an ACS capability, but all members of the device class call the reset function which does no checking that an ACS capability exists. We've just corrupted config space for the device. > pcie_aer_root_reset(d); > pci_bridge_reset(qdev); > pci_bridge_disable_base_limit(d); > @@ -106,6 +107,9 @@ static void rp_realize(PCIDevice *d, Error **errp) > pcie_aer_root_init(d); > rp_aer_vector_update(d); > > + if (rpc->acs_offset) { > + pcie_acs_init(d, rpc->acs_offset); > + } > return; > > err: > diff --git a/include/hw/pci/pcie_port.h b/include/hw/pci/pcie_port.h > index df242a0..09586f4 100644 > --- a/include/hw/pci/pcie_port.h > +++ b/include/hw/pci/pcie_port.h > @@ -78,6 +78,7 @@ typedef struct PCIERootPortClass { > int exp_offset; > int aer_offset; > int ssvid_offset; > + int acs_offset; /* If nonzero, optional ACS capability offset */ > int ssid; > } PCIERootPortClass; > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/2] gen_pcie_root_port: Add ACS (Access Control Services) capability 2019-01-24 17:33 ` Alex Williamson @ 2019-01-24 20:36 ` Knut Omang 0 siblings, 0 replies; 7+ messages in thread From: Knut Omang @ 2019-01-24 20:36 UTC (permalink / raw) To: Alex Williamson Cc: qemu-devel, Michael S . Tsirkin, Marcel Apfelbaum, Stefan Hajnoczi, Elijah Shakkour, Tal Attaly On Thu, 2019-01-24 at 10:33 -0700, Alex Williamson wrote: > On Thu, 24 Jan 2019 11:12:53 +0100 > Knut Omang <knut.omang@oracle.com> wrote: > > > Claim ACS support in the generic PCIe root port to allow > > passthrough of individual functions of a device to different > > guests (in a nested virt.setting) with VFIO. > > Without this patch, all functions of a device, such as all VFs of > > an SR/IOV device, will end up in the same IOMMU group. > > A similar situation occurs on Windows with Hyper-V. > > > > In the single function device case, it also has a small cosmetic > > benefit in that the root port itself is not grouped with > > the device. VFIO handles that situation in that binding rules > > only apply to endpoints, so it does not limit passthrough in > > those cases. > > > > Signed-off-by: Knut Omang <knut.omang@oracle.com> > > --- > > hw/pci-bridge/gen_pcie_root_port.c | 2 ++ > > hw/pci-bridge/pcie_root_port.c | 4 ++++ > > include/hw/pci/pcie_port.h | 1 + > > 3 files changed, 7 insertions(+) > > > > diff --git a/hw/pci-bridge/gen_pcie_root_port.c b/hw/pci-bridge/gen_pcie_root_port.c > > index 9766edb..b5a5ecc 100644 > > --- a/hw/pci-bridge/gen_pcie_root_port.c > > +++ b/hw/pci-bridge/gen_pcie_root_port.c > > @@ -20,6 +20,7 @@ > > OBJECT_CHECK(GenPCIERootPort, (obj), TYPE_GEN_PCIE_ROOT_PORT) > > > > #define GEN_PCIE_ROOT_PORT_AER_OFFSET 0x100 > > +#define GEN_PCIE_ROOT_PORT_ACS_OFFSET 0x148 > > So you prefer that everyone passing through here decode these to figure > out that ACS_OFFSET is (AER_OFFSET + ERR_SIZEOF) since my comment on v1 > was ignored? Sorry, not at all - I managed to overlook your comment - will fix it, > > #define GEN_PCIE_ROOT_PORT_MSIX_NR_VECTOR 1 > > > > typedef struct GenPCIERootPort { > > @@ -149,6 +150,7 @@ static void gen_rp_dev_class_init(ObjectClass *klass, void *data) > > rpc->interrupts_init = gen_rp_interrupts_init; > > rpc->interrupts_uninit = gen_rp_interrupts_uninit; > > rpc->aer_offset = GEN_PCIE_ROOT_PORT_AER_OFFSET; > > + rpc->acs_offset = GEN_PCIE_ROOT_PORT_ACS_OFFSET; > > } > > > > static const TypeInfo gen_rp_dev_info = { > > diff --git a/hw/pci-bridge/pcie_root_port.c b/hw/pci-bridge/pcie_root_port.c > > index 34ad767..a0b4cf7 100644 > > --- a/hw/pci-bridge/pcie_root_port.c > > +++ b/hw/pci-bridge/pcie_root_port.c > > @@ -47,6 +47,7 @@ static void rp_reset(DeviceState *qdev) > > pcie_cap_deverr_reset(d); > > pcie_cap_slot_reset(d); > > pcie_cap_arifwd_reset(d); > > + pcie_cap_acs_reset(d); > > Only the generic root port initializes acs_offset to enable an ACS > capability, but all members of the device class call the reset function > which does no checking that an ACS capability exists. We've just > corrupted config space for the device. Ouch! Not good at all, sorry! Will look at it (after a good night's sleep this time..) Thanks! Knut > > pcie_aer_root_reset(d); > > pci_bridge_reset(qdev); > > pci_bridge_disable_base_limit(d); > > @@ -106,6 +107,9 @@ static void rp_realize(PCIDevice *d, Error **errp) > > pcie_aer_root_init(d); > > rp_aer_vector_update(d); > > > > + if (rpc->acs_offset) { > > + pcie_acs_init(d, rpc->acs_offset); > > + } > > return; > > > > err: > > diff --git a/include/hw/pci/pcie_port.h b/include/hw/pci/pcie_port.h > > index df242a0..09586f4 100644 > > --- a/include/hw/pci/pcie_port.h > > +++ b/include/hw/pci/pcie_port.h > > @@ -78,6 +78,7 @@ typedef struct PCIERootPortClass { > > int exp_offset; > > int aer_offset; > > int ssvid_offset; > > + int acs_offset; /* If nonzero, optional ACS capability offset */ > > int ssid; > > } PCIERootPortClass; > > > ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-01-24 20:47 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-01-24 10:12 [Qemu-devel] [PATCH v2 0/2] pcie: Add simple ACS "support" to the generic PCIe root port Knut Omang 2019-01-24 10:12 ` [Qemu-devel] [PATCH v2 1/2] pcie: Add a simple PCIe ACS (Access Control Services) helper function Knut Omang 2019-01-24 17:22 ` Alex Williamson 2019-01-24 20:14 ` Knut Omang 2019-01-24 10:12 ` [Qemu-devel] [PATCH v2 2/2] gen_pcie_root_port: Add ACS (Access Control Services) capability Knut Omang 2019-01-24 17:33 ` Alex Williamson 2019-01-24 20:36 ` Knut Omang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).