Re: [PATCH v3 2/2] hw/virtio-pci Added AER capability.

[Andrew Melnichenko <andrew@daynix.com>]

[-- Attachment #1: Type: text/plain, Size: 4103 bytes --]

Ok,
Main motivation:

> According to Microsoft driver\device certification requirements for next
> version of Window Server, PCIe device must support AER.
> The exact quote of Microsoft certification requirements:
> "Windows Server PCI Express devices are required to support Advanced
> Error Reporting [AER] as defined in PCI Express Base Specification version
> 2.1.*”*
>
 and

> Does management need ability to enable this capability?
>
Actually, yes. Can you provide their email address?

I'll prepare a new patch and I'll remove bugzilla link.

On Mon, Oct 5, 2020 at 8:46 PM Michael S. Tsirkin <mst@redhat.com> wrote:

> On Mon, Oct 05, 2020 at 02:56:01PM +0300, andrew@daynix.com wrote:
> > From: Andrew <andrew@daynix.com>
> >
> > Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1878465
>
> That's a private bug - what information can you share about
> the motivation for the patch?
>
> > Added AER capability for virtio-pci devices.
> > Also added property for devices, by default AER is disabled.
> >
> > Signed-off-by: Andrew Melnychenko <andrew@daynix.com>
> > ---
> >  hw/virtio/virtio-pci.c | 16 ++++++++++++++++
> >  hw/virtio/virtio-pci.h |  4 ++++
> >  2 files changed, 20 insertions(+)
> >
> > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> > index ae60c1e249..e0a7936f9c 100644
> > --- a/hw/virtio/virtio-pci.c
> > +++ b/hw/virtio/virtio-pci.c
> > @@ -1807,6 +1807,12 @@ static void virtio_pci_realize(PCIDevice
> *pci_dev, Error **errp)
> >           */
> >          pci_set_word(pci_dev->config + pos + PCI_PM_PMC, 0x3);
> >
> > +        if (proxy->flags & VIRTIO_PCI_FLAG_AER) {
> > +            pcie_aer_init(pci_dev, PCI_ERR_VER, last_pcie_cap_offset,
> > +                          PCI_ERR_SIZEOF, NULL);
> > +            last_pcie_cap_offset += PCI_ERR_SIZEOF;
> > +        }
> > +
> >          if (proxy->flags & VIRTIO_PCI_FLAG_INIT_DEVERR) {
> >              /* Init error enabling flags */
> >              pcie_cap_deverr_init(pci_dev);
> > @@ -1848,7 +1854,15 @@ static void virtio_pci_realize(PCIDevice
> *pci_dev, Error **errp)
> >
> >  static void virtio_pci_exit(PCIDevice *pci_dev)
> >  {
> > +    VirtIOPCIProxy *proxy = VIRTIO_PCI(pci_dev);
> > +    bool pcie_port = pci_bus_is_express(pci_get_bus(pci_dev)) &&
> > +                     !pci_bus_is_root(pci_get_bus(pci_dev));
> > +
> >      msix_uninit_exclusive_bar(pci_dev);
> > +    if (proxy->flags & VIRTIO_PCI_FLAG_AER && pcie_port &&
> > +        pci_is_express(pci_dev)) {
> > +        pcie_aer_exit(pci_dev);
> > +    }
> >  }
> >
> >  static void virtio_pci_reset(DeviceState *qdev)
> > @@ -1901,6 +1915,8 @@ static Property virtio_pci_properties[] = {
> >                      VIRTIO_PCI_FLAG_INIT_PM_BIT, true),
> >      DEFINE_PROP_BIT("x-pcie-flr-init", VirtIOPCIProxy, flags,
> >                      VIRTIO_PCI_FLAG_INIT_FLR_BIT, true),
> > +    DEFINE_PROP_BIT("aer", VirtIOPCIProxy, flags,
> > +                    VIRTIO_PCI_FLAG_AER_BIT, false),
> >      DEFINE_PROP_END_OF_LIST(),
> >  };
> >
>
> Does management need ability to enable this capability?
> If yes let's cc them. If no let's rename to x-aer so we don't
> commit to a stable interface ...
>
>
> > diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h
> > index 91096f0291..3986b4f0e3 100644
> > --- a/hw/virtio/virtio-pci.h
> > +++ b/hw/virtio/virtio-pci.h
> > @@ -45,6 +45,7 @@ enum {
> >      VIRTIO_PCI_FLAG_INIT_LNKCTL_BIT,
> >      VIRTIO_PCI_FLAG_INIT_PM_BIT,
> >      VIRTIO_PCI_FLAG_INIT_FLR_BIT,
> > +    VIRTIO_PCI_FLAG_AER_BIT,
> >  };
> >
> >  /* Need to activate work-arounds for buggy guests at vmstate load. */
> > @@ -84,6 +85,9 @@ enum {
> >  /* Init Function Level Reset capability */
> >  #define VIRTIO_PCI_FLAG_INIT_FLR (1 << VIRTIO_PCI_FLAG_INIT_FLR_BIT)
> >
> > +/* Advanced Error Reporting capability */
> > +#define VIRTIO_PCI_FLAG_AER (1 << VIRTIO_PCI_FLAG_AER_BIT)
> > +
> >  typedef struct {
> >      MSIMessage msg;
> >      int virq;
> > --
> > 2.28.0
>
>

[-- Attachment #2: Type: text/html, Size: 5891 bytes --]