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

[Alexander Graf]

> Am 26.05.2014 um 02:24 schrieb Gavin Shan <address@hidden>:
> 
>> 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.

They could just not implement the callback yet. Eventually we may want EEH 
emulation for the sake of testing.


Alex