Re: [Qemu-devel] [PATCH v16 00/14] vfio-pci: pass the aer error to guest

[Chen Fan <chen.fan.fnst@cn.fujitsu.com>]

On 02/03/2016 09:57 PM, Michael S. Tsirkin wrote:
> On Wed, Feb 03, 2016 at 04:54:01PM +0800, Chen Fan wrote:
>> On 01/17/2016 02:34 AM, Michael S. Tsirkin wrote:
>>> On Tue, Jan 12, 2016 at 10:43:01AM +0800, Cao jin wrote:
>>>> From: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
>>>>
>>>> For now, for vfio pci passthough devices when qemu receives
>>>> an error from host aer report, currentlly just terminate the guest,
>>>> but usually user want to know what error occurred but stopping the
>>>> guest, so this patches add aer capability support for vfio device,
>>>> and pass the error to guest, and have guest driver to recover
>>> >from the error.
>>> I would like to see a version of this patchset that doesn't
>>> depend on pci core changes.
>>> I think that if you make this simplifying assumption:
>>>
>>> - all devices on same bus in guest are on same bus in host
>>>
>>> then you can handle both reset and hotplug simply in function 0
>>> since it will belong to vfio.
>>>
>>> So we can have a version without pci core changes that simply assumes
>>> this, and things will just work.
>>>
>>>
>>> Now, if we wanted to enforce this limitation, I think the
>>> cleanest way would be to add a callback in struct PCIDevice:
>>>
>>> 	bool is_valid_function(PCIDevice *newfunction)
>>>
>>> and call it as each function is added.
>>> This way aer function can validate that each function
>>> added shares the same bus.
>>> And this way issues will be detected directly and not when
>>> function 0 is added.
>>>
>>> I would prefer this validation code to be a patch on top so we can merge
>>> the functionality directly and avoid blocking it while we figure out the
>>> best api to validate things.
>>>
>>> I don't see why making guest topology match host would
>>> ever be a problem, but if it's required to support
>>> configurations where these differ, I'd like to see
>>> an attempt to address that be split out, after aer
>>> is supported.
>> Hi Michael,
>>
>> Just think about this more,  I think we also should check the vfio
>> devices whether on the same bus at the time of function 0 is added.
>> because we don't know the affected devices by a bus reset have
>> already all been assigned to VM.
> This is something vfio in kernel should check.
> You can't rely on qemu being well behaved, so don't
> even try to catch cases which would break host in userspace.
>
> qemu should only worry about not breaking guest.
>
>
>> for example, the multi-function's hotplug.
>> devices on same bus in host are added to VM one by one. when we
>> test one device, we haven't yet added the other devices.
>> so I think
>> the patch should like below. then we could add a vfio_is_valid_function in
>> vfio
>> to test each device whether the affected devices on the same bus.
>>
>> Thanks,
>> Chen
>>
>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>> index d940f79..7163b56 100644
>> --- a/hw/pci/pci.c
>> +++ b/hw/pci/pci.c
>> @@ -1836,6 +1836,38 @@ PCIDevice *pci_find_device(PCIBus *bus, int bus_num,
>> uint8_t devfn)
>>       return bus->devices[devfn];
>>   }
>>
>> +static int pci_bus_check_devices(PCIBus *bus)
>> +{
>> +    PCIDeviceClass *pc;
>> +    int i, ret = 0;
>> +
>> +    for (i = 0; i < ARRAY_SIZE(bus->devices); ++i) {
>> +        if (!bus->devices[i]) {
>> +            continue;
>> +        }
>> +
>> +        pc = PCI_DEVICE_GET_CLASS(bus->devices[i]);
>> +        if (!pc->is_valid_func) {
>> +            continue;
>> +        }
>> +
>> +        ret = pc->is_valid_func(bus->devices[i], bus);
>> +        if (!ret) {
>> +            return -1;
>> +        }
>> +    }
>> +    return 0;
>> +}
>> +
>> +static bool pci_is_valid_function(PCIDevice *pdev, PCIBus *bus)
>> +{
>> +    if (pdev->bus == bus) {
>> +        return true;
>> +    }
>> +
>> +    return false;
>> +}
>> +
> I don't really understand what is this one doing.
> Why do we need a default function?
if the vfio driver in kernel can handle the bus reset for any one
device in qemu without the affected devices assigned. I think
we don't need this default one.
BTW, IIRC at present the devices on the same bus in host can
be assigned to different VM, so if we want to support this kind of
bus reset for an independent device when enable aer, aren't we
limiting the case that others devices on the same bus must be
assigned to current VM?

