qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH RFC v11 2/4] vfio: new function to init aer cap


From: Cao jin
Subject: Re: [Qemu-devel] [PATCH RFC v11 2/4] vfio: new function to init aer cap for vfio device
Date: Fri, 20 Jan 2017 14:03:42 +0800
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.1.0


On 01/19/2017 06:09 AM, Alex Williamson wrote:
> On Sat, 31 Dec 2016 17:13:06 +0800
> Cao jin <address@hidden> wrote:
> 
>> From: Chen Fan <address@hidden>
>>
>> Introduce new function to initilize AER capability registers
>> for vfio-pci device.
>>
>> Signed-off-by: Chen Fan <address@hidden>
>> Signed-off-by: Dou Liyang <address@hidden>
>> Signed-off-by: Cao jin <address@hidden>
>> ---
>>  hw/vfio/pci.c | 87 
>> +++++++++++++++++++++++++++++++++++++++++++++++++++++++----
>>  hw/vfio/pci.h |  3 +++
>>  2 files changed, 85 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>> index d7dbe0e..76a8ac3 100644
>> --- a/hw/vfio/pci.c
>> +++ b/hw/vfio/pci.c
>> @@ -1851,18 +1851,81 @@ out:
>>      return 0;
>>  }
>>  
>> -static void vfio_add_ext_cap(VFIOPCIDevice *vdev)
>> +static int vfio_setup_aer(VFIOPCIDevice *vdev, uint8_t cap_ver,
>> +                          int pos, uint16_t size, Error **errp)
>> +{
>> +    PCIDevice *pdev = &vdev->pdev;
>> +    PCIDevice *dev_iter;
>> +    uint8_t type;
>> +    uint32_t errcap;
>> +
>> +    /* In case the physical device has AER cap while user doesn't enable 
>> AER,
>> +     * still allocate the config space in the emulated device for AER */
> 
> Bad comment style
> 
> /*
>  * Multi-line comments should
>  * look like this.
>  */
> 
> /* Single line comments may look like this */
> 
> /* Muli-line comments may
>  * absolutely not look like this */
> 
>> +    if (!(vdev->features & VFIO_FEATURE_ENABLE_AER)) {
>> +        pcie_add_capability(pdev, PCI_EXT_CAP_ID_ERR,
>> +                            cap_ver, pos, size);
>> +        return 0;
>> +    }
>> +
>> +    dev_iter = pci_bridge_get_device(pdev->bus);
>> +    if (!dev_iter) {
>> +        goto error;
>> +    }
>> +
>> +    while (dev_iter) {
>> +        if (!pci_is_express(dev_iter)) {
>> +            goto error;
>> +        }
>> +
>> +        type = pcie_cap_get_type(dev_iter);
>> +        if ((type != PCI_EXP_TYPE_ROOT_PORT &&
>> +             type != PCI_EXP_TYPE_UPSTREAM &&
>> +             type != PCI_EXP_TYPE_DOWNSTREAM)) {
>> +            goto error;
>> +        }
>> +
>> +        if (!dev_iter->exp.aer_cap) {
>> +            goto error;
>> +        }
>> +
>> +        dev_iter = pci_bridge_get_device(dev_iter->bus);
>> +    }
>> +
>> +    errcap = vfio_pci_read_config(pdev, pos + PCI_ERR_CAP, 4);
>> +    /*
>> +     * The ability to record multiple headers is depending on
>> +     * the state of the Multiple Header Recording Capable bit and
>> +     * enabled by the Multiple Header Recording Enable bit.
>> +     */
>> +    if ((errcap & PCI_ERR_CAP_MHRC) &&
>> +        (errcap & PCI_ERR_CAP_MHRE)) {
>> +        pdev->exp.aer_log.log_max = PCIE_AER_LOG_MAX_DEFAULT;
>> +    } else {
>> +        pdev->exp.aer_log.log_max = 0;
>> +    }
>> +
>> +    pcie_cap_deverr_init(pdev);
>> +    return pcie_aer_init(pdev, cap_ver, pos, size);
>> +
>> +error:
>> +    error_setg(errp, "vfio: Unable to enable AER for device %s, parent bus "
>> +               "does not support AER signaling", vdev->vbasedev.name);
>> +    return -1;
> 
> If we're only handling non-fatal errors, should we place the burden on
> the user to know when to add the aer flag for the device or should we
> just enable it opportunistically as available?

If we only handle non-fatal error, It make sense to me that we don't
need aer property as a switch and enable aer opportunistically as
available, it is no harm.

But considering that:
1. non-fatal error support is incomplete AER functionality, so I think
make it defaults to off is reasonable.

2. we may support fatal error too in the future, that will bring the
configuration restriction we used before. In this condition, make it
defaults to off is your discussion result(I guess). If this is the
finally choice, adopt it a little earlier is acceptable?

3. from another perspective, if 'aer' property shows in the future once
we support fatal-error too, would it seems that the 'aer' property is
dedicated to fatal-error only?(of course we could make it as the switch
of both error at that time)

> Should pcie_aer_init() be the one testing the topology?  It doesn't
> seem vfio specific anymore.
>

It does is not vfio specific, but I guess not. Question: could a
aer-capable device plugged under certain (root/downstream)port that is
not aer-capable? the answer I think is YES. If topology testing is done
in pcie_aer_init(), that means the answer is NO.

-- 
Sincerely,
Cao jin





reply via email to

[Prev in Thread] Current Thread [Next in Thread]