Re: [Qemu-ppc] [PATCH v6 2/3] VFIO: Introduce EEH handler

[Gavin Shan]

On Thu, May 22, 2014 at 09:11:07PM -0600, Alex Williamson wrote:
>On Thu, 2014-05-22 at 18:26 +1000, Gavin Shan wrote:
>> The patch introduces vfio_eeh_handler(), which is going to handle
>> EEH RTAS request routed by sPAPR platform.
>> 
>> Signed-off-by: Gavin Shan <address@hidden>
>> ---
>>  hw/misc/vfio.c         | 102 
>> +++++++++++++++++++++++++++++++++++++++++++++++++
>>  include/hw/misc/vfio.h |   1 +
>>  2 files changed, 103 insertions(+)
>> 
>> diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c
>> index 0796abf..1899b71 100644
>> --- a/hw/misc/vfio.c
>> +++ b/hw/misc/vfio.c
>> @@ -32,6 +32,7 @@
>>  #include "hw/pci/msi.h"
>>  #include "hw/pci/msix.h"
>>  #include "hw/pci/pci.h"
>> +#include "hw/pci/pci_bus.h"
>>  #include "qemu-common.h"
>>  #include "qemu/error-report.h"
>>  #include "qemu/event_notifier.h"
>> @@ -188,6 +189,7 @@ typedef struct VFIOMSIXInfo {
>>  typedef struct VFIODevice {
>>      PCIDevice pdev;
>>      int fd;
>> +    int pe_addr; /* EEH PE address */
>>      VFIOINTx intx;
>>      unsigned int config_size;
>>      uint8_t *emulated_config_bits; /* QEMU emulated bits, little-endian */
>> @@ -4086,6 +4088,9 @@ static int vfio_initfn(PCIDevice *pdev)
>>      add_boot_device_path(vdev->bootindex, &pdev->qdev, NULL);
>>      vfio_register_err_notifier(vdev);
>>  
>> +    /* Initialize EEH PE address to invalid one */
>> +    vdev->pe_addr = -1;
>> +
>>      return 0;
>>  
>>  out_teardown:
>> @@ -4310,3 +4315,100 @@ put_group_exit:
>>  
>>      return n;
>>  }
>> +
>> +static VFIODevice *vfio_dev_find_by_addr(PCIBus *rootbus, uint32_t bdn)
>> +{
>> +    VFIOGroup *group;
>> +    VFIODevice *vdev;
>> +    PCIDevice *pdev;
>> +
>> +    QLIST_FOREACH(group, &group_list, next) {
>> +        QLIST_FOREACH(vdev, &group->device_list, next) {
>> +            pdev = &vdev->pdev;
>> +            if (rootbus == pci_device_root_bus(pdev) &&
>> +                pdev == pci_find_device(rootbus,
>> +                                        (bdn >> 16) & 0xFF, (bdn >> 8) & 
>> 0xFF))
>> +                    return vdev;
>> +        }
>> +    }
>> +
>> +    return NULL;
>> +}
>> +
>> +static VFIODevice *vfio_dev_find_by_pe(PCIBus *rootbus, uint32_t pe_addr)
>
>VFIODevice.pe_addr is signed
>

Will fix it in next revision, thanks.

>> +{
>> +    VFIOGroup *group;
>> +    VFIODevice *vdev;
>> +    PCIDevice *pdev;
>> +
>> +    QLIST_FOREACH(group, &group_list, next) {
>> +        QLIST_FOREACH(vdev, &group->device_list, next) {
>> +            pdev = &vdev->pdev;
>> +            if (rootbus == pci_device_root_bus(pdev) &&
>> +                vdev->pe_addr == pe_addr)
>> +                    return vdev;
>> +        }
>> +    }
>> +
>> +    return NULL;
>> +}
>> +
>> +int vfio_eeh_handler(int32_t cmd, void *data, PCIBus *rootbus, uint32_t 
>> addr)
>> +{
>> +    VFIODevice *vdev;
>> +    struct vfio_eeh_pe_set_option *option;
>> +    struct vfio_eeh_pe_get_addr *get_addr;
>> +    int ret = 0;
>> +
>> +    switch (cmd) {
>> +    case VFIO_EEH_PE_SET_OPTION:
>> +        option = data;
>> +        if (option->option == 1) {
>
>What's 1?  Define it
>

The problem is which header file should define it. If I define it
in sPAPR specific header file, vfio.c has to include that header
file. That possiblly breaks build on other platforms (e.g. x86).
Even though it won't block x86 build, nobody want see vfio.c to
include platform specific header file, right? :-)

>> +             vdev = vfio_dev_find_by_addr(rootbus, addr);
>> +        } else {
>> +             vdev = vfio_dev_find_by_pe(rootbus, addr);
>> +        }
>> +
>> +        if (!vdev) {
>> +            return -ENODEV;
>> +        }
>> +
>> +        ret = ioctl(vdev->fd, VFIO_EEH_PE_SET_OPTION, option);
>> +        break;
>> +    case VFIO_EEH_PE_GET_ADDR:
>> +        get_addr = data;
>> +        vdev = vfio_dev_find_by_addr(rootbus, addr);
>> +        if (!vdev) {
>> +            return -ENODEV;
>> +        }
>> +
>> +        if (get_addr->option == 0) {
>
>Magic numbers
>

Same question as above.

>> +            if (vdev->pe_addr != -1) {
>> +                 get_addr->info = vdev->pe_addr;
>> +            } else {
>> +                ret = ioctl(vdev->fd, VFIO_EEH_PE_GET_ADDR, get_addr);
>> +                if (ret == 0) {
>> +                    vdev->pe_addr = get_addr->info;
>> +                }
>> +            }
>> +        } else {
>> +            get_addr->info = 2;
>> +        }
>> +
>> +        break;
>> +    case VFIO_EEH_PE_GET_STATE:
>> +    case VFIO_EEH_PE_RESET:
>> +    case VFIO_EEH_PE_CONFIGURE:
>> +        vdev = vfio_dev_find_by_pe(rootbus, addr);
>> +        if (!vdev) {
>> +            return -ENODEV;
>> +        }
>> +
>> +        ret = ioctl(vdev->fd, cmd, data);
>> +        break;
>> +    default:
>> +        return -EINVAL;
>> +    }
>> +
>> +    return ret;
>> +}
>> diff --git a/include/hw/misc/vfio.h b/include/hw/misc/vfio.h
>> index 53ec665..05cce40 100644
>> --- a/include/hw/misc/vfio.h
>> +++ b/include/hw/misc/vfio.h
>> @@ -30,4 +30,5 @@ static inline long vfio_kvm_notify(Notifier *n, unsigned 
>> request, void *data)
>>      return p.ret;
>>  }
>>  
>> +extern int vfio_eeh_handler(int32_t cmd, void *data, PCIBus *rootbus, 
>> uint32_t addr);
>>  #endif

Thanks,
Gavin