Re: [Qemu-devel] [PATCH v5 6/7] vfio: add 'x-aer' property to expose aercap

[Alex Williamson <alex.williamson@redhat.com>]

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
> >