diff mbox

x86: Fix build following c/s 623c720f "x86: use CLFLUSHOPT when available"

Message ID 1455218743-12751-1-git-send-email-andrew.cooper3@citrix.com (mailing list archive)
State New, archived
Headers show

Commit Message

Andrew Cooper Feb. 11, 2016, 7:25 p.m. UTC
CentOS 7 gets into trouble when compiling Xen citing:

  flushtlb.c: Assembler messages:
  flushtlb.c:149: Error: value of 256 too large for field of 1 bytes at 1

The line number is wrong, and the error message not helpful.  It turns out
that the intermediate generated assembly was

  # 139 "arch/x86/flushtlb.c" 1
      661:
      rex clflush (%r15)
  662:
  .pushsection .altinstructions,"a"

and it was having trouble combining the explicit REX prefix with the REX.B
required for the use of %r15.

Follow what Linux does and use a redundant %ds prefix instead, for a final
generated instruction of `3e 41 0f ae 3f`

While modifying this line, fix the indentation which was out by one space.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
---
 xen/arch/x86/flushtlb.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Comments

Andrew Cooper Feb. 11, 2016, 7:41 p.m. UTC | #1
On 11/02/16 19:25, Andrew Cooper wrote:
> CentOS 7 gets into trouble when compiling Xen citing:
>
>   flushtlb.c: Assembler messages:
>   flushtlb.c:149: Error: value of 256 too large for field of 1 bytes at 1
>
> The line number is wrong, and the error message not helpful.  It turns out
> that the intermediate generated assembly was
>
>   # 139 "arch/x86/flushtlb.c" 1
>       661:
>       rex clflush (%r15)
>   662:
>   .pushsection .altinstructions,"a"
>
> and it was having trouble combining the explicit REX prefix with the REX.B
> required for the use of %r15.
>
> Follow what Linux does and use a redundant %ds prefix instead, for a final
> generated instruction of `3e 41 0f ae 3f`
>
> While modifying this line, fix the indentation which was out by one space.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> ---
>  xen/arch/x86/flushtlb.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/xen/arch/x86/flushtlb.c b/xen/arch/x86/flushtlb.c
> index 90a004f..727434e 100644
> --- a/xen/arch/x86/flushtlb.c
> +++ b/xen/arch/x86/flushtlb.c
> @@ -141,10 +141,10 @@ void flush_area_local(const void *va, unsigned int flags)
>          {
>              alternative(ASM_NOP3, "sfence", X86_FEATURE_CLFLUSHOPT);
>              for ( i = 0; i < sz; i += c->x86_clflush_size )
> -                 alternative_input("rex clflush %0",
> -                                   "data16 clflush %0",
> -                                   X86_FEATURE_CLFLUSHOPT,
> -                                   "m" (((const char *)va)[i]));
> +                alternative_input(".byte 0x3e; clflush %0", /* %ds override. */
> +                                  "data16 clflush %0",      /* clflushopt.   */
> +                                  X86_FEATURE_CLFLUSHOPT,
> +                                  "m" (((const char *)va)[i]));
>          }
>          else
>          {

It turns out that Clang is far more useful at diagnosing this issue than
GCC.

flushtlb.c:144:18: error: invalid instruction mnemonic 'rex'
                 alternative_input("rex clflush %0",
                 ^
/local/xen.git/xen/include/asm/alternative.h:98:16: note: expanded from
macro 'alternative_input'
        asm volatile (ALTERNATIVE(oldinstr, newinstr, feature)          \
                      ^
/local/xen.git/xen/include/asm/alternative.h:60:2: note: expanded from
macro 'ALTERNATIVE'
        OLDINSTR(oldinstr)                                                \
        ^
/local/xen.git/xen/include/asm/alternative.h:28:40: note: expanded from
macro 'OLDINSTR'
#define OLDINSTR(oldinstr)      "661:\n\t" oldinstr "\n662:\n"
                                       ^
<inline asm>:2:2: note: instantiated into assembly here
        rex clflush (%r15,%rdx)
        ^~~~~~~~~~~~~~~~~~~~~~~
1 error generated.

~Andrew
Douglas Goldstein Feb. 11, 2016, 10:14 p.m. UTC | #2
On 2/11/16 1:25 PM, Andrew Cooper wrote:
> CentOS 7 gets into trouble when compiling Xen citing:
> 
>   flushtlb.c: Assembler messages:
>   flushtlb.c:149: Error: value of 256 too large for field of 1 bytes at 1
> 
> The line number is wrong, and the error message not helpful.  It turns out
> that the intermediate generated assembly was
> 
>   # 139 "arch/x86/flushtlb.c" 1
>       661:
>       rex clflush (%r15)
>   662:
>   .pushsection .altinstructions,"a"
> 
> and it was having trouble combining the explicit REX prefix with the REX.B
> required for the use of %r15.
> 
> Follow what Linux does and use a redundant %ds prefix instead, for a final
> generated instruction of `3e 41 0f ae 3f`
> 
> While modifying this line, fix the indentation which was out by one space.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Tested-by: Doug Goldstein <cardoe@cardoe.com>

Confirmed to fix the build issue. Results:
https://travis-ci.org/cardoe/xen/builds/108653604

> ---
> CC: Jan Beulich <JBeulich@suse.com>
> ---
>  xen/arch/x86/flushtlb.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/x86/flushtlb.c b/xen/arch/x86/flushtlb.c
> index 90a004f..727434e 100644
> --- a/xen/arch/x86/flushtlb.c
> +++ b/xen/arch/x86/flushtlb.c
> @@ -141,10 +141,10 @@ void flush_area_local(const void *va, unsigned int flags)
>          {
>              alternative(ASM_NOP3, "sfence", X86_FEATURE_CLFLUSHOPT);
>              for ( i = 0; i < sz; i += c->x86_clflush_size )
> -                 alternative_input("rex clflush %0",
> -                                   "data16 clflush %0",
> -                                   X86_FEATURE_CLFLUSHOPT,
> -                                   "m" (((const char *)va)[i]));
> +                alternative_input(".byte 0x3e; clflush %0", /* %ds override. */
> +                                  "data16 clflush %0",      /* clflushopt.   */
> +                                  X86_FEATURE_CLFLUSHOPT,
> +                                  "m" (((const char *)va)[i]));
>          }
>          else
>          {
>
Jan Beulich Feb. 12, 2016, 8:13 a.m. UTC | #3
>>> On 11.02.16 at 20:41, <andrew.cooper3@citrix.com> wrote:
> On 11/02/16 19:25, Andrew Cooper wrote:
>> --- a/xen/arch/x86/flushtlb.c
>> +++ b/xen/arch/x86/flushtlb.c
>> @@ -141,10 +141,10 @@ void flush_area_local(const void *va, unsigned int flags)
>>          {
>>              alternative(ASM_NOP3, "sfence", X86_FEATURE_CLFLUSHOPT);
>>              for ( i = 0; i < sz; i += c->x86_clflush_size )
>> -                 alternative_input("rex clflush %0",
>> -                                   "data16 clflush %0",
>> -                                   X86_FEATURE_CLFLUSHOPT,
>> -                                   "m" (((const char *)va)[i]));
>> +                alternative_input(".byte 0x3e; clflush %0", /* %ds override. */
>> +                                  "data16 clflush %0",      /* clflushopt.  
>  */
>> +                                  X86_FEATURE_CLFLUSHOPT,
>> +                                  "m" (((const char *)va)[i]));
>>          }
>>          else
>>          {
> 
> It turns out that Clang is far more useful at diagnosing this issue than
> GCC.
> 
> flushtlb.c:144:18: error: invalid instruction mnemonic 'rex'
>                  alternative_input("rex clflush %0",
>                  ^

Except that 'rex' is by no means invalid. If anything Clang's internal
assembler doesn't support it (and hence is not gas compatible).

Jan
Jan Beulich Feb. 12, 2016, 8:23 a.m. UTC | #4
>>> On 11.02.16 at 20:25, <andrew.cooper3@citrix.com> wrote:
> CentOS 7 gets into trouble when compiling Xen citing:
> 
>   flushtlb.c: Assembler messages:
>   flushtlb.c:149: Error: value of 256 too large for field of 1 bytes at 1
> 
> The line number is wrong, and the error message not helpful.  It turns out
> that the intermediate generated assembly was
> 
>   # 139 "arch/x86/flushtlb.c" 1
>       661:
>       rex clflush (%r15)
>   662:
>   .pushsection .altinstructions,"a"
> 
> and it was having trouble combining the explicit REX prefix with the REX.B
> required for the use of %r15.

What gas version is this? I just checked with 2.20, which has no
problem combining an explicit with a generated REX prefix. Or
wait, no, your description of the issue is wrong: It actually is the
folding of the two REX prefixes which causes the problem, since
that results in the replacement instruction being one byte longer
than the to be replaced one.

> Follow what Linux does and use a redundant %ds prefix instead, for a final
> generated instruction of `3e 41 0f ae 3f`
> 
> While modifying this line, fix the indentation which was out by one space.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

I think before committing I'll extend your patch by introducing
and using Linux'es NOP_DS_PREFIX.

Jan
Andrew Cooper Feb. 12, 2016, 9:47 a.m. UTC | #5
On 12/02/16 08:13, Jan Beulich wrote:
>>>> On 11.02.16 at 20:41, <andrew.cooper3@citrix.com> wrote:
>> On 11/02/16 19:25, Andrew Cooper wrote:
>>> --- a/xen/arch/x86/flushtlb.c
>>> +++ b/xen/arch/x86/flushtlb.c
>>> @@ -141,10 +141,10 @@ void flush_area_local(const void *va, unsigned int flags)
>>>          {
>>>              alternative(ASM_NOP3, "sfence", X86_FEATURE_CLFLUSHOPT);
>>>              for ( i = 0; i < sz; i += c->x86_clflush_size )
>>> -                 alternative_input("rex clflush %0",
>>> -                                   "data16 clflush %0",
>>> -                                   X86_FEATURE_CLFLUSHOPT,
>>> -                                   "m" (((const char *)va)[i]));
>>> +                alternative_input(".byte 0x3e; clflush %0", /* %ds override. */
>>> +                                  "data16 clflush %0",      /* clflushopt.  
>>  */
>>> +                                  X86_FEATURE_CLFLUSHOPT,
>>> +                                  "m" (((const char *)va)[i]));
>>>          }
>>>          else
>>>          {
>> It turns out that Clang is far more useful at diagnosing this issue than
>> GCC.
>>
>> flushtlb.c:144:18: error: invalid instruction mnemonic 'rex'
>>                  alternative_input("rex clflush %0",
>>                  ^
> Except that 'rex' is by no means invalid. If anything Clang's internal
> assembler doesn't support it (and hence is not gas compatible).

That is tangential to the point.  The useful part is the end message:

> <inline asm>:2:2: note: instantiated into assembly here
>         rex clflush (%r15,%rdx)
>         ^~~~~~~~~~~~~~~~~~~~~~~

which makes it substantially more clear that there is both a REX prefix
and REX.B required.

~Andrew
Andrew Cooper Feb. 12, 2016, 9:51 a.m. UTC | #6
On 12/02/16 08:23, Jan Beulich wrote:
>>>> On 11.02.16 at 20:25, <andrew.cooper3@citrix.com> wrote:
>> CentOS 7 gets into trouble when compiling Xen citing:
>>
>>   flushtlb.c: Assembler messages:
>>   flushtlb.c:149: Error: value of 256 too large for field of 1 bytes at 1
>>
>> The line number is wrong, and the error message not helpful.  It turns out
>> that the intermediate generated assembly was
>>
>>   # 139 "arch/x86/flushtlb.c" 1
>>       661:
>>       rex clflush (%r15)
>>   662:
>>   .pushsection .altinstructions,"a"
>>
>> and it was having trouble combining the explicit REX prefix with the REX.B
>> required for the use of %r15.
> What gas version is this? I just checked with 2.20, which has no
> problem combining an explicit with a generated REX prefix.

bash-4.2$ as --version
GNU assembler version 2.23.52.0.1-30.el7_1.2 20130226


>  Or
> wait, no, your description of the issue is wrong: It actually is the
> folding of the two REX prefixes which causes the problem,

This is what I said.  What do you think I meant with "trouble combining
the" ?

>  since
> that results in the replacement instruction being one byte longer
> than the to be replaced one.

But that is still the case with an explicit %ds override.  The assembler
still has to insert an extra byte somehow.

>
>> Follow what Linux does and use a redundant %ds prefix instead, for a final
>> generated instruction of `3e 41 0f ae 3f`
>>
>> While modifying this line, fix the indentation which was out by one space.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> I think before committing I'll extend your patch by introducing
> and using Linux'es NOP_DS_PREFIX.

Ok.

~Andrew
Jan Beulich Feb. 12, 2016, 10 a.m. UTC | #7
>>> On 12.02.16 at 10:51, <andrew.cooper3@citrix.com> wrote:
> On 12/02/16 08:23, Jan Beulich wrote:
>>>>> On 11.02.16 at 20:25, <andrew.cooper3@citrix.com> wrote:
>>> CentOS 7 gets into trouble when compiling Xen citing:
>>>
>>>   flushtlb.c: Assembler messages:
>>>   flushtlb.c:149: Error: value of 256 too large for field of 1 bytes at 1
>>>
>>> The line number is wrong, and the error message not helpful.  It turns out
>>> that the intermediate generated assembly was
>>>
>>>   # 139 "arch/x86/flushtlb.c" 1
>>>       661:
>>>       rex clflush (%r15)
>>>   662:
>>>   .pushsection .altinstructions,"a"
>>>
>>> and it was having trouble combining the explicit REX prefix with the REX.B
>>> required for the use of %r15.
>> What gas version is this? I just checked with 2.20, which has no
>> problem combining an explicit with a generated REX prefix.
> 
> bash-4.2$ as --version
> GNU assembler version 2.23.52.0.1-30.el7_1.2 20130226
> 
> 
>>  Or
>> wait, no, your description of the issue is wrong: It actually is the
>> folding of the two REX prefixes which causes the problem,
> 
> This is what I said.  What do you think I meant with "trouble combining
> the" ?

Argh - I meant to say "It actually isn't ...".

>>  since
>> that results in the replacement instruction being one byte longer
>> than the to be replaced one.
> 
> But that is still the case with an explicit %ds override.  The assembler
> still has to insert an extra byte somehow.

No. We now always have one non-REX prefix, and both instructions
will have the same REX/ModRM/SIB encoding.

Jan
Andrew Cooper Feb. 12, 2016, 10:02 a.m. UTC | #8
On 12/02/16 10:00, Jan Beulich wrote:
>>>> On 12.02.16 at 10:51, <andrew.cooper3@citrix.com> wrote:
>> On 12/02/16 08:23, Jan Beulich wrote:
>>>>>> On 11.02.16 at 20:25, <andrew.cooper3@citrix.com> wrote:
>>>> CentOS 7 gets into trouble when compiling Xen citing:
>>>>
>>>>   flushtlb.c: Assembler messages:
>>>>   flushtlb.c:149: Error: value of 256 too large for field of 1 bytes at 1
>>>>
>>>> The line number is wrong, and the error message not helpful.  It turns out
>>>> that the intermediate generated assembly was
>>>>
>>>>   # 139 "arch/x86/flushtlb.c" 1
>>>>       661:
>>>>       rex clflush (%r15)
>>>>   662:
>>>>   .pushsection .altinstructions,"a"
>>>>
>>>> and it was having trouble combining the explicit REX prefix with the REX.B
>>>> required for the use of %r15.
>>> What gas version is this? I just checked with 2.20, which has no
>>> problem combining an explicit with a generated REX prefix.
>> bash-4.2$ as --version
>> GNU assembler version 2.23.52.0.1-30.el7_1.2 20130226
>>
>>
>>>  Or
>>> wait, no, your description of the issue is wrong: It actually is the
>>> folding of the two REX prefixes which causes the problem,
>> This is what I said.  What do you think I meant with "trouble combining
>> the" ?
> Argh - I meant to say "It actually isn't ...".
>
>>>  since
>>> that results in the replacement instruction being one byte longer
>>> than the to be replaced one.
>> But that is still the case with an explicit %ds override.  The assembler
>> still has to insert an extra byte somehow.
> No. We now always have one non-REX prefix, and both instructions
> will have the same REX/ModRM/SIB encoding.

I see now, given your wording on the patch committed.

In hindsight this should have been obvious, but GCCs error message was
particularly unhelpful at diagnosing the issue.

~Andrew
Jan Beulich Feb. 12, 2016, 10:12 a.m. UTC | #9
>>> On 12.02.16 at 11:02, <andrew.cooper3@citrix.com> wrote:
> On 12/02/16 10:00, Jan Beulich wrote:
>>>>> On 12.02.16 at 10:51, <andrew.cooper3@citrix.com> wrote:
>>> On 12/02/16 08:23, Jan Beulich wrote:
>>>>>>> On 11.02.16 at 20:25, <andrew.cooper3@citrix.com> wrote:
>>>>> CentOS 7 gets into trouble when compiling Xen citing:
>>>>>
>>>>>   flushtlb.c: Assembler messages:
>>>>>   flushtlb.c:149: Error: value of 256 too large for field of 1 bytes at 1
>>>>>
>>>>> The line number is wrong, and the error message not helpful.  It turns out
>>>>> that the intermediate generated assembly was
>>>>>
>>>>>   # 139 "arch/x86/flushtlb.c" 1
>>>>>       661:
>>>>>       rex clflush (%r15)
>>>>>   662:
>>>>>   .pushsection .altinstructions,"a"
>>>>>
>>>>> and it was having trouble combining the explicit REX prefix with the REX.B
>>>>> required for the use of %r15.
>>>> What gas version is this? I just checked with 2.20, which has no
>>>> problem combining an explicit with a generated REX prefix.
>>> bash-4.2$ as --version
>>> GNU assembler version 2.23.52.0.1-30.el7_1.2 20130226
>>>
>>>
>>>>  Or
>>>> wait, no, your description of the issue is wrong: It actually is the
>>>> folding of the two REX prefixes which causes the problem,
>>> This is what I said.  What do you think I meant with "trouble combining
>>> the" ?
>> Argh - I meant to say "It actually isn't ...".
>>
>>>>  since
>>>> that results in the replacement instruction being one byte longer
>>>> than the to be replaced one.
>>> But that is still the case with an explicit %ds override.  The assembler
>>> still has to insert an extra byte somehow.
>> No. We now always have one non-REX prefix, and both instructions
>> will have the same REX/ModRM/SIB encoding.
> 
> I see now, given your wording on the patch committed.
> 
> In hindsight this should have been obvious, but GCCs error message was
> particularly unhelpful at diagnosing the issue.

It was actually an assembler error message, and I can't really see
how we could improve that (since afaict the intended checking can
only be done at assembly time).

Jan
Andrew Cooper Feb. 12, 2016, 10:50 a.m. UTC | #10
On 12/02/16 10:12, Jan Beulich wrote:
>>>> On 12.02.16 at 11:02, <andrew.cooper3@citrix.com> wrote:
>> On 12/02/16 10:00, Jan Beulich wrote:
>>>>>> On 12.02.16 at 10:51, <andrew.cooper3@citrix.com> wrote:
>>>> On 12/02/16 08:23, Jan Beulich wrote:
>>>>>>>> On 11.02.16 at 20:25, <andrew.cooper3@citrix.com> wrote:
>>>>>> CentOS 7 gets into trouble when compiling Xen citing:
>>>>>>
>>>>>>   flushtlb.c: Assembler messages:
>>>>>>   flushtlb.c:149: Error: value of 256 too large for field of 1 bytes at 1
>>>>>>
>>>>>> The line number is wrong, and the error message not helpful.  It turns out
>>>>>> that the intermediate generated assembly was
>>>>>>
>>>>>>   # 139 "arch/x86/flushtlb.c" 1
>>>>>>       661:
>>>>>>       rex clflush (%r15)
>>>>>>   662:
>>>>>>   .pushsection .altinstructions,"a"
>>>>>>
>>>>>> and it was having trouble combining the explicit REX prefix with the REX.B
>>>>>> required for the use of %r15.
>>>>> What gas version is this? I just checked with 2.20, which has no
>>>>> problem combining an explicit with a generated REX prefix.
>>>> bash-4.2$ as --version
>>>> GNU assembler version 2.23.52.0.1-30.el7_1.2 20130226
>>>>
>>>>
>>>>>  Or
>>>>> wait, no, your description of the issue is wrong: It actually is the
>>>>> folding of the two REX prefixes which causes the problem,
>>>> This is what I said.  What do you think I meant with "trouble combining
>>>> the" ?
>>> Argh - I meant to say "It actually isn't ...".
>>>
>>>>>  since
>>>>> that results in the replacement instruction being one byte longer
>>>>> than the to be replaced one.
>>>> But that is still the case with an explicit %ds override.  The assembler
>>>> still has to insert an extra byte somehow.
>>> No. We now always have one non-REX prefix, and both instructions
>>> will have the same REX/ModRM/SIB encoding.
>> I see now, given your wording on the patch committed.
>>
>> In hindsight this should have been obvious, but GCCs error message was
>> particularly unhelpful at diagnosing the issue.
> It was actually an assembler error message, and I can't really see
> how we could improve that (since afaict the intended checking can
> only be done at assembly time).

Oh right.  It is an assembler BUILD_BUG_ON().

Is there anything useful we can do to get the error message to properly
point at alternative.h: DISCARD_ENTRY()?  That would at least have helped.

As it stood, the actual reported error line was a closing brace after
the wbinvd() call.

~Andrew
Jan Beulich Feb. 12, 2016, 10:57 a.m. UTC | #11
>>> On 12.02.16 at 11:50, <andrew.cooper3@citrix.com> wrote:
> On 12/02/16 10:12, Jan Beulich wrote:
>>>>> On 12.02.16 at 11:02, <andrew.cooper3@citrix.com> wrote:
>>> On 12/02/16 10:00, Jan Beulich wrote:
>>>>>>> On 12.02.16 at 10:51, <andrew.cooper3@citrix.com> wrote:
>>>>> On 12/02/16 08:23, Jan Beulich wrote:
>>>>>>>>> On 11.02.16 at 20:25, <andrew.cooper3@citrix.com> wrote:
>>>>>>> CentOS 7 gets into trouble when compiling Xen citing:
>>>>>>>
>>>>>>>   flushtlb.c: Assembler messages:
>>>>>>>   flushtlb.c:149: Error: value of 256 too large for field of 1 bytes at 1
>>>>>>>
>>>>>>> The line number is wrong, and the error message not helpful.  It turns out
>>>>>>> that the intermediate generated assembly was
>>>>>>>
>>>>>>>   # 139 "arch/x86/flushtlb.c" 1
>>>>>>>       661:
>>>>>>>       rex clflush (%r15)
>>>>>>>   662:
>>>>>>>   .pushsection .altinstructions,"a"
>>>>>>>
>>>>>>> and it was having trouble combining the explicit REX prefix with the REX.B
>>>>>>> required for the use of %r15.
>>>>>> What gas version is this? I just checked with 2.20, which has no
>>>>>> problem combining an explicit with a generated REX prefix.
>>>>> bash-4.2$ as --version
>>>>> GNU assembler version 2.23.52.0.1-30.el7_1.2 20130226
>>>>>
>>>>>
>>>>>>  Or
>>>>>> wait, no, your description of the issue is wrong: It actually is the
>>>>>> folding of the two REX prefixes which causes the problem,
>>>>> This is what I said.  What do you think I meant with "trouble combining
>>>>> the" ?
>>>> Argh - I meant to say "It actually isn't ...".
>>>>
>>>>>>  since
>>>>>> that results in the replacement instruction being one byte longer
>>>>>> than the to be replaced one.
>>>>> But that is still the case with an explicit %ds override.  The assembler
>>>>> still has to insert an extra byte somehow.
>>>> No. We now always have one non-REX prefix, and both instructions
>>>> will have the same REX/ModRM/SIB encoding.
>>> I see now, given your wording on the patch committed.
>>>
>>> In hindsight this should have been obvious, but GCCs error message was
>>> particularly unhelpful at diagnosing the issue.
>> It was actually an assembler error message, and I can't really see
>> how we could improve that (since afaict the intended checking can
>> only be done at assembly time).
> 
> Oh right.  It is an assembler BUILD_BUG_ON().
> 
> Is there anything useful we can do to get the error message to properly
> point at alternative.h: DISCARD_ENTRY()?  That would at least have helped.

I'd like to, but I can't see how, not the least because ...

> As it stood, the actual reported error line was a closing brace after
> the wbinvd() call.

... I've also noticed that, but the assembler would just use whatever
line directive the compiler had put in the assembly file.

Jan
Andrew Cooper Feb. 12, 2016, 11:05 a.m. UTC | #12
On 12/02/16 10:57, Jan Beulich wrote:
>>>> On 12.02.16 at 11:50, <andrew.cooper3@citrix.com> wrote:
>> On 12/02/16 10:12, Jan Beulich wrote:
>>>>>> On 12.02.16 at 11:02, <andrew.cooper3@citrix.com> wrote:
>>>> On 12/02/16 10:00, Jan Beulich wrote:
>>>>>>>> On 12.02.16 at 10:51, <andrew.cooper3@citrix.com> wrote:
>>>>>> On 12/02/16 08:23, Jan Beulich wrote:
>>>>>>>>>> On 11.02.16 at 20:25, <andrew.cooper3@citrix.com> wrote:
>>>>>>>> CentOS 7 gets into trouble when compiling Xen citing:
>>>>>>>>
>>>>>>>>   flushtlb.c: Assembler messages:
>>>>>>>>   flushtlb.c:149: Error: value of 256 too large for field of 1 bytes at 1
>>>>>>>>
>>>>>>>> The line number is wrong, and the error message not helpful.  It turns out
>>>>>>>> that the intermediate generated assembly was
>>>>>>>>
>>>>>>>>   # 139 "arch/x86/flushtlb.c" 1
>>>>>>>>       661:
>>>>>>>>       rex clflush (%r15)
>>>>>>>>   662:
>>>>>>>>   .pushsection .altinstructions,"a"
>>>>>>>>
>>>>>>>> and it was having trouble combining the explicit REX prefix with the REX.B
>>>>>>>> required for the use of %r15.
>>>>>>> What gas version is this? I just checked with 2.20, which has no
>>>>>>> problem combining an explicit with a generated REX prefix.
>>>>>> bash-4.2$ as --version
>>>>>> GNU assembler version 2.23.52.0.1-30.el7_1.2 20130226
>>>>>>
>>>>>>
>>>>>>>  Or
>>>>>>> wait, no, your description of the issue is wrong: It actually is the
>>>>>>> folding of the two REX prefixes which causes the problem,
>>>>>> This is what I said.  What do you think I meant with "trouble combining
>>>>>> the" ?
>>>>> Argh - I meant to say "It actually isn't ...".
>>>>>
>>>>>>>  since
>>>>>>> that results in the replacement instruction being one byte longer
>>>>>>> than the to be replaced one.
>>>>>> But that is still the case with an explicit %ds override.  The assembler
>>>>>> still has to insert an extra byte somehow.
>>>>> No. We now always have one non-REX prefix, and both instructions
>>>>> will have the same REX/ModRM/SIB encoding.
>>>> I see now, given your wording on the patch committed.
>>>>
>>>> In hindsight this should have been obvious, but GCCs error message was
>>>> particularly unhelpful at diagnosing the issue.
>>> It was actually an assembler error message, and I can't really see
>>> how we could improve that (since afaict the intended checking can
>>> only be done at assembly time).
>> Oh right.  It is an assembler BUILD_BUG_ON().
>>
>> Is there anything useful we can do to get the error message to properly
>> point at alternative.h: DISCARD_ENTRY()?  That would at least have helped.
> I'd like to, but I can't see how, not the least because ...
>
>> As it stood, the actual reported error line was a closing brace after
>> the wbinvd() call.
> ... I've also noticed that, but the assembler would just use whatever
> line directive the compiler had put in the assembly file.

Hmm.

Well, the one saving grace is that if you google the error message, you
now get to this thread.  Hopefully this might help someone else out of a
similar situation in the future.

~Andrew
diff mbox

Patch

diff --git a/xen/arch/x86/flushtlb.c b/xen/arch/x86/flushtlb.c
index 90a004f..727434e 100644
--- a/xen/arch/x86/flushtlb.c
+++ b/xen/arch/x86/flushtlb.c
@@ -141,10 +141,10 @@  void flush_area_local(const void *va, unsigned int flags)
         {
             alternative(ASM_NOP3, "sfence", X86_FEATURE_CLFLUSHOPT);
             for ( i = 0; i < sz; i += c->x86_clflush_size )
-                 alternative_input("rex clflush %0",
-                                   "data16 clflush %0",
-                                   X86_FEATURE_CLFLUSHOPT,
-                                   "m" (((const char *)va)[i]));
+                alternative_input(".byte 0x3e; clflush %0", /* %ds override. */
+                                  "data16 clflush %0",      /* clflushopt.   */
+                                  X86_FEATURE_CLFLUSHOPT,
+                                  "m" (((const char *)va)[i]));
         }
         else
         {