| Message ID | 1483175588-17006-3-git-send-email-caoj.fnst@cn.fujitsu.com |
|---|---|
| State | New |
| Headers | show |
On Sat, 31 Dec 2016 17:13:06 +0800 Cao jin <caoj.fnst@cn.fujitsu.com> wrote: > From: Chen Fan <chen.fan.fnst@cn.fujitsu.com> > > Introduce new function to initilize AER capability registers > for vfio-pci device. > > Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com> > Signed-off-by: Dou Liyang <douly.fnst@cn.fujitsu.com> > Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com> > --- > 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); > + } > + > + 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? Should pcie_aer_init() be the one testing the topology? It doesn't seem vfio specific anymore. > +} > + > +static int vfio_add_ext_cap(VFIOPCIDevice *vdev, Error **errp) > { > PCIDevice *pdev = &vdev->pdev; > uint32_t header; > uint16_t cap_id, next, size; > uint8_t cap_ver; > uint8_t *config; > + int ret = 0; > > /* Only add extended caps if we have them and the guest can see them */ > if (!pci_is_express(pdev) || !pci_bus_is_express(pdev->bus) || > !pci_get_long(pdev->config + PCI_CONFIG_SPACE_SIZE)) { > - return; > + return 0; > } > > /* > @@ -1911,6 +1974,9 @@ static void vfio_add_ext_cap(VFIOPCIDevice *vdev) > PCI_EXT_CAP_NEXT_MASK); > > switch (cap_id) { > + case PCI_EXT_CAP_ID_ERR: > + ret = vfio_setup_aer(vdev, cap_ver, next, size, errp); > + break; > case PCI_EXT_CAP_ID_SRIOV: /* Read-only VF BARs confuse OVMF */ > case PCI_EXT_CAP_ID_ARI: /* XXX Needs next function virtualization */ > trace_vfio_add_ext_cap_dropped(vdev->vbasedev.name, cap_id, next); > @@ -1919,6 +1985,9 @@ static void vfio_add_ext_cap(VFIOPCIDevice *vdev) > pcie_add_capability(pdev, cap_id, cap_ver, next, size); > } > > + if (ret) { > + goto out; > + } > } > > /* Cleanup chain head ID if necessary */ > @@ -1926,8 +1995,9 @@ static void vfio_add_ext_cap(VFIOPCIDevice *vdev) > pci_set_word(pdev->config + PCI_CONFIG_SPACE_SIZE, 0); > } > > +out: > g_free(config); > - return; > + return ret; > } > > static int vfio_add_capabilities(VFIOPCIDevice *vdev, Error **errp) > @@ -1945,8 +2015,8 @@ static int vfio_add_capabilities(VFIOPCIDevice *vdev, Error **errp) > return ret; > } > > - vfio_add_ext_cap(vdev); > - return 0; > + ret = vfio_add_ext_cap(vdev, errp); > + return ret; > } > > static void vfio_pci_pre_reset(VFIOPCIDevice *vdev) > @@ -2769,6 +2839,13 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) > goto out_teardown; > } > > + if ((vdev->features & VFIO_FEATURE_ENABLE_AER) && > + !pdev->exp.aer_cap) { > + error_setg(errp, "vfio: Unable to enable AER for device %s, device " > + "does not support AER signaling", vdev->vbasedev.name); > + return; > + } > + > if (vdev->vga) { > vfio_vga_quirk_setup(vdev); > } > diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h > index a8366bb..64701c4 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" > @@ -132,6 +133,8 @@ typedef struct VFIOPCIDevice { > #define VFIO_FEATURE_ENABLE_IGD_OPREGION_BIT 2 > #define VFIO_FEATURE_ENABLE_IGD_OPREGION \ > (1 << VFIO_FEATURE_ENABLE_IGD_OPREGION_BIT) > +#define VFIO_FEATURE_ENABLE_AER_BIT 3 > +#define VFIO_FEATURE_ENABLE_AER (1 << VFIO_FEATURE_ENABLE_AER_BIT) > int32_t bootindex; > uint32_t igd_gms; > uint8_t pm_cap;
On 01/19/2017 06:09 AM, Alex Williamson wrote: > On Sat, 31 Dec 2016 17:13:06 +0800 > Cao jin <caoj.fnst@cn.fujitsu.com> wrote: > >> From: Chen Fan <chen.fan.fnst@cn.fujitsu.com> >> >> Introduce new function to initilize AER capability registers >> for vfio-pci device. >> >> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com> >> Signed-off-by: Dou Liyang <douly.fnst@cn.fujitsu.com> >> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com> >> --- >> 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); >> + } >> + >> + 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) > 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.
On Fri, 20 Jan 2017 14:03:42 +0800 Cao jin <caoj.fnst@cn.fujitsu.com> wrote: > On 01/19/2017 06:09 AM, Alex Williamson wrote: > > On Sat, 31 Dec 2016 17:13:06 +0800 > > Cao jin <caoj.fnst@cn.fujitsu.com> wrote: > > > >> From: Chen Fan <chen.fan.fnst@cn.fujitsu.com> > >> > >> Introduce new function to initilize AER capability registers > >> for vfio-pci device. > >> > >> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com> > >> Signed-off-by: Dou Liyang <douly.fnst@cn.fujitsu.com> > >> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com> > >> --- > >> 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
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 */ + 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); + } + + 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; +} + +static int vfio_add_ext_cap(VFIOPCIDevice *vdev, Error **errp) { PCIDevice *pdev = &vdev->pdev; uint32_t header; uint16_t cap_id, next, size; uint8_t cap_ver; uint8_t *config; + int ret = 0; /* Only add extended caps if we have them and the guest can see them */ if (!pci_is_express(pdev) || !pci_bus_is_express(pdev->bus) || !pci_get_long(pdev->config + PCI_CONFIG_SPACE_SIZE)) { - return; + return 0; } /* @@ -1911,6 +1974,9 @@ static void vfio_add_ext_cap(VFIOPCIDevice *vdev) PCI_EXT_CAP_NEXT_MASK); switch (cap_id) { + case PCI_EXT_CAP_ID_ERR: + ret = vfio_setup_aer(vdev, cap_ver, next, size, errp); + break; case PCI_EXT_CAP_ID_SRIOV: /* Read-only VF BARs confuse OVMF */ case PCI_EXT_CAP_ID_ARI: /* XXX Needs next function virtualization */ trace_vfio_add_ext_cap_dropped(vdev->vbasedev.name, cap_id, next); @@ -1919,6 +1985,9 @@ static void vfio_add_ext_cap(VFIOPCIDevice *vdev) pcie_add_capability(pdev, cap_id, cap_ver, next, size); } + if (ret) { + goto out; + } } /* Cleanup chain head ID if necessary */ @@ -1926,8 +1995,9 @@ static void vfio_add_ext_cap(VFIOPCIDevice *vdev) pci_set_word(pdev->config + PCI_CONFIG_SPACE_SIZE, 0); } +out: g_free(config); - return; + return ret; } static int vfio_add_capabilities(VFIOPCIDevice *vdev, Error **errp) @@ -1945,8 +2015,8 @@ static int vfio_add_capabilities(VFIOPCIDevice *vdev, Error **errp) return ret; } - vfio_add_ext_cap(vdev); - return 0; + ret = vfio_add_ext_cap(vdev, errp); + return ret; } static void vfio_pci_pre_reset(VFIOPCIDevice *vdev) @@ -2769,6 +2839,13 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) goto out_teardown; } + if ((vdev->features & VFIO_FEATURE_ENABLE_AER) && + !pdev->exp.aer_cap) { + error_setg(errp, "vfio: Unable to enable AER for device %s, device " + "does not support AER signaling", vdev->vbasedev.name); + return; + } + if (vdev->vga) { vfio_vga_quirk_setup(vdev); } diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h index a8366bb..64701c4 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" @@ -132,6 +133,8 @@ typedef struct VFIOPCIDevice { #define VFIO_FEATURE_ENABLE_IGD_OPREGION_BIT 2 #define VFIO_FEATURE_ENABLE_IGD_OPREGION \ (1 << VFIO_FEATURE_ENABLE_IGD_OPREGION_BIT) +#define VFIO_FEATURE_ENABLE_AER_BIT 3 +#define VFIO_FEATURE_ENABLE_AER (1 << VFIO_FEATURE_ENABLE_AER_BIT) int32_t bootindex; uint32_t igd_gms; uint8_t pm_cap;