Re: [Qemu-devel] [PATCH v13 2/3] sPAPR: Implement EEH RTAS calls

[Gavin Shan]

On Tue, Dec 16, 2014 at 12:13:03AM +0100, Alexander Graf wrote:
>On 16.12.14 00:08, Gavin Shan wrote:
>> On Mon, Dec 15, 2014 at 03:52:17PM +0100, Alexander Graf wrote:
>>> On 15.12.14 01:15, Gavin Shan wrote:
>>>> The emulation for EEH RTAS requests from guest isn't covered
>>>> by QEMU yet and the patch implements them.
>>>>
>>>> The patch defines constants used by EEH RTAS calls and adds
>>>> callback sPAPRPHBClass::eeh_handler, which is going to be used
>>>> this way:
>>>>
>>>>   * RTAS calls are received in spapr_pci.c, sanity check is done
>>>>     there.
>>>>   * RTAS handlers handle what they can. If there is something it
>>>>     cannot handle and sPAPRPHBClass::eeh_handler callback is defined,
>>>>     it is called.
>>>>   * sPAPRPHBClass::eeh_handler is only implemented for VFIO now. It
>>>>     does ioctl() to the IOMMU container fd to complete the call. Error
>>>>     codes from that ioctl() are transferred back to the guest.
>>>>
>>>> [aik: defined RTAS tokens for EEH RTAS calls]
>>>> Signed-off-by: Gavin Shan <address@hidden>
>>>> ---
>>>>  hw/ppc/spapr_pci.c          | 246 
>>>> ++++++++++++++++++++++++++++++++++++++++++++
>>>>  include/hw/pci-host/spapr.h |   7 ++
>>>>  include/hw/ppc/spapr.h      |  43 +++++++-
>>>>  3 files changed, 294 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
>>>> index 3d70efe..3bb1971 100644
>>>> --- a/hw/ppc/spapr_pci.c
>>>> +++ b/hw/ppc/spapr_pci.c
>>>> @@ -406,6 +406,233 @@ static void 
>>>> rtas_ibm_query_interrupt_source_number(PowerPCCPU *cpu,
>>>>      rtas_st(rets, 2, 1);/* 0 == level; 1 == edge */
>>>>  }
>>>>  
>>>> +static int rtas_handle_eeh_request(sPAPREnvironment *spapr,
>>>> +                                   uint64_t buid, uint32_t req, uint32_t 
>>>> opt)
>>>> +{
>>>> +    sPAPRPHBState *sphb = spapr_pci_find_phb(spapr, buid);
>>>> +    sPAPRPHBClass *info = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb);
>>>
>>> What happens when you try to cast NULL? Could a guest process invoke a
>>> host assert() through this and abort the whole VM?
>>>
>> 
>> Yes, it would cause core dump. I had one experiment to force assigning
>> NULL to "sphb" before doing the cast, the whole VM is aborted. So I
>> guess you're happy to have something as follows. If you're not suggesting
>> something else, I'll update the code as follows in next version:
>> 
>>      sPAPRPHBState *sphb = spapr_pci_find_phb(spapr, buid);
>>      sPAPRPHBClass *info;
>> 
>>      if (!sphb) {
>>          return -ENODEV;
>>      }
>> 
>>      info = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb);
>>      if (!info->eeh_handler) {
>>          return -ENOENT;
>>      }
>> 
>>      return info->eeh_handler(sphb, req, opt);
>
>Yes, I think this is a lot safer. And yes, the other patch looks sane to me.
>

Thank you for your time reviewing this. Will update in next version.

>> 
>>>> +
>>>> +    if (!sphb || !info->eeh_handler) {
>>>> +        return -ENOENT;
>>>> +    }
>>>> +
>>>> +    return info->eeh_handler(sphb, req, opt);
>>>> +}
>>>> +
>>>> +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)
>>>> +{
>>>> +    uint32_t addr, option;
>>>> +    uint64_t buid = ((uint64_t)rtas_ld(args, 1) << 32) | rtas_ld(args, 2);
>>>> +    int ret;
>>>> +
>>>> +    if ((nargs != 4) || (nret != 1)) {
>>>> +        goto param_error_exit;
>>>> +    }
>>>> +
>>>> +    addr = rtas_ld(args, 0);
>>>> +    option = rtas_ld(args, 3);
>>>> +    switch (option) {
>>>> +    case RTAS_EEH_ENABLE:
>>>> +        if (!spapr_pci_find_dev(spapr, buid, addr)) {
>>>> +            goto param_error_exit;
>>>> +        }
>>>> +        break;
>>>> +    case RTAS_EEH_DISABLE:
>>>> +    case RTAS_EEH_THAW_IO:
>>>> +    case RTAS_EEH_THAW_DMA:
>>>
>>> So these don't use the addr hint?
>>>
>> 
>> No, there're no address as argument of this RTAS call "ibm,set-eeh-option".
>> The RTAS call has 4 arguments, all of them are 32-bits: BUID high part, BUID
>> low part, PE address, option. The option could be one of: enable/disable EEH
>> functionality, enable IO path, enable DMA path.
>
>Well, I'm just wondering that ENABLE wants to make sure there's a device
>and the others don't.
>

Oh, I misunderstood your question. Yes, you're correct. All options
except ENABLE will have address check in rtas_handle_eeh_request()
where we check on "BUID" since each PHB and PE have one-to-one
relationship.

ENABLE and other options are using different address: enable
uses config address of one specific device, but other options use PE
address. From guest's sides, it enables EEH capability on basis
of PCI device and left options are supported on basis of PE. Each
PE could contain one or multiple PCI devices.

DISABLE option isn't used until now.

Thanks,
Gavin

>
>Alex
>