Re: [Qemu-ppc] [PATCH v6 3/3] sPAPR: Support EEH RTAS services

[Alexander Graf]

On 23.05.14 02:00, Gavin Shan wrote:
> On Thu, May 22, 2014 at 12:03:11PM +0200, Alexander Graf wrote:
>
> .../...
>
> > > +static void rtas_ibm_set_eeh_option(PowerPCCPU *cpu,
> > > +                                    sPAPREnvironment *spapr,
> > > +                                    uint32_t token, uint32_t nargs,
> > > +                                    target_ulong args, uint32_t nret,
> > > +                                    target_ulong rets)
> > > +{
> .../...
>
> > > +
> > > +    addr = rtas_ld(args, 0);
> > > +    option.argsz = sizeof(option);
> > > +    option.option = rtas_ld(args, 3);
> > > +    if (option.option > 3) {
> > > +        rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
> > > +        return;
> > > +    }
> > > +
> > > +    ret = vfio_eeh_handler(VFIO_EEH_PE_SET_OPTION, &option,
> > > +                           sphb->parent_obj.bus, addr);
> > There is no reason EEH would be tied to VFIO. We could just as well
> > emulate EEH. I guess what you really want here is to find the device
> > the guest is trying to access and then call a function in its class
> > to handle that particular eeh event.
> You're right. Firstly, double-check if I catched your point. what you
> want to see is something like following piece of code in spapr_pci.c?
>
> static void rtas_ibm_set_eeh_option(PowerPCCPU *cpu,
>                                      sPAPREnvironment *spapr,
>                                      uint32_t token, uint32_t nargs,
>                                      target_ulong args, uint32_t nret,
>                                      target_ulong rets)
> {
>      VFIODevice *vdev;
>      VFIODeviceClass *vc;

I don't want to see VFIO show up anywhere in RTAS code. We're trying to 
do EEH operations on a device. Whether that device is backed by VFIO is 
an implementation detail that should be hidden away.

>      :
>      vdev = vfio_find_dev_by_addr(sphb->parent_obj.bus, addr);
>      if (!vdev) {
>          rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
>          return;
>      }
>
>      vc = VFIODevice_to_VFIODeviceClass(vdev);
>      ret = vc->set_eeh_option(vdev, option);

But yes, the path to that leads through class abstraction :). VFIO is 
just not the right class.


Alex

>      if (ret < 0) {
>          rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
>          return;
>      }
>
>      rtas_st(rets, 0, RTAS_OUT_PARAM_SUCCESS);
> }
>
> But we don't have VFIODevice class defined in any header file
> yet and VFIODevice is defined in hw/misc/vfio.c as well.
>
> Thanks,
> Gavin