diff mbox series

[v3,2/2] hw/virtio-pci Added AER capability.

Message ID 20201005115601.103791-3-andrew@daynix.com
State New
Headers show
Series hw/virtio-pci: AER capability | expand

Commit Message

Andrew Melnichenko Oct. 5, 2020, 11:56 a.m. UTC 
From: Andrew <andrew@daynix.com>

Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1878465
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(+)

Comments

Michael S. Tsirkin Oct. 5, 2020, 5:46 p.m. UTC | #1 
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
Yan Vugenfirer Oct. 7, 2020, 6:41 a.m. UTC | #2 
Hi Michael,



> On 5 Oct 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?

We need AER feature in order to pass MS WHQL tests for the future Windows Server versions.
According to Microsoft driver\device certification requirements for next version of Window Server, PCIe devices must support AER.
The exact quote from 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.”


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

QE is using libvirt in order to spawn test setups. So I think it should be used by management as well.
Whom should Andrew CC?

Best regards,
Yan.
> 
> 
>> 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
Andrew Melnichenko Oct. 7, 2020, 7:13 a.m. UTC | #3 
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
>
>
diff mbox series

Patch

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(),
 };
 
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;