For PCIe device support AER(Advanced Error Reporting), from the pcie spec 3.0 chapter 6.2.5, ERR_COR, ERR_NONFATAL, and ERR_FATAL can be forwarded from the secondary interface to the primary interface, only require the SERR# Enable bit in the Bridge Control register is set.
and at the kernel side, we found only _HPP() method can enable SERR#, So here we want to turn on this bit.
Signed-off-by: Chen Fan chen.fan.fnst@cn.fujitsu.com --- src/fw/pciinit.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/src/fw/pciinit.c b/src/fw/pciinit.c index 34279a4..28ed1af 100644 --- a/src/fw/pciinit.c +++ b/src/fw/pciinit.c @@ -310,6 +310,10 @@ static void pci_bios_init_device(struct pci_device *pci) /* enable memory mappings */ pci_config_maskw(bdf, PCI_COMMAND, 0, PCI_COMMAND_IO | PCI_COMMAND_MEMORY | PCI_COMMAND_SERR); + /* enable SERR# for forwording */ + if (pci->header_type & PCI_HEADER_TYPE_BRIDGE) + pci_config_maskw(bdf, PCI_BRIDGE_CONTROL, 0, + PCI_BRIDGE_CTL_SERR); }
static void pci_bios_init_devices(void)
Dear Chen,
thank you for your patch.
You misspelled `forw*a*rding` in the summary and the comment. I have some more nitpicks.
Am Mittwoch, den 28.01.2015, 16:05 +0800 schrieb Chen Fan:
For PCIe device support AER(Advanced Error Reporting), from the
Please add a space before `(`.
pcie spec 3.0 chapter 6.2.5, ERR_COR, ERR_NONFATAL, and ERR_FATAL can be forwarded from the secondary interface to the primary interface, only require the SERR# Enable bit in the Bridge Control register is set.
… to be set.
and at the kernel side, we found only _HPP() method can enable
*A*nd …
SERR#, So here we want to turn on this bit.
s/So/so/
Signed-off-by: Chen Fan chen.fan.fnst@cn.fujitsu.com
How did you test this?
src/fw/pciinit.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/src/fw/pciinit.c b/src/fw/pciinit.c index 34279a4..28ed1af 100644 --- a/src/fw/pciinit.c +++ b/src/fw/pciinit.c @@ -310,6 +310,10 @@ static void pci_bios_init_device(struct pci_device *pci) /* enable memory mappings */ pci_config_maskw(bdf, PCI_COMMAND, 0, PCI_COMMAND_IO | PCI_COMMAND_MEMORY | PCI_COMMAND_SERR);
- /* enable SERR# for forwording */
forw*a*rding
- if (pci->header_type & PCI_HEADER_TYPE_BRIDGE)
pci_config_maskw(bdf, PCI_BRIDGE_CONTROL, 0,PCI_BRIDGE_CTL_SERR);}
static void pci_bios_init_devices(void)
Otherwise this looks good.
Thanks,
Paul
Hi Paul,
Thanks for your attention and help me to correct the misspelling.
On 01/29/2015 06:21 AM, Paul Menzel wrote:
Dear Chen,
thank you for your patch.
You misspelled `forw*a*rding` in the summary and the comment. I have some more nitpicks.
Am Mittwoch, den 28.01.2015, 16:05 +0800 schrieb Chen Fan:
For PCIe device support AER(Advanced Error Reporting), from the
Please add a space before `(`.
pcie spec 3.0 chapter 6.2.5, ERR_COR, ERR_NONFATAL, and ERR_FATAL can be forwarded from the secondary interface to the primary interface, only require the SERR# Enable bit in the Bridge Control register is set.
… to be set.
and at the kernel side, we found only _HPP() method can enable
*A*nd …
SERR#, So here we want to turn on this bit.
s/So/so/
Signed-off-by: Chen Fan chen.fan.fnst@cn.fujitsu.com
How did you test this?
I used qemu to test this patch, and also I had sent qemu patchset series to improve aer report. if you have interest in the patches, pls see: http://lists.nongnu.org/archive/html/qemu-devel/2015-01/msg03947.html
src/fw/pciinit.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/src/fw/pciinit.c b/src/fw/pciinit.c index 34279a4..28ed1af 100644 --- a/src/fw/pciinit.c +++ b/src/fw/pciinit.c @@ -310,6 +310,10 @@ static void pci_bios_init_device(struct pci_device *pci) /* enable memory mappings */ pci_config_maskw(bdf, PCI_COMMAND, 0, PCI_COMMAND_IO | PCI_COMMAND_MEMORY | PCI_COMMAND_SERR);
- /* enable SERR# for forwording */
forw*a*rding
if (pci->header_type & PCI_HEADER_TYPE_BRIDGE)
pci_config_maskw(bdf, PCI_BRIDGE_CONTROL, 0,PCI_BRIDGE_CTL_SERR);}
static void pci_bios_init_devices(void)
Otherwise this looks good.
Thanks, Chen
Thanks,
Paul
On Wed, Jan 28, 2015 at 11:21:35PM +0100, Paul Menzel wrote:
Dear Chen,
thank you for your patch.
You misspelled `forw*a*rding` in the summary and the comment. I have some more nitpicks.
Am Mittwoch, den 28.01.2015, 16:05 +0800 schrieb Chen Fan:
For PCIe device support AER(Advanced Error Reporting), from the
Please add a space before `(`.
Hi Paul,
Thanks for reviewing the patch. I agree that the comments in the code should be spelled correctly. However, I don't think we need to be that picky about the commit comments - the commit comments do need to be understandable and descriptive of the change - but if it's understandable we shouldn't need multiple submit cycles for an otherwise good change.
-Kevin
On Wed, Jan 28, 2015 at 04:05:13PM +0800, Chen Fan wrote:
For PCIe device support AER(Advanced Error Reporting), from the pcie spec 3.0 chapter 6.2.5, ERR_COR, ERR_NONFATAL, and ERR_FATAL can be forwarded from the secondary interface to the primary interface, only require the SERR# Enable bit in the Bridge Control register is set.
and at the kernel side, we found only _HPP() method can enable SERR#, So here we want to turn on this bit.
Signed-off-by: Chen Fan chen.fan.fnst@cn.fujitsu.com
src/fw/pciinit.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/src/fw/pciinit.c b/src/fw/pciinit.c index 34279a4..28ed1af 100644 --- a/src/fw/pciinit.c +++ b/src/fw/pciinit.c @@ -310,6 +310,10 @@ static void pci_bios_init_device(struct pci_device *pci) /* enable memory mappings */ pci_config_maskw(bdf, PCI_COMMAND, 0, PCI_COMMAND_IO | PCI_COMMAND_MEMORY | PCI_COMMAND_SERR);
- /* enable SERR# for forwording */
- if (pci->header_type & PCI_HEADER_TYPE_BRIDGE)
pci_config_maskw(bdf, PCI_BRIDGE_CONTROL, 0,PCI_BRIDGE_CTL_SERR);}
Thanks for submitting your patch.
I'm not that familiar with all the details of the PCI specification. Can you elaborate on what doesn't work if this patch is not present, or what does work or work better when present? Is this associated with a QEMU feature or change?
-Kevin
On 02/01/2015 07:01 AM, Kevin O'Connor wrote:
On Wed, Jan 28, 2015 at 04:05:13PM +0800, Chen Fan wrote:
For PCIe device support AER(Advanced Error Reporting), from the pcie spec 3.0 chapter 6.2.5, ERR_COR, ERR_NONFATAL, and ERR_FATAL can be forwarded from the secondary interface to the primary interface, only require the SERR# Enable bit in the Bridge Control register is set.
and at the kernel side, we found only _HPP() method can enable SERR#, So here we want to turn on this bit.
Signed-off-by: Chen Fan chen.fan.fnst@cn.fujitsu.com
src/fw/pciinit.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/src/fw/pciinit.c b/src/fw/pciinit.c index 34279a4..28ed1af 100644 --- a/src/fw/pciinit.c +++ b/src/fw/pciinit.c @@ -310,6 +310,10 @@ static void pci_bios_init_device(struct pci_device *pci) /* enable memory mappings */ pci_config_maskw(bdf, PCI_COMMAND, 0, PCI_COMMAND_IO | PCI_COMMAND_MEMORY | PCI_COMMAND_SERR);
- /* enable SERR# for forwording */
- if (pci->header_type & PCI_HEADER_TYPE_BRIDGE)
pci_config_maskw(bdf, PCI_BRIDGE_CONTROL, 0,PCI_BRIDGE_CTL_SERR);Thanks for submitting your patch.
I'm not that familiar with all the details of the PCI specification. Can you elaborate on what doesn't work if this patch is not present, or what does work or work better when present? Is this associated with a QEMU feature or change?
With this patch, the aer error inject feature for pcie device in QEMU will work . the hmp command line is called pcie_aer_inject_error. a endpoint device propagates error messages to guest that need the parent bridge type devices enable the SERR# in bridge control reigster from pcie spec 3.0 chapter 6.2.5, and currently, the register bit is not turned on, so the command pcie_aer_inject_error is not work. and if the patch is ok, could you check in this patch?
Thanks, Chen
-Kevin .
On 2/1/15 6:13 PM, Chen Fan wrote:
On 02/01/2015 07:01 AM, Kevin O'Connor wrote:
On Wed, Jan 28, 2015 at 04:05:13PM +0800, Chen Fan wrote:
For PCIe device support AER(Advanced Error Reporting), from the pcie spec 3.0 chapter 6.2.5, ERR_COR, ERR_NONFATAL, and ERR_FATAL can be forwarded from the secondary interface to the primary interface, only require the SERR# Enable bit in the Bridge Control register is set.
and at the kernel side, we found only _HPP() method can enable SERR#, So here we want to turn on this bit.
Signed-off-by: Chen Fan chen.fan.fnst@cn.fujitsu.com
src/fw/pciinit.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/src/fw/pciinit.c b/src/fw/pciinit.c index 34279a4..28ed1af 100644 --- a/src/fw/pciinit.c +++ b/src/fw/pciinit.c @@ -310,6 +310,10 @@ static void pci_bios_init_device(struct pci_device *pci) /* enable memory mappings */ pci_config_maskw(bdf, PCI_COMMAND, 0, PCI_COMMAND_IO | PCI_COMMAND_MEMORY | PCI_COMMAND_SERR);
- /* enable SERR# for forwording */
- if (pci->header_type & PCI_HEADER_TYPE_BRIDGE)
pci_config_maskw(bdf, PCI_BRIDGE_CONTROL, 0,PCI_BRIDGE_CTL_SERR);Thanks for submitting your patch.
I'm not that familiar with all the details of the PCI specification. Can you elaborate on what doesn't work if this patch is not present, or what does work or work better when present? Is this associated with a QEMU feature or change?
With this patch, the aer error inject feature for pcie device in QEMU will work . the hmp command line is called pcie_aer_inject_error. a endpoint device propagates error messages to guest that need the parent bridge type devices enable the SERR# in bridge control reigster from pcie spec 3.0 chapter 6.2.5, and currently, the register bit is not turned on, so the command pcie_aer_inject_error is not work. and if the patch is ok, could you check in this patch?
Patch looks fine to me. Certain PCI switches, like PEX87xx, provide mechanism to explicitly inject errors (unlike aer_inject tool where just interrupt is raised). However, if SERR bit is not enabled in the bridges, error is not percolated to root port, probably because kernel expects BIOS to enable this bit. CC'ing Bjorn who can correct if this incorrect understanding.
_nishank
On 2/1/15 6:13 PM, Chen Fan wrote:
On 02/01/2015 07:01 AM, Kevin O'Connor wrote:
On Wed, Jan 28, 2015 at 04:05:13PM +0800, Chen Fan wrote:
For PCIe device support AER(Advanced Error Reporting), from the pcie spec 3.0 chapter 6.2.5, ERR_COR, ERR_NONFATAL, and ERR_FATAL can be forwarded from the secondary interface to the primary interface, only require the SERR# Enable bit in the Bridge Control register is set.
and at the kernel side, we found only _HPP() method can enable SERR#, So here we want to turn on this bit.
Signed-off-by: Chen Fan chen.fan.fnst@cn.fujitsu.com
src/fw/pciinit.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/src/fw/pciinit.c b/src/fw/pciinit.c index 34279a4..28ed1af 100644 --- a/src/fw/pciinit.c +++ b/src/fw/pciinit.c @@ -310,6 +310,10 @@ static void pci_bios_init_device(struct pci_device *pci) /* enable memory mappings */ pci_config_maskw(bdf, PCI_COMMAND, 0, PCI_COMMAND_IO | PCI_COMMAND_MEMORY | PCI_COMMAND_SERR);
- /* enable SERR# for forwording */
- if (pci->header_type & PCI_HEADER_TYPE_BRIDGE)
pci_config_maskw(bdf, PCI_BRIDGE_CONTROL, 0,PCI_BRIDGE_CTL_SERR);Thanks for submitting your patch.
I'm not that familiar with all the details of the PCI specification. Can you elaborate on what doesn't work if this patch is not present, or what does work or work better when present? Is this associated with a QEMU feature or change?
With this patch, the aer error inject feature for pcie device in QEMU will work . the hmp command line is called pcie_aer_inject_error. a endpoint device propagates error messages to guest that need the parent bridge type devices enable the SERR# in bridge control reigster from pcie spec 3.0 chapter 6.2.5, and currently, the register bit is not turned on, so the command pcie_aer_inject_error is not work. and if the patch is ok, could you check in this patch?
Patch looks fine to me. Certain PCI switches, like PEX87xx, provide mechanism to explicitly inject errors (unlike aer_inject tool where just interrupt is raised). However, if SERR bit is not enabled in the bridges, error is not percolated to root port, probably because kernel expects BIOS to enable this bit. CC'ing Bjorn who can correct if this incorrect understanding.
_nishank
On Sun, Feb 1, 2015 at 10:08 PM, Nishank Trivedi nishank.trivedi@netapp.com wrote:
On 2/1/15 6:13 PM, Chen Fan wrote:
On 02/01/2015 07:01 AM, Kevin O'Connor wrote:
On Wed, Jan 28, 2015 at 04:05:13PM +0800, Chen Fan wrote:
For PCIe device support AER(Advanced Error Reporting), from the pcie spec 3.0 chapter 6.2.5, ERR_COR, ERR_NONFATAL, and ERR_FATAL can be forwarded from the secondary interface to the primary interface, only require the SERR# Enable bit in the Bridge Control register is set.
and at the kernel side, we found only _HPP() method can enable SERR#, So here we want to turn on this bit.
Signed-off-by: Chen Fan chen.fan.fnst@cn.fujitsu.com
src/fw/pciinit.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/src/fw/pciinit.c b/src/fw/pciinit.c index 34279a4..28ed1af 100644 --- a/src/fw/pciinit.c +++ b/src/fw/pciinit.c @@ -310,6 +310,10 @@ static void pci_bios_init_device(struct pci_device *pci) /* enable memory mappings */ pci_config_maskw(bdf, PCI_COMMAND, 0, PCI_COMMAND_IO | PCI_COMMAND_MEMORY | PCI_COMMAND_SERR);
- /* enable SERR# for forwording */
- if (pci->header_type & PCI_HEADER_TYPE_BRIDGE)
pci_config_maskw(bdf, PCI_BRIDGE_CONTROL, 0,PCI_BRIDGE_CTL_SERR);Thanks for submitting your patch.
I'm not that familiar with all the details of the PCI specification. Can you elaborate on what doesn't work if this patch is not present, or what does work or work better when present? Is this associated with a QEMU feature or change?
With this patch, the aer error inject feature for pcie device in QEMU will work . the hmp command line is called pcie_aer_inject_error. a endpoint device propagates error messages to guest that need the parent bridge type devices enable the SERR# in bridge control reigster from pcie spec 3.0 chapter 6.2.5, and currently, the register bit is not turned on, so the command pcie_aer_inject_error is not work. and if the patch is ok, could you check in this patch?
Patch looks fine to me. Certain PCI switches, like PEX87xx, provide mechanism to explicitly inject errors (unlike aer_inject tool where just interrupt is raised). However, if SERR bit is not enabled in the bridges, error is not percolated to root port, probably because kernel expects BIOS to enable this bit. CC'ing Bjorn who can correct if this incorrect understanding.
I think your understanding of PCI_BRIDGE_CTL_SERR is correct. Linux doesn't really have formal expectations about how platforms should set PCI_BRIDGE_CTL_SERR. It's possible that Linux should be more proactive about enabling it. I think it's just an area that nobody has paid much attention to.
Bjorn
On Wed, Jan 28, 2015 at 04:05:13PM +0800, Chen Fan wrote:
For PCIe device support AER(Advanced Error Reporting), from the pcie spec 3.0 chapter 6.2.5, ERR_COR, ERR_NONFATAL, and ERR_FATAL can be forwarded from the secondary interface to the primary interface, only require the SERR# Enable bit in the Bridge Control register is set.
and at the kernel side, we found only _HPP() method can enable SERR#, So here we want to turn on this bit.
I fixed the spelling of "forwarding" and applied this patch.
Thanks. -Kevin
On 02/25/2015 12:59 AM, Kevin O'Connor wrote:
On Wed, Jan 28, 2015 at 04:05:13PM +0800, Chen Fan wrote:
For PCIe device support AER(Advanced Error Reporting), from the pcie spec 3.0 chapter 6.2.5, ERR_COR, ERR_NONFATAL, and ERR_FATAL can be forwarded from the secondary interface to the primary interface, only require the SERR# Enable bit in the Bridge Control register is set.
and at the kernel side, we found only _HPP() method can enable SERR#, So here we want to turn on this bit.
I fixed the spelling of "forwarding" and applied this patch.
Thanks, Chen
Thanks. -Kevin
-
Bjorn Helgaas -
Chen Fan -
Kevin O'Connor -
Nishank Trivedi -
Paul Menzel