[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [RFC v7 11/11] vfio: add 'aer' property to expose aerca
From: |
Alex Williamson |
Subject: |
Re: [Qemu-devel] [RFC v7 11/11] vfio: add 'aer' property to expose aercap |
Date: |
Tue, 19 May 2015 13:34:25 -0600 |
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 <address@hidden>
> ---
> 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",\
- [Qemu-devel] [RFC v7 08/11] vfio: add aer support for vfio device, (continued)
- [Qemu-devel] [RFC v7 08/11] vfio: add aer support for vfio device, Chen Fan, 2015/05/19
- [Qemu-devel] [RFC v7 06/11] vfio: add pcie extanded capability support, Chen Fan, 2015/05/19
- [Qemu-devel] [RFC v7 07/11] aer: impove pcie_aer_init to support vfio device, Chen Fan, 2015/05/19
- [Qemu-devel] [RFC v7 09/11] pcie_aer: expose pcie_aer_msg() interface, Chen Fan, 2015/05/19
- [Qemu-devel] [RFC v7 05/11] vfio: do hot bus reset when do virtual secondary bus reset, Chen Fan, 2015/05/19
- [Qemu-devel] [RFC v7 10/11] vfio-pci: pass the aer error to guest, Chen Fan, 2015/05/19
- [Qemu-devel] [RFC v7 11/11] vfio: add 'aer' property to expose aercap, Chen Fan, 2015/05/19
- Re: [Qemu-devel] [RFC v7 11/11] vfio: add 'aer' property to expose aercap,
Alex Williamson <=