| Message ID | 7d730be4f52fcd790b83a8329a17a099b2b6ceb9.1447231392.git.chen.fan.fnst@cn.fujitsu.com |
|---|---|
| State | New |
| Headers | show |
On Wed, 2015-11-11 at 18:34 +0800, Cao jin wrote: > From: Chen Fan <chen.fan.fnst@cn.fujitsu.com> > > 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 <chen.fan.fnst@cn.fujitsu.com> > --- What if VFIO_FEATURE_ENABLE_AER is enabled for a device that doesn't posses an AER capability or isn't attached to a PCIe bus? It appears that we silently ignore it, which would lead to unregistering a hotplug notifier that was never registered in 09/13 and needing to test both VFIO_FEATURE_ENABLE_AER and exp.aer_cap in 12/13 as well as the inconsistency that we often only test for VFIO_FEATURE_ENABLE_AER when really we expect that to imply that AER is setup and enabled for the device. It seems like we need to error either within vfio_add_capabilities() or after calling it if VFIO_FEATURE_ENABLE_AER is specified but not configured. If a user expects AER to be enabled for a device by specifying aer=on, we need to fail if that's not possible. > hw/vfio/pci.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++--- > hw/vfio/pci.h | 3 +++ > 2 files changed, 82 insertions(+), 3 deletions(-) > > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c > index 4bc2b51..2d34edf 100644 > --- a/hw/vfio/pci.c > +++ b/hw/vfio/pci.c > @@ -1806,6 +1806,68 @@ static int vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t pos) ... > + > + pcie_cap_deverr_init(pdev); > + ret = pcie_aer_init(pdev, pos, size); > + if (ret) { > + return ret; > + } This branch is unnecessary, we can simply: return pcie_aer_init(pdev, pos, size); if we get this far. Thanks, Alex
On 11/12/2015 04:49 AM, Alex Williamson wrote: > On Wed, 2015-11-11 at 18:34 +0800, Cao jin wrote: >> From: Chen Fan <chen.fan.fnst@cn.fujitsu.com> >> >> 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 <chen.fan.fnst@cn.fujitsu.com> >> --- > > > What if VFIO_FEATURE_ENABLE_AER is enabled for a device that doesn't > posses an AER capability or isn't attached to a PCIe bus? It appears > that we silently ignore it, which would lead to unregistering a hotplug > notifier that was never registered in 09/13 and needing to test both > VFIO_FEATURE_ENABLE_AER and exp.aer_cap in 12/13 as well as the > inconsistency that we often only test for VFIO_FEATURE_ENABLE_AER when > really we expect that to imply that AER is setup and enabled for the > device. It seems like we need to error either within > vfio_add_capabilities() or after calling it if VFIO_FEATURE_ENABLE_AER > is specified but not configured. If a user expects AER to be enabled > for a device by specifying aer=on, we need to fail if that's not > possible. make sense, how about adding aer cap dynamically by object_property_add_bool, then we can use set function to check whether the aer is valid or not. > >> hw/vfio/pci.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++--- >> hw/vfio/pci.h | 3 +++ >> 2 files changed, 82 insertions(+), 3 deletions(-) >> >> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c >> index 4bc2b51..2d34edf 100644 >> --- a/hw/vfio/pci.c >> +++ b/hw/vfio/pci.c >> @@ -1806,6 +1806,68 @@ static int vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t pos) > ... >> + >> + pcie_cap_deverr_init(pdev); >> + ret = pcie_aer_init(pdev, pos, size); >> + if (ret) { >> + return ret; >> + } > > This branch is unnecessary, we can simply: > > return pcie_aer_init(pdev, pos, size); > > if we get this far. Thanks, OK > > Alex > > . >
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index 4bc2b51..2d34edf 100644 --- a/hw/vfio/pci.c +++ b/hw/vfio/pci.c @@ -1806,6 +1806,68 @@ 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; + int ret; + + 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); + 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, pdev->exp.aer_cap + 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); + ret = pcie_aer_init(pdev, pos, size); + if (ret) { + return ret; + } + + return 0; + +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; @@ -1813,6 +1875,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; /* * In order to avoid breaking config space, create a copy to @@ -1834,16 +1897,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) diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h index f004d52..48c1f69 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" @@ -127,6 +128,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;