Re: [Qemu-devel] [Qemu-ppc] [RFC 03/12] spapr_pci: Eliminate class callbacks

[David Gibson]

On Tue, Nov 24, 2015 at 10:00:15AM +0100, Alexander Graf wrote:
> 
> 
> On 19.11.15 05:29, David Gibson wrote:
> > 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 <address@hidden>
> > ---
> >  hw/ppc/spapr_pci.c          | 44 
> > ++++++++++++++++++++++----------------------
> >  hw/ppc/spapr_pci_vfio.c     | 18 +++++++-----------
> >  include/hw/pci-host/spapr.h | 13 +++++++++----
> >  3 files changed, 38 insertions(+), 37 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> > index 55fa8db..9203d15 100644
> > --- a/hw/ppc/spapr_pci.c
> > +++ b/hw/ppc/spapr_pci.c
> > @@ -91,6 +91,13 @@ PCIDevice *spapr_pci_find_dev(sPAPRMachineState *spapr, 
> > uint64_t buid,
> >      return pci_find_device(phb->bus, bus_num, devfn);
> >  }
> >  
> > +static bool spapr_phb_eeh_available(sPAPRPHBState *sphb)
> > +{
> > +    sPAPRPHBClass *spc = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb);
> > +
> > +    return spc->eeh_available;
> > +}
> > +
> >  static uint32_t rtas_pci_cfgaddr(uint32_t arg)
> >  {
> >      /* This handles the encoding of extended config space addresses */
> > @@ -430,7 +437,6 @@ static void rtas_ibm_set_eeh_option(PowerPCCPU *cpu,
> >                                      target_ulong rets)
> >  {
> >      sPAPRPHBState *sphb;
> > -    sPAPRPHBClass *spc;
> >      uint32_t addr, option;
> >      uint64_t buid;
> >      int ret;
> > @@ -448,12 +454,11 @@ static void rtas_ibm_set_eeh_option(PowerPCCPU *cpu,
> >          goto param_error_exit;
> >      }
> >  
> > -    spc = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb);
> > -    if (!spc->eeh_set_option) {
> > +    if (!spapr_phb_eeh_available(sphb)) {
> >          goto param_error_exit;
> >      }
> >  
> > -    ret = spc->eeh_set_option(sphb, addr, option);
> > +    ret = spapr_phb_vfio_eeh_set_option(sphb, addr, option);
> 
> First of all, I think the direction you're taking with this series is sound.
> 
> However the hunk above would break compilation on systems that don't
> have VFIO enabled (such as Windows hosts for example).

Ah drat, good point.

> The same holds true for the defines you're using later in the series.
> Things like VFIO_EEH_PE_GET_STATE are not defined for non-Linux hosts.
> 
> I think the best path to get to where we really want to be is to define
> qemu internal defines and eeh helpers for emulated phb mode. Make sure
> the constants match the VFIO constants and use them interchangibly (with
> a big fat comment saying that they are, and a number of ifdefs making
> sure they stay identical).

I'll see what I can do.

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature