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

[Yan Vugenfirer <yvugenfi@redhat.com>]

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