| Message ID | Pine.LNX.4.64.1311041651390.4290@digraph.polyomino.org.uk (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Scott Wood |
| Headers | show |
On Mon, 2013-11-04 at 16:52 +0000, Joseph S. Myers wrote: > From: Joseph Myers <joseph@codesourcery.com> > > The e500 SPE floating-point emulation code clears existing exceptions > (__FPU_FPSCR &= ~FP_EX_MASK;) before ORing in the exceptions from the > emulated operation. However, these exception bits are the "sticky", > cumulative exception bits, and should only be cleared by the user > program setting SPEFSCR, not implicitly by any floating-point > instruction (whether executed purely by the hardware or emulated). > The spurious clearing of these bits shows up as missing exceptions in > glibc testing. > > Fixing this, however, is not as simple as just not clearing the bits, > because while the bits may be from previous floating-point operations > (in which case they should not be cleared), the processor can also set > the sticky bits itself before the interrupt for an exception occurs, > and this can happen in cases when IEEE 754 semantics are that the > sticky bit should not be set. Specifically, the "invalid" sticky bit > is set in various cases with non-finite operands, where IEEE 754 > semantics do not involve raising such an exception, and the > "underflow" sticky bit is set in cases of exact underflow, whereas > IEEE 754 semantics are that this flag is set only for inexact > underflow. Thus, for correct emulation the kernel needs to know the > setting of these two sticky bits before the instruction being > emulated. > > When a floating-point operation raises an exception, the kernel can > note the state of the sticky bits immediately afterwards. Some > <fenv.h> functions that affect the state of these bits, such as > fesetenv and feholdexcept, need to use prctl with PR_GET_FPEXC and > PR_SET_FPEXC anyway, and so it is natural to record the state of those > bits during that call into the kernel and so avoid any need for a > separate call into the kernel to inform it of a change to those bits. > Thus, the interface I chose to use (in this patch and the glibc port) > is that one of those prctl calls must be made after any userspace > change to those sticky bits, other than through a floating-point > operation that traps into the kernel anyway. This sounds like an incompatible change to userspace API. What about older glibc? What about user code that directly manipulates these bits rather than going through libc, or uses a libc other than glibc? Where is this API requirement documented? I think the impact of this could be reduced by using this mechanism only to clear bits, rather than set them. That is, if the exception bit is unset, don't set it just because it's set in spefscr_last -- but if it's not set in spefscr_last, and the emulation code doesn't want to set it, then clear it. Are there any cases where the exception bit can be set without the kernel taking a trap, or is userspace manipulation limited to clearing the bits? -Scott
On Fri, 22 Nov 2013, Scott Wood wrote: > This sounds like an incompatible change to userspace API. What about > older glibc? What about user code that directly manipulates these bits > rather than going through libc, or uses a libc other than glibc? Where > is this API requirement documented? The previous EGLIBC port, and the uClibc code copied from it, is fundamentally broken as regards any use of prctl for floating-point exceptions because it didn't use the PR_FP_EXC_SW_ENABLE bit in its prctl calls (and did various worse things, such as passing a pointer when prctl expected an integer). If you avoid anything where prctl is used, the clearing of sticky bits still means it will never give anything approximating correct exception semantics with existing kernels. I don't believe the patch makes things any worse for existing code that doesn't try to inform the kernel of changes to sticky bits - such code may get incorrect exceptions in some cases, but it would have done so anyway in other cases. This is the best API I could come up with to fix the fundamentally broken nature of what came before, taking into account that in many cases a prctl call is already needed along with userspace manipulation of exception bits. I'm not aware of any kernel documentation where this sort of subarchitecture-specific API detail is documented. (The API also includes such things as needing to leave the spefscr trap-enable bits set and use prctl to control whether SIGFPE results from exceptions.) > I think the impact of this could be reduced by using this mechanism only > to clear bits, rather than set them. That is, if the exception bit is > unset, don't set it just because it's set in spefscr_last -- but if it's > not set in spefscr_last, and the emulation code doesn't want to set it, > then clear it. It should already be the case in this patch that if a bit is clear in spefscr, and set in spefscr_last (i.e. userspace did not inform the kernel of clearing the bit, and no traps since then have resulted in the kernel noticing it was cleared), it won't get set unless the emulation code wants to set it. The sole place spefscr_last is read is in the statement "__FPU_FPSCR &= ~(FP_EX_INVALID | FP_EX_UNDERFLOW) | current->thread.spefscr_last;" - if the bit is already clear in spefscr, this statement has no effect on it. > Are there any cases where the exception bit can be set without the > kernel taking a trap, or is userspace manipulation limited to clearing > the bits? Userspace can both set and clear the bits without a trap. For example, fesetenv restores a saved value of spefscr which may both set and clear bits (and then it calls prctl because it needs to do so anyway to restore the saved state for which exceptions were enabled). fesetexceptflag restores saved state of particular exceptions without a trap (so needs to call prctl specially to inform the kernel of a change).
On Sat, 2013-11-23 at 01:22 +0000, Joseph S. Myers wrote: > On Fri, 22 Nov 2013, Scott Wood wrote: > > > This sounds like an incompatible change to userspace API. What about > > older glibc? What about user code that directly manipulates these bits > > rather than going through libc, or uses a libc other than glibc? Where > > is this API requirement documented? > > The previous EGLIBC port, and the uClibc code copied from it, is > fundamentally broken as regards any use of prctl for floating-point > exceptions because it didn't use the PR_FP_EXC_SW_ENABLE bit in its prctl > calls (and did various worse things, such as passing a pointer when prctl > expected an integer). If you avoid anything where prctl is used, the > clearing of sticky bits still means it will never give anything > approximating correct exception semantics with existing kernels. I don't > believe the patch makes things any worse for existing code that doesn't > try to inform the kernel of changes to sticky bits - such code may get > incorrect exceptions in some cases, but it would have done so anyway in > other cases. OK -- please mention this in the changelog. > This is the best API I could come up with to fix the fundamentally broken > nature of what came before, taking into account that in many cases a prctl > call is already needed along with userspace manipulation of exception > bits. I'm not aware of any kernel documentation where this sort of > subarchitecture-specific API detail is documented. (The API also includes > such things as needing to leave the spefscr trap-enable bits set and use > prctl to control whether SIGFPE results from exceptions.) I don't know of a formal place for it, but there should at least be a code comment somewhere. > > I think the impact of this could be reduced by using this mechanism only > > to clear bits, rather than set them. That is, if the exception bit is > > unset, don't set it just because it's set in spefscr_last -- but if it's > > not set in spefscr_last, and the emulation code doesn't want to set it, > > then clear it. > > It should already be the case in this patch that if a bit is clear in > spefscr, and set in spefscr_last (i.e. userspace did not inform the kernel > of clearing the bit, and no traps since then have resulted in the kernel > noticing it was cleared), it won't get set unless the emulation code wants > to set it. The sole place spefscr_last is read is in the statement > "__FPU_FPSCR &= ~(FP_EX_INVALID | FP_EX_UNDERFLOW) | > current->thread.spefscr_last;" - if the bit is already clear in spefscr, > this statement has no effect on it. OK -- I must have misread it before. -Scott
Hi, I am working with an p4080 based board. I am trying to get 2 PCIE controllers probed properly. In uboot I have no problems scanning and discovering what is connected to both controllers/PCI bridges. For both PCIE1/2 uboot sets up the Primary, secondary and Subordinate bus numbers to 0,1,1 respectively. When linux boots up and probes the controllers, PCIE1 is probed and the bridge scanned properly but PCIE2 is probed at the bridge but not attempted a scan. I see this message "pci 0001:02:00.0: bridge configuration invalid ([bus 01-01]), reconfiguring " I updated uboot to set the secondary and subordinate numbers to 2 (left the primary number to 0) and a subsequent kernel boot scanned the bus for PCIE2 successfully. I found these numbers to be very critical since the device tree blob (bus-range) for pci is also based off these. I'd like to get a good fix rather than the uboot hack and get better understanding of the problem. If there are any pointers someone could provide it would be awesome. Thank you Regards Ruchika
Any ideas/pointers anyone ? Thank you On 12/6/2013 6:48 PM, Ruchika wrote: > > Hi, > I am working with an p4080 based board. I am trying to get 2 PCIE > controllers probed properly. > > In uboot I have no problems scanning and discovering what is connected > to both controllers/PCI bridges. > > For both PCIE1/2 uboot sets up the Primary, secondary and Subordinate > bus numbers to 0,1,1 respectively. > > When linux boots up and probes the controllers, PCIE1 is probed and the > bridge scanned properly but PCIE2 is probed at the bridge but not > attempted a scan. > I see this message > "pci 0001:02:00.0: bridge configuration invalid ([bus 01-01]), > reconfiguring > " > > I updated uboot to set the secondary and subordinate numbers to 2 (left > the primary number to 0) and a subsequent kernel boot scanned the bus > for PCIE2 successfully. > I found these numbers to be very critical since the device tree blob > (bus-range) for pci is also based off these. > > I'd like to get a good fix rather than the uboot hack and get better > understanding of the problem. If there are any pointers someone could > provide it would be awesome. > > Thank you > Regards > Ruchika > > >
On Fri, 2013-12-06 at 18:48 -0600, Ruchika wrote: > Hi, > I am working with an p4080 based board. I am trying to get 2 PCIE > controllers probed properly. > > In uboot I have no problems scanning and discovering what is connected > to both controllers/PCI bridges. > > For both PCIE1/2 uboot sets up the Primary, secondary and Subordinate > bus numbers to 0,1,1 respectively. > > When linux boots up and probes the controllers, PCIE1 is probed and the > bridge scanned properly but PCIE2 is probed at the bridge but not > attempted a scan. > I see this message > "pci 0001:02:00.0: bridge configuration invalid ([bus 01-01]), reconfiguring > " > > I updated uboot to set the secondary and subordinate numbers to 2 (left > the primary number to 0) and a subsequent kernel boot scanned the bus > for PCIE2 successfully. > I found these numbers to be very critical since the device tree blob > (bus-range) for pci is also based off these. > > I'd like to get a good fix rather than the uboot hack and get better > understanding of the problem. If there are any pointers someone could > provide it would be awesome. This is the code that prints that: /* Check if setup is sensible at all */ if (!pass && (primary != bus->number || secondary <= bus->number || secondary > subordinate)) { dev_info(&dev->dev, "bridge configuration invalid ([bus %02x-%0 secondary, subordinate); broken = 1; } Start by printing out more information to determine which of those checks is failing (e.g. what is primary and bus->number). If it turns out that U-Boot is configuring the PCI bus incorrectly, send e-mail to the U-Boot list. Be sure to mention what version of Linux and U-Boot you're using. -Scott
diff --git a/arch/powerpc/include/asm/processor.h b/arch/powerpc/include/asm/processor.h index ce4de5a..0b02e23 100644 --- a/arch/powerpc/include/asm/processor.h +++ b/arch/powerpc/include/asm/processor.h @@ -237,6 +237,8 @@ struct thread_struct { unsigned long evr[32]; /* upper 32-bits of SPE regs */ u64 acc; /* Accumulator */ unsigned long spefscr; /* SPE & eFP status */ + unsigned long spefscr_last; /* SPEFSCR value on last prctl + call or trap return */ int used_spe; /* set if process has used spe */ #endif /* CONFIG_SPE */ #ifdef CONFIG_PPC_TRANSACTIONAL_MEM @@ -303,7 +305,9 @@ struct thread_struct { (_ALIGN_UP(sizeof(init_thread_info), 16) + (unsigned long) &init_stack) #ifdef CONFIG_SPE -#define SPEFSCR_INIT .spefscr = SPEFSCR_FINVE | SPEFSCR_FDBZE | SPEFSCR_FUNFE | SPEFSCR_FOVFE, +#define SPEFSCR_INIT \ + .spefscr = SPEFSCR_FINVE | SPEFSCR_FDBZE | SPEFSCR_FUNFE | SPEFSCR_FOVFE, \ + .spefscr_last = SPEFSCR_FINVE | SPEFSCR_FDBZE | SPEFSCR_FUNFE | SPEFSCR_FOVFE, #else #define SPEFSCR_INIT #endif diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c index 96d2fdf..e3b91f1 100644 --- a/arch/powerpc/kernel/process.c +++ b/arch/powerpc/kernel/process.c @@ -1151,6 +1151,7 @@ int set_fpexc_mode(struct task_struct *tsk, unsigned int val) if (val & PR_FP_EXC_SW_ENABLE) { #ifdef CONFIG_SPE if (cpu_has_feature(CPU_FTR_SPE)) { + tsk->thread.spefscr_last = mfspr(SPRN_SPEFSCR); tsk->thread.fpexc_mode = val & (PR_FP_EXC_SW_ENABLE | PR_FP_ALL_EXCEPT); return 0; @@ -1182,9 +1183,10 @@ int get_fpexc_mode(struct task_struct *tsk, unsigned long adr) if (tsk->thread.fpexc_mode & PR_FP_EXC_SW_ENABLE) #ifdef CONFIG_SPE - if (cpu_has_feature(CPU_FTR_SPE)) + if (cpu_has_feature(CPU_FTR_SPE)) { + tsk->thread.spefscr_last = mfspr(SPRN_SPEFSCR); val = tsk->thread.fpexc_mode; - else + } else return -EINVAL; #else return -EINVAL; diff --git a/arch/powerpc/math-emu/math_efp.c b/arch/powerpc/math-emu/math_efp.c index a73f088..59835c6 100644 --- a/arch/powerpc/math-emu/math_efp.c +++ b/arch/powerpc/math-emu/math_efp.c @@ -630,9 +630,27 @@ update_ccr: regs->ccr |= (IR << ((7 - ((speinsn >> 23) & 0x7)) << 2)); update_regs: - __FPU_FPSCR &= ~FP_EX_MASK; + /* + * If the "invalid" exception sticky bit was set by the + * processor for non-finite input, but was not set before the + * instruction being emulated, clear it. Likewise for the + * "underflow" bit, which may have been set by the processor + * for exact underflow, not just inexact underflow when the + * flag should be set for IEEE 754 semantics. Other sticky + * exceptions will only be set by the processor when they are + * correct according to IEEE 754 semantics, and we must not + * clear sticky bits that were already set before the emulated + * instruction as they represent the user-visible sticky + * exception status. "inexact" traps to kernel are not + * required for IEEE semantics and are not enabled by default, + * so the "inexact" sticky bit may have been set by a previous + * instruction without the kernel being aware of it. + */ + __FPU_FPSCR + &= ~(FP_EX_INVALID | FP_EX_UNDERFLOW) | current->thread.spefscr_last; __FPU_FPSCR |= (FP_CUR_EXCEPTIONS & FP_EX_MASK); mtspr(SPRN_SPEFSCR, __FPU_FPSCR); + current->thread.spefscr_last = __FPU_FPSCR; current->thread.evr[fc] = vc.wp[0]; regs->gpr[fc] = vc.wp[1];