Warning: unpredictable: identical transfer and status registers --`stxr w4,x5,[x4] using aarch64 poky gcc 8.3

Message ID DB7PR04MB4490DCC3F3454AB084EA1DC088660@DB7PR04MB4490.eurprd04.prod.outlook.com
State New
Headers show
Series
  • Warning: unpredictable: identical transfer and status registers --`stxr w4,x5,[x4] using aarch64 poky gcc 8.3
Related show

Commit Message

Peng Fan Feb. 13, 2019, 1:44 p.m.
Hi,

We met an issue when building a piece jailhouse hypervisor code, "stxr   %w0, %3, %2\n\t" is 
compiled as "stxr w4,x5,[x4]" which triggers the warning
"Warning: unpredictable: identical transfer and status registers"
After folder the do while into asm code, it is compiled as "stxr w1, x4, [x2]", no warning.

I think "stxr w4,x5,[x4]" is wrong according to arm64 spec.

aarch64 poky gcc 8.3 triggers warning, aarch64 poky gcc 7.3 does not trigger warning, but has generated same machine code. 
Not sure this is bug or not, is my patch the correct fix? 
please help.

Detailed info below, the jailhouse repo code: 
https://github.com/siemens/jailhouse/blob/master/hypervisor/arch/arm64/include/asm/bitops.h#L77

static inline int test_and_set_bit(int nr, volatile unsigned long *addr) {
        u32 ret;
        u64 test, tmp;

        BITOPT_ALIGN(nr, addr);

        /* AARCH64_TODO: using Inner Shareable DMB at the moment,
         * revisit when we will deal with shareability domains */

        do {
                asm volatile (
                        "ldxr   %3, %2\n\t"
                        "ands   %1, %3, %4\n\t"
                        "b.ne   1f\n\t"
                        "orr    %3, %3, %4\n\t"
                        "1:\n\t"
                        "stxr   %w0, %3, %2\n\t"
                        "dmb    ish\n\t"
                        : "=r" (ret), "=&r" (test),
                          "+Q" (*(volatile unsigned long *)addr),
                          "=r" (tmp)
                        : "r" (1ul << nr));
        } while (ret);
        return !!(test);
}

void panic_printk(const char *fmt, ...)
{
        unsigned long cpu_id = phys_processor_id();
        va_list ap;

        if (test_and_set_bit(0, &panic_in_progress) && panic_cpu != cpu_id)
                return;
        panic_cpu = cpu_id;

        va_start(ap, fmt);

        __vprintk(fmt, ap);

        va_end(ap);
}

