Re: [PATCH V5] powerpc/85xx: Add machine check handler to fix PCIe erratum on mpc85xx
On 04/11/2013 04:14:51 AM, Jia Hongtao-B38951 wrote: > -Original Message- > From: Wood Scott-B07421 > Sent: Thursday, April 11, 2013 5:52 AM > To: Jia Hongtao-B38951 > Cc: linuxppc-dev@lists.ozlabs.org; ga...@kernel.crashing.org; Wood Scott- > B07421; Li Yang-R58472; Jia Hongtao-B38951 > Subject: Re: [PATCH V5] powerpc/85xx: Add machine check handler to fix > PCIe erratum on mpc85xx > > The X and (especially for PCI) BRX versions are important -- probably > more so than the U versions. I doubt we need the A variant. Then I will add X and BRX variant and remove A variant. > > If you do support the A variant, why are you not sign-extending the > value? Just curious, sign-extending the value means fill rd with 0x? Or 0x on 64-bit (even if you're not going to implement 64-bit instructions, the ones you do implement shouldn't misbehave there). You could write it as "regs->gpr[rd] = ~0UL". -Scott ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH V5] powerpc/85xx: Add machine check handler to fix PCIe erratum on mpc85xx
> -Original Message-
> From: Wood Scott-B07421
> Sent: Thursday, April 11, 2013 5:52 AM
> To: Jia Hongtao-B38951
> Cc: linuxppc-dev@lists.ozlabs.org; ga...@kernel.crashing.org; Wood Scott-
> B07421; Li Yang-R58472; Jia Hongtao-B38951
> Subject: Re: [PATCH V5] powerpc/85xx: Add machine check handler to fix
> PCIe erratum on mpc85xx
>
> On 04/08/2013 03:26:54 AM, Jia Hongtao wrote:
> > @@ -826,6 +829,124 @@ u64 fsl_pci_immrbar_base(struct pci_controller
> > *hose)
> > return 0;
> > }
> >
> > +#ifdef CONFIG_E500
> > +
> > +#define OP_LWZ 32
> > +#define OP_LWZU 33
> > +#define OP_LBZ 34
> > +#define OP_LBZU 35
> > +#define OP_LHZ 40
> > +#define OP_LHZU 41
> > +#define OP_LHA 42
> > +#define OP_LHAU 43
>
> Can you move these to asm/ppc-opcode.h (or possibly
> asm/ppc-disassemble.h if we don't want to mix the two methods of
> describing instructions)?
Yes, mix the two methods is not appropriate.
asm/ppc-disassemble.h is a nice choice.
>
> > +static int mcheck_handle_load(struct pt_regs *regs, u32 inst)
> > +{
> > + unsigned int rd, ra, d;
> > +
> > + rd = get_rt(inst);
> > + ra = get_ra(inst);
> > + d = get_d(inst);
> > +
> > + switch (get_op(inst)) {
> > + case OP_LWZ:
> > + regs->gpr[rd] = 0x;
> > + break;
> > +
> > + case OP_LWZU:
> > + regs->gpr[rd] = 0x;
> > + regs->gpr[ra] += (s16)d;
> > + break;
> > +
> > + case OP_LBZ:
> > + regs->gpr[rd] = 0xff;
> > + break;
> > +
> > + case OP_LBZU:
> > + regs->gpr[rd] = 0xff;
> > + regs->gpr[ra] += (s16)d;
> > + break;
> > +
> > + case OP_LHZ:
> > + regs->gpr[rd] = 0x;
> > + break;
> > +
> > + case OP_LHZU:
> > + regs->gpr[rd] = 0x;
> > + regs->gpr[ra] += (s16)d;
> > + break;
> > +
> > + case OP_LHA:
> > + regs->gpr[rd] = 0x;
> > + break;
> > +
> > + case OP_LHAU:
> > + regs->gpr[rd] = 0x;
> > + regs->gpr[ra] += (s16)d;
> > + break;
>
> The X and (especially for PCI) BRX versions are important -- probably
> more so than the U versions. I doubt we need the A variant.
Then I will add X and BRX variant and remove A variant.
>
> If you do support the A variant, why are you not sign-extending the
> value?
Just curious, sign-extending the value means fill rd with 0x?
>
> Is this erratum present on any 64-bit chips?
No. This erratum only happened in e500 core chips.
>
> -Scott
Thanks.
-Hongtao
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH V5] powerpc/85xx: Add machine check handler to fix PCIe erratum on mpc85xx
On 04/08/2013 03:26:54 AM, Jia Hongtao wrote:
@@ -826,6 +829,124 @@ u64 fsl_pci_immrbar_base(struct pci_controller
*hose)
return 0;
}
+#ifdef CONFIG_E500
+
+#define OP_LWZ 32
+#define OP_LWZU 33
+#define OP_LBZ 34
+#define OP_LBZU 35
+#define OP_LHZ 40
+#define OP_LHZU 41
+#define OP_LHA 42
+#define OP_LHAU 43
Can you move these to asm/ppc-opcode.h (or possibly
asm/ppc-disassemble.h if we don't want to mix the two methods of
describing instructions)?
+static int mcheck_handle_load(struct pt_regs *regs, u32 inst)
+{
+ unsigned int rd, ra, d;
+
+ rd = get_rt(inst);
+ ra = get_ra(inst);
+ d = get_d(inst);
+
+ switch (get_op(inst)) {
+ case OP_LWZ:
+ regs->gpr[rd] = 0x;
+ break;
+
+ case OP_LWZU:
+ regs->gpr[rd] = 0x;
+ regs->gpr[ra] += (s16)d;
+ break;
+
+ case OP_LBZ:
+ regs->gpr[rd] = 0xff;
+ break;
+
+ case OP_LBZU:
+ regs->gpr[rd] = 0xff;
+ regs->gpr[ra] += (s16)d;
+ break;
+
+ case OP_LHZ:
+ regs->gpr[rd] = 0x;
+ break;
+
+ case OP_LHZU:
+ regs->gpr[rd] = 0x;
+ regs->gpr[ra] += (s16)d;
+ break;
+
+ case OP_LHA:
+ regs->gpr[rd] = 0x;
+ break;
+
+ case OP_LHAU:
+ regs->gpr[rd] = 0x;
+ regs->gpr[ra] += (s16)d;
+ break;
The X and (especially for PCI) BRX versions are important -- probably
more so than the U versions. I doubt we need the A variant.
If you do support the A variant, why are you not sign-extending the
value?
Is this erratum present on any 64-bit chips?
-Scott
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH V5] powerpc/85xx: Add machine check handler to fix PCIe erratum on mpc85xx
Hi Scott,
I added load instruction handler for the skipped instruction.
For now most common load instructions are handled in this patch.
Any advice for this?
Thanks.
-Hongtao.
> -Original Message-
> From: Jia Hongtao-B38951
> Sent: Monday, April 08, 2013 4:27 PM
> To: linuxppc-dev@lists.ozlabs.org; ga...@kernel.crashing.org
> Cc: Wood Scott-B07421; Li Yang-R58472; Jia Hongtao-B38951
> Subject: [PATCH V5] powerpc/85xx: Add machine check handler to fix PCIe
> erratum on mpc85xx
>
> A PCIe erratum of mpc85xx may causes a core hang when a link of PCIe goes
> down. when the link goes down, Non-posted transactions issued via the
> ATMU requiring completion result in an instruction stall.
> At the same time a machine-check exception is generated to the core to
> allow further processing by the handler. We implements the handler which
> skips the instruction caused the stall.
>
> This patch depends on patch:
> powerpc/85xx: Add platform_device declaration to fsl_pci.h
>
> Signed-off-by: Zhao Chenhui
> Signed-off-by: Li Yang
> Signed-off-by: Liu Shuo
> Signed-off-by: Jia Hongtao
> ---
> Changes for V4:
> * Fill rd with all-Fs if the skipped instruction is load and emulate the
> instruction.
> * Let KVM/QEMU deal with the exception if the machine check comes from
> KVM.
>
> arch/powerpc/kernel/cpu_setup_fsl_booke.S | 2 +-
> arch/powerpc/kernel/traps.c | 3 +
> arch/powerpc/sysdev/fsl_pci.c | 121
> ++
> arch/powerpc/sysdev/fsl_pci.h | 6 ++
> 4 files changed, 131 insertions(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/kernel/cpu_setup_fsl_booke.S
> b/arch/powerpc/kernel/cpu_setup_fsl_booke.S
> index dcd8819..f1bde90 100644
> --- a/arch/powerpc/kernel/cpu_setup_fsl_booke.S
> +++ b/arch/powerpc/kernel/cpu_setup_fsl_booke.S
> @@ -66,7 +66,7 @@ _GLOBAL(__setup_cpu_e500v2)
> bl __e500_icache_setup
> bl __e500_dcache_setup
> bl __setup_e500_ivors
> -#ifdef CONFIG_FSL_RIO
> +#if defined(CONFIG_FSL_RIO) || defined(CONFIG_FSL_PCI)
> /* Ensure that RFXE is set */
> mfspr r3,SPRN_HID1
> orisr3,r3,HID1_RFXE@h
> diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
> index a008cf5..dd275a4 100644
> --- a/arch/powerpc/kernel/traps.c
> +++ b/arch/powerpc/kernel/traps.c
> @@ -59,6 +59,7 @@
> #include
> #include
> #include
> +#include
>
> #if defined(CONFIG_DEBUGGER) || defined(CONFIG_KEXEC) int
> (*__debugger)(struct pt_regs *regs) __read_mostly; @@ -556,6 +557,8 @@
> int machine_check_e500(struct pt_regs *regs)
> if (reason & MCSR_BUS_RBERR) {
> if (fsl_rio_mcheck_exception(regs))
> return 1;
> + if (fsl_pci_mcheck_exception(regs))
> + return 1;
> }
>
> printk("Machine check in kernel mode.\n"); diff --git
> a/arch/powerpc/sysdev/fsl_pci.c b/arch/powerpc/sysdev/fsl_pci.c index
> 682084d..48326cd 100644
> --- a/arch/powerpc/sysdev/fsl_pci.c
> +++ b/arch/powerpc/sysdev/fsl_pci.c
> @@ -26,11 +26,14 @@
> #include
> #include
> #include
> +#include
>
> #include
> #include
> #include
> +#include
> #include
> +#include
> #include
> #include
>
> @@ -826,6 +829,124 @@ u64 fsl_pci_immrbar_base(struct pci_controller
> *hose)
> return 0;
> }
>
> +#ifdef CONFIG_E500
> +
> +#define OP_LWZ 32
> +#define OP_LWZU 33
> +#define OP_LBZ 34
> +#define OP_LBZU 35
> +#define OP_LHZ 40
> +#define OP_LHZU 41
> +#define OP_LHA 42
> +#define OP_LHAU 43
> +
> +static int mcheck_handle_load(struct pt_regs *regs, u32 inst) {
> + unsigned int rd, ra, d;
> +
> + rd = get_rt(inst);
> + ra = get_ra(inst);
> + d = get_d(inst);
> +
> + switch (get_op(inst)) {
> + case OP_LWZ:
> + regs->gpr[rd] = 0x;
> + break;
> +
> + case OP_LWZU:
> + regs->gpr[rd] = 0x;
> + regs->gpr[ra] += (s16)d;
> + break;
> +
> + case OP_LBZ:
> + regs->gpr[rd] = 0xff;
> + break;
> +
> + case OP_LBZU:
> + regs->gpr[rd] = 0xff;
> + regs->gpr[ra] += (s16)d;
> + break;
> +
> + case OP_LHZ:
> + regs->gpr[rd] = 0x;
> + break;
> +
> + case OP_LHZU:
> + regs->gpr[rd] = 0x;
> + regs->gpr[ra] += (s16)d;
> + break;
> +
> + case OP_LHA:
> + regs->gpr[rd] = 0x;
> + break;
> +
> + case OP_LHAU:
> + regs->gpr[rd] = 0x;
> + regs->gpr[ra] += (s16)d;
> + break;
> +
> + default:
> + return 0;
> + }
> +
> + return 1;
> +}
> +
> +static int is_in_pci_mem_space(phys_addr_t addr) {
> + struct pci_controller *hose;
> + struct resource *res;
> + int i;
> +
> + list_for_each_entry(hose, &hose_list, list_node) {
> + if (!early_find_capability(hose, 0,
