Re: [Qemu-devel] [RFC v3 08/10] vfio-pci: add VFIO_FEATURE_ENABLE_AER_CAP feature

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

On 02/27/2015 06:18 AM, Alex Williamson wrote:
> On Thu, 2015-02-26 at 14:46 +0800, Chen Fan wrote:
>> On 02/11/2015 12:39 AM, Alex Williamson wrote:
>>> On Tue, 2015-02-10 at 15:03 +0800, Chen Fan wrote:
>>>> add a new "aercap" feature in vfio device, for controlling
>>>> whether expose aer capability.
>>>>
>>>> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
>>>> ---
>>>>    hw/vfio/pci.c | 10 ++++++++--
>>>>    1 file changed, 8 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>>>> index bf314a1..c21b40b 100644
>>>> --- a/hw/vfio/pci.c
>>>> +++ b/hw/vfio/pci.c
>>>> @@ -138,6 +138,8 @@ typedef struct VFIOMSIXInfo {
>>>>    enum {
>>>>    #define VFIO_FEATURE_ENABLE_VGA_BIT 0
>>>>        VFIO_FEATURE_ENABLE_VGA = (1 << VFIO_FEATURE_ENABLE_VGA_BIT),
>>>> +#define VFIO_FEATURE_ENABLE_AER_CAP_BIT 1
>>>> +    VFIO_FEATURE_ENABLE_AER_CAP = (1 << VFIO_FEATURE_ENABLE_AER_CAP_BIT),
>>>>    };
>>>>    
>>>>    typedef struct VFIOPCIDevice {
>>>> @@ -2721,8 +2723,10 @@ static int vfio_add_ext_cap(VFIOPCIDevice *vdev, uint16_t pos)
>>>>    
>>>>        switch (cap_id) {
>>>>        case PCI_EXT_CAP_ID_ERR:
>>>> -        pcie_add_capability(pdev, cap_id, cap_ver, pos, size);
>>>> -        vfio_setup_pcie_aer(vdev, pos);
>>>> +        if (vdev->features & VFIO_FEATURE_ENABLE_AER_CAP) {
>>>> +            pcie_add_capability(pdev, cap_id, cap_ver, pos, size);
>>>> +            vfio_setup_pcie_aer(vdev, pos);
>>>> +        }
>> Hi Alex,
>>       sorry for replaying so late. I am just back from holiday. :)
>>
>>> Maybe the question should be why we're adding extended capabilities at
>>> all if the chipset doesn't expose them.  If we boot on 440fx, all
>>> extended capability parsing could be disabled.  We could then add an
>>> x-aer=off option to vfio-pci to allow the user to disable aer
>>> specifically if they want.
>> your meaning is adding two option:
>>
>> 1) one controls exposing extended caps to device. in particular on
>> 440fx chipset we must disable it.
>>
>> 2) the other one controls exposing aer capability to user specifically.
>>
>> right?
> Right, a flag toggled by the chipset type doesn't work because whether
> we want to expose AER is dependent on the exposed bus type to the guest.
> Even if we're using an express chipset like q35, the device could be
> behind a PCIe-to-PCI bridge in the guest and the guest would not have
> access to extended config space, including AER.  Therefore it seems like
> we simply need to test pci_bus_is_express() for a device and skip all
> extended capability parsing when false.
>
> Then we've solved the bus topology problem and we only need to be
> concerned about whether the user has a need to disable AER by choice.
> We probably do need to allow that for compatibility with existing PCIe
> guest configurations.  I'd probably name it something like "disable_aer"
> because we don't want to give the user any ideas that it can be used to
> toggle AER on, only off.  Thanks,
Hi Alex,

I has made a v4 patch which added a 'x-aer' to enable/disable aer capability
and remake the parsing extended caps method.
pls help to review it. :)

Thanks,
Chen

>
> Alex
>
>>>>            break;
>>>>        default:
>>>>            pcie_add_capability(pdev, cap_id, cap_ver, pos, size);
>>>> @@ -3487,6 +3491,8 @@ static Property vfio_pci_dev_properties[] = {
>>>>        DEFINE_PROP_BIT("x-vga", VFIOPCIDevice, features,
>>>>                        VFIO_FEATURE_ENABLE_VGA_BIT, false),
>>>>        DEFINE_PROP_INT32("bootindex", VFIOPCIDevice, bootindex, -1),
>>>> +    DEFINE_PROP_BIT("aercap", VFIOPCIDevice, features,
>>>> +                    VFIO_FEATURE_ENABLE_AER_CAP_BIT, true),
>>>>        /*
>>>>         * TODO - support passed fds... is this necessary?
>>>>         * DEFINE_PROP_STRING("vfiofd", VFIOPCIDevice, vfiofd_name),
>>>
>>> .
>>>
>
>
> .
>