| Message ID | 1490260163-6157-2-git-send-email-caoj.fnst@cn.fujitsu.com |
|---|---|
| State | New |
| Headers | show |
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.
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?
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
On Thu, Mar 23, 2017 at 05:09:21PM +0800, Cao jin 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> The two configurations aren't very different. If you disable AER automatically in (2) you should do so in (1) as well. > --- > 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) > + { > + 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; > -- > 1.8.3.1 > >
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) + { + 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;