Re: [PATCH V4] powerpc/85xx: Add machine check handler to fix PCIe erratum on mpc85xx
On Mon, Mar 4, 2013 at 2:40 AM, Jia Hongtao wrote: > 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. Can you explain at a high level how just skipping an instruction solves anything? If you just skip a load/store and continue like nothing is wrong, isn't your system possibly in a really bad state. And if the core is already hung, due to the PCI link going down, isn't it too late? How does skipping help? Stuart ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH V4] 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. Just skipping the instruction doesn't seem a good idea. But I suspect that re-initialising the PCI interface is also almost impossible. Does the mpc83xx have the same errata? We've seen machine-check faults using the CSB bridge on an 83xx doing a 'pio' access after a PEX_DMA transfer to certain target addresses stalls - software gives up waiting for the dma. The target is an fpga, nothing is mapped at those addesses - but we'd expect to get ~0u back as happens on other slave windows. I also remember some problems with single word DMA. David ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH V4] powerpc/85xx: Add machine check handler to fix PCIe erratum on mpc85xx
On 03/04/2013 10:16:10 AM, Stuart Yoder wrote: On Mon, Mar 4, 2013 at 2:40 AM, Jia Hongtao wrote: > 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. Can you explain at a high level how just skipping an instruction solves anything? If you just skip a load/store and continue like nothing is wrong, isn't your system possibly in a really bad state. If the instruction was a load, we probably at least want to fill the destination register with 0x or similar. And if the core is already hung, due to the PCI link going down, isn't it too late? How does skipping help? Maybe the machine check unhangs the core? Is there an erratum number for this? -Scott ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH V4] powerpc/85xx: Add machine check handler to fix PCIe erratum on mpc85xx
> -Original Message- > From: David Laight [mailto:david.lai...@aculab.com] > Sent: Tuesday, March 05, 2013 1:16 AM > To: Jia Hongtao-B38951; linuxppc-dev@lists.ozlabs.org; > ga...@kernel.crashing.org > Cc: Wood Scott-B07421 > Subject: RE: [PATCH V4] 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. > > Just skipping the instruction doesn't seem a good idea. > But I suspect that re-initialising the PCI interface is also almost > impossible. This *skipping* is the best way I thought for this errata. It's not perfect but works. -Hongtao. > > Does the mpc83xx have the same errata? > We've seen machine-check faults using the CSB bridge on an 83xx doing a > 'pio' access after a PEX_DMA transfer to certain target addresses stalls > - software gives up waiting for the dma. > The target is an fpga, nothing is mapped at those addesses - but we'd > expect to get ~0u back as happens on other slave windows. > > I also remember some problems with single word DMA. > > David > > ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH V4] powerpc/85xx: Add machine check handler to fix PCIe erratum on mpc85xx
> -Original Message- > From: Wood Scott-B07421 > Sent: Tuesday, March 05, 2013 7:46 AM > To: Stuart Yoder > Cc: Jia Hongtao-B38951; linuxppc-dev@lists.ozlabs.org; Kumar Gala > Subject: Re: [PATCH V4] powerpc/85xx: Add machine check handler to fix > PCIe erratum on mpc85xx > > On 03/04/2013 10:16:10 AM, Stuart Yoder wrote: > > On Mon, Mar 4, 2013 at 2:40 AM, Jia Hongtao > > wrote: > > > 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. > > > > Can you explain at a high level how just skipping an instruction > > solves > > anything? If you just skip a load/store and continue like nothing is > > wrong, isn't your system possibly in a really bad state. > > If the instruction was a load, we probably at least want to fill the > destination register with 0x or similar. You discuss this with Liu Shuo about a year ago. here is the log: " On 02/01/2012 02:18 AM, shuo@freescale.com wrote: > v3 : Skip the instruction only. Don't access the user space memory in > mechine check. It may be the least bad option for now, but be aware that there's a small chance that this will cause a leak of sensitive information (such as a piece of a crypto key that happened to be sitting in the register to be loaded into). -Scott " -Hongtao. > > > And if the core is already hung, due to the PCI link going down, isn't > > it too late? How does skipping help? > > Maybe the machine check unhangs the core? > > Is there an erratum number for this? > > -Scott ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH V4] powerpc/85xx: Add machine check handler to fix PCIe erratum on mpc85xx
On 03/05/2013 04:12:30 AM, Jia Hongtao-B38951 wrote: > -Original Message- > From: Wood Scott-B07421 > Sent: Tuesday, March 05, 2013 7:46 AM > To: Stuart Yoder > Cc: Jia Hongtao-B38951; linuxppc-dev@lists.ozlabs.org; Kumar Gala > Subject: Re: [PATCH V4] powerpc/85xx: Add machine check handler to fix > PCIe erratum on mpc85xx > > On 03/04/2013 10:16:10 AM, Stuart Yoder wrote: > > On Mon, Mar 4, 2013 at 2:40 AM, Jia Hongtao > > wrote: > > > 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. > > > > Can you explain at a high level how just skipping an instruction > > solves > > anything? If you just skip a load/store and continue like nothing is > > wrong, isn't your system possibly in a really bad state. > > If the instruction was a load, we probably at least want to fill the > destination register with 0x or similar. You discuss this with Liu Shuo about a year ago. here is the log: " On 02/01/2012 02:18 AM, shuo@freescale.com wrote: > v3 : Skip the instruction only. Don't access the user space memory in > mechine check. It may be the least bad option for now, but be aware that there's a small chance that this will cause a leak of sensitive information (such as a piece of a crypto key that happened to be sitting in the register to be loaded into). Yes, that's (one reason) why you'd want to fill in a known value. Note the "for now". :-) -Scott ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH V4] powerpc/85xx: Add machine check handler to fix PCIe erratum on mpc85xx
> -Original Message- > From: Wood Scott-B07421 > Sent: Wednesday, March 06, 2013 2:48 AM > To: Jia Hongtao-B38951 > Cc: Wood Scott-B07421; Stuart Yoder; linuxppc-dev@lists.ozlabs.org; Kumar > Gala > Subject: Re: [PATCH V4] powerpc/85xx: Add machine check handler to fix > PCIe erratum on mpc85xx > > On 03/05/2013 04:12:30 AM, Jia Hongtao-B38951 wrote: > > > > > > > -Original Message- > > > From: Wood Scott-B07421 > > > Sent: Tuesday, March 05, 2013 7:46 AM > > > To: Stuart Yoder > > > Cc: Jia Hongtao-B38951; linuxppc-dev@lists.ozlabs.org; Kumar Gala > > > Subject: Re: [PATCH V4] powerpc/85xx: Add machine check handler to > > fix > > > PCIe erratum on mpc85xx > > > > > > On 03/04/2013 10:16:10 AM, Stuart Yoder wrote: > > > > On Mon, Mar 4, 2013 at 2:40 AM, Jia Hongtao > > > > wrote: > > > > > 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. > > > > > > > > Can you explain at a high level how just skipping an instruction > > > > solves > > > > anything? If you just skip a load/store and continue like > > nothing is > > > > wrong, isn't your system possibly in a really bad state. > > > > > > If the instruction was a load, we probably at least want to fill the > > > destination register with 0x or similar. > > > > You discuss this with Liu Shuo about a year ago. > > here is the log: > > > > " > > On 02/01/2012 02:18 AM, shuo@freescale.com wrote: > > > v3 : Skip the instruction only. Don't access the user space memory > > in > > > mechine check. > > > > It may be the least bad option for now, but be aware that there's a > > small chance that this will cause a leak of sensitive information > > (such as a piece of a crypto key that happened to be sitting in the > > register to be loaded into). > > Yes, that's (one reason) why you'd want to fill in a known value. Note > the "for now". :-) > > -Scott I think there is no overwhelming reason to fill the destination register with 0x. There's a small chance that 0x is treated as regular data rather than an error sign. Also setting this register may influence the user space under certain circumstance. So I think just ignore the skipped instruction is an acceptable option for this fix. -Hongtao. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH V4] powerpc/85xx: Add machine check handler to fix PCIe erratum on mpc85xx
> > Yes, that's (one reason) why you'd want to fill in a known value. Note > > the "for now". :-) > > > > -Scott > > I think there is no overwhelming reason to fill the destination register > with 0x. > > There's a small chance that 0x is treated as regular data rather > than an error sign. > > Also setting this register may influence the user space under certain > circumstance. > > So I think just ignore the skipped instruction is an acceptable option for > this fix. The 'random' value is just as likely to affect the reader, but only for some values - so you'll get almost impossible to repeat bugs. If a fixed value (0 or ~0) has an adverse effect, at least it will have the same every time. Read errors are also likely to affect device drivers reading status bits, since these are very likely 'write to clear' any driver would have to be willing to process the 'dummy' value in a manner that won't loop forever (especially in an ISR). You don't need every access to be via a function that explicitly (somehow) detects that the fault happened, but knowing that a specific value might be caused by a dead PCIe bus, and being able to find out whether that is true (to avoid looping forever) is probably useful. This is probably similar to what a driver needs to recover from an external PCIe list being unplugged. David ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH V4] powerpc/85xx: Add machine check handler to fix PCIe erratum on mpc85xx
> -Original Message-
> From: David Laight [mailto:david.lai...@aculab.com]
> Sent: Wednesday, March 06, 2013 6:24 PM
> To: Jia Hongtao-B38951; Wood Scott-B07421
> Cc: linuxppc-dev@lists.ozlabs.org; Stuart Yoder
> Subject: RE: [PATCH V4] powerpc/85xx: Add machine check handler to fix
> PCIe erratum on mpc85xx
>
> > > Yes, that's (one reason) why you'd want to fill in a known value.
> > > Note the "for now". :-)
> > >
> > > -Scott
> >
> > I think there is no overwhelming reason to fill the destination
> > register with 0x.
> >
> > There's a small chance that 0x is treated as regular data
> > rather than an error sign.
> >
> > Also setting this register may influence the user space under certain
> > circumstance.
> >
> > So I think just ignore the skipped instruction is an acceptable option
> > for this fix.
>
> The 'random' value is just as likely to affect the reader, but only for
> some values - so you'll get almost impossible to repeat bugs.
> If a fixed value (0 or ~0) has an adverse effect, at least it will have
> the same every time.
>
> Read errors are also likely to affect device drivers reading status bits,
> since these are very likely 'write to clear' any driver would have to be
> willing to process the 'dummy' value in a manner that won't loop forever
> (especially in an ISR).
>
> You don't need every access to be via a function that explicitly
> (somehow) detects that the fault happened, but knowing that a specific
> value might be caused by a dead PCIe bus, and being able to find out
> whether that is true (to avoid looping forever) is probably useful.
>
> This is probably similar to what a driver needs to recover from an
> external PCIe list being unplugged.
>
> David
>
>
In my understanding filling the register could warn the executing process
an error occurred in some cases. But no way to fix the wrong behavior caused
by the instruction lost. So let's say that filling the register may benefit
a little.
On the other side, we should not access to the addresses of unknown process
in Linux kernel. We must get the instruction before filling the register.
If the instruction is not in the cache we have to access to the unknown
addresses to get it. For system security I think this is strictly forbidden.
Here is the ideas from Scott:
"
> + if (is_in_pci_mem_space(addr)) {
> + inst = *(unsigned int *)regs->nip;
Be careful about taking a fault here. A simple TLB miss should be safe
given that we shouldn't be accessing PCIe in the middle of exception
code, but what if the mapping has gone away (e.g. a userspace driver had
its code munmap()ed or swapped out)? What if permissions allow execute
but not read (not sure if Linux will allow this, but the hardware does)?
What if it happened in a KVM guest? You can't access guest addresses
directly.
"
Although I think filling the register have some advantages but it's should
be forbidden for security reason.
-Hongtao.
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH V4] powerpc/85xx: Add machine check handler to fix PCIe erratum on mpc85xx
> In my understanding filling the register could warn the executing process > an error occurred in some cases. But no way to fix the wrong behavior caused > by the instruction lost. So let's say that filling the register may benefit > a little. IIRC the only ppc instructions that should be accessing PCIe space are simple memory reads/writes, locked exchanges probably don't work. writes will be async - so we are talking about memory reads. > On the other side, we should not access to the addresses of unknown process > in Linux kernel. We must get the instruction before filling the register. > If the instruction is not in the cache we have to access to the unknown > addresses to get it. For system security I think this is strictly forbidden. Eh??? The kernel fault handler will know whether the fault is from kernel or userspace (in which case it must be the current process), and will almost certainly already have code that looks at the faulting instruction sequence. If the faulting code address is 'user', then the normal functions for reading user addresses have to be used. If the faulting address is 'kernel' it might be in an ISR - restricting what can be done - but the instruction is definitely readable. > Although I think filling the register have some advantages but it's should > be forbidden for security reason. I'm sure security would say exactly the opposite. David ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH V4] powerpc/85xx: Add machine check handler to fix PCIe erratum on mpc85xx
On 03/07/2013 02:06:05 AM, Jia Hongtao-B38951 wrote:
Here is the ideas from Scott:
"
> + if (is_in_pci_mem_space(addr)) {
> + inst = *(unsigned int *)regs->nip;
Be careful about taking a fault here. A simple TLB miss should be
safe
given that we shouldn't be accessing PCIe in the middle of exception
code, but what if the mapping has gone away (e.g. a userspace driver
had
its code munmap()ed or swapped out)? What if permissions allow
execute
but not read (not sure if Linux will allow this, but the hardware
does)?
What if it happened in a KVM guest? You can't access guest addresses
directly.
"
That means you need to be careful about how you read the instruction,
not that you shouldn't do it at all.
-Scott
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH V4] powerpc/85xx: Add machine check handler to fix PCIe erratum on mpc85xx
> -Original Message-
> From: Wood Scott-B07421
> Sent: Friday, March 08, 2013 12:38 AM
> To: Jia Hongtao-B38951
> Cc: David Laight; Wood Scott-B07421; linuxppc-dev@lists.ozlabs.org;
> Stuart Yoder
> Subject: Re: [PATCH V4] powerpc/85xx: Add machine check handler to fix
> PCIe erratum on mpc85xx
>
> On 03/07/2013 02:06:05 AM, Jia Hongtao-B38951 wrote:
> > Here is the ideas from Scott:
> > "
> > > + if (is_in_pci_mem_space(addr)) {
> > > + inst = *(unsigned int *)regs->nip;
> >
> > Be careful about taking a fault here. A simple TLB miss should be
> > safe given that we shouldn't be accessing PCIe in the middle of
> > exception code, but what if the mapping has gone away (e.g. a
> > userspace driver had its code munmap()ed or swapped out)? What if
> > permissions allow execute but not read (not sure if Linux will allow
> > this, but the hardware does)?
> >
> > What if it happened in a KVM guest? You can't access guest addresses
> > directly.
> > "
>
> That means you need to be careful about how you read the instruction, not
> that you shouldn't do it at all.
>
> -Scott
I agree.
Do you have a more secure way to get the instruction?
Or what should be done to avoid permission break issue?
Thanks.
-Hongtao
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH V4] powerpc/85xx: Add machine check handler to fix PCIe erratum on mpc85xx
On 03/08/2013 02:01:46 AM, Jia Hongtao-B38951 wrote:
> -Original Message-
> From: Wood Scott-B07421
> Sent: Friday, March 08, 2013 12:38 AM
> To: Jia Hongtao-B38951
> Cc: David Laight; Wood Scott-B07421; linuxppc-dev@lists.ozlabs.org;
> Stuart Yoder
> Subject: Re: [PATCH V4] powerpc/85xx: Add machine check handler to
fix
> PCIe erratum on mpc85xx
>
> On 03/07/2013 02:06:05 AM, Jia Hongtao-B38951 wrote:
> > Here is the ideas from Scott:
> > "
> > > +if (is_in_pci_mem_space(addr)) {
> > > +inst = *(unsigned int *)regs->nip;
> >
> > Be careful about taking a fault here. A simple TLB miss should be
> > safe given that we shouldn't be accessing PCIe in the middle of
> > exception code, but what if the mapping has gone away (e.g. a
> > userspace driver had its code munmap()ed or swapped out)? What if
> > permissions allow execute but not read (not sure if Linux will
allow
> > this, but the hardware does)?
> >
> > What if it happened in a KVM guest? You can't access guest
addresses
> > directly.
> > "
>
> That means you need to be careful about how you read the
instruction, not
> that you shouldn't do it at all.
>
> -Scott
I agree.
Do you have a more secure way to get the instruction?
Or what should be done to avoid permission break issue?
probe_kernel_address() should take care of userspace issues. As for
KVM, if you see MSR_GS set, bail out and don't apply the workaround.
Let KVM/QEMU deal with it as it wishes (e.g. reflect to the guest and
let its machine check handler do the skipping). On PR-mode KVM (e.g.
on e500v2-based chips) there is no MSR_GS and it just looks like
userspace code -- for now just pretend it is user mode.
-Scott
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH V4] powerpc/85xx: Add machine check handler to fix PCIe erratum on mpc85xx
> -Original Message-
> From: Wood Scott-B07421
> Sent: Saturday, March 09, 2013 8:49 AM
> To: Jia Hongtao-B38951
> Cc: Wood Scott-B07421; David Laight; linuxppc-dev@lists.ozlabs.org;
> Stuart Yoder
> Subject: Re: [PATCH V4] powerpc/85xx: Add machine check handler to fix
> PCIe erratum on mpc85xx
>
> On 03/08/2013 02:01:46 AM, Jia Hongtao-B38951 wrote:
> >
> >
> > > -Original Message-
> > > From: Wood Scott-B07421
> > > Sent: Friday, March 08, 2013 12:38 AM
> > > To: Jia Hongtao-B38951
> > > Cc: David Laight; Wood Scott-B07421; linuxppc-dev@lists.ozlabs.org;
> > > Stuart Yoder
> > > Subject: Re: [PATCH V4] powerpc/85xx: Add machine check handler to
> > fix
> > > PCIe erratum on mpc85xx
> > >
> > > On 03/07/2013 02:06:05 AM, Jia Hongtao-B38951 wrote:
> > > > Here is the ideas from Scott:
> > > > "
> > > > > + if (is_in_pci_mem_space(addr)) {
> > > > > + inst = *(unsigned int *)regs->nip;
> > > >
> > > > Be careful about taking a fault here. A simple TLB miss should be
> > > > safe given that we shouldn't be accessing PCIe in the middle of
> > > > exception code, but what if the mapping has gone away (e.g. a
> > > > userspace driver had its code munmap()ed or swapped out)? What if
> > > > permissions allow execute but not read (not sure if Linux will
> > allow
> > > > this, but the hardware does)?
> > > >
> > > > What if it happened in a KVM guest? You can't access guest
> > addresses
> > > > directly.
> > > > "
> > >
> > > That means you need to be careful about how you read the
> > instruction, not
> > > that you shouldn't do it at all.
> > >
> > > -Scott
> >
> > I agree.
> >
> > Do you have a more secure way to get the instruction?
> > Or what should be done to avoid permission break issue?
>
> probe_kernel_address() should take care of userspace issues. As for
> KVM, if you see MSR_GS set, bail out and don't apply the workaround.
> Let KVM/QEMU deal with it as it wishes (e.g. reflect to the guest and
> let its machine check handler do the skipping). On PR-mode KVM (e.g.
> on e500v2-based chips) there is no MSR_GS and it just looks like
> userspace code -- for now just pretend it is user mode.
>
> -Scott
Hi Scott,
Is that OK if I use the following code?
u32 inst;
int ret;
if (is_in_pci_mem_space(addr)) {
if (!user_mode(regs)) {
ret = probe_kernel_address(regs->nip, inst);
if (!ret) {
rd = get_rt(inst);
regs->gpr[rd] = 0x;
}
}
regs->nip += 4;
return 1;
}
Thanks.
-Hongtao.
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH V4] powerpc/85xx: Add machine check handler to fix PCIe erratum on mpc85xx
> Is that OK if I use the following code?
...
> if (is_in_pci_mem_space(addr)) {
> if (!user_mode(regs)) {
> ret = probe_kernel_address(regs->nip, inst);
>
> if (!ret) {
> rd = get_rt(inst);
> regs->gpr[rd] = 0x;
> }
> }
Don't you need to check that the instruction is actually
a memory read?
I also know that there are people mapping PCIe addresses
directly into userspace for 'simple' access to things like
fpga devices.
I suspect that such devices are the ones likely to generate
the faulting cycles. So you probably do want to handle
faults from normal userspace addresses.
David
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH V4] powerpc/85xx: Add machine check handler to fix PCIe erratum on mpc85xx
On 03/12/2013 02:40:39 AM, Jia Hongtao-B38951 wrote:
> -Original Message-
> From: Wood Scott-B07421
> Sent: Saturday, March 09, 2013 8:49 AM
> To: Jia Hongtao-B38951
> Cc: Wood Scott-B07421; David Laight; linuxppc-dev@lists.ozlabs.org;
> Stuart Yoder
> Subject: Re: [PATCH V4] powerpc/85xx: Add machine check handler to
fix
> PCIe erratum on mpc85xx
>
> On 03/08/2013 02:01:46 AM, Jia Hongtao-B38951 wrote:
> >
> >
> > > -Original Message-
> > > From: Wood Scott-B07421
> > > Sent: Friday, March 08, 2013 12:38 AM
> > > To: Jia Hongtao-B38951
> > > Cc: David Laight; Wood Scott-B07421;
linuxppc-dev@lists.ozlabs.org;
> > > Stuart Yoder
> > > Subject: Re: [PATCH V4] powerpc/85xx: Add machine check handler
to
> > fix
> > > PCIe erratum on mpc85xx
> > >
> > > On 03/07/2013 02:06:05 AM, Jia Hongtao-B38951 wrote:
> > > > Here is the ideas from Scott:
> > > > "
> > > > > + if (is_in_pci_mem_space(addr)) {
> > > > > + inst = *(unsigned int *)regs->nip;
> > > >
> > > > Be careful about taking a fault here. A simple TLB miss
should be
> > > > safe given that we shouldn't be accessing PCIe in the middle
of
> > > > exception code, but what if the mapping has gone away (e.g. a
> > > > userspace driver had its code munmap()ed or swapped out)?
What if
> > > > permissions allow execute but not read (not sure if Linux will
> > allow
> > > > this, but the hardware does)?
> > > >
> > > > What if it happened in a KVM guest? You can't access guest
> > addresses
> > > > directly.
> > > > "
> > >
> > > That means you need to be careful about how you read the
> > instruction, not
> > > that you shouldn't do it at all.
> > >
> > > -Scott
> >
> > I agree.
> >
> > Do you have a more secure way to get the instruction?
> > Or what should be done to avoid permission break issue?
>
> probe_kernel_address() should take care of userspace issues. As for
> KVM, if you see MSR_GS set, bail out and don't apply the workaround.
> Let KVM/QEMU deal with it as it wishes (e.g. reflect to the guest
and
> let its machine check handler do the skipping). On PR-mode KVM
(e.g.
> on e500v2-based chips) there is no MSR_GS and it just looks like
> userspace code -- for now just pretend it is user mode.
>
> -Scott
Hi Scott,
Is that OK if I use the following code?
u32 inst;
int ret;
if (is_in_pci_mem_space(addr)) {
if (!user_mode(regs)) {
ret = probe_kernel_address(regs->nip, inst);
Hmm, seems there's no probe_user_address() -- for userspace we
basically want the same thing minus the KERNEL_DS. See
arch/powerpc/perf/callchain.c for an example.
You also need to skip this if (regs->msr & MSR_GS) as I mentioned above.
if (!ret) {
rd = get_rt(inst);
regs->gpr[rd] = 0x;
}
Check whether the instruction is a load, as David pointed out. Also
check the size of the load, whether it was load with update
instruction, etc.
-Scott
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH V4] powerpc/85xx: Add machine check handler to fix PCIe erratum on mpc85xx
> Hmm, seems there's no probe_user_address() -- for userspace we > basically want the same thing minus the KERNEL_DS. See > arch/powerpc/perf/callchain.c for an example. Isn't that just copy_from_user() ? David ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH V4] powerpc/85xx: Add machine check handler to fix PCIe erratum on mpc85xx
On 03/13/2013 04:40:40 AM, David Laight wrote: > Hmm, seems there's no probe_user_address() -- for userspace we > basically want the same thing minus the KERNEL_DS. See > arch/powerpc/perf/callchain.c for an example. Isn't that just copy_from_user() ? Plus pagefault_disable/enable(). -Scott ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH V4] powerpc/85xx: Add machine check handler to fix PCIe erratum on mpc85xx
> -Original Message- > From: Wood Scott-B07421 > Sent: Thursday, March 14, 2013 12:38 AM > To: David Laight > Cc: Jia Hongtao-B38951; Wood Scott-B07421; linuxppc-dev@lists.ozlabs.org; > Stuart Yoder > Subject: Re: [PATCH V4] powerpc/85xx: Add machine check handler to fix > PCIe erratum on mpc85xx > > On 03/13/2013 04:40:40 AM, David Laight wrote: > > > Hmm, seems there's no probe_user_address() -- for userspace we > > > basically want the same thing minus the KERNEL_DS. See > > > arch/powerpc/perf/callchain.c for an example. > > > > Isn't that just copy_from_user() ? > > Plus pagefault_disable/enable(). > > -Scott pagefault_disable() is identical to preempt_disable(). So I think this could not avoid other cpu to swap out the instruction we want to read back. probe_kernel_address() also have the same issue. Does this mean we can't just use pagefault_disable() to prevent from getting the wrong instruction? -Hongtao. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH V4] powerpc/85xx: Add machine check handler to fix PCIe erratum on mpc85xx
On 03/14/2013 09:47:58 PM, Jia Hongtao-B38951 wrote: > -Original Message- > From: Wood Scott-B07421 > Sent: Thursday, March 14, 2013 12:38 AM > To: David Laight > Cc: Jia Hongtao-B38951; Wood Scott-B07421; linuxppc-dev@lists.ozlabs.org; > Stuart Yoder > Subject: Re: [PATCH V4] powerpc/85xx: Add machine check handler to fix > PCIe erratum on mpc85xx > > On 03/13/2013 04:40:40 AM, David Laight wrote: > > > Hmm, seems there's no probe_user_address() -- for userspace we > > > basically want the same thing minus the KERNEL_DS. See > > > arch/powerpc/perf/callchain.c for an example. > > > > Isn't that just copy_from_user() ? > > Plus pagefault_disable/enable(). > > -Scott pagefault_disable() is identical to preempt_disable(). So I think this could not avoid other cpu to swap out the instruction we want to read back. probe_kernel_address() also have the same issue. That's not the point -- the point is to let the page fault handler know that it should go directly to bad_page_fault(). Do not pass handle_mm_fault(). Do not collect a page from disk. Granted, we're already in atomic context which will have that effect due to being in the machine check handler, but it's better to be explicit about it and not depend on how pagefault_diasble() is implemented. -Scott ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH V4] powerpc/85xx: Add machine check handler to fix PCIe erratum on mpc85xx
> -Original Message-
> From: Wood Scott-B07421
> Sent: Saturday, March 16, 2013 12:35 AM
> To: Jia Hongtao-B38951
> Cc: Wood Scott-B07421; David Laight; linuxppc-dev@lists.ozlabs.org;
> Stuart Yoder
> Subject: Re: [PATCH V4] powerpc/85xx: Add machine check handler to fix
> PCIe erratum on mpc85xx
>
> On 03/14/2013 09:47:58 PM, Jia Hongtao-B38951 wrote:
> >
> > > -Original Message-
> > > From: Wood Scott-B07421
> > > Sent: Thursday, March 14, 2013 12:38 AM
> > > To: David Laight
> > > Cc: Jia Hongtao-B38951; Wood Scott-B07421;
> > linuxppc-dev@lists.ozlabs.org;
> > > Stuart Yoder
> > > Subject: Re: [PATCH V4] powerpc/85xx: Add machine check handler to
> > fix
> > > PCIe erratum on mpc85xx
> > >
> > > On 03/13/2013 04:40:40 AM, David Laight wrote:
> > > > > Hmm, seems there's no probe_user_address() -- for userspace we
> > > > > basically want the same thing minus the KERNEL_DS. See
> > > > > arch/powerpc/perf/callchain.c for an example.
> > > >
> > > > Isn't that just copy_from_user() ?
> > >
> > > Plus pagefault_disable/enable().
> > >
> > > -Scott
> >
> > pagefault_disable() is identical to preempt_disable(). So I think this
> > could not avoid other cpu to swap out the instruction we want to read
> > back.
> > probe_kernel_address() also have the same issue.
>
> That's not the point -- the point is to let the page fault handler know
> that it should go directly to bad_page_fault(). Do not pass
> handle_mm_fault(). Do not collect a page from disk.
>
> Granted, we're already in atomic context which will have that effect
> due to being in the machine check handler, but it's better to be
> explicit about it and not depend on how pagefault_diasble() is
> implemented.
>
> -Scott
Based on the comments I updated the machine check handler.
Changes from last version:
* Check MSR_GS state
* Check if the instruction is LD
* Handle the user space issue
The updated machine check handler is as following:
int fsl_pci_mcheck_exception(struct pt_regs *regs)
{
unsigned int op, rd;
u32 inst;
int ret;
phys_addr_t addr = 0;
/* Let KVM/QEMU deal with the exception */
if (regs->msr & MSR_GS)
return 0;
#ifdef CONFIG_PHYS_64BIT
addr = mfspr(SPRN_MCARU);
addr <<= 32;
#endif
addr += mfspr(SPRN_MCAR);
if (is_in_pci_mem_space(addr)) {
if (user_mode(regs)) {
pagefault_disable();
ret = copy_from_user(&(inst), (u32 __user *)regs->nip,
sizeof(inst));
pagefault_enable();
} else {
ret = probe_kernel_address(regs->nip, inst);
}
op = get_op(inst);
/* Check if the instruction is LD */
if (!ret && (op == 111010)) {
rd = get_rt(inst);
regs->gpr[rd] = 0x;
}
regs->nip += 4;
return 1;
}
return 0;
}
BTW, I'm still not sure how to deal with LD instruction with update.
Any comments and suggestions are welcomed.
Thanks.
-Hongtao.
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH V4] powerpc/85xx: Add machine check handler to fix PCIe erratum on mpc85xx
On 03/29/2013 03:03:51 AM, Jia Hongtao-B38951 wrote:
> -Original Message-
> From: Wood Scott-B07421
> Sent: Saturday, March 16, 2013 12:35 AM
> To: Jia Hongtao-B38951
> Cc: Wood Scott-B07421; David Laight; linuxppc-dev@lists.ozlabs.org;
> Stuart Yoder
> Subject: Re: [PATCH V4] powerpc/85xx: Add machine check handler to
fix
> PCIe erratum on mpc85xx
>
> On 03/14/2013 09:47:58 PM, Jia Hongtao-B38951 wrote:
> >
> > > -Original Message-
> > > From: Wood Scott-B07421
> > > Sent: Thursday, March 14, 2013 12:38 AM
> > > To: David Laight
> > > Cc: Jia Hongtao-B38951; Wood Scott-B07421;
> > linuxppc-dev@lists.ozlabs.org;
> > > Stuart Yoder
> > > Subject: Re: [PATCH V4] powerpc/85xx: Add machine check handler
to
> > fix
> > > PCIe erratum on mpc85xx
> > >
> > > On 03/13/2013 04:40:40 AM, David Laight wrote:
> > > > > Hmm, seems there's no probe_user_address() -- for userspace
we
> > > > > basically want the same thing minus the KERNEL_DS. See
> > > > > arch/powerpc/perf/callchain.c for an example.
> > > >
> > > > Isn't that just copy_from_user() ?
> > >
> > > Plus pagefault_disable/enable().
> > >
> > > -Scott
> >
> > pagefault_disable() is identical to preempt_disable(). So I think
this
> > could not avoid other cpu to swap out the instruction we want to
read
> > back.
> > probe_kernel_address() also have the same issue.
>
> That's not the point -- the point is to let the page fault handler
know
> that it should go directly to bad_page_fault(). Do not pass
> handle_mm_fault(). Do not collect a page from disk.
>
> Granted, we're already in atomic context which will have that effect
> due to being in the machine check handler, but it's better to be
> explicit about it and not depend on how pagefault_diasble() is
> implemented.
>
> -Scott
Based on the comments I updated the machine check handler.
Changes from last version:
* Check MSR_GS state
* Check if the instruction is LD
* Handle the user space issue
The updated machine check handler is as following:
int fsl_pci_mcheck_exception(struct pt_regs *regs)
{
unsigned int op, rd;
u32 inst;
int ret;
phys_addr_t addr = 0;
/* Let KVM/QEMU deal with the exception */
if (regs->msr & MSR_GS)
return 0;
#ifdef CONFIG_PHYS_64BIT
addr = mfspr(SPRN_MCARU);
addr <<= 32;
#endif
addr += mfspr(SPRN_MCAR);
if (is_in_pci_mem_space(addr)) {
if (user_mode(regs)) {
pagefault_disable();
ret = copy_from_user(&(inst), (u32 __user
*)regs->nip, sizeof(inst));
pagefault_enable();
You could use get_user() instead of copy_from_user().
} else {
ret = probe_kernel_address(regs->nip, inst);
}
op = get_op(inst);
/* Check if the instruction is LD */
if (!ret && (op == 111010)) {
rd = get_rt(inst);
regs->gpr[rd] = 0x;
}
regs->nip += 4;
return 1;
}
return 0;
}
BTW, I'm still not sure how to deal with LD instruction with update.
You would need to do the update yourself. Or just say that's a case
you don't handle, and return 0.
Again, please check for the size of the load operation.
-Scott
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH V4] powerpc/85xx: Add machine check handler to fix PCIe erratum on mpc85xx
> -Original Message-
> From: Wood Scott-B07421
> Sent: Saturday, March 30, 2013 12:34 AM
> To: Jia Hongtao-B38951
> Cc: Wood Scott-B07421; David Laight; linuxppc-dev@lists.ozlabs.org;
> Stuart Yoder
> Subject: Re: [PATCH V4] powerpc/85xx: Add machine check handler to fix
> PCIe erratum on mpc85xx
>
> On 03/29/2013 03:03:51 AM, Jia Hongtao-B38951 wrote:
> >
> >
> > > -Original Message-
> > > From: Wood Scott-B07421
> > > Sent: Saturday, March 16, 2013 12:35 AM
> > > To: Jia Hongtao-B38951
> > > Cc: Wood Scott-B07421; David Laight; linuxppc-dev@lists.ozlabs.org;
> > > Stuart Yoder
> > > Subject: Re: [PATCH V4] powerpc/85xx: Add machine check handler to
> > fix
> > > PCIe erratum on mpc85xx
> > >
> > > On 03/14/2013 09:47:58 PM, Jia Hongtao-B38951 wrote:
> > > >
> > > > > -Original Message-
> > > > > From: Wood Scott-B07421
> > > > > Sent: Thursday, March 14, 2013 12:38 AM
> > > > > To: David Laight
> > > > > Cc: Jia Hongtao-B38951; Wood Scott-B07421;
> > > > linuxppc-dev@lists.ozlabs.org;
> > > > > Stuart Yoder
> > > > > Subject: Re: [PATCH V4] powerpc/85xx: Add machine check handler
> > to
> > > > fix
> > > > > PCIe erratum on mpc85xx
> > > > >
> > > > > On 03/13/2013 04:40:40 AM, David Laight wrote:
> > > > > > > Hmm, seems there's no probe_user_address() -- for userspace
> > we
> > > > > > > basically want the same thing minus the KERNEL_DS. See
> > > > > > > arch/powerpc/perf/callchain.c for an example.
> > > > > >
> > > > > > Isn't that just copy_from_user() ?
> > > > >
> > > > > Plus pagefault_disable/enable().
> > > > >
> > > > > -Scott
> > > >
> > > > pagefault_disable() is identical to preempt_disable(). So I think
> > this
> > > > could not avoid other cpu to swap out the instruction we want to
> > read
> > > > back.
> > > > probe_kernel_address() also have the same issue.
> > >
> > > That's not the point -- the point is to let the page fault handler
> > know
> > > that it should go directly to bad_page_fault(). Do not pass
> > > handle_mm_fault(). Do not collect a page from disk.
> > >
> > > Granted, we're already in atomic context which will have that effect
> > > due to being in the machine check handler, but it's better to be
> > > explicit about it and not depend on how pagefault_diasble() is
> > > implemented.
> > >
> > > -Scott
> >
> >
> > Based on the comments I updated the machine check handler.
> >
> > Changes from last version:
> > * Check MSR_GS state
> > * Check if the instruction is LD
> > * Handle the user space issue
> >
> > The updated machine check handler is as following:
> >
> > int fsl_pci_mcheck_exception(struct pt_regs *regs) {
> > unsigned int op, rd;
> > u32 inst;
> > int ret;
> > phys_addr_t addr = 0;
> >
> > /* Let KVM/QEMU deal with the exception */
> > if (regs->msr & MSR_GS)
> > return 0;
> >
> > #ifdef CONFIG_PHYS_64BIT
> > addr = mfspr(SPRN_MCARU);
> > addr <<= 32;
> > #endif
> > addr += mfspr(SPRN_MCAR);
> >
> > if (is_in_pci_mem_space(addr)) {
> > if (user_mode(regs)) {
> > pagefault_disable();
> > ret = copy_from_user(&(inst), (u32 __user
> > *)regs->nip, sizeof(inst));
> > pagefault_enable();
>
> You could use get_user() instead of copy_from_user().
Got it.
>
> > } else {
> > ret = probe_kernel_address(regs->nip, inst);
> > }
> >
> > op = get_op(inst);
> > /* Check if the instruction is LD */
> > if (!ret && (op == 111010)) {
> > rd = get_rt(inst);
> > regs->gpr[rd] = 0x;
> > }
> >
> > regs->nip += 4;
> > return 1;
> > }
> >
> > return 0;
> > }
> >
> > BTW, I'm still not sure how to deal
Re: [PATCH V4] powerpc/85xx: Add machine check handler to fix PCIe erratum on mpc85xx
On 04/02/2013 04:28:10 AM, Jia Hongtao-B38951 wrote: > -Original Message- > From: Wood Scott-B07421 > Sent: Saturday, March 30, 2013 12:34 AM > To: Jia Hongtao-B38951 > Cc: Wood Scott-B07421; David Laight; linuxppc-dev@lists.ozlabs.org; > Stuart Yoder > Subject: Re: [PATCH V4] powerpc/85xx: Add machine check handler to fix > PCIe erratum on mpc85xx > > On 03/29/2013 03:03:51 AM, Jia Hongtao-B38951 wrote: > > BTW, I'm still not sure how to deal with LD instruction with update. > > You would need to do the update yourself. Or just say that's a case you > don't handle, and return 0. > > Again, please check for the size of the load operation. > > -Scott For informing error to the process that hold the stall instruction we need to do: 1. Verify the instruction is load. 2. Fill the rd register with ~0UL. 3. Deal with the load instruction with update. Here is the problems: 1. So many load instructions to handle. There are dozens of load instructions and most of them with different op code. Like: If you don't want to handle all of them, then don't, but in case you run into an instruction you don't handle, don't skip it -- just let the normal machine check handler run. lbz: 1 0 0 0 1 0 lhz: 1 0 1 0 0 0 lwz: 1 0 0 0 0 0 ld : 1 1 1 0 1 0 ... Is there any available API for verifying the load instruction? I don't know of anything in terms of an *API*... after all, you're not just "verifying" it, you're extracting information to determine how to emulate the instruction. As for code you could borrow from, there's KVM emulation and probably other places. 2. For different size of load operation could we just fill the rd register with ~0UL? Who knows in what ways the compiler is making assumptions about the upper bits being zero after an lbz, etc... 3. A load instruction with update could not just verified by op code. I'd like to leave it along. I think we could not fix but just inform the error by filling the rd with ~0UL. Could you explain why should we care about the update? If you're emulating the instruction, you need to handle all of that instruction's effects. If you're not going to emulate the instruction, don't skip it. -Scott ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
