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

[Gavin Shan]

On Fri, May 23, 2014 at 02:25:57PM +0200, Alexander Graf wrote:
>On 23.05.14 14:14, Gavin Shan wrote:
>>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 :-)
>
>Andreas, is there anything like an "implements" in QOM? I would like
>to have a PCIDevice that "implements EEH" and thus makes an eeh call
>available.
>

Andreas might have idea on this and lets see what suggestion Andreas will
have.

I originally thought that "interfaces" could be used for the purpose. However,
it seems "interfaces" also needs to create additional "interface" class/object
for EEH functionality and it's not much different from the above "EEHDevice"
from the perspective.

>Or should we just extend the normal PCIDevice class with an eeh callback?
>

I think it would be the simple way to go. However, I probably need one more
flag (e.g. PCI_DEV_VFIO and PCIDevice::flags) to distinguish VFIO device from
PCI device because emulated PCI device shouldn't have EEH functionality.

Thanks,
Gavin