Re: [Qemu-devel] [RFC v7 11/11] vfio: add 'aer' property to expose aercap

[Alex Williamson <alex.williamson@redhat.com>]

On Tue, 2015-05-19 at 12:42 +0800, Chen Fan wrote:
> add 'aer' property to let user able to decide whether expose
> the aer capability. by default we should disable aer feature,
> because it needs configuration restrictions.

But the previous patch already enabled it, so a bisect might get it
enabled then disabled.

> 
> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
> ---
>  hw/i386/pc_q35.c    |  4 +++
>  hw/vfio/pci.c       | 84 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/compat.h |  8 +++++
>  3 files changed, 96 insertions(+)
> 
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index e67f2de..94a3a1c 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -439,6 +439,10 @@ static QEMUMachine pc_q35_machine_v2_3 = {
>      PC_Q35_2_3_MACHINE_OPTIONS,
>      .name = "pc-q35-2.3",
>      .init = pc_q35_init_2_3,
> +    .compat_props = (GlobalProperty[]) {
> +        HW_COMPAT_2_3,
> +        { /* end of list */ }
> +    },
>  };
>  
>  #define PC_Q35_2_2_MACHINE_OPTIONS PC_Q35_2_3_MACHINE_OPTIONS
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index f2fc5d6..b4b9495 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -42,6 +42,7 @@
>  #include "trace.h"
>  #include "hw/vfio/vfio.h"
>  #include "hw/vfio/vfio-common.h"
> +#include "hw/pci/pci_bridge.h"
>  
>  struct VFIOPCIDevice;
>  
> @@ -161,6 +162,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;
> @@ -2821,10 +2824,31 @@ static int vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t pos)
>  static int vfio_setup_aer(VFIOPCIDevice *vdev, int pos, uint16_t size)
>  {
>      PCIDevice *pdev = &vdev->pdev;
> +    PCIDevice *dev_iter;
>      uint8_t *exp_cap = pdev->config + pdev->exp.exp_cap;
>      uint32_t severity, errcap;
> +    uint8_t type;
>      int ret;
>  
> +    if (!(vdev->features & VFIO_FEATURE_ENABLE_AER)) {
> +        return 0;
> +    }
> +
> +    dev_iter = pci_bridge_get_device(pdev->bus);
> +    if (!dev_iter) {

So this is testing that it can't be on the root complex endpoint.

> +        goto error;
> +    }
> +
> +    while (dev_iter) {
> +        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;
> +        }

And this tests that we have PCIe devices up to the root complex, but do
do we need to check whether they support aer?

> +        dev_iter = pci_bridge_get_device(dev_iter->bus);
> +    }
> +
>      errcap = vfio_pci_read_config(pdev, pdev->exp.aer_cap + PCI_ERR_CAP, 4);
>      /*
>       * The ability to record multiple headers is depending on
> @@ -2850,6 +2874,11 @@ static int vfio_setup_aer(VFIOPCIDevice *vdev, int pos, uint16_t size)
>      pci_long_test_and_clear_mask(exp_cap + PCI_ERR_UNCOR_SEVER, ~severity);
>  
>      return 0;
> +
> +error:
> +    error_report("vfio: Unable to enable AER for device %s, parent bus "
> +                 "do not support signal AER", vdev->vbasedev.name);
> +    return -1;
>  }
>  
>  static int vfio_add_ext_cap(VFIOPCIDevice *vdev, const uint8_t *config)
> @@ -3733,9 +3762,52 @@ static void vfio_check_host_bus_reset(VFIODevice *vbasedev)
>      vdev->has_bus_reset = true;
>  
>  out:
> +    if ((vdev->features & VFIO_FEATURE_ENABLE_AER) &&
> +        !vdev->has_bus_reset) {
> +        error_report("vfio: Cannot enable AER for device %s,"
> +                     "which is not support host bus reset.",
> +                     vdev->vbasedev.name);
> +        exit(1);
> +    }

And this is done via the machine init callback, so again we're not doing
anything to support hotplug.

>      g_free(info);
>  }
>  
> +static int vfio_pci_validate_aer_devices(DeviceState *dev,
> +                                         void *opaque)
> +{
> +    VFIOPCIDevice *vdev;
> +
> +    if (!object_dynamic_cast(OBJECT(dev), "vfio-pci")) {
> +        return 0;
> +    }
> +
> +    vdev = DO_UPCAST(VFIOPCIDevice, pdev, PCI_DEVICE(dev));
> +    if (!vdev->has_bus_reset) {
> +        error_report("vfio: device %s is not support host bus reset,"
> +                     "but on the same bus has vfio device enable AER.",
> +                     vdev->vbasedev.name);
> +        exit(1);
> +    }
> +    return 0;
> +}
> +
> +static void vfio_check_virtual_bus_reset(VFIODevice *vbasedev)
> +{
> +    VFIOPCIDevice *vdev = container_of(vbasedev, VFIOPCIDevice, vbasedev);
> +
> +    if (!(vdev->features & VFIO_FEATURE_ENABLE_AER)) {
> +        return;
> +    }
> +
> +    /*
> +     * Verify that we have all the vfio devices support host bus reset
> +     * on the bus
> +     */
> +    qbus_walk_children(BUS(vdev->pdev.bus), NULL, NULL,
> +                       vfio_pci_validate_aer_devices,
> +                       NULL, NULL);
> +}
> +
>  static void vfio_pci_machine_done_notify(Notifier *notifier, void *unused)
>  {
>      VFIOGroup *group;
> @@ -3746,6 +3818,16 @@ static void vfio_pci_machine_done_notify(Notifier *notifier, void *unused)
>               vfio_check_host_bus_reset(vbasedev);
>           }
>      }
> +
> +    /*
> +     * All devices need support host bus reset on each virtual bus
> +     * if one device enable AER.
> +     */
> +    QLIST_FOREACH(group, &vfio_group_list, next) {
> +         QLIST_FOREACH(vbasedev, &group->device_list, next) {
> +             vfio_check_virtual_bus_reset(vbasedev);
> +         }
> +    }

