| Message ID | a8fedc4bb3883a417d44e338065ae40113479631.1426155432.git.chen.fan.fnst@cn.fujitsu.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Mar 12, 2015 at 06:23:58PM +0800, Chen Fan wrote: > add 'x-aer' property to let user able to decide whether expose > the aer capability. > > Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com> If it's exposed to users, it should not have the x- prefix. That prefix means "internal only". > --- > hw/vfio/pci.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c > index 8966c49..0517091 100644 > --- a/hw/vfio/pci.c > +++ b/hw/vfio/pci.c > @@ -159,6 +159,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; > @@ -2731,6 +2733,10 @@ static int vfio_setup_aer(VFIOPCIDevice *vdev, int pos, uint16_t size) > uint32_t severity, errcap; > int ret; > > + if (!(vdev->features & VFIO_FEATURE_ENABLE_AER)) { > + return 0; > + } > + > 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 > @@ -3691,6 +3697,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("x-aer", VFIOPCIDevice, features, > + VFIO_FEATURE_ENABLE_AER_BIT, true), > DEFINE_PROP_INT32("bootindex", VFIOPCIDevice, bootindex, -1), > DEFINE_PROP_BOOL("x-mmap", VFIOPCIDevice, vbasedev.allow_mmap, true), > /* > -- > 1.9.3 >
On Wed, 2015-03-18 at 14:23 +0100, Michael S. Tsirkin wrote: > On Thu, Mar 12, 2015 at 06:23:58PM +0800, Chen Fan wrote: > > add 'x-aer' property to let user able to decide whether expose > > the aer capability. > > > > Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com> > > If it's exposed to users, it should not have the x- prefix. > That prefix means "internal only". It's my understanding that the x- prefix is to be used for any "unsupported" option that should not be picked up by libvirt or other management tools as an official API. It may be removed or changed at any point. That doesn't imply it's internal-only. See for example x-vga, which is toggled by the user, but has a number of complicated issues to call officially supported. Or x-req, which allows a user to opt-out of the collaborative device request interface, but should not be the default and should not be easily accessible. I still don't know if x- is really the correct option in this case. If the guest is known not to support AER, then the user may desire the stop VM behavior. I also don't understand how a guest can necessarily recover from an AER since things like bus reset aren't exposed to guests except for very limited cases. Thanks, Alex > > --- > > hw/vfio/pci.c | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c > > index 8966c49..0517091 100644 > > --- a/hw/vfio/pci.c > > +++ b/hw/vfio/pci.c > > @@ -159,6 +159,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; > > @@ -2731,6 +2733,10 @@ static int vfio_setup_aer(VFIOPCIDevice *vdev, int pos, uint16_t size) > > uint32_t severity, errcap; > > int ret; > > > > + if (!(vdev->features & VFIO_FEATURE_ENABLE_AER)) { > > + return 0; > > + } > > + > > 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 > > @@ -3691,6 +3697,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("x-aer", VFIOPCIDevice, features, > > + VFIO_FEATURE_ENABLE_AER_BIT, true), > > DEFINE_PROP_INT32("bootindex", VFIOPCIDevice, bootindex, -1), > > DEFINE_PROP_BOOL("x-mmap", VFIOPCIDevice, vbasedev.allow_mmap, true), > > /* > > -- > > 1.9.3 > >
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index 8966c49..0517091 100644 --- a/hw/vfio/pci.c +++ b/hw/vfio/pci.c @@ -159,6 +159,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; @@ -2731,6 +2733,10 @@ static int vfio_setup_aer(VFIOPCIDevice *vdev, int pos, uint16_t size) uint32_t severity, errcap; int ret; + if (!(vdev->features & VFIO_FEATURE_ENABLE_AER)) { + return 0; + } + 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 @@ -3691,6 +3697,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("x-aer", VFIOPCIDevice, features, + VFIO_FEATURE_ENABLE_AER_BIT, true), DEFINE_PROP_INT32("bootindex", VFIOPCIDevice, bootindex, -1), DEFINE_PROP_BOOL("x-mmap", VFIOPCIDevice, vbasedev.allow_mmap, true), /*
add 'x-aer' property to let user able to decide whether expose the aer capability. Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com> --- hw/vfio/pci.c | 8 ++++++++ 1 file changed, 8 insertions(+)