From 3153119e9bb79ebd82e08d2fde80d1c3a039b6b2 Mon Sep 17 00:00:00 2001 From: David Gibson Date: Wed, 9 Mar 2016 11:56:06 +1100 Subject: vfio: Start improving VFIO/EEH interface At present the code handling IBM's Enhanced Error Handling (EEH) interface on VFIO devices operates by bypassing the usual VFIO logic with vfio_container_ioctl(). That's a poorly designed interface with unclear semantics about exactly what can be operated on. In particular it operates on a single vfio container internally (hence the name), but takes an address space and group id, from which it deduces the container in a rather roundabout way. groupids are something that code outside vfio shouldn't even be aware of. This patch creates new interfaces for EEH operations. Internally we have vfio_eeh_container_op() which takes a VFIOContainer object directly. For external use we have vfio_eeh_as_ok() which determines if an AddressSpace is usable for EEH (at present this means it has a single container with exactly one group attached), and vfio_eeh_as_op() which will perform an operation on an AddressSpace in the unambiguous case, and otherwise returns an error. This interface still isn't great, but it's enough of an improvement to allow a number of cleanups in other places. Signed-off-by: David Gibson Reviewed-by: Alexey Kardashevskiy Acked-by: Alex Williamson --- include/hw/vfio/vfio.h | 2 ++ 1 file changed, 2 insertions(+) (limited to 'include/hw') diff --git a/include/hw/vfio/vfio.h b/include/hw/vfio/vfio.h index 0b26cd8..fd3933b 100644 --- a/include/hw/vfio/vfio.h +++ b/include/hw/vfio/vfio.h @@ -5,5 +5,7 @@ extern int vfio_container_ioctl(AddressSpace *as, int32_t groupid, int req, void *param); +bool vfio_eeh_as_ok(AddressSpace *as); +int vfio_eeh_as_op(AddressSpace *as, uint32_t op); #endif -- cgit v1.1 From fbb4e983415dc5a15e167dd00bc4564c57121915 Mon Sep 17 00:00:00 2001 From: David Gibson Date: Mon, 29 Feb 2016 17:45:05 +1100 Subject: spapr_pci: Eliminate class callbacks The EEH operations in the spapr-vfio-pci-host-bridge no longer rely on the special groupid field in sPAPRPHBVFIOState. So we can simplify, removing the class specific callbacks with direct calls based on a simple spapr_phb_eeh_enabled() helper. For now we implement that in terms of a boolean in the class, but we'll continue to clean that up later. On its own this is a rather strange way of doing things, but it's a useful intermediate step to further cleanups. Signed-off-by: David Gibson Reviewed-by: Alexey Kardashevskiy --- include/hw/pci-host/spapr.h | 37 +++++++++++++++++++++++++++++++++---- 1 file changed, 33 insertions(+), 4 deletions(-) (limited to 'include/hw') diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h index 7de5e02..0b936c6 100644 --- a/include/hw/pci-host/spapr.h +++ b/include/hw/pci-host/spapr.h @@ -49,10 +49,7 @@ struct sPAPRPHBClass { PCIHostBridgeClass parent_class; void (*finish_realize)(sPAPRPHBState *sphb, Error **errp); - int (*eeh_set_option)(sPAPRPHBState *sphb, unsigned int addr, int option); - int (*eeh_get_state)(sPAPRPHBState *sphb, int *state); - int (*eeh_reset)(sPAPRPHBState *sphb, int option); - int (*eeh_configure)(sPAPRPHBState *sphb); + bool eeh_available; }; typedef struct spapr_pci_msi { @@ -137,4 +134,36 @@ sPAPRPHBState *spapr_pci_find_phb(sPAPRMachineState *spapr, uint64_t buid); PCIDevice *spapr_pci_find_dev(sPAPRMachineState *spapr, uint64_t buid, uint32_t config_addr); +/* VFIO EEH hooks */ +#ifdef CONFIG_LINUX +int spapr_phb_vfio_eeh_set_option(sPAPRPHBState *sphb, + unsigned int addr, int option); +int spapr_phb_vfio_eeh_get_state(sPAPRPHBState *sphb, int *state); +int spapr_phb_vfio_eeh_reset(sPAPRPHBState *sphb, int option); +int spapr_phb_vfio_eeh_configure(sPAPRPHBState *sphb); +void spapr_phb_vfio_reset(DeviceState *qdev); +#else +static inline int spapr_phb_vfio_eeh_set_option(sPAPRPHBState *sphb, + unsigned int addr, int option) +{ + return RTAS_OUT_HW_ERROR; +} +static inline int spapr_phb_vfio_eeh_get_state(sPAPRPHBState *sphb, + int *state) +{ + return RTAS_OUT_HW_ERROR; +} +static inline int spapr_phb_vfio_eeh_reset(sPAPRPHBState *sphb, int option) +{ + return RTAS_OUT_HW_ERROR; +} +static inline int spapr_phb_vfio_eeh_configure(sPAPRPHBState *sphb) +{ + return RTAS_OUT_HW_ERROR; +} +static inline void spapr_phb_vfio_reset(DeviceState *qdev) +{ +} +#endif + #endif /* __HW_SPAPR_PCI_H__ */ -- cgit v1.1 From c1fa017c7e9b017ec0a75f8added7f9c4864fe8a Mon Sep 17 00:00:00 2001 From: David Gibson Date: Mon, 29 Feb 2016 17:19:42 +1100 Subject: spapr_pci: Allow EEH on spapr-pci-host-bridge Now that the EEH code is independent of the special spapr-vfio-pci-host-bridge device, we can allow it on all spapr PCI host bridges instead. We do this by changing spapr_phb_eeh_available() to be based on the vfio_eeh_as_ok() call instead of the host bridge class. Because the value of vfio_eeh_as_ok() can change with devices being hotplugged or unplugged, this can potentially lead to some strange edge cases where the guest starts using EEH, then it starts failing because of a change in status. However, it's not really any worse than the current situation. Cases that would have worked previously will still work (i.e. VFIO devices from at most one VFIO IOMMU group per vPHB), it's just that it's no longer necessary to use spapr-vfio-pci-host-bridge with the groupid pre-specified. Signed-off-by: David Gibson Reviewed-by: Alexey Kardashevskiy --- include/hw/pci-host/spapr.h | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) (limited to 'include/hw') diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h index 0b936c6..19a95e0 100644 --- a/include/hw/pci-host/spapr.h +++ b/include/hw/pci-host/spapr.h @@ -49,7 +49,6 @@ struct sPAPRPHBClass { PCIHostBridgeClass parent_class; void (*finish_realize)(sPAPRPHBState *sphb, Error **errp); - bool eeh_available; }; typedef struct spapr_pci_msi { @@ -136,6 +135,7 @@ PCIDevice *spapr_pci_find_dev(sPAPRMachineState *spapr, uint64_t buid, /* VFIO EEH hooks */ #ifdef CONFIG_LINUX +bool spapr_phb_eeh_available(sPAPRPHBState *sphb); int spapr_phb_vfio_eeh_set_option(sPAPRPHBState *sphb, unsigned int addr, int option); int spapr_phb_vfio_eeh_get_state(sPAPRPHBState *sphb, int *state); @@ -143,6 +143,10 @@ int spapr_phb_vfio_eeh_reset(sPAPRPHBState *sphb, int option); int spapr_phb_vfio_eeh_configure(sPAPRPHBState *sphb); void spapr_phb_vfio_reset(DeviceState *qdev); #else +static inline bool spapr_phb_eeh_available(sPAPRPHBState *sphb) +{ + return false; +} static inline int spapr_phb_vfio_eeh_set_option(sPAPRPHBState *sphb, unsigned int addr, int option) { -- cgit v1.1 From 72700d7e733948fa7fbb735ccdf2209931c88476 Mon Sep 17 00:00:00 2001 From: David Gibson Date: Mon, 29 Feb 2016 17:19:50 +1100 Subject: spapr_pci: (Mostly) remove spapr-pci-vfio-host-bridge Now that the regular spapr-pci-host-bridge can handle EEH, there are only two things that spapr-pci-vfio-host-bridge does differently: 1. automatically sizes its DMA window to match the host IOMMU 2. checks if the attached VFIO container is backed by the VFIO_SPAPR_TCE_IOMMU type on the host (1) is not particularly useful, since the default window used by the regular host bridge will work with the host IOMMU configuration on all current systems anyway. Plus, automatically changing guest visible configuration (such as the DMA window) based on host settings is generally a bad idea. It's not definitively broken, since spapr-pci-vfio-host-bridge is only supposed to support VFIO devices which can't be migrated anyway, but still. (2) is not really useful, because if a guest tries to configure EEH on a different host IOMMU, the first call will fail and that will be that. It's possible there are scripts or tools out there which expect spapr-pci-vfio-host-bridge, so we don't remove it entirely. This patch reduces it to just a stub for backwards compatibility. Signed-off-by: David Gibson Reviewed-by: Alexey Kardashevskiy --- include/hw/pci-host/spapr.h | 11 ----------- 1 file changed, 11 deletions(-) (limited to 'include/hw') diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h index 19a95e0..a08235e 100644 --- a/include/hw/pci-host/spapr.h +++ b/include/hw/pci-host/spapr.h @@ -28,14 +28,10 @@ #include "hw/ppc/xics.h" #define TYPE_SPAPR_PCI_HOST_BRIDGE "spapr-pci-host-bridge" -#define TYPE_SPAPR_PCI_VFIO_HOST_BRIDGE "spapr-pci-vfio-host-bridge" #define SPAPR_PCI_HOST_BRIDGE(obj) \ OBJECT_CHECK(sPAPRPHBState, (obj), TYPE_SPAPR_PCI_HOST_BRIDGE) -#define SPAPR_PCI_VFIO_HOST_BRIDGE(obj) \ - OBJECT_CHECK(sPAPRPHBVFIOState, (obj), TYPE_SPAPR_PCI_VFIO_HOST_BRIDGE) - #define SPAPR_PCI_HOST_BRIDGE_CLASS(klass) \ OBJECT_CLASS_CHECK(sPAPRPHBClass, (klass), TYPE_SPAPR_PCI_HOST_BRIDGE) #define SPAPR_PCI_HOST_BRIDGE_GET_CLASS(obj) \ @@ -43,7 +39,6 @@ typedef struct sPAPRPHBClass sPAPRPHBClass; typedef struct sPAPRPHBState sPAPRPHBState; -typedef struct sPAPRPHBVFIOState sPAPRPHBVFIOState; struct sPAPRPHBClass { PCIHostBridgeClass parent_class; @@ -90,12 +85,6 @@ struct sPAPRPHBState { QLIST_ENTRY(sPAPRPHBState) list; }; -struct sPAPRPHBVFIOState { - sPAPRPHBState phb; - - int32_t iommugroupid; -}; - #define SPAPR_PCI_MAX_INDEX 255 #define SPAPR_PCI_BASE_BUID 0x800000020000000ULL -- cgit v1.1 From a36304fdca8ae38f756f2c86158ce1f0831e7fbc Mon Sep 17 00:00:00 2001 From: David Gibson Date: Mon, 29 Feb 2016 17:20:00 +1100 Subject: spapr_pci: Remove finish_realize hook Now that spapr-pci-vfio-host-bridge is reduced to just a stub, there is only one implementation of the finish_realize hook in sPAPRPHBClass. So, we can fold that implementation into its (single) caller, and remove the hook. That's the last thing left in sPAPRPHBClass, so that can go away as well. Signed-off-by: David Gibson Reviewed-by: Alexey Kardashevskiy --- include/hw/pci-host/spapr.h | 12 ------------ 1 file changed, 12 deletions(-) (limited to 'include/hw') diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h index a08235e..03ee006 100644 --- a/include/hw/pci-host/spapr.h +++ b/include/hw/pci-host/spapr.h @@ -32,20 +32,8 @@ #define SPAPR_PCI_HOST_BRIDGE(obj) \ OBJECT_CHECK(sPAPRPHBState, (obj), TYPE_SPAPR_PCI_HOST_BRIDGE) -#define SPAPR_PCI_HOST_BRIDGE_CLASS(klass) \ - OBJECT_CLASS_CHECK(sPAPRPHBClass, (klass), TYPE_SPAPR_PCI_HOST_BRIDGE) -#define SPAPR_PCI_HOST_BRIDGE_GET_CLASS(obj) \ - OBJECT_GET_CLASS(sPAPRPHBClass, (obj), TYPE_SPAPR_PCI_HOST_BRIDGE) - -typedef struct sPAPRPHBClass sPAPRPHBClass; typedef struct sPAPRPHBState sPAPRPHBState; -struct sPAPRPHBClass { - PCIHostBridgeClass parent_class; - - void (*finish_realize)(sPAPRPHBState *sphb, Error **errp); -}; - typedef struct spapr_pci_msi { uint32_t first_irq; uint32_t num; -- cgit v1.1 From 3356128cd13d7ec7689b7cddd3efbfbc5339a262 Mon Sep 17 00:00:00 2001 From: David Gibson Date: Wed, 9 Mar 2016 11:57:20 +1100 Subject: vfio: Eliminate vfio_container_ioctl() vfio_container_ioctl() was a bad interface that bypassed abstraction boundaries, had semantics that sat uneasily with its name, and was unsafe in many realistic circumstances. Now that spapr-pci-vfio-host-bridge has been folded into spapr-pci-host-bridge, there are no more users, so remove it. Signed-off-by: David Gibson Reviewed-by: Alexey Kardashevskiy Acked-by: Alex Williamson --- include/hw/vfio/vfio.h | 2 -- 1 file changed, 2 deletions(-) (limited to 'include/hw') diff --git a/include/hw/vfio/vfio.h b/include/hw/vfio/vfio.h index fd3933b..7153604 100644 --- a/include/hw/vfio/vfio.h +++ b/include/hw/vfio/vfio.h @@ -3,8 +3,6 @@ #include "qemu/typedefs.h" -extern int vfio_container_ioctl(AddressSpace *as, int32_t groupid, - int req, void *param); bool vfio_eeh_as_ok(AddressSpace *as); int vfio_eeh_as_op(AddressSpace *as, uint32_t op); -- cgit v1.1