Re: [Qemu-devel] [PATCH v3 1/3] pcie aer: verify if AER functionality is available

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

On Tue, 28 Mar 2017 21:47:30 +0800
Cao jin <caoj.fnst@cn.fujitsu.com> wrote:

> On 03/25/2017 06:12 AM, Alex Williamson wrote:
> > On Thu, 23 Mar 2017 17:09:21 +0800
> > Cao jin <caoj.fnst@cn.fujitsu.com> wrote:
> >   
> >> For devices which support AER function, verify it can work or not in the
> >> system:
> >> 1. AER capable device is a PCIe device, it can't be plugged into PCI bus
> >> 2. If root port doesn't support AER, then there is no need to expose the
> >>    AER capability
> >>
> >> Signed-off-by: Dou Liyang <douly.fnst@cn.fujitsu.com>
> >> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> >> ---
> >>  hw/pci/pcie_aer.c | 28 ++++++++++++++++++++++++++++
> >>  1 file changed, 28 insertions(+)
> >>
> >> diff --git a/hw/pci/pcie_aer.c b/hw/pci/pcie_aer.c
> >> index daf1f65..a2e9818 100644
> >> --- a/hw/pci/pcie_aer.c
> >> +++ b/hw/pci/pcie_aer.c
> >> @@ -100,6 +100,34 @@ static void aer_log_clear_all_err(PCIEAERLog *aer_log)
> >>  int pcie_aer_init(PCIDevice *dev, uint8_t cap_ver, uint16_t offset,
> >>                    uint16_t size, Error **errp)
> >>  {
> >> +    PCIDevice *parent_dev;
> >> +    uint8_t type;
> >> +    uint8_t parent_type;
> >> +
> >> +    /* Topology test: see if there is need to expose AER cap */
> >> +    type = pcie_cap_get_type(dev);
> >> +    parent_dev = pci_bridge_get_device(dev->bus);
> >> +    while (parent_dev) {
> >> +        parent_type = pcie_cap_get_type(parent_dev);
> >> +
> >> +        if (type == PCI_EXP_TYPE_ENDPOINT &&
> >> +                (parent_type != PCI_EXP_TYPE_ROOT_PORT &&
> >> +                 parent_type != PCI_EXP_TYPE_DOWNSTREAM)) {
> >> +            error_setg(errp, "Parent device is not a PCIe component");
> >> +            return -ENOTSUP;
> >> +        }
> >> +        
> >> +        if (parent_type == PCI_EXP_TYPE_ROOT_PORT) {
> >> +            if (!parent_dev->exp.aer_cap)
> >> +            {  
> > 
> > Curly brace at the end of the previous line.
> >   
> >> +                error_setg(errp, "Root port does not support AER");
> >> +                return -ENOTSUP;
> >> +            }
> >> +        }
> >> +
> >> +        parent_dev = pci_bridge_get_device(parent_dev->bus);
> >> +    }
> >> +
> >>      pcie_add_capability(dev, PCI_EXT_CAP_ID_ERR, cap_ver,
> >>                          offset, size);
> >>      dev->exp.aer_cap = offset;  
> > 
> > This patch makes existing configurations including PCIe root ports,
> > upstream ports, downstream ports, and e1000e fail if they do not meet
> > this new configuration requirement.
> >   
> 
> Yes, I noticed that e1000e could be realized on i440fx, which I think is
> not possible in real world, like the commit log(1.) said.
> 
> But for those ports, what are the conditions they will fail with this patch?

It seems that the algorithm expects a PCIe device on a PCIe machine
type to have PCIe components all the way to the root bus.  This is easy
to break with a Q35 machine, DMI-to-PCI bridge (ie. PCIe-to-PCI
bridge), and a PCIe root port.  Such a configuration isn't exactly
legitimate from a PCI topology perspective, but QEMU will let you do it.
If we had generic PCIe-to-PCI and PCI-to-PCIe bridges, it could even be
done with a legitimate PCI topology.  Thanks,

Alex