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

[Alex Williamson]

On Fri, 20 Jan 2017 14:03:42 +0800
Cao jin <address@hidden> wrote:

> 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);
> >> +    }

Does AER really require PCIe and AER support all the way to the root
bus?  Shouldn't it be sufficient for the device to be downstream of a
downstream port supporting AER?  If we inject an error, it's injected
via the direct immediate upstream parent, right?

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

If handling of fatal errors will impose configuration restrictions then
it needs to have its own option and it needs to default to off.  We
cannot reuse "aer" since that would make old VMs incompatible with new
QEMU.  Therefore the question is whether there's value in having an
option for non-fatal errors vs enabling it opportunistically.  We could
certainly default to enabling it when available and adding an
experimental option (x-disable-guest-aer-forward) to allow users to
disable it for debug purposes.
 
> > 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.

Do we have any requirement to expose a device's AER capability in such
a configuration?  We could also argue that there's little value in
doing so, or simply expose the capability w/o any QEMU AER backing for
those cases.  Thanks,

Alex