Re: [Qemu-devel] [RFC PATCH 11/14] spapr_pci: Allow EEH on spapr-pci-host-bridge

[Alex Williamson <alex.williamson@redhat.com>]

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 <david@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);