The asm code:
0000ffffc0207a94 <panic_printk>:
    ffffc0207a94:       a9b67bfd        stp     x29, x30, [sp, #-160]!
    ffffc0207a98:       910003fd        mov     x29, sp
    ffffc0207a9c:       f9000bf3        str     x19, [sp, #16]
    ffffc0207aa0:       aa0003f3        mov     x19, x0
    ffffc0207aa4:       a9068ba1        stp     x1, x2, [x29, #104]
    ffffc0207aa8:       a90793a3        stp     x3, x4, [x29, #120]
    ffffc0207aac:       a9089ba5        stp     x5, x6, [x29, #136]
    ffffc0207ab0:       f9004fa7        str     x7, [x29, #152]
    ffffc0207ab4:       97ffed27        bl      ffffc0202f50 <phys_processor_id>
    ffffc0207ab8:       f0000081        adrp    x1, ffffc021a000 <system_config>
    ffffc0207abc:       d2800022        mov     x2, #0x1                        // #1
    ffffc0207ac0:       91006024        add     x4, x1, #0x18
    ffffc0207ac4:       c85f7c85        ldxr    x5, [x4]
    ffffc0207ac8:       ea0200a3        ands    x3, x5, x2
    ffffc0207acc:       54000041        b.ne    ffffc0207ad4 <panic_printk+0x40>  // b.any
    ffffc0207ad0:       aa0200a5        orr     x5, x5, x2
    ffffc0207ad4:       c8047c85        stxr    w4, x5, [x4]
    ffffc0207ad8:       d5033bbf        dmb     ish
    ffffc0207adc:       35ffff24        cbnz    w4, ffffc0207ac0 <panic_printk+0x2c>

Note pc ffffc0207ad4 has instruction stxr w4, x5, [x4] which is triggers warning using aarch64 poky gcc 8.3

I did a fix to the code as below:
Then the asm code as below:
0000ffffc02079e0 <panic_printk>:
    ffffc02079e0:       a9b67bfd        stp     x29, x30, [sp, #-160]!
    ffffc02079e4:       910003fd        mov     x29, sp
    ffffc02079e8:       f9000bf3        str     x19, [sp, #16]
    ffffc02079ec:       aa0003f3        mov     x19, x0
    ffffc02079f0:       a9068be1        stp     x1, x2, [sp, #104]
    ffffc02079f4:       a90793e3        stp     x3, x4, [sp, #120]
    ffffc02079f8:       a9089be5        stp     x5, x6, [sp, #136]
    ffffc02079fc:       f9004fe7        str     x7, [sp, #152]
    ffffc0207a00:       97ffed44        bl      ffffc0202f10 <phys_processor_id>
    ffffc0207a04:       d0000082        adrp    x2, ffffc0219000 <system_config>
    ffffc0207a08:       d2800021        mov     x1, #0x1                        // #1
    ffffc0207a0c:       91006042        add     x2, x2, #0x18
    ffffc0207a10:       c85f7c44        ldxr    x4, [x2]
    ffffc0207a14:       ea010083        ands    x3, x4, x1
    ffffc0207a18:       54000041        b.ne    ffffc0207a20 <panic_printk+0x40>  // b.any
    ffffc0207a1c:       aa010084        orr     x4, x4, x1
    ffffc0207a20:       c8017c44        stxr    w1, x4, [x2]
    ffffc0207a24:       d5033bbf        dmb     ish
    ffffc0207a28:       35ffff41        cbnz    w1, ffffc0207a10 <panic_printk+0x30>

And there is no warning, the instruction is correct "stxr w1, x4, [x2]"


Thanks,
Peng.

Comments

Nick Clifton Feb. 19, 2019, 3:41 p.m. | #1
Hi Peng,

> Not sure this is bug or not, is my patch the correct fix? 

> please help.


I think that you have reported this problem to the wrong mailing list...

> I did a fix to the code as below:

> --- a/hypervisor/arch/arm64/include/asm/bitops.h

> +++ b/hypervisor/arch/arm64/include/asm/bitops.h


This is not part of the binutils sources. :-)

For what it is worth, I think that your analysis is correct,
and that the warning message from the assembler is valid.  So
patching the hypervisor code would be in order.  I cannot say
whether the patch itself is correct however.

Cheers
  Nick
Peng Fan Feb. 20, 2019, 2:07 a.m. | #2
Hi Nick

> -----Original Message-----

> From: Nick Clifton [mailto:nickc@redhat.com]

> Sent: 2019年2月19日 23:41

> To: Peng Fan <peng.fan@nxp.com>; binutils@sourceware.org

> Cc: Jan Kiszka <jan.kiszka@siemens.com>

> Subject: Re: Warning: unpredictable: identical transfer and status registers

> --`stxr w4,x5,[x4] using aarch64 poky gcc 8.3

> 

> Hi Peng,

> 

> > Not sure this is bug or not, is my patch the correct fix?

> > please help.

> 

> I think that you have reported this problem to the wrong mailing list...

> 

> > I did a fix to the code as below:

> > --- a/hypervisor/arch/arm64/include/asm/bitops.h

> > +++ b/hypervisor/arch/arm64/include/asm/bitops.h

> 

> This is not part of the binutils sources. :-)

> 

> For what it is worth, I think that your analysis is correct, and that the warning

> message from the assembler is valid.  So patching the hypervisor code would

> be in order.  I cannot say whether the patch itself is correct however.


Understand. It was the code error that not add early-clobber.

Thanks,
Peng.

> 

> Cheers

>   Nick

Patch

--- a/hypervisor/arch/arm64/include/asm/bitops.h
+++ b/hypervisor/arch/arm64/include/asm/bitops.h
@@ -83,21 +83,21 @@  static inline int test_and_set_bit(int nr, volatile unsigned long *addr)

        /* AARCH64_TODO: using Inner Shareable DMB at the moment,
         * revisit when we will deal with shareability domains */
+       asm volatile (
+               "1:\n\t"
+               "ldxr   %3, %2\n\t"
+               "ands   %1, %3, %4\n\t"
+               "b.ne   2f\n\t"
+               "orr    %3, %3, %4\n\t"
+               "2:\n\t"
+               "stxr   %w0, %3, %2\n\t"
+               "dmb    ish\n\t"
+               "cbnz   %w0, 1b\n\t"
+               : "=r" (ret), "=&r" (test),
+                 "+Q" (*(volatile unsigned long *)addr),
+                 "=r" (tmp)
+               : "r" (1ul << nr));

-       do {
-               asm volatile (
-                       "ldxr   %3, %2\n\t"
-                       "ands   %1, %3, %4\n\t"
-                       "b.ne   1f\n\t"
-                       "orr    %3, %3, %4\n\t"
-                       "1:\n\t"
-                       "stxr   %w0, %3, %2\n\t"
-                       "dmb    ish\n\t"
-                       : "=r" (ret), "=&r" (test),
-                         "+Q" (*(volatile unsigned long *)addr),
-                         "=r" (tmp)
-                       : "r" (1ul << nr));
-       } while (ret);
        return !!(test);
 }