Re: [Qemu-devel] [PATCH v13 07/13] vfio: add aer support for vfio device

[Cao jin <caoj.fnst@cn.fujitsu.com>]

On 11/12/2015 04:49 AM, Alex Williamson wrote:
> On Wed, 2015-11-11 at 18:34 +0800, Cao jin wrote:
>> From: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
>>
>> Calling pcie_aer_init to initilize aer related registers for
>> vfio device, then reload physical related registers to expose
>> device capability.
>>
>> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
>> ---
>
>
> What if VFIO_FEATURE_ENABLE_AER is enabled for a device that doesn't
> posses an AER capability or isn't attached to a PCIe bus?  It appears
> that we silently ignore it, which would lead to unregistering a hotplug
> notifier that was never registered in 09/13 and needing to test both
> VFIO_FEATURE_ENABLE_AER and exp.aer_cap in 12/13 as well as the
> inconsistency that we often only test for VFIO_FEATURE_ENABLE_AER when
> really we expect that to imply that AER is setup and enabled for the
> device.  It seems like we need to error either within
> vfio_add_capabilities() or after calling it if VFIO_FEATURE_ENABLE_AER
> is specified but not configured.  If a user expects AER to be enabled
> for a device by specifying aer=on, we need to fail if that's not
> possible.

make sense, how about adding aer cap dynamically by 
object_property_add_bool, then we can use set function to check whether 
the aer is valid or not.

>
>>   hw/vfio/pci.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
>>   hw/vfio/pci.h |  3 +++
>>   2 files changed, 82 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>> index 4bc2b51..2d34edf 100644
>> --- a/hw/vfio/pci.c
>> +++ b/hw/vfio/pci.c
>> @@ -1806,6 +1806,68 @@ static int vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t pos)
> ...
>> +
>> +    pcie_cap_deverr_init(pdev);
>> +    ret = pcie_aer_init(pdev, pos, size);
>> +    if (ret) {
>> +        return ret;
>> +    }
>
> This branch is unnecessary, we can simply:
>
> return pcie_aer_init(pdev, pos, size);
>
> if we get this far.  Thanks,

OK

>
> Alex
>
> .
>

-- 
Yours Sincerely,

Cao Jin