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

[Alexander Graf]

> Am 23.05.2014 um 07:00 schrieb Gavin Shan <address@hidden>:
> 
>> 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? :-)

It's part of the VFIO ABI, so it should be exported by the kernel's vfio 
headers.

Alex

> 
>>> +             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
>