Thanks,
Chen
>>   static void pci_qdev_realize(DeviceState *qdev, Error **errp)
>>   {
>>       PCIDevice *pci_dev = (PCIDevice *)qdev;
>> @@ -1878,6 +1910,14 @@ static void pci_qdev_realize(DeviceState *qdev, Error
>> **errp)
>>           pci_qdev_unrealize(DEVICE(pci_dev), NULL);
>>           return;
>>       }
>> +
>> +    if (DEVICE(pci_dev)->hotplugged &&
>> +        pci_get_function_0(pci_dev) == pci_dev &&
>> +        pci_bus_check_devices(bus)) {
>> +        error_setg(errp, "failed to hotplug function 0");
>> +        pci_qdev_unrealize(DEVICE(pci_dev), NULL);
>> +        return;
>> +    }
>>   }
>>
>>   static void pci_default_realize(PCIDevice *dev, Error **errp)
>> @@ -2390,6 +2430,7 @@ static void pci_device_class_init(ObjectClass *klass,
>> void *data)
>>       k->bus_type = TYPE_PCI_BUS;
>>       k->props = pci_props;
>>       pc->realize = pci_default_realize;
>> +    pc->is_valid_func = pci_is_valid_function;
>>   }
>>
>>   AddressSpace *pci_device_iommu_address_space(PCIDevice *dev)
>> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
>> index dedf277..a89580f 100644
>> --- a/include/hw/pci/pci.h
>> +++ b/include/hw/pci/pci.h
>> @@ -191,6 +191,7 @@ typedef struct PCIDeviceClass {
>>
>>       void (*realize)(PCIDevice *dev, Error **errp);
>>       int (*init)(PCIDevice *dev);/* TODO convert to realize() and remove */
>> +    bool (*is_valid_func)(PCIDevice *dev, PCIBus *bus);
>>       PCIUnregisterFunc *exit;
>>       PCIConfigReadFunc *config_read;
>>       PCIConfigWriteFunc *config_write;
>>
>>
>>
>>>
>>>
>>>> v15-v16:
>>>>     10/14, 11/14 are new to introduce a reset sequence id to specify the
>>>>     vfio devices has been reset for that reset. other patches aren't modified.
>>>>
>>>> v14-v15:
>>>>     1. add device hot reset callback
>>>>     2. add bus_in_reset for vfio device to avoid multi do host bus reset
>>>>
>>>> v13-v14:
>>>>     1. for multifunction device, requiring all functions enable AER.(9/13)
>>>>     2. due to all affected functions receive error signal, ignore no
>>>>        error occurred function. (12/13)
>>>>
>>>> v12-v13:
>>>>     1. since support multifuncion hotplug, here add callback to enable aer.
>>>>     2. add pci device pre+post reset for aer host reset.
>>>>
>>>> Chen Fan (14):
>>>>    vfio: extract vfio_get_hot_reset_info as a single function
>>>>    vfio: squeeze out vfio_pci_do_hot_reset for support bus reset
>>>>    pcie: modify the capability size assert
>>>>    vfio: make the 4 bytes aligned for capability size
>>>>    vfio: add pcie extanded capability support
>>>>    aer: impove pcie_aer_init to support vfio device
>>>>    vfio: add aer support for vfio device
>>>>    vfio: add check host bus reset is support or not
>>>>    add check reset mechanism when hotplug vfio device
>>>>    pci: introduce pci bus pre reset
>>>>    vfio: introduce last reset sequence id
>>>>    pcie_aer: expose pcie_aer_msg() interface
>>>>    vfio-pci: pass the aer error to guest
>>>>    vfio: add 'aer' property to expose aercap
>>>>
>>>>   hw/pci-bridge/ioh3420.c            |   2 +-
>>>>   hw/pci-bridge/xio3130_downstream.c |   2 +-
>>>>   hw/pci-bridge/xio3130_upstream.c   |   2 +-
>>>>   hw/pci/pci.c                       |  42 +++
>>>>   hw/pci/pci_bridge.c                |   3 +
>>>>   hw/pci/pcie.c                      |   2 +-
>>>>   hw/pci/pcie_aer.c                  |   6 +-
>>>>   hw/vfio/pci.c                      | 616 +++++++++++++++++++++++++++++++++----
>>>>   hw/vfio/pci.h                      |   9 +
>>>>   include/hw/pci/pci.h               |   1 +
>>>>   include/hw/pci/pci_bus.h           |   8 +
>>>>   include/hw/pci/pcie_aer.h          |   3 +-
>>>>   12 files changed, 624 insertions(+), 72 deletions(-)
>>>>
>>>> -- 
>>>> 1.9.3
>>>>
>>>>
>>> .
>>>
>>
>
> .
>