Re: [Qemu-devel] [PATCH v2 04/11] vfio: add aer support for vfio device

[Chen Fan]

On 03/09/2016 06:55 AM, Alex Williamson wrote:
> On Mon, 7 Mar 2016 11:22:57 +0800
> Cao jin <address@hidden> wrote:
>
> > From: Chen Fan <address@hidden>
> >
> > 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 <address@hidden>
> > ---
> >   hw/vfio/pci.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
> >   hw/vfio/pci.h |  3 +++
> >   2 files changed, 81 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> > index e64cce3..8ec9b25 100644
> > --- a/hw/vfio/pci.c
> > +++ b/hw/vfio/pci.c
> > @@ -1868,6 +1868,62 @@ static int vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t 
> > pos)
> >       return 0;
> >   }
> >   
> > +static int vfio_setup_aer(VFIOPCIDevice *vdev, uint8_t cap_ver,
> > +                          int pos, uint16_t size)
> > +{
> > +    PCIDevice *pdev = &vdev->pdev;
> > +    PCIDevice *dev_iter;
> > +    uint8_t type;
> > +    uint32_t errcap;
> > +
> > +    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) {
> > +        type = pcie_cap_get_type(dev_iter);
> This asserts if dev_iter doesn't have an express capability so do we
> really ever get to the error goto in practice?  I think an average user
> is going to get more information from your error_report than from an
> assert.  Thanks,
you are right, I will fix it in the next version.

Thanks,
Chen

> Alex
>
> > +        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, pos, size);
> > +
> > +error:
> > +    error_report("vfio: Unable to enable AER for device %s, parent bus "
> > +                 "does not support AER signaling", vdev->vbasedev.name);
> > +    return -1;
> > +}
> > +
> >   static int vfio_add_ext_cap(VFIOPCIDevice *vdev)
> >   {
> >       PCIDevice *pdev = &vdev->pdev;
> > @@ -1875,6 +1931,7 @@ static int vfio_add_ext_cap(VFIOPCIDevice *vdev)
> >       uint16_t cap_id, next, size;
> >       uint8_t cap_ver;
> >       uint8_t *config;
> > +    int ret = 0;
> >   
> >       /*
> >        * pcie_add_capability always inserts the new capability at the tail
> > @@ -1898,16 +1955,29 @@ static int vfio_add_ext_cap(VFIOPCIDevice *vdev)
> >            */
> >           size = vfio_ext_cap_max_size(config, next);
> >   
> > -        pcie_add_capability(pdev, cap_id, cap_ver, next, size);
> > -        pci_set_long(dev->config + next, PCI_EXT_CAP(cap_id, cap_ver, 0));
> > +        switch (cap_id) {
> > +        case PCI_EXT_CAP_ID_ERR:
> > +            ret = vfio_setup_aer(vdev, cap_ver, next, size);
> > +            break;
> > +        default:
> > +            pcie_add_capability(pdev, cap_id, cap_ver, next, size);
> > +            break;
> > +        }
> > +
> > +        if (ret) {
> > +            goto out;
> > +        }
> > +
> > +        pci_set_long(pdev->config + next, PCI_EXT_CAP(cap_id, cap_ver, 0));
> >   
> >           /* Use emulated next pointer to allow dropping extended caps */
> >           pci_long_test_and_set_mask(vdev->emulated_config_bits + next,
> >                                      PCI_EXT_CAP_NEXT_MASK);
> >       }
> >   
> > +out:
> >       g_free(config);
> > -    return 0;
> > +    return ret;
> >   }
> >   
> >   static int vfio_add_capabilities(VFIOPCIDevice *vdev)
> > @@ -2662,6 +2732,11 @@ static int vfio_initfn(PCIDevice *pdev)
> >           goto out_teardown;
> >       }
> >   
> > +    if ((vdev->features & VFIO_FEATURE_ENABLE_AER) &&
> > +        !pdev->exp.aer_cap) {
> > +        goto out_teardown;
> > +    }
> > +
> >       /* QEMU emulates all of MSI & MSIX */
> >       if (pdev->cap_present & QEMU_PCI_CAP_MSIX) {
> >           memset(vdev->emulated_config_bits + pdev->msix_cap, 0xff,
> > diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
> > index 6256587..e0c53f2 100644
> > --- a/hw/vfio/pci.h
> > +++ b/hw/vfio/pci.h
> > @@ -15,6 +15,7 @@
> >   #include "qemu-common.h"
> >   #include "exec/memory.h"
> >   #include "hw/pci/pci.h"
> > +#include "hw/pci/pci_bridge.h"
> >   #include "hw/vfio/vfio-common.h"
> >   #include "qemu/event_notifier.h"
> >   #include "qemu/queue.h"
> > @@ -128,6 +129,8 @@ typedef struct VFIOPCIDevice {
> >   #define VFIO_FEATURE_ENABLE_VGA (1 << VFIO_FEATURE_ENABLE_VGA_BIT)
> >   #define VFIO_FEATURE_ENABLE_REQ_BIT 1
> >   #define VFIO_FEATURE_ENABLE_REQ (1 << VFIO_FEATURE_ENABLE_REQ_BIT)
> > +#define VFIO_FEATURE_ENABLE_AER_BIT 2
> > +#define VFIO_FEATURE_ENABLE_AER (1 << VFIO_FEATURE_ENABLE_AER_BIT)
> >       int32_t bootindex;
> >       uint8_t pm_cap;
> >       bool has_vga;
>
>
> .