| Message ID | 22dfc3f7905e924561fd8729b2eae6846bef7a51.1425280224.git.chen.fan.fnst@cn.fujitsu.com |
|---|---|
| State | New |
| Headers | show |
On Mon, 2015-03-02 at 15:16 +0800, Chen Fan wrote: > add 'x-aer' option to disable aer capability if user > want. I'm generally one to favor using the x- flag, but we need to figure out if we need to make this be a supported option or not. We also need to decide whether we should include your previous patch that toggled this support based on machine type. We don't care about migration compatibility, but users might depend on specific error behavior and we don't want to break that for existing machine types. This would include only the q35 machine types through QEMU 2.3, I believe. My impression is that it should probably be supported. There may be cases where the guest OS does not support AER and we want to be able to invoke the old vm_stop() behavior. > Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com> > --- > hw/vfio/pci.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c > index db4ba23..5471437 100644 > --- a/hw/vfio/pci.c > +++ b/hw/vfio/pci.c > @@ -156,6 +156,8 @@ typedef struct VFIOPCIDevice { > uint32_t features; > #define VFIO_FEATURE_ENABLE_VGA_BIT 0 > #define VFIO_FEATURE_ENABLE_VGA (1 << VFIO_FEATURE_ENABLE_VGA_BIT) > +#define VFIO_FEATURE_ENABLE_AER_BIT 1 > +#define VFIO_FEATURE_ENABLE_AER (1 << VFIO_FEATURE_ENABLE_AER_BIT) Bit 1 is already taken by: 47cbe50 vfio-pci: Enable device request notification support > int32_t bootindex; > uint8_t pm_cap; > bool has_vga; > @@ -2728,6 +2730,10 @@ static int vfio_setup_aer(VFIOPCIDevice *vdev, uint8_t cap_ver, > uint32_t severity; > int ret; > > + if (!(vdev->features & VFIO_FEATURE_ENABLE_AER)) { > + return 0; > + } > + > pdev->exp.aer_log.log_max = PCIE_AER_LOG_MAX_DEFAULT; > ret = pcie_aer_init(pdev, cap_ver, pos, size); > if (ret) { > @@ -3569,6 +3575,8 @@ static Property vfio_pci_dev_properties[] = { > intx.mmap_timeout, 1100), > DEFINE_PROP_BIT("x-vga", VFIOPCIDevice, features, > VFIO_FEATURE_ENABLE_VGA_BIT, false), > + DEFINE_PROP_BIT("x-aer", VFIOPCIDevice, features, > + VFIO_FEATURE_ENABLE_AER_BIT, true), > DEFINE_PROP_INT32("bootindex", VFIOPCIDevice, bootindex, -1), > /* > * TODO - support passed fds... is this necessary?
On 03/10/2015 04:29 AM, Alex Williamson wrote: > On Mon, 2015-03-02 at 15:16 +0800, Chen Fan wrote: >> add 'x-aer' option to disable aer capability if user >> want. > I'm generally one to favor using the x- flag, but we need to figure out > if we need to make this be a supported option or not. We also need to > decide whether we should include your previous patch that toggled this > support based on machine type. We don't care about migration > compatibility, but users might depend on specific error behavior and we > don't want to break that for existing machine types. This would include > only the q35 machine types through QEMU 2.3, I believe. > > My impression is that it should probably be supported. There may be > cases where the guest OS does not support AER and we want to be able to > invoke the old vm_stop() behavior. In order to compatible with existing machine type behavior, I think we should let user decide whether need to enable aer on qemu. by default disabled. if the guest OS support AER, user may enable aer, otherwise, also invoke the vm_stop() behavior. Thanks, Chen > >> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com> >> --- >> hw/vfio/pci.c | 8 ++++++++ >> 1 file changed, 8 insertions(+) >> >> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c >> index db4ba23..5471437 100644 >> --- a/hw/vfio/pci.c >> +++ b/hw/vfio/pci.c >> @@ -156,6 +156,8 @@ typedef struct VFIOPCIDevice { >> uint32_t features; >> #define VFIO_FEATURE_ENABLE_VGA_BIT 0 >> #define VFIO_FEATURE_ENABLE_VGA (1 << VFIO_FEATURE_ENABLE_VGA_BIT) >> +#define VFIO_FEATURE_ENABLE_AER_BIT 1 >> +#define VFIO_FEATURE_ENABLE_AER (1 << VFIO_FEATURE_ENABLE_AER_BIT) > Bit 1 is already taken by: > > 47cbe50 vfio-pci: Enable device request notification support > >> int32_t bootindex; >> uint8_t pm_cap; >> bool has_vga; >> @@ -2728,6 +2730,10 @@ static int vfio_setup_aer(VFIOPCIDevice *vdev, uint8_t cap_ver, >> uint32_t severity; >> int ret; >> >> + if (!(vdev->features & VFIO_FEATURE_ENABLE_AER)) { >> + return 0; >> + } >> + >> pdev->exp.aer_log.log_max = PCIE_AER_LOG_MAX_DEFAULT; >> ret = pcie_aer_init(pdev, cap_ver, pos, size); >> if (ret) { >> @@ -3569,6 +3575,8 @@ static Property vfio_pci_dev_properties[] = { >> intx.mmap_timeout, 1100), >> DEFINE_PROP_BIT("x-vga", VFIOPCIDevice, features, >> VFIO_FEATURE_ENABLE_VGA_BIT, false), >> + DEFINE_PROP_BIT("x-aer", VFIOPCIDevice, features, >> + VFIO_FEATURE_ENABLE_AER_BIT, true), >> DEFINE_PROP_INT32("bootindex", VFIOPCIDevice, bootindex, -1), >> /* >> * TODO - support passed fds... is this necessary? > > > . >
On Wed, 2015-03-11 at 11:42 +0800, Chen Fan wrote: > On 03/10/2015 04:29 AM, Alex Williamson wrote: > > On Mon, 2015-03-02 at 15:16 +0800, Chen Fan wrote: > >> add 'x-aer' option to disable aer capability if user > >> want. > > I'm generally one to favor using the x- flag, but we need to figure out > > if we need to make this be a supported option or not. We also need to > > decide whether we should include your previous patch that toggled this > > support based on machine type. We don't care about migration > > compatibility, but users might depend on specific error behavior and we > > don't want to break that for existing machine types. This would include > > only the q35 machine types through QEMU 2.3, I believe. > > > > My impression is that it should probably be supported. There may be > > cases where the guest OS does not support AER and we want to be able to > > invoke the old vm_stop() behavior. > In order to compatible with existing machine type behavior, I think > we should let user decide whether need to enable aer on qemu. > by default disabled. if the guest OS support AER, user may > enable aer, otherwise, also invoke the vm_stop() behavior. Yes, for existing machine types, the current behavior needs to be maintained. The user can be given the option to enable AER and override the machine setting. For new machine types, I expect we'd want to enable AER automatically with the option to disable it. This would be for Q35 or other express chipsets only though since we can't expose AER to the guest for devices exposed on a conventional bus. Thanks, Alex > >> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com> > >> --- > >> hw/vfio/pci.c | 8 ++++++++ > >> 1 file changed, 8 insertions(+) > >> > >> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c > >> index db4ba23..5471437 100644 > >> --- a/hw/vfio/pci.c > >> +++ b/hw/vfio/pci.c > >> @@ -156,6 +156,8 @@ typedef struct VFIOPCIDevice { > >> uint32_t features; > >> #define VFIO_FEATURE_ENABLE_VGA_BIT 0 > >> #define VFIO_FEATURE_ENABLE_VGA (1 << VFIO_FEATURE_ENABLE_VGA_BIT) > >> +#define VFIO_FEATURE_ENABLE_AER_BIT 1 > >> +#define VFIO_FEATURE_ENABLE_AER (1 << VFIO_FEATURE_ENABLE_AER_BIT) > > Bit 1 is already taken by: > > > > 47cbe50 vfio-pci: Enable device request notification support > > > >> int32_t bootindex; > >> uint8_t pm_cap; > >> bool has_vga; > >> @@ -2728,6 +2730,10 @@ static int vfio_setup_aer(VFIOPCIDevice *vdev, uint8_t cap_ver, > >> uint32_t severity; > >> int ret; > >> > >> + if (!(vdev->features & VFIO_FEATURE_ENABLE_AER)) { > >> + return 0; > >> + } > >> + > >> pdev->exp.aer_log.log_max = PCIE_AER_LOG_MAX_DEFAULT; > >> ret = pcie_aer_init(pdev, cap_ver, pos, size); > >> if (ret) { > >> @@ -3569,6 +3575,8 @@ static Property vfio_pci_dev_properties[] = { > >> intx.mmap_timeout, 1100), > >> DEFINE_PROP_BIT("x-vga", VFIOPCIDevice, features, > >> VFIO_FEATURE_ENABLE_VGA_BIT, false), > >> + DEFINE_PROP_BIT("x-aer", VFIOPCIDevice, features, > >> + VFIO_FEATURE_ENABLE_AER_BIT, true), > >> DEFINE_PROP_INT32("bootindex", VFIOPCIDevice, bootindex, -1), > >> /* > >> * TODO - support passed fds... is this necessary? > > > > > > . > > >
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index db4ba23..5471437 100644 --- a/hw/vfio/pci.c +++ b/hw/vfio/pci.c @@ -156,6 +156,8 @@ typedef struct VFIOPCIDevice { uint32_t features; #define VFIO_FEATURE_ENABLE_VGA_BIT 0 #define VFIO_FEATURE_ENABLE_VGA (1 << VFIO_FEATURE_ENABLE_VGA_BIT) +#define VFIO_FEATURE_ENABLE_AER_BIT 1 +#define VFIO_FEATURE_ENABLE_AER (1 << VFIO_FEATURE_ENABLE_AER_BIT) int32_t bootindex; uint8_t pm_cap; bool has_vga; @@ -2728,6 +2730,10 @@ static int vfio_setup_aer(VFIOPCIDevice *vdev, uint8_t cap_ver, uint32_t severity; int ret; + if (!(vdev->features & VFIO_FEATURE_ENABLE_AER)) { + return 0; + } + pdev->exp.aer_log.log_max = PCIE_AER_LOG_MAX_DEFAULT; ret = pcie_aer_init(pdev, cap_ver, pos, size); if (ret) { @@ -3569,6 +3575,8 @@ static Property vfio_pci_dev_properties[] = { intx.mmap_timeout, 1100), DEFINE_PROP_BIT("x-vga", VFIOPCIDevice, features, VFIO_FEATURE_ENABLE_VGA_BIT, false), + DEFINE_PROP_BIT("x-aer", VFIOPCIDevice, features, + VFIO_FEATURE_ENABLE_AER_BIT, true), DEFINE_PROP_INT32("bootindex", VFIOPCIDevice, bootindex, -1), /* * TODO - support passed fds... is this necessary?
add 'x-aer' option to disable aer capability if user want. Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com> --- hw/vfio/pci.c | 8 ++++++++ 1 file changed, 8 insertions(+)