On Sat, 2015-09-19 at 17:18 +1000, David Gibson wrote:
> The pseries machine type has two variants of the PCI Host Bridge device:
> spapr-pci-host-bridge and spapr-pci-vfio-host-bridge. Originally, only the
> latter could support VFIO devices, but we've now extended the VFIO code so
> that they both can.
>
> However, it's still only spapr-pci-vfio-host-bridge which supports the PAPR
> Enhanced Error Handling (EEH) interfaces. The reason is that we don't yet
> have a way to determine the correct VFIOGroup for EEH operations on the
> "plain" host bridge.
>
> Handling this in general is problematic due to some limitations in the
> current code, and some bugs in the kernel EEH interfaces. However, it's
> easy enough to do for the unambiguous case: where there is only one VFIO
> group used on the whole host bridge i.e. one Partitionable Endpoint (PE).
>
> This re-implements spapr_phb_check_vfio_group() in terms of the host
> bridge's Partitionable Endpoints, allowing EEH on any host bridge in the
> unambiguous case. This is enough to make spapr-pci-host-bridge support
> EEH as well as spapr-pci-vfio-host-bridge, since the latter only supported
> devices from one IOMMU group anyway (although it didn't properly
> enforce that).
>
> Signed-off-by: David Gibson <da...@gibson.dropbear.id.au>
> ---
> hw/ppc/spapr_pci.c | 36 ++++++++++++++++++++++++++++++++++++
> hw/ppc/spapr_pci_vfio.c | 18 ------------------
> include/hw/pci-host/spapr.h | 1 -
> 3 files changed, 36 insertions(+), 19 deletions(-)
>
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 81ad3ae..5614b45 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -41,6 +41,8 @@
> #include "hw/ppc/spapr_drc.h"
> #include "sysemu/device_tree.h"
>
> +#include "hw/vfio/vfio-common.h"
> +
> /* Copied from the kernel arch/powerpc/platforms/pseries/msi.c */
> #define RTAS_QUERY_FN 0
> #define RTAS_CHANGE_FN 1
> @@ -446,6 +448,40 @@ static sPAPRPHBGuestPE
> *spapr_phb_pe_by_device(sPAPRPHBState *phb,
> return pe;
> }
>
> +static int spapr_phb_check_vfio_group(sPAPRPHBState *phb, VFIOGroup **gpp)
> +{
> + sPAPRPHBGuestPE *pe;
> +
> + if (QLIST_EMPTY(&phb->pe_list)) {
> + /* No EEH capable devices on this PHB */
> + return RTAS_OUT_PARAM_ERROR;
> + }
> +
> + /* Limitations in both qemu and the kernel mean that, for now, EEH
> + * won't work if there are devices from more than one PE
> + * (i.e. IOMMU group) on the same PHB */
> + pe = QLIST_FIRST(&phb->pe_list);
> + if (QLIST_NEXT(pe, list)) {
> + error_report("spapr-pci-host-bridge: EEH attempted on PHB with
> multiple"
> + " IOMMU groups");
Don't wrap lines with search-able strings
> + return RTAS_OUT_HW_ERROR;
> + }
> +
> + if (!object_dynamic_cast(OBJECT(phb), "spapr-pci-vfio-host-bridge")) {
> + sPAPRPHBVFIOState *svphb = SPAPR_PCI_VFIO_HOST_BRIDGE(phb);
> + /* FIXME: this is an abstraction violation */
> + if (pe->group->groupid != svphb->iommugroupid) {
> + error_report("spapr-pci-vfio-host-bridge: Bad IOMMU group");
> + return RTAS_OUT_HW_ERROR;
> + }
> + }
> +
> + if (gpp) {
> + *gpp = pe->group;
> + }
> + return RTAS_OUT_SUCCESS;
The structure of this function finally makes some sense once we get
here. I'm still not sure whether RTAS error codes make sense though,
but it's just a nit. I'd still prefer the function was renamed, "check"
indicates a verification, not a return. The primary purpose of calling
this function is to get a group, not simply to check the validity.
> +}
> +
> static void rtas_ibm_set_eeh_option(PowerPCCPU *cpu,
> sPAPRMachineState *spapr,
> uint32_t token, uint32_t nargs,
> diff --git a/hw/ppc/spapr_pci_vfio.c b/hw/ppc/spapr_pci_vfio.c
> index b61923c..a08292a 100644
> --- a/hw/ppc/spapr_pci_vfio.c
> +++ b/hw/ppc/spapr_pci_vfio.c
> @@ -24,29 +24,11 @@
> #include "hw/vfio/vfio.h"
> #include "hw/vfio/vfio-eeh.h"
>
> -#include "hw/vfio/vfio-common.h"
> -
> static Property spapr_phb_vfio_properties[] = {
> DEFINE_PROP_INT32("iommu", sPAPRPHBVFIOState, iommugroupid, -1),
> DEFINE_PROP_END_OF_LIST(),
> };
>
> -int spapr_phb_check_vfio_group(sPAPRPHBState *phb, VFIOGroup **gpp)
> -{
> - VFIOGroup *group;
> -
> - if (!object_dynamic_cast(OBJECT(phb), "spapr-pci-vfio-host-bridge")) {
> - return RTAS_OUT_PARAM_ERROR;
> - }
> -
> - /* FIXME: this is an abstraction violation */
> - group = vfio_get_group(SPAPR_PCI_VFIO_HOST_BRIDGE(phb)->iommugroupid,
> - &phb->iommu_as);
> - if (gpp)
> - *gpp = group;
> - return RTAS_OUT_SUCCESS;
> -}
> -
> static void spapr_phb_vfio_finish_realize(sPAPRPHBState *sphb, Error **errp)
> {
> sPAPRPHBVFIOState *svphb = SPAPR_PCI_VFIO_HOST_BRIDGE(sphb);
> diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h
> index 535e5ef..8c8d187 100644
> --- a/include/hw/pci-host/spapr.h
> +++ b/include/hw/pci-host/spapr.h
> @@ -138,7 +138,6 @@ void spapr_pci_msi_init(sPAPRMachineState *spapr, hwaddr
> addr);
>
> void spapr_pci_rtas_init(void);
>
> -int spapr_phb_check_vfio_group(sPAPRPHBState *phb, VFIOGroup **gpp);
> sPAPRPHBState *spapr_pci_find_phb(sPAPRMachineState *spapr, uint64_t buid);
> PCIDevice *spapr_pci_find_dev(sPAPRMachineState *spapr, uint64_t buid,
> uint32_t config_addr);
