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

[Gavin Shan]

On Fri, May 23, 2014 at 12:01:05PM +0200, Alexander Graf wrote:
>
>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.
>

Ok. It seems that current QEMU implementation isn't what you want
at all.

>>
>>     :
>>     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.
>

I guess you probably want something like this:

- Introduce totally new EEHDevice and EEHClass.
- When realizing VFIODevice (in vfio_initfn()), create EEHDevice and
  and put it into linked list of sPAPRPHBState. From EEHDevice, we
  can maintain one "PCIDevice *pdev" for further querying about the
  PCI config address of the EEHDevice (actually VFIODevice).

Don't you think it's much more complicated than what we have? I'm not
sure. I seems that QEMU prefer implementing logic with class and object :-)

Thanks,
Gavin