Re: [Qemu-ppc] [PATCH v15 2/2] sPAPR: Implement sPAPRPHBClass::eeh_handler

[Gavin Shan]

On Wed, Jan 14, 2015 at 12:41:59PM +1100, David Gibson wrote:
>On Mon, Jan 05, 2015 at 11:26:28AM +1100, Gavin Shan wrote:
>> The patch implements sPAPRPHBClass::eeh_handler so that the
>> EEH RTAS requests can be routed to VFIO for further handling.
>> 
>> Signed-off-by: Gavin Shan <address@hidden>
>> ---
>>  hw/ppc/spapr_pci_vfio.c | 56 
>> +++++++++++++++++++++++++++++++++++++++++++++++++
>>  hw/vfio/common.c        |  1 +
>>  2 files changed, 57 insertions(+)
>> 
>> diff --git a/hw/ppc/spapr_pci_vfio.c b/hw/ppc/spapr_pci_vfio.c
>> index 144912b..73652a9 100644
>> --- a/hw/ppc/spapr_pci_vfio.c
>> +++ b/hw/ppc/spapr_pci_vfio.c
>> @@ -71,6 +71,61 @@ static void spapr_phb_vfio_finish_realize(sPAPRPHBState 
>> *sphb, Error **errp)
>>                                  spapr_tce_get_iommu(tcet));
>>  }
>>  
>> +static int spapr_phb_vfio_eeh_handler(sPAPRPHBState *sphb, int req, int opt)
>> +{
>> +    sPAPRPHBVFIOState *svphb = SPAPR_PCI_VFIO_HOST_BRIDGE(sphb);
>> +    struct vfio_eeh_pe_op op = { .argsz = sizeof(op) };
>
>This is a local variable, which means it won't be initialized.  You
>never memset() it and it's not obvious that all fields get
>initialized, which makes it dangerous to pass to an ioctl().
>

Yeah, we might get uncertain "flags" passed to ioctl(). I'll change the
code like this if you agree:

        struct vfio_eeh_pe_op op;

        memset(&op, 0, sizeof(op));
        op.argsz = sizeof(op);

>> +    int cmd;
>> +
>> +    switch (req) {
>> +    case RTAS_EEH_REQ_SET_OPTION:
>> +        switch (opt) {
>> +        case RTAS_EEH_DISABLE:
>> +            cmd = VFIO_EEH_PE_DISABLE;
>> +            break;
>> +        case RTAS_EEH_ENABLE:
>> +            cmd = VFIO_EEH_PE_ENABLE;
>> +            break;
>> +        case RTAS_EEH_THAW_IO:
>> +            cmd = VFIO_EEH_PE_UNFREEZE_IO;
>> +            break;
>> +        case RTAS_EEH_THAW_DMA:
>> +            cmd = VFIO_EEH_PE_UNFREEZE_DMA;
>> +            break;
>> +        default:
>> +            return -EINVAL;
>> +        }
>> +        break;
>> +    case RTAS_EEH_REQ_GET_STATE:
>> +        cmd = VFIO_EEH_PE_GET_STATE;
>> +        break;
>> +    case RTAS_EEH_REQ_RESET:
>> +        switch (opt) {
>> +        case RTAS_SLOT_RESET_DEACTIVATE:
>> +            cmd = VFIO_EEH_PE_RESET_DEACTIVATE;
>> +            break;
>> +        case RTAS_SLOT_RESET_HOT:
>> +            cmd = VFIO_EEH_PE_RESET_HOT;
>> +            break;
>> +        case RTAS_SLOT_RESET_FUNDAMENTAL:
>> +            cmd = VFIO_EEH_PE_RESET_FUNDAMENTAL;
>> +            break;
>> +        default:
>> +            return -EINVAL;
>> +        }
>> +        break;
>> +    case RTAS_EEH_REQ_CONFIGURE:
>> +        cmd = VFIO_EEH_PE_CONFIGURE;
>> +        break;
>> +    default:
>> +         return -EINVAL;
>> +    }
>> +
>> +    op.op = cmd;
>> +    return vfio_container_ioctl(&svphb->phb.iommu_as, svphb->iommugroupid,
>> +                                VFIO_EEH_PE_OP, &op);
>
>Don't you need some sort of translation from the errnos the ioctl()
>returns into RTAS error codes?
>

Currently, errors from ioctl() is simply translated to RTAS_OUT_HW_ERROR if
the RTAS call supports it. Otherwise, RTAS_OUT_PARAM_ERROR will be returned.

Thanks,
Gavin

>> +}
>> +
>>  static void spapr_phb_vfio_reset(DeviceState *qdev)
>>  {
>>      /* Do nothing */
>> @@ -84,6 +139,7 @@ static void spapr_phb_vfio_class_init(ObjectClass *klass, 
>> void *data)
>>      dc->props = spapr_phb_vfio_properties;
>>      dc->reset = spapr_phb_vfio_reset;
>>      spc->finish_realize = spapr_phb_vfio_finish_realize;
>> +    spc->eeh_handler = spapr_phb_vfio_eeh_handler;
>>  }
>>  
>>  static const TypeInfo spapr_phb_vfio_info = {
>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>> index cf483ff..8a10c8b 100644
>> --- a/hw/vfio/common.c
>> +++ b/hw/vfio/common.c
>> @@ -948,6 +948,7 @@ int vfio_container_ioctl(AddressSpace *as, int32_t 
>> groupid,
>>      switch (req) {
>>      case VFIO_CHECK_EXTENSION:
>>      case VFIO_IOMMU_SPAPR_TCE_GET_INFO:
>> +    case VFIO_EEH_PE_OP:
>>          break;
>>      default:
>>          /* Return an error on unknown requests */
>
>-- 
>David Gibson                   | I'll have my music baroque, and my code
>david AT gibson.dropbear.id.au | minimalist, thank you.  NOT _the_ _other_
>                               | _way_ _around_!
>http://www.ozlabs.org/~dgibson