I'm not sure I see how this call path can catch anything missed by
vfio_check_host_bus_reset().

>  }
>  
>  static Notifier machine_notifier = {
> @@ -4004,6 +4086,8 @@ static Property vfio_pci_dev_properties[] = {
>                      VFIO_FEATURE_ENABLE_VGA_BIT, false),
>      DEFINE_PROP_BIT("x-req", VFIOPCIDevice, features,
>                      VFIO_FEATURE_ENABLE_REQ_BIT, true),
> +    DEFINE_PROP_BIT("aer", VFIOPCIDevice, features,
> +                    VFIO_FEATURE_ENABLE_AER_BIT, false),
>      DEFINE_PROP_INT32("bootindex", VFIOPCIDevice, bootindex, -1),
>      DEFINE_PROP_BOOL("x-mmap", VFIOPCIDevice, vbasedev.allow_mmap, true),
>      /*
> diff --git a/include/hw/compat.h b/include/hw/compat.h
> index 313682a..f722919 100644
> --- a/include/hw/compat.h
> +++ b/include/hw/compat.h
> @@ -1,7 +1,15 @@
>  #ifndef HW_COMPAT_H
>  #define HW_COMPAT_H
>  
> +#define HW_COMPAT_2_3 \
> +        {\
> +            .driver   = "vfio-pci",\
> +            .property = "x-aer",\

It's defined as "aer" above, not "x-aer".  I'm not sure why we need this
though if the default value is off anyway.

> +            .value    = "off",\
> +        }
> +
>  #define HW_COMPAT_2_1 \
> +        HW_COMPAT_2_3, \
>          {\
>              .driver   = "intel-hda",\
>              .property = "old_msi_addr",\