[PATCH 0/7] x86/alternatives: Support for automatic padding calculations

[PATCH 0/7] x86/alternatives: Support for automatic padding calculations

[Andrew Cooper]

This is the end result of a lot of work I started during the Spectre/Meltdown
embargo window, and deferred because it was taking too long.  It finally
resolves the explict padding calculations for the SPEC_CTRL alternatives.

Andrew Cooper (7):
  x86/alt: Drop unused alternative infrastructure
  x86/alt: Clean up struct alt_instr and its users
  x86/alt: Clean up the assembly used to generate alternatives
  x86/asm: Remove opencoded uses of altinstruction_entry
  x86/alt: Support for automatic padding calculations
  x86/alt: Drop explicit padding of origin sites
  x86/build: Use new .nop directive when available

 xen/arch/x86/Rules.mk                 |   1 +
 xen/arch/x86/alternative.c            |  48 +++++++++---
 xen/arch/x86/x86_64/compat/entry.S    |  26 +++----
 xen/arch/x86/x86_64/entry.S           |  20 +----
 xen/include/asm-x86/alternative-asm.h |  93 +++++++++++++++-------
 xen/include/asm-x86/alternative.h     | 143 ++++++++++++++++++----------------
 xen/include/asm-x86/asm_defns.h       |  32 +++-----
 xen/include/asm-x86/nops.h            |   7 --
 xen/include/asm-x86/spec_ctrl_asm.h   |  19 ++---
 9 files changed, 210 insertions(+), 179 deletions(-)

-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 1/7] x86/alt: Drop unused alternative infrastructure

[Andrew Cooper]

ALTERNATIVE_3 is more complicated than ALTERNATIVE_2 when it comes to
calculating extra padding length, and we have no need for the complexity.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
 xen/include/asm-x86/alternative.h | 25 -------------------------
 1 file changed, 25 deletions(-)

diff --git a/xen/include/asm-x86/alternative.h b/xen/include/asm-x86/alternative.h
index ba537d6..13ac104 100644
--- a/xen/include/asm-x86/alternative.h
+++ b/xen/include/asm-x86/alternative.h
@@ -65,11 +65,6 @@ extern void alternative_instructions(void);
 	ALTERNATIVE(oldinstr, newinstr1, feature1)			  \
 	ALTERNATIVE_N(newinstr2, feature2, 2)
 
-#define ALTERNATIVE_3(oldinstr, newinstr1, feature1, newinstr2, feature2, \
-		      newinstr3, feature3)				  \
-	ALTERNATIVE_2(oldinstr, newinstr1, feature1, newinstr2, feature2) \
-	ALTERNATIVE_N(newinstr3, feature3, 3)
-
 /*
  * Alternative instructions for different CPU types or capabilities.
  *
@@ -118,26 +113,6 @@ extern void alternative_instructions(void);
 				   newinstr2, feature2)			\
 		     : output : input)
 
-/*
- * This is similar to alternative_io. But it has three features and
- * respective instructions.
- *
- * If CPU has feature3, newinstr3 is used.
- * Otherwise, if CPU has feature2, newinstr2 is used.
- * Otherwise, if CPU has feature1, newinstr1 is used.
- * Otherwise, oldinstr is used.
- */
-#define alternative_io_3(oldinstr, newinstr1, feature1, newinstr2,	\
-			 feature2, newinstr3, feature3, output,		\
-			 input...)					\
-	asm volatile(ALTERNATIVE_3(oldinstr, newinstr1, feature1,	\
-				   newinstr2, feature2, newinstr3,	\
-				   feature3)				\
-		     : output : input)
-
-/* Use this macro(s) if you need more than one output parameter. */
-#define ASM_OUTPUT2(a...) a
-
 #endif /*  !__ASSEMBLY__  */
 
 #endif /* __X86_ALTERNATIVE_H__ */
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/7] x86/alt: Drop unused alternative infrastructure

[Wei Liu]

On Mon, Feb 12, 2018 at 11:23:01AM +0000, Andrew Cooper wrote:
> ALTERNATIVE_3 is more complicated than ALTERNATIVE_2 when it comes to
> calculating extra padding length, and we have no need for the complexity.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Wei Liu <wei.liu2@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/7] x86/alt: Drop unused alternative infrastructure

[Roger Pau Monné]

On Mon, Feb 12, 2018 at 11:23:01AM +0000, Andrew Cooper wrote:
> ALTERNATIVE_3 is more complicated than ALTERNATIVE_2 when it comes to
> calculating extra padding length, and we have no need for the complexity.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

I guess you also don't foresee any new features requiring this
functionality?

I assume this was added because the alternatives framework was picked
wholesale from Linux, but we have diverged enough that we don't plan
to sync anymore with Linux?

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/7] x86/alt: Drop unused alternative infrastructure

[Andrew Cooper]

On 12/02/18 15:56, Roger Pau Monné wrote:
> On Mon, Feb 12, 2018 at 11:23:01AM +0000, Andrew Cooper wrote:
>> ALTERNATIVE_3 is more complicated than ALTERNATIVE_2 when it comes to
>> calculating extra padding length, and we have no need for the complexity.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
>
> I guess you also don't foresee any new features requiring this
> functionality?
>
> I assume this was added because the alternatives framework was picked
> wholesale from Linux, but we have diverged enough that we don't plan
> to sync anymore with Linux?

Linux has dropped it as well.  If we find a usecase, we can reintroduce
it, but someone is going to have some "fun" with max() handling.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/7] x86/alt: Drop unused alternative infrastructure

[Jan Beulich]

>>> On 12.02.18 at 12:23, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/include/asm-x86/alternative.h
> +++ b/xen/include/asm-x86/alternative.h
> @@ -65,11 +65,6 @@ extern void alternative_instructions(void);
>  	ALTERNATIVE(oldinstr, newinstr1, feature1)			  \
>  	ALTERNATIVE_N(newinstr2, feature2, 2)
>  
> -#define ALTERNATIVE_3(oldinstr, newinstr1, feature1, newinstr2, feature2, \
> -		      newinstr3, feature3)				  \
> -	ALTERNATIVE_2(oldinstr, newinstr1, feature1, newinstr2, feature2) \
> -	ALTERNATIVE_N(newinstr3, feature3, 3)
> -
>  /*
>   * Alternative instructions for different CPU types or capabilities.
>   *

While this one is fine, ...

> @@ -118,26 +113,6 @@ extern void alternative_instructions(void);
>  				   newinstr2, feature2)			\
>  		     : output : input)
>  
> -/*
> - * This is similar to alternative_io. But it has three features and
> - * respective instructions.
> - *
> - * If CPU has feature3, newinstr3 is used.
> - * Otherwise, if CPU has feature2, newinstr2 is used.
> - * Otherwise, if CPU has feature1, newinstr1 is used.
> - * Otherwise, oldinstr is used.
> - */
> -#define alternative_io_3(oldinstr, newinstr1, feature1, newinstr2,	\
> -			 feature2, newinstr3, feature3, output,		\
> -			 input...)					\
> -	asm volatile(ALTERNATIVE_3(oldinstr, newinstr1, feature1,	\
> -				   newinstr2, feature2, newinstr3,	\
> -				   feature3)				\
> -		     : output : input)
> -
> -/* Use this macro(s) if you need more than one output parameter. */
> -#define ASM_OUTPUT2(a...) a

... I'm having patches to post which use both of these, so I'd
very much prefer them to not go away. It is simply a lack of time
which resulted in me not having posted that series already.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/7] x86/alt: Drop unused alternative infrastructure

[Andrew Cooper]

On 13/02/18 14:22, Jan Beulich wrote:
>>>> On 12.02.18 at 12:23, <andrew.cooper3@citrix.com> wrote:
>> --- a/xen/include/asm-x86/alternative.h
>> +++ b/xen/include/asm-x86/alternative.h
>> @@ -65,11 +65,6 @@ extern void alternative_instructions(void);
>>  	ALTERNATIVE(oldinstr, newinstr1, feature1)			  \
>>  	ALTERNATIVE_N(newinstr2, feature2, 2)
>>  
>> -#define ALTERNATIVE_3(oldinstr, newinstr1, feature1, newinstr2, feature2, \
>> -		      newinstr3, feature3)				  \
>> -	ALTERNATIVE_2(oldinstr, newinstr1, feature1, newinstr2, feature2) \
>> -	ALTERNATIVE_N(newinstr3, feature3, 3)
>> -
>>  /*
>>   * Alternative instructions for different CPU types or capabilities.
>>   *
> While this one is fine, ...
>
>> @@ -118,26 +113,6 @@ extern void alternative_instructions(void);
>>  				   newinstr2, feature2)			\
>>  		     : output : input)
>>  
>> -/*
>> - * This is similar to alternative_io. But it has three features and
>> - * respective instructions.
>> - *
>> - * If CPU has feature3, newinstr3 is used.
>> - * Otherwise, if CPU has feature2, newinstr2 is used.
>> - * Otherwise, if CPU has feature1, newinstr1 is used.
>> - * Otherwise, oldinstr is used.
>> - */
>> -#define alternative_io_3(oldinstr, newinstr1, feature1, newinstr2,	\
>> -			 feature2, newinstr3, feature3, output,		\
>> -			 input...)					\
>> -	asm volatile(ALTERNATIVE_3(oldinstr, newinstr1, feature1,	\
>> -				   newinstr2, feature2, newinstr3,	\
>> -				   feature3)				\
>> -		     : output : input)
>> -
>> -/* Use this macro(s) if you need more than one output parameter. */
>> -#define ASM_OUTPUT2(a...) a
> ... I'm having patches to post which use both of these, so I'd
> very much prefer them to not go away. It is simply a lack of time
> which resulted in me not having posted that series already.

In which case I'll need to review that patch before commenting on this
one (i.e. whether it actually needs to be an alternative_3, or whether
there is a shorter way to do it).

The problem is that the gas_max() expression expands its parameters 5
times, and ISTR a report on LKML saying that older version of GAS
couldn't cope with the eventual expansion of the 3-replacement version.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/7] x86/alt: Drop unused alternative infrastructure

[Jan Beulich]

>>> On 13.02.18 at 15:41, <andrew.cooper3@citrix.com> wrote:
> On 13/02/18 14:22, Jan Beulich wrote:
>>>>> On 12.02.18 at 12:23, <andrew.cooper3@citrix.com> wrote:
>>> @@ -118,26 +113,6 @@ extern void alternative_instructions(void);
>>>  				   newinstr2, feature2)			\
>>>  		     : output : input)
>>>  
>>> -/*
>>> - * This is similar to alternative_io. But it has three features and
>>> - * respective instructions.
>>> - *
>>> - * If CPU has feature3, newinstr3 is used.
>>> - * Otherwise, if CPU has feature2, newinstr2 is used.
>>> - * Otherwise, if CPU has feature1, newinstr1 is used.
>>> - * Otherwise, oldinstr is used.
>>> - */
>>> -#define alternative_io_3(oldinstr, newinstr1, feature1, newinstr2,	\
>>> -			 feature2, newinstr3, feature3, output,		\
>>> -			 input...)					\
>>> -	asm volatile(ALTERNATIVE_3(oldinstr, newinstr1, feature1,	\
>>> -				   newinstr2, feature2, newinstr3,	\
>>> -				   feature3)				\
>>> -		     : output : input)
>>> -
>>> -/* Use this macro(s) if you need more than one output parameter. */
>>> -#define ASM_OUTPUT2(a...) a
>> ... I'm having patches to post which use both of these, so I'd
>> very much prefer them to not go away. It is simply a lack of time
>> which resulted in me not having posted that series already.
> 
> In which case I'll need to review that patch before commenting on this
> one (i.e. whether it actually needs to be an alternative_3, or whether
> there is a shorter way to do it).

I've checked again - if the NOP replacement of the unused space in
the original instruction was taken care of (which I understand later
patches in this series arrange for), I would need only the 2-variants
form. I'll need ASM_OUTPUT2() in any case, though.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/7] x86/alt: Drop unused alternative infrastructure

[Jan Beulich]

>>> On 13.02.18 at 15:41, <andrew.cooper3@citrix.com> wrote:
> On 13/02/18 14:22, Jan Beulich wrote:
>>>>> On 12.02.18 at 12:23, <andrew.cooper3@citrix.com> wrote:
>>> --- a/xen/include/asm-x86/alternative.h
>>> +++ b/xen/include/asm-x86/alternative.h
>>> @@ -65,11 +65,6 @@ extern void alternative_instructions(void);
>>>  	ALTERNATIVE(oldinstr, newinstr1, feature1)			  \
>>>  	ALTERNATIVE_N(newinstr2, feature2, 2)
>>>  
>>> -#define ALTERNATIVE_3(oldinstr, newinstr1, feature1, newinstr2, feature2, \
>>> -		      newinstr3, feature3)				  \
>>> -	ALTERNATIVE_2(oldinstr, newinstr1, feature1, newinstr2, feature2) \
>>> -	ALTERNATIVE_N(newinstr3, feature3, 3)
>>> -
>>>  /*
>>>   * Alternative instructions for different CPU types or capabilities.
>>>   *
>> While this one is fine, ...
>>
>>> @@ -118,26 +113,6 @@ extern void alternative_instructions(void);
>>>  				   newinstr2, feature2)			\
>>>  		     : output : input)
>>>  
>>> -/*
>>> - * This is similar to alternative_io. But it has three features and
>>> - * respective instructions.
>>> - *
>>> - * If CPU has feature3, newinstr3 is used.
>>> - * Otherwise, if CPU has feature2, newinstr2 is used.
>>> - * Otherwise, if CPU has feature1, newinstr1 is used.
>>> - * Otherwise, oldinstr is used.
>>> - */
>>> -#define alternative_io_3(oldinstr, newinstr1, feature1, newinstr2,	\
>>> -			 feature2, newinstr3, feature3, output,		\
>>> -			 input...)					\
>>> -	asm volatile(ALTERNATIVE_3(oldinstr, newinstr1, feature1,	\
>>> -				   newinstr2, feature2, newinstr3,	\
>>> -				   feature3)				\
>>> -		     : output : input)
>>> -
>>> -/* Use this macro(s) if you need more than one output parameter. */
>>> -#define ASM_OUTPUT2(a...) a
>> ... I'm having patches to post which use both of these, so I'd
>> very much prefer them to not go away. It is simply a lack of time
>> which resulted in me not having posted that series already.
> 
> In which case I'll need to review that patch before commenting on this
> one (i.e. whether it actually needs to be an alternative_3, or whether
> there is a shorter way to do it).
> 
> The problem is that the gas_max() expression expands its parameters 5
> times, and ISTR a report on LKML saying that older version of GAS
> couldn't cope with the eventual expansion of the 3-replacement version.

As per my earlier reply, with ASM_OUTPUT2() retained
Reviewed-by: Jan Beulich <jbeulich@suse.com>
provided all of this (minus patch 7) goes in together (at which
point I'll have quite some re-basing fun).

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 2/7] x86/alt: Clean up struct alt_instr and its users

[Andrew Cooper]

 * Rename some fields for consistency and clarity, and use standard types.
 * Don't opencode the use of ALT_{ORIG,REPL}_PTR().

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
 xen/arch/x86/alternative.c        | 24 ++++++++++++------------
 xen/include/asm-x86/alternative.h | 12 ++++++------
 2 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/xen/arch/x86/alternative.c b/xen/arch/x86/alternative.c
index 5c8b6f6..f8ddab5 100644
--- a/xen/arch/x86/alternative.c
+++ b/xen/arch/x86/alternative.c
@@ -163,8 +163,6 @@ void init_or_livepatch apply_alternatives(const struct alt_instr *start,
                                           const struct alt_instr *end)
 {
     const struct alt_instr *a;
-    u8 *instr, *replacement;
-    u8 insnbuf[MAX_PATCH_LEN];
 
     printk(KERN_INFO "alt table %p -> %p\n", start, end);
 
@@ -179,23 +177,25 @@ void init_or_livepatch apply_alternatives(const struct alt_instr *start,
      */
     for ( a = start; a < end; a++ )
     {
-        instr = (u8 *)&a->instr_offset + a->instr_offset;
-        replacement = (u8 *)&a->repl_offset + a->repl_offset;
-        BUG_ON(a->replacementlen > a->instrlen);
-        BUG_ON(a->instrlen > sizeof(insnbuf));
+        uint8_t *orig = ALT_ORIG_PTR(a);
+        uint8_t *repl = ALT_REPL_PTR(a);
+        uint8_t buf[MAX_PATCH_LEN];
+
+        BUG_ON(a->repl_len > a->orig_len);
+        BUG_ON(a->orig_len > sizeof(buf));
         BUG_ON(a->cpuid >= NCAPINTS * 32);
+
         if ( !boot_cpu_has(a->cpuid) )
             continue;
 
-        memcpy(insnbuf, replacement, a->replacementlen);
+        memcpy(buf, repl, a->repl_len);
 
         /* 0xe8/0xe9 are relative branches; fix the offset. */
-        if ( a->replacementlen >= 5 && (*insnbuf & 0xfe) == 0xe8 )
-            *(s32 *)(insnbuf + 1) += replacement - instr;
+        if ( a->repl_len >= 5 && (*buf & 0xfe) == 0xe8 )
+            *(s32 *)(buf + 1) += repl - orig;
 
-        add_nops(insnbuf + a->replacementlen,
-                 a->instrlen - a->replacementlen);
-        text_poke(instr, insnbuf, a->instrlen);
+        add_nops(buf + a->repl_len, a->orig_len - a->repl_len);
+        text_poke(orig, buf, a->orig_len);
     }
 }
 
diff --git a/xen/include/asm-x86/alternative.h b/xen/include/asm-x86/alternative.h
index 13ac104..fefa87d 100644
--- a/xen/include/asm-x86/alternative.h
+++ b/xen/include/asm-x86/alternative.h
@@ -9,15 +9,15 @@
 #include <xen/types.h>
 
 struct alt_instr {
-    s32 instr_offset;       /* original instruction */
-    s32 repl_offset;        /* offset to replacement instruction */
-    u16 cpuid;              /* cpuid bit set for replacement */
-    u8  instrlen;           /* length of original instruction */
-    u8  replacementlen;     /* length of new instruction, <= instrlen */
+    int32_t  orig_offset;   /* original instruction */
+    int32_t  repl_offset;   /* offset to replacement instruction */
+    uint16_t cpuid;         /* cpuid bit set for replacement */
+    uint8_t  orig_len;      /* length of original instruction */
+    uint8_t  repl_len;      /* length of new instruction, <= instrlen */
 };
 
 #define __ALT_PTR(a,f)      ((u8 *)((void *)&(a)->f + (a)->f))
-#define ALT_ORIG_PTR(a)     __ALT_PTR(a, instr_offset)
+#define ALT_ORIG_PTR(a)     __ALT_PTR(a, orig_offset)
 #define ALT_REPL_PTR(a)     __ALT_PTR(a, repl_offset)
 
 extern void add_nops(void *insns, unsigned int len);
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/7] x86/alt: Clean up struct alt_instr and its users

[Wei Liu]

On Mon, Feb 12, 2018 at 11:23:02AM +0000, Andrew Cooper wrote:
>  * Rename some fields for consistency and clarity, and use standard types.
>  * Don't opencode the use of ALT_{ORIG,REPL}_PTR().

And change u8 etc.

> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> ---
>  xen/arch/x86/alternative.c        | 24 ++++++++++++------------
>  xen/include/asm-x86/alternative.h | 12 ++++++------
>  2 files changed, 18 insertions(+), 18 deletions(-)
> 
> diff --git a/xen/arch/x86/alternative.c b/xen/arch/x86/alternative.c
> index 5c8b6f6..f8ddab5 100644
> --- a/xen/arch/x86/alternative.c
> +++ b/xen/arch/x86/alternative.c
> @@ -163,8 +163,6 @@ void init_or_livepatch apply_alternatives(const struct alt_instr *start,
>                                            const struct alt_instr *end)
>  {
>      const struct alt_instr *a;
> -    u8 *instr, *replacement;
> -    u8 insnbuf[MAX_PATCH_LEN];
>  
>      printk(KERN_INFO "alt table %p -> %p\n", start, end);
>  
> @@ -179,23 +177,25 @@ void init_or_livepatch apply_alternatives(const struct alt_instr *start,
>       */
>      for ( a = start; a < end; a++ )
>      {
> -        instr = (u8 *)&a->instr_offset + a->instr_offset;
> -        replacement = (u8 *)&a->repl_offset + a->repl_offset;
> -        BUG_ON(a->replacementlen > a->instrlen);
> -        BUG_ON(a->instrlen > sizeof(insnbuf));
> +        uint8_t *orig = ALT_ORIG_PTR(a);
> +        uint8_t *repl = ALT_REPL_PTR(a);
> +        uint8_t buf[MAX_PATCH_LEN];
> +
> +        BUG_ON(a->repl_len > a->orig_len);
> +        BUG_ON(a->orig_len > sizeof(buf));
>          BUG_ON(a->cpuid >= NCAPINTS * 32);
> +
>          if ( !boot_cpu_has(a->cpuid) )
>              continue;
>  
> -        memcpy(insnbuf, replacement, a->replacementlen);
> +        memcpy(buf, repl, a->repl_len);
>  
>          /* 0xe8/0xe9 are relative branches; fix the offset. */
> -        if ( a->replacementlen >= 5 && (*insnbuf & 0xfe) == 0xe8 )
> -            *(s32 *)(insnbuf + 1) += replacement - instr;
> +        if ( a->repl_len >= 5 && (*buf & 0xfe) == 0xe8 )
> +            *(s32 *)(buf + 1) += repl - orig;

Mind changing s32 to int32_t?

In any case:

Reviewed-by: Wei Liu <wei.liu2@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/7] x86/alt: Clean up struct alt_instr and its users

[Roger Pau Monné]

On Mon, Feb 12, 2018 at 11:23:02AM +0000, Andrew Cooper wrote:
>  * Rename some fields for consistency and clarity, and use standard types.
>  * Don't opencode the use of ALT_{ORIG,REPL}_PTR().
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> ---
>  xen/arch/x86/alternative.c        | 24 ++++++++++++------------
>  xen/include/asm-x86/alternative.h | 12 ++++++------
>  2 files changed, 18 insertions(+), 18 deletions(-)
> 
> diff --git a/xen/arch/x86/alternative.c b/xen/arch/x86/alternative.c
> index 5c8b6f6..f8ddab5 100644
> --- a/xen/arch/x86/alternative.c
> +++ b/xen/arch/x86/alternative.c
> @@ -163,8 +163,6 @@ void init_or_livepatch apply_alternatives(const struct alt_instr *start,
>                                            const struct alt_instr *end)
>  {
>      const struct alt_instr *a;
> -    u8 *instr, *replacement;
> -    u8 insnbuf[MAX_PATCH_LEN];
>  
>      printk(KERN_INFO "alt table %p -> %p\n", start, end);
>  
> @@ -179,23 +177,25 @@ void init_or_livepatch apply_alternatives(const struct alt_instr *start,
>       */
>      for ( a = start; a < end; a++ )
>      {
> -        instr = (u8 *)&a->instr_offset + a->instr_offset;
> -        replacement = (u8 *)&a->repl_offset + a->repl_offset;
> -        BUG_ON(a->replacementlen > a->instrlen);
> -        BUG_ON(a->instrlen > sizeof(insnbuf));
> +        uint8_t *orig = ALT_ORIG_PTR(a);
> +        uint8_t *repl = ALT_REPL_PTR(a);
> +        uint8_t buf[MAX_PATCH_LEN];
> +
> +        BUG_ON(a->repl_len > a->orig_len);
> +        BUG_ON(a->orig_len > sizeof(buf));

Would be nice to have an assembly BUILD_BUG_ON equivalent so that this
could be turned into a compile time check and added to ALTINSTR_ENTRY.

>          BUG_ON(a->cpuid >= NCAPINTS * 32);
> +
>          if ( !boot_cpu_has(a->cpuid) )
>              continue;
>  
> -        memcpy(insnbuf, replacement, a->replacementlen);
> +        memcpy(buf, repl, a->repl_len);
>  
>          /* 0xe8/0xe9 are relative branches; fix the offset. */
> -        if ( a->replacementlen >= 5 && (*insnbuf & 0xfe) == 0xe8 )
> -            *(s32 *)(insnbuf + 1) += replacement - instr;
> +        if ( a->repl_len >= 5 && (*buf & 0xfe) == 0xe8 )
> +            *(s32 *)(buf + 1) += repl - orig;

(int32_t *)

>  
> -        add_nops(insnbuf + a->replacementlen,
> -                 a->instrlen - a->replacementlen);
> -        text_poke(instr, insnbuf, a->instrlen);
> +        add_nops(buf + a->repl_len, a->orig_len - a->repl_len);
> +        text_poke(orig, buf, a->orig_len);
>      }
>  }
>  
> diff --git a/xen/include/asm-x86/alternative.h b/xen/include/asm-x86/alternative.h
> index 13ac104..fefa87d 100644
> --- a/xen/include/asm-x86/alternative.h
> +++ b/xen/include/asm-x86/alternative.h
> @@ -9,15 +9,15 @@
>  #include <xen/types.h>
>  
>  struct alt_instr {
> -    s32 instr_offset;       /* original instruction */
> -    s32 repl_offset;        /* offset to replacement instruction */
> -    u16 cpuid;              /* cpuid bit set for replacement */
> -    u8  instrlen;           /* length of original instruction */
> -    u8  replacementlen;     /* length of new instruction, <= instrlen */
> +    int32_t  orig_offset;   /* original instruction */
> +    int32_t  repl_offset;   /* offset to replacement instruction */
> +    uint16_t cpuid;         /* cpuid bit set for replacement */
> +    uint8_t  orig_len;      /* length of original instruction */
> +    uint8_t  repl_len;      /* length of new instruction, <= instrlen */
>  };
>  
>  #define __ALT_PTR(a,f)      ((u8 *)((void *)&(a)->f + (a)->f))

While there maybe you could also change the above to uint8_t...

> -#define ALT_ORIG_PTR(a)     __ALT_PTR(a, instr_offset)
> +#define ALT_ORIG_PTR(a)     __ALT_PTR(a, orig_offset)
>  #define ALT_REPL_PTR(a)     __ALT_PTR(a, repl_offset)
>  
>  extern void add_nops(void *insns, unsigned int len);
> -- 
> 2.1.4
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/7] x86/alt: Clean up struct alt_instr and its users

[Wei Liu]

On Mon, Feb 12, 2018 at 04:52:21PM +0000, Roger Pau Monné wrote:
> On Mon, Feb 12, 2018 at 11:23:02AM +0000, Andrew Cooper wrote:
> >  * Rename some fields for consistency and clarity, and use standard types.
> >  * Don't opencode the use of ALT_{ORIG,REPL}_PTR().
> > 
> > No functional change.
> > 
> > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> > ---
> > CC: Jan Beulich <JBeulich@suse.com>
> > CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > CC: Roger Pau Monné <roger.pau@citrix.com>
> > CC: Wei Liu <wei.liu2@citrix.com>
> > ---
> >  xen/arch/x86/alternative.c        | 24 ++++++++++++------------
> >  xen/include/asm-x86/alternative.h | 12 ++++++------
> >  2 files changed, 18 insertions(+), 18 deletions(-)
> > 
> > diff --git a/xen/arch/x86/alternative.c b/xen/arch/x86/alternative.c
> > index 5c8b6f6..f8ddab5 100644
> > --- a/xen/arch/x86/alternative.c
> > +++ b/xen/arch/x86/alternative.c
> > @@ -163,8 +163,6 @@ void init_or_livepatch apply_alternatives(const struct alt_instr *start,
> >                                            const struct alt_instr *end)
> >  {
> >      const struct alt_instr *a;
> > -    u8 *instr, *replacement;
> > -    u8 insnbuf[MAX_PATCH_LEN];
> >  
> >      printk(KERN_INFO "alt table %p -> %p\n", start, end);
> >  
> > @@ -179,23 +177,25 @@ void init_or_livepatch apply_alternatives(const struct alt_instr *start,
> >       */
> >      for ( a = start; a < end; a++ )
> >      {
> > -        instr = (u8 *)&a->instr_offset + a->instr_offset;
> > -        replacement = (u8 *)&a->repl_offset + a->repl_offset;
> > -        BUG_ON(a->replacementlen > a->instrlen);
> > -        BUG_ON(a->instrlen > sizeof(insnbuf));
> > +        uint8_t *orig = ALT_ORIG_PTR(a);
> > +        uint8_t *repl = ALT_REPL_PTR(a);
> > +        uint8_t buf[MAX_PATCH_LEN];
> > +
> > +        BUG_ON(a->repl_len > a->orig_len);
> > +        BUG_ON(a->orig_len > sizeof(buf));
> 
> Would be nice to have an assembly BUILD_BUG_ON equivalent so that this
> could be turned into a compile time check and added to ALTINSTR_ENTRY.

 This function is also used to livepatch the hypervisor.

 I would actually suggest not use BUG_ON here instead but I didn't want
 to add noise to the problem at hand. :-)

 Wei.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/7] x86/alt: Clean up struct alt_instr and its users

[Andrew Cooper]

On 12/02/18 17:18, Wei Liu wrote:
> On Mon, Feb 12, 2018 at 04:52:21PM +0000, Roger Pau Monné wrote:
>> On Mon, Feb 12, 2018 at 11:23:02AM +0000, Andrew Cooper wrote:
>>>  * Rename some fields for consistency and clarity, and use standard types.
>>>  * Don't opencode the use of ALT_{ORIG,REPL}_PTR().
>>>
>>> No functional change.
>>>
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
>>
>>> ---
>>> CC: Jan Beulich <JBeulich@suse.com>
>>> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>>> CC: Roger Pau Monné <roger.pau@citrix.com>
>>> CC: Wei Liu <wei.liu2@citrix.com>
>>> ---
>>>  xen/arch/x86/alternative.c        | 24 ++++++++++++------------
>>>  xen/include/asm-x86/alternative.h | 12 ++++++------
>>>  2 files changed, 18 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/xen/arch/x86/alternative.c b/xen/arch/x86/alternative.c
>>> index 5c8b6f6..f8ddab5 100644
>>> --- a/xen/arch/x86/alternative.c
>>> +++ b/xen/arch/x86/alternative.c
>>> @@ -163,8 +163,6 @@ void init_or_livepatch apply_alternatives(const struct alt_instr *start,
>>>                                            const struct alt_instr *end)
>>>  {
>>>      const struct alt_instr *a;
>>> -    u8 *instr, *replacement;
>>> -    u8 insnbuf[MAX_PATCH_LEN];
>>>  
>>>      printk(KERN_INFO "alt table %p -> %p\n", start, end);
>>>  
>>> @@ -179,23 +177,25 @@ void init_or_livepatch apply_alternatives(const struct alt_instr *start,
>>>       */
>>>      for ( a = start; a < end; a++ )
>>>      {
>>> -        instr = (u8 *)&a->instr_offset + a->instr_offset;
>>> -        replacement = (u8 *)&a->repl_offset + a->repl_offset;
>>> -        BUG_ON(a->replacementlen > a->instrlen);
>>> -        BUG_ON(a->instrlen > sizeof(insnbuf));
>>> +        uint8_t *orig = ALT_ORIG_PTR(a);
>>> +        uint8_t *repl = ALT_REPL_PTR(a);
>>> +        uint8_t buf[MAX_PATCH_LEN];
>>> +
>>> +        BUG_ON(a->repl_len > a->orig_len);
>>> +        BUG_ON(a->orig_len > sizeof(buf));
>> Would be nice to have an assembly BUILD_BUG_ON equivalent so that this
>> could be turned into a compile time check and added to ALTINSTR_ENTRY.
>  This function is also used to livepatch the hypervisor.
>
>  I would actually suggest not use BUG_ON here instead but I didn't want
>  to add noise to the problem at hand. :-)

We've got some assembler time checks already, and I've introduced more
in patch 5 which include this sizeof(buf) one.

As for BUG_ON()s, I considered dropping them as well, but the only thing
which will happen if these are skipped is that we will crash in less
obvious ways in the patch points.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/7] x86/alt: Clean up struct alt_instr and its users

[Jan Beulich]

 >>> On 12.02.18 at 12:23, <andrew.cooper3@citrix.com> wrote:
> * Rename some fields for consistency and clarity, and use standard types.
>  * Don't opencode the use of ALT_{ORIG,REPL}_PTR().
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/7] x86/alt: Clean up struct alt_instr and its users

[Konrad Rzeszutek Wilk]

On Mon, Feb 12, 2018 at 11:23:02AM +0000, Andrew Cooper wrote:
>  * Rename some fields for consistency and clarity, and use standard types.
>  * Don't opencode the use of ALT_{ORIG,REPL}_PTR().
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 3/7] x86/alt: Clean up the assembly used to generate alternatives

[Andrew Cooper]

 * On the C side, switch to using local lables rather than hardcoded numbers.
 * Rename parameters and lables to be consistent with alt_instr names, and
   consistent between the the C and asm versions.
 * On the asm side, factor some expressions out into macros to aid clarity.
 * Consistently declare section attributes.

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
 xen/include/asm-x86/alternative-asm.h | 57 +++++++++++++++++--------------
 xen/include/asm-x86/alternative.h     | 64 +++++++++++++++++++----------------
 2 files changed, 67 insertions(+), 54 deletions(-)

diff --git a/xen/include/asm-x86/alternative-asm.h b/xen/include/asm-x86/alternative-asm.h
index 6640e85..150bd1a 100644
--- a/xen/include/asm-x86/alternative-asm.h
+++ b/xen/include/asm-x86/alternative-asm.h
@@ -9,60 +9,67 @@
  * enough information for the alternatives patching code to patch an
  * instruction. See apply_alternatives().
  */
-.macro altinstruction_entry orig alt feature orig_len alt_len
+.macro altinstruction_entry orig repl feature orig_len repl_len
     .long \orig - .
-    .long \alt - .
+    .long \repl - .
     .word \feature
     .byte \orig_len
-    .byte \alt_len
+    .byte \repl_len
 .endm
 
+#define orig_len               (.L\@_orig_e       -     .L\@_orig_s)
+#define repl_len(nr)           (.L\@_repl_e\()nr  -     .L\@_repl_s\()nr)
+#define decl_repl(insn, nr)     .L\@_repl_s\()nr: insn; .L\@_repl_e\()nr:
+
 .macro ALTERNATIVE oldinstr, newinstr, feature
-.Lold_start_\@:
+.L\@_orig_s:
     \oldinstr
-.Lold_end_\@:
+.L\@_orig_e:
 
     .pushsection .altinstructions, "a", @progbits
-    altinstruction_entry .Lold_start_\@, .Lnew_start_\@, \feature, \
-        (.Lold_end_\@ - .Lold_start_\@), (.Lnew_end_\@ - .Lnew_start_\@)
+    altinstruction_entry .L\@_orig_s, .L\@_repl_s1, \feature, \
+        orig_len, repl_len(1)
 
     .section .discard, "a", @progbits
     /* Assembler-time check that \newinstr isn't longer than \oldinstr. */
-    .byte 0xff + (.Lnew_end_\@ - .Lnew_start_\@) - (.Lold_end_\@ - .Lold_start_\@)
+    .byte 0xff + repl_len(1) - orig_len
 
     .section .altinstr_replacement, "ax", @progbits
-.Lnew_start_\@:
-    \newinstr
-.Lnew_end_\@:
+
+    decl_repl(\newinstr, 1)
+
     .popsection
 .endm
 
 .macro ALTERNATIVE_2 oldinstr, newinstr1, feature1, newinstr2, feature2
-.Lold_start_\@:
+.L\@_orig_s:
     \oldinstr
-.Lold_end_\@:
+.L\@_orig_e:
 
     .pushsection .altinstructions, "a", @progbits
-    altinstruction_entry .Lold_start_\@, .Lnew1_start_\@, \feature1, \
-        (.Lold_end_\@ - .Lold_start_\@), (.Lnew1_end_\@ - .Lnew1_start_\@)
-    altinstruction_entry .Lold_start_\@, .Lnew2_start_\@, \feature2, \
-        (.Lold_end_\@ - .Lold_start_\@), (.Lnew2_end_\@ - .Lnew2_start_\@)
+
+    altinstruction_entry .L\@_orig_s, .L\@_repl_s1, \feature1, \
+        orig_len, repl_len(1)
+    altinstruction_entry .L\@_orig_s, .L\@_repl_s2, \feature2, \
+        orig_len, repl_len(2)
 
     .section .discard, "a", @progbits
     /* Assembler-time check that \newinstr{1,2} aren't longer than \oldinstr. */
-    .byte 0xff + (.Lnew1_end_\@ - .Lnew1_start_\@) - (.Lold_end_\@ - .Lold_start_\@)
-    .byte 0xff + (.Lnew2_end_\@ - .Lnew2_start_\@) - (.Lold_end_\@ - .Lold_start_\@)
+    .byte 0xff + repl_len(1) - orig_len
+    .byte 0xff + repl_len(2) - orig_len
 
     .section .altinstr_replacement, "ax", @progbits
-.Lnew1_start_\@:
-    \newinstr1
-.Lnew1_end_\@:
-.Lnew2_start_\@:
-    \newinstr2
-.Lnew2_end_\@:
+
+    decl_repl(\newinstr1, 1)
+    decl_repl(\newinstr2, 2)
+
     .popsection
 .endm
 
+#undef decl_repl
+#undef repl_len
+#undef orig_len
+
 #endif /* __ASSEMBLY__ */
 #endif /* _ASM_X86_ALTERNATIVE_ASM_H_ */
 
diff --git a/xen/include/asm-x86/alternative.h b/xen/include/asm-x86/alternative.h
index fefa87d..1e5cfbd 100644
--- a/xen/include/asm-x86/alternative.h
+++ b/xen/include/asm-x86/alternative.h
@@ -26,44 +26,50 @@ extern void apply_alternatives(const struct alt_instr *start,
                                const struct alt_instr *end);
 extern void alternative_instructions(void);
 
-#define OLDINSTR(oldinstr)      "661:\n\t" oldinstr "\n662:\n"
+#define OLDINSTR(oldinstr)      ".L%=_orig_s:\n\t" oldinstr "\n.L%=_orig_e:\n"
 
-#define b_replacement(number)   "663"#number
-#define e_replacement(number)   "664"#number
+#define repl_s(num)             ".L%=_repl_s"#num
+#define repl_e(num)             ".L%=_repl_e"#num
 
-#define alt_slen "662b-661b"
-#define alt_rlen(number) e_replacement(number)"f-"b_replacement(number)"f"
+#define alt_orig_len            "(.L%=_orig_e - .L%=_orig_s)"
+#define alt_repl_len(num)       "(" repl_e(num) " - " repl_s(num) ")"
 
-#define ALTINSTR_ENTRY(feature, number)                                       \
-        " .long 661b - .\n"                             /* label           */ \
-        " .long " b_replacement(number)"f - .\n"        /* new instruction */ \
-        " .word " __stringify(feature) "\n"             /* feature bit     */ \
-        " .byte " alt_slen "\n"                         /* source len      */ \
-        " .byte " alt_rlen(number) "\n"                 /* replacement len */
+#define ALTINSTR_ENTRY(feature, num)                                    \
+        " .long .L%=_orig_s - .\n"                /* label           */ \
+        " .long " repl_s(num)" - .\n"             /* new instruction */ \
+        " .word " __stringify(feature) "\n"       /* feature bit     */ \
+        " .byte " alt_orig_len "\n"               /* source len      */ \
+        " .byte " alt_repl_len(num) "\n"          /* replacement len */
 
-#define DISCARD_ENTRY(number)                           /* rlen <= slen */    \
-        " .byte 0xff + (" alt_rlen(number) ") - (" alt_slen ")\n"
+#define DISCARD_ENTRY(num)                        /* repl <= orig */    \
+        " .byte 0xff + (" alt_repl_len(num) ") - (" alt_orig_len ")\n"
 
-#define ALTINSTR_REPLACEMENT(newinstr, feature, number) /* replacement */     \
-        b_replacement(number)":\n\t" newinstr "\n" e_replacement(number) ":\n\t"
-
-#define ALTERNATIVE_N(newinstr, feature, number)	\
-	".pushsection .altinstructions,\"a\"\n"		\
-	ALTINSTR_ENTRY(feature, number)			\
-	".section .discard,\"a\",@progbits\n"		\
-	DISCARD_ENTRY(number)				\
-	".section .altinstr_replacement, \"ax\"\n"	\
-	ALTINSTR_REPLACEMENT(newinstr, feature, number)	\
-	".popsection\n"
+#define ALTINSTR_REPLACEMENT(newinstr, num)       /* replacement */     \
+        repl_s(num)":\n\t" newinstr "\n" repl_e(num) ":\n\t"
 
 /* alternative assembly primitive: */
-#define ALTERNATIVE(oldinstr, newinstr, feature)			  \
-	OLDINSTR(oldinstr)						  \
-	ALTERNATIVE_N(newinstr, feature, 1)
+#define ALTERNATIVE(oldinstr, newinstr, feature)                        \
+        OLDINSTR(oldinstr)                                              \
+        ".pushsection .altinstructions, \"a\", @progbits\n"             \
+        ALTINSTR_ENTRY(feature, 1)                                      \
+        ".section .discard, \"a\", @progbits\n"                         \
+        DISCARD_ENTRY(1)                                                \
+        ".section .altinstr_replacement, \"ax\", @progbits\n"           \
+        ALTINSTR_REPLACEMENT(newinstr, 1)                               \
+        ".popsection\n"
 
 #define ALTERNATIVE_2(oldinstr, newinstr1, feature1, newinstr2, feature2) \
-	ALTERNATIVE(oldinstr, newinstr1, feature1)			  \
-	ALTERNATIVE_N(newinstr2, feature2, 2)
+        OLDINSTR(oldinstr)                                              \
+        ".pushsection .altinstructions, \"a\", @progbits\n"             \
+        ALTINSTR_ENTRY(feature1, 1)                                     \
+        ALTINSTR_ENTRY(feature2, 2)                                     \
+        ".section .discard, \"a\", @progbits\n"                         \
+        DISCARD_ENTRY(1)                                                \
+        DISCARD_ENTRY(2)                                                \
+        ".section .altinstr_replacement, \"ax\", @progbits\n"           \
+        ALTINSTR_REPLACEMENT(newinstr1, 1)                              \
+        ALTINSTR_REPLACEMENT(newinstr2, 2)                              \
+        ".popsection\n"
 
 /*
  * Alternative instructions for different CPU types or capabilities.
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 3/7] x86/alt: Clean up the assembly used to generate alternatives

[Wei Liu]

On Mon, Feb 12, 2018 at 11:23:03AM +0000, Andrew Cooper wrote:
>  * On the C side, switch to using local lables rather than hardcoded numbers.
>  * Rename parameters and lables to be consistent with alt_instr names, and
>    consistent between the the C and asm versions.
>  * On the asm side, factor some expressions out into macros to aid clarity.
>  * Consistently declare section attributes.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Wei Liu <wei.liu2@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 3/7] x86/alt: Clean up the assembly used to generate alternatives

[Roger Pau Monné]

On Mon, Feb 12, 2018 at 11:23:03AM +0000, Andrew Cooper wrote:
>  * On the C side, switch to using local lables rather than hardcoded numbers.
                                          ^ labels
>  * Rename parameters and lables to be consistent with alt_instr names, and
                           ^ labels
>    consistent between the the C and asm versions.
>  * On the asm side, factor some expressions out into macros to aid clarity.
>  * Consistently declare section attributes.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Just one nit...

> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> ---
>  xen/include/asm-x86/alternative-asm.h | 57 +++++++++++++++++--------------
>  xen/include/asm-x86/alternative.h     | 64 +++++++++++++++++++----------------
>  2 files changed, 67 insertions(+), 54 deletions(-)
> 
> diff --git a/xen/include/asm-x86/alternative-asm.h b/xen/include/asm-x86/alternative-asm.h
> index 6640e85..150bd1a 100644
> --- a/xen/include/asm-x86/alternative-asm.h
> +++ b/xen/include/asm-x86/alternative-asm.h
> @@ -9,60 +9,67 @@
>   * enough information for the alternatives patching code to patch an
>   * instruction. See apply_alternatives().
>   */
> -.macro altinstruction_entry orig alt feature orig_len alt_len
> +.macro altinstruction_entry orig repl feature orig_len repl_len
>      .long \orig - .
> -    .long \alt - .
> +    .long \repl - .
>      .word \feature
>      .byte \orig_len
> -    .byte \alt_len
> +    .byte \repl_len
>  .endm
>  
> +#define orig_len               (.L\@_orig_e       -     .L\@_orig_s)
> +#define repl_len(nr)           (.L\@_repl_e\()nr  -     .L\@_repl_s\()nr)
> +#define decl_repl(insn, nr)     .L\@_repl_s\()nr: insn; .L\@_repl_e\()nr:

I would also introduce a decl_orig(insn), seeing that ".L\@_orig_s" is
already used in two different places (ALTERNATIVE and ALTERNATIVE_2).

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 3/7] x86/alt: Clean up the assembly used to generate alternatives

[Andrew Cooper]

On 12/02/18 17:26, Roger Pau Monné wrote:
> On Mon, Feb 12, 2018 at 11:23:03AM +0000, Andrew Cooper wrote:
>>  * On the C side, switch to using local lables rather than hardcoded numbers.
>                                           ^ labels
>>  * Rename parameters and lables to be consistent with alt_instr names, and
>                            ^ labels
>>    consistent between the the C and asm versions.
>>  * On the asm side, factor some expressions out into macros to aid clarity.
>>  * Consistently declare section attributes.
>>
>> No functional change.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
>
> Just one nit...
>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>> CC: Roger Pau Monné <roger.pau@citrix.com>
>> CC: Wei Liu <wei.liu2@citrix.com>
>> ---
>>  xen/include/asm-x86/alternative-asm.h | 57 +++++++++++++++++--------------
>>  xen/include/asm-x86/alternative.h     | 64 +++++++++++++++++++----------------
>>  2 files changed, 67 insertions(+), 54 deletions(-)
>>
>> diff --git a/xen/include/asm-x86/alternative-asm.h b/xen/include/asm-x86/alternative-asm.h
>> index 6640e85..150bd1a 100644
>> --- a/xen/include/asm-x86/alternative-asm.h
>> +++ b/xen/include/asm-x86/alternative-asm.h
>> @@ -9,60 +9,67 @@
>>   * enough information for the alternatives patching code to patch an
>>   * instruction. See apply_alternatives().
>>   */
>> -.macro altinstruction_entry orig alt feature orig_len alt_len
>> +.macro altinstruction_entry orig repl feature orig_len repl_len
>>      .long \orig - .
>> -    .long \alt - .
>> +    .long \repl - .
>>      .word \feature
>>      .byte \orig_len
>> -    .byte \alt_len
>> +    .byte \repl_len
>>  .endm
>>  
>> +#define orig_len               (.L\@_orig_e       -     .L\@_orig_s)
>> +#define repl_len(nr)           (.L\@_repl_e\()nr  -     .L\@_repl_s\()nr)
>> +#define decl_repl(insn, nr)     .L\@_repl_s\()nr: insn; .L\@_repl_e\()nr:
> I would also introduce a decl_orig(insn), seeing that ".L\@_orig_s" is
> already used in two different places (ALTERNATIVE and ALTERNATIVE_2).

Actually, in combination with patch 5, that makes things less clear.  (I
already did as you suggested, then took it out.)

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 3/7] x86/alt: Clean up the assembly used to generate alternatives

[Jan Beulich]

 >>> On 12.02.18 at 12:23, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/include/asm-x86/alternative-asm.h
> +++ b/xen/include/asm-x86/alternative-asm.h
> @@ -9,60 +9,67 @@
>   * enough information for the alternatives patching code to patch an
>   * instruction. See apply_alternatives().
>   */
> -.macro altinstruction_entry orig alt feature orig_len alt_len
> +.macro altinstruction_entry orig repl feature orig_len repl_len
>      .long \orig - .
> -    .long \alt - .
> +    .long \repl - .
>      .word \feature
>      .byte \orig_len
> -    .byte \alt_len
> +    .byte \repl_len
>  .endm
>  
> +#define orig_len               (.L\@_orig_e       -     .L\@_orig_s)
> +#define repl_len(nr)           (.L\@_repl_e\()nr  -     .L\@_repl_s\()nr)
> +#define decl_repl(insn, nr)     .L\@_repl_s\()nr: insn; .L\@_repl_e\()nr:

Wouldn't it work equally well but look slightly less odd if you used
\(nr) instead of \()nr?

> --- a/xen/include/asm-x86/alternative.h
> +++ b/xen/include/asm-x86/alternative.h
> @@ -26,44 +26,50 @@ extern void apply_alternatives(const struct alt_instr *start,
>                                 const struct alt_instr *end);
>  extern void alternative_instructions(void);
>  
> -#define OLDINSTR(oldinstr)      "661:\n\t" oldinstr "\n662:\n"
> +#define OLDINSTR(oldinstr)      ".L%=_orig_s:\n\t" oldinstr "\n.L%=_orig_e:\n"

Isn't this too similar a naming scheme to what the assembler side
uses? I.e. is it entirely certain that no C file will ever (indirectly)
include alternative-asm.h, potentially resulting in a label name
clash then?

Here please also don't forget that you're competing with the
compiler for the .L name space, so some better disambiguation
may be advisable (e.g. starting the names with .LXEN).

> -#define b_replacement(number)   "663"#number
> -#define e_replacement(number)   "664"#number
> +#define repl_s(num)             ".L%=_repl_s"#num
> +#define repl_e(num)             ".L%=_repl_e"#num

Since you don't (and can't) #undef them, how about alt_repl_s()
and alt_repl_e()?

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 3/7] x86/alt: Clean up the assembly used to generate alternatives

[Andrew Cooper]

On 13/02/18 14:37, Jan Beulich wrote:
>  >>> On 12.02.18 at 12:23, <andrew.cooper3@citrix.com> wrote:
>> --- a/xen/include/asm-x86/alternative-asm.h
>> +++ b/xen/include/asm-x86/alternative-asm.h
>> @@ -9,60 +9,67 @@
>>   * enough information for the alternatives patching code to patch an
>>   * instruction. See apply_alternatives().
>>   */
>> -.macro altinstruction_entry orig alt feature orig_len alt_len
>> +.macro altinstruction_entry orig repl feature orig_len repl_len
>>      .long \orig - .
>> -    .long \alt - .
>> +    .long \repl - .
>>      .word \feature
>>      .byte \orig_len
>> -    .byte \alt_len
>> +    .byte \repl_len
>>  .endm
>>  
>> +#define orig_len               (.L\@_orig_e       -     .L\@_orig_s)
>> +#define repl_len(nr)           (.L\@_repl_e\()nr  -     .L\@_repl_s\()nr)
>> +#define decl_repl(insn, nr)     .L\@_repl_s\()nr: insn; .L\@_repl_e\()nr:
> Wouldn't it work equally well but look slightly less odd if you used
> \(nr) instead of \()nr?

How would that work?  \() is the token separator.

>
>> --- a/xen/include/asm-x86/alternative.h
>> +++ b/xen/include/asm-x86/alternative.h
>> @@ -26,44 +26,50 @@ extern void apply_alternatives(const struct alt_instr *start,
>>                                 const struct alt_instr *end);
>>  extern void alternative_instructions(void);
>>  
>> -#define OLDINSTR(oldinstr)      "661:\n\t" oldinstr "\n662:\n"
>> +#define OLDINSTR(oldinstr)      ".L%=_orig_s:\n\t" oldinstr "\n.L%=_orig_e:\n"
> Isn't this too similar a naming scheme to what the assembler side
> uses? I.e. is it entirely certain that no C file will ever (indirectly)
> include alternative-asm.h, potentially resulting in a label name
> clash then?

It is intended to be the same, for consistency.  As there are no asm (
".include alternatives-asm.h" ), there is no chance of both definitions
existing in the same translation unit.

As for potentially doing an asm level include, the resulting is
prohibitively awkward to use, so I don't think it is a worry.

>
> Here please also don't forget that you're competing with the
> compiler for the .L name space, so some better disambiguation
> may be advisable (e.g. starting the names with .LXEN).
>
>> -#define b_replacement(number)   "663"#number
>> -#define e_replacement(number)   "664"#number
>> +#define repl_s(num)             ".L%=_repl_s"#num
>> +#define repl_e(num)             ".L%=_repl_e"#num
> Since you don't (and can't) #undef them, how about alt_repl_s()
> and alt_repl_e()?

Ok.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 3/7] x86/alt: Clean up the assembly used to generate alternatives

[Jan Beulich]

>>> On 23.02.18 at 15:03, <andrew.cooper3@citrix.com> wrote:
> On 13/02/18 14:37, Jan Beulich wrote:
>>  >>> On 12.02.18 at 12:23, <andrew.cooper3@citrix.com> wrote:
>>> --- a/xen/include/asm-x86/alternative-asm.h
>>> +++ b/xen/include/asm-x86/alternative-asm.h
>>> @@ -9,60 +9,67 @@
>>>   * enough information for the alternatives patching code to patch an
>>>   * instruction. See apply_alternatives().
>>>   */
>>> -.macro altinstruction_entry orig alt feature orig_len alt_len
>>> +.macro altinstruction_entry orig repl feature orig_len repl_len
>>>      .long \orig - .
>>> -    .long \alt - .
>>> +    .long \repl - .
>>>      .word \feature
>>>      .byte \orig_len
>>> -    .byte \alt_len
>>> +    .byte \repl_len
>>>  .endm
>>>  
>>> +#define orig_len               (.L\@_orig_e       -     .L\@_orig_s)
>>> +#define repl_len(nr)           (.L\@_repl_e\()nr  -     .L\@_repl_s\()nr)
>>> +#define decl_repl(insn, nr)     .L\@_repl_s\()nr: insn; .L\@_repl_e\()nr:
>> Wouldn't it work equally well but look slightly less odd if you used
>> \(nr) instead of \()nr?
> 
> How would that work?  \() is the token separator.

When there's nothing inside the parentheses, this construct
can be used as a token separator, but that's not its main
purpose. Instead \(<text>) means to take <text> literally,
without e.g. expanding macro arguments inside it.

>>> --- a/xen/include/asm-x86/alternative.h
>>> +++ b/xen/include/asm-x86/alternative.h
>>> @@ -26,44 +26,50 @@ extern void apply_alternatives(const struct alt_instr 
> *start,
>>>                                 const struct alt_instr *end);
>>>  extern void alternative_instructions(void);
>>>  
>>> -#define OLDINSTR(oldinstr)      "661:\n\t" oldinstr "\n662:\n"
>>> +#define OLDINSTR(oldinstr)      ".L%=_orig_s:\n\t" oldinstr "\n.L%=_orig_e:\n"
>> Isn't this too similar a naming scheme to what the assembler side
>> uses? I.e. is it entirely certain that no C file will ever (indirectly)
>> include alternative-asm.h, potentially resulting in a label name
>> clash then?
> 
> It is intended to be the same, for consistency.  As there are no asm (
> ".include alternatives-asm.h" ), there is no chance of both definitions
> existing in the same translation unit.
> 
> As for potentially doing an asm level include, the resulting is
> prohibitively awkward to use, so I don't think it is a worry.

Well, okay then.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 3/7] x86/alt: Clean up the assembly used to generate alternatives

[Andrew Cooper]

On 23/02/18 15:12, Jan Beulich wrote:
>>>> On 23.02.18 at 15:03, <andrew.cooper3@citrix.com> wrote:
>> On 13/02/18 14:37, Jan Beulich wrote:
>>>  >>> On 12.02.18 at 12:23, <andrew.cooper3@citrix.com> wrote:
>>>> --- a/xen/include/asm-x86/alternative-asm.h
>>>> +++ b/xen/include/asm-x86/alternative-asm.h
>>>> @@ -9,60 +9,67 @@
>>>>   * enough information for the alternatives patching code to patch an
>>>>   * instruction. See apply_alternatives().
>>>>   */
>>>> -.macro altinstruction_entry orig alt feature orig_len alt_len
>>>> +.macro altinstruction_entry orig repl feature orig_len repl_len
>>>>      .long \orig - .
>>>> -    .long \alt - .
>>>> +    .long \repl - .
>>>>      .word \feature
>>>>      .byte \orig_len
>>>> -    .byte \alt_len
>>>> +    .byte \repl_len
>>>>  .endm
>>>>  
>>>> +#define orig_len               (.L\@_orig_e       -     .L\@_orig_s)
>>>> +#define repl_len(nr)           (.L\@_repl_e\()nr  -     .L\@_repl_s\()nr)
>>>> +#define decl_repl(insn, nr)     .L\@_repl_s\()nr: insn; .L\@_repl_e\()nr:
>>> Wouldn't it work equally well but look slightly less odd if you used
>>> \(nr) instead of \()nr?
>> How would that work?  \() is the token separator.
> When there's nothing inside the parentheses, this construct
> can be used as a token separator, but that's not its main
> purpose. Instead \(<text>) means to take <text> literally,
> without e.g. expanding macro arguments inside it.

I've never come across it before, and I still can't find reference to it
in the as manual.

As for why not to use it, Clang has no idea what \(nr) means, meaning
that it doesn't expand the construct in the way you describe.  (Although
I see that GCC/AS do behave as you describe).

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 3/7] x86/alt: Clean up the assembly used to generate alternatives

[Jan Beulich]

>>> On 23.02.18 at 17:24, <andrew.cooper3@citrix.com> wrote:
> On 23/02/18 15:12, Jan Beulich wrote:
>>>>> On 23.02.18 at 15:03, <andrew.cooper3@citrix.com> wrote:
>>> On 13/02/18 14:37, Jan Beulich wrote:
>>>>  >>> On 12.02.18 at 12:23, <andrew.cooper3@citrix.com> wrote:
>>>>> --- a/xen/include/asm-x86/alternative-asm.h
>>>>> +++ b/xen/include/asm-x86/alternative-asm.h
>>>>> @@ -9,60 +9,67 @@
>>>>>   * enough information for the alternatives patching code to patch an
>>>>>   * instruction. See apply_alternatives().
>>>>>   */
>>>>> -.macro altinstruction_entry orig alt feature orig_len alt_len
>>>>> +.macro altinstruction_entry orig repl feature orig_len repl_len
>>>>>      .long \orig - .
>>>>> -    .long \alt - .
>>>>> +    .long \repl - .
>>>>>      .word \feature
>>>>>      .byte \orig_len
>>>>> -    .byte \alt_len
>>>>> +    .byte \repl_len
>>>>>  .endm
>>>>>  
>>>>> +#define orig_len               (.L\@_orig_e       -     .L\@_orig_s)
>>>>> +#define repl_len(nr)           (.L\@_repl_e\()nr  -     .L\@_repl_s\()nr)
>>>>> +#define decl_repl(insn, nr)     .L\@_repl_s\()nr: insn; .L\@_repl_e\()nr:
>>>> Wouldn't it work equally well but look slightly less odd if you used
>>>> \(nr) instead of \()nr?
>>> How would that work?  \() is the token separator.
>> When there's nothing inside the parentheses, this construct
>> can be used as a token separator, but that's not its main
>> purpose. Instead \(<text>) means to take <text> literally,
>> without e.g. expanding macro arguments inside it.
> 
> I've never come across it before, and I still can't find reference to it
> in the as manual.
> 
> As for why not to use it, Clang has no idea what \(nr) means, meaning
> that it doesn't expand the construct in the way you describe.  (Although
> I see that GCC/AS do behave as you describe).

Oh, if clang can't cope, then leave it as is.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 4/7] x86/asm: Remove opencoded uses of altinstruction_entry

[Andrew Cooper]

With future changes, altinstruction_entry is going to become more complicated
to use.  Furthermore, there are already ALTERNATIVE* macros which can be used
to avoid opencoding the creation of replacement information.

For ASM_STAC, ASM_CLAC and CR4_PV32_RESTORE, this means the removal of all
hardocded label numbers.  For the cr4_pv32 alternatives, this means hardcoding
the extra space required in the original patch site, but the hardcoding will
be removed by a later patch.

No change to any functionality, but the handling of nops inside the original
patch sites are a bit different.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
 xen/arch/x86/x86_64/compat/entry.S | 26 +++++++++-----------------
 xen/arch/x86/x86_64/entry.S        | 20 +++-----------------
 xen/include/asm-x86/asm_defns.h    | 32 +++++++++++---------------------
 3 files changed, 23 insertions(+), 55 deletions(-)

diff --git a/xen/arch/x86/x86_64/compat/entry.S b/xen/arch/x86/x86_64/compat/entry.S
index 707c746..e93770f 100644
--- a/xen/arch/x86/x86_64/compat/entry.S
+++ b/xen/arch/x86/x86_64/compat/entry.S
@@ -109,13 +109,10 @@ ENTRY(compat_restore_all_guest)
         ASSERT_INTERRUPTS_DISABLED
         mov   $~(X86_EFLAGS_IOPL|X86_EFLAGS_NT|X86_EFLAGS_VM),%r11d
         and   UREGS_eflags(%rsp),%r11d
-.Lcr4_orig:
-        .skip .Lcr4_alt_end - .Lcr4_alt, 0x90
-.Lcr4_orig_end:
-        .pushsection .altinstr_replacement, "ax"
-.Lcr4_alt:
+
+.macro alt_cr4_pv32
         testb $3,UREGS_cs(%rsp)
-        jpe   .Lcr4_alt_end
+        jpe   2f
         mov   CPUINFO_cr4-CPUINFO_guest_cpu_user_regs(%rsp), %rax
         and   $~XEN_CR4_PV32_BITS, %rax
 1:
@@ -133,17 +130,12 @@ ENTRY(compat_restore_all_guest)
          */
         cmp   %rax, CPUINFO_cr4-CPUINFO_guest_cpu_user_regs(%rsp)
         jne   1b
-.Lcr4_alt_end:
-        .section .altinstructions, "a"
-        altinstruction_entry .Lcr4_orig, .Lcr4_orig, X86_FEATURE_ALWAYS, \
-                             (.Lcr4_orig_end - .Lcr4_orig), 0
-        altinstruction_entry .Lcr4_orig, .Lcr4_alt, X86_FEATURE_XEN_SMEP, \
-                             (.Lcr4_orig_end - .Lcr4_orig), \
-                             (.Lcr4_alt_end - .Lcr4_alt)
-        altinstruction_entry .Lcr4_orig, .Lcr4_alt, X86_FEATURE_XEN_SMAP, \
-                             (.Lcr4_orig_end - .Lcr4_orig), \
-                             (.Lcr4_alt_end - .Lcr4_alt)
-        .popsection
+2:
+.endm
+	ALTERNATIVE_2 ".skip 45, 0x90", \
+            alt_cr4_pv32, X86_FEATURE_XEN_SMEP, \
+            alt_cr4_pv32, X86_FEATURE_XEN_SMAP
+
         or    $X86_EFLAGS_IF,%r11
         mov   %r11d,UREGS_eflags(%rsp)
 
diff --git a/xen/arch/x86/x86_64/entry.S b/xen/arch/x86/x86_64/entry.S
index 58f652d..bd3819a 100644
--- a/xen/arch/x86/x86_64/entry.S
+++ b/xen/arch/x86/x86_64/entry.S
@@ -557,23 +557,9 @@ handle_exception_saved:
         testb $X86_EFLAGS_IF>>8,UREGS_eflags+1(%rsp)
         jz    exception_with_ints_disabled
 
-.Lcr4_pv32_orig:
-        jmp   .Lcr4_pv32_done
-        .skip (.Lcr4_pv32_alt_end - .Lcr4_pv32_alt) - (. - .Lcr4_pv32_orig), 0xcc
-        .pushsection .altinstr_replacement, "ax"
-.Lcr4_pv32_alt:
-        mov   VCPU_domain(%rbx),%rax
-.Lcr4_pv32_alt_end:
-        .section .altinstructions, "a"
-        altinstruction_entry .Lcr4_pv32_orig, .Lcr4_pv32_alt, \
-                             X86_FEATURE_XEN_SMEP, \
-                             (.Lcr4_pv32_alt_end - .Lcr4_pv32_alt), \
-                             (.Lcr4_pv32_alt_end - .Lcr4_pv32_alt)
-        altinstruction_entry .Lcr4_pv32_orig, .Lcr4_pv32_alt, \
-                             X86_FEATURE_XEN_SMAP, \
-                             (.Lcr4_pv32_alt_end - .Lcr4_pv32_alt), \
-                             (.Lcr4_pv32_alt_end - .Lcr4_pv32_alt)
-        .popsection
+        ALTERNATIVE_2 "jmp .Lcr4_pv32_done; .skip 2, 0x90", \
+            __stringify(mov VCPU_domain(%rbx), %rax), X86_FEATURE_XEN_SMEP, \
+            __stringify(mov VCPU_domain(%rbx), %rax), X86_FEATURE_XEN_SMAP
 
         testb $3,UREGS_cs(%rsp)
         jz    .Lcr4_pv32_done
diff --git a/xen/include/asm-x86/asm_defns.h b/xen/include/asm-x86/asm_defns.h
index aee14ba..b9140de 100644
--- a/xen/include/asm-x86/asm_defns.h
+++ b/xen/include/asm-x86/asm_defns.h
@@ -192,18 +192,13 @@ void ret_from_intr(void);
 #define __ASM_STAC      .byte 0x0f,0x01,0xcb
 
 #ifdef __ASSEMBLY__
-#define ASM_AC(op)                                                     \
-        661: ASM_NOP3;                                                 \
-        .pushsection .altinstr_replacement, "ax";                      \
-        662: __ASM_##op;                                               \
-        .popsection;                                                   \
-        .pushsection .altinstructions, "a";                            \
-        altinstruction_entry 661b, 661b, X86_FEATURE_ALWAYS, 3, 0;     \
-        altinstruction_entry 661b, 662b, X86_FEATURE_XEN_SMAP, 3, 3;       \
-        .popsection
-
-#define ASM_STAC ASM_AC(STAC)
-#define ASM_CLAC ASM_AC(CLAC)
+#define ASM_STAC                                        \
+    ALTERNATIVE __stringify(ASM_NOP3),                  \
+        __stringify(__ASM_STAC), X86_FEATURE_XEN_SMAP
+
+#define ASM_CLAC                                        \
+    ALTERNATIVE __stringify(ASM_NOP3),                  \
+        __stringify(__ASM_CLAC), X86_FEATURE_XEN_SMAP
 
 .macro write_cr3 val:req, tmp1:req, tmp2:req
         mov   %cr4, %\tmp1
@@ -214,15 +209,10 @@ void ret_from_intr(void);
         mov   %\tmp2, %cr4
 .endm
 
-#define CR4_PV32_RESTORE                                           \
-        667: ASM_NOP5;                                             \
-        .pushsection .altinstr_replacement, "ax";                  \
-        668: call cr4_pv32_restore;                                \
-        .section .altinstructions, "a";                            \
-        altinstruction_entry 667b, 667b, X86_FEATURE_ALWAYS, 5, 0; \
-        altinstruction_entry 667b, 668b, X86_FEATURE_XEN_SMEP, 5, 5;   \
-        altinstruction_entry 667b, 668b, X86_FEATURE_XEN_SMAP, 5, 5;   \
-        .popsection
+#define CR4_PV32_RESTORE                                \
+    ALTERNATIVE_2 __stringify(ASM_NOP5),                \
+        "call cr4_pv32_restore", X86_FEATURE_XEN_SMEP,  \
+        "call cr4_pv32_restore", X86_FEATURE_XEN_SMAP
 
 #else
 static always_inline void clac(void)
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 4/7] x86/asm: Remove opencoded uses of altinstruction_entry

[Wei Liu]

On Mon, Feb 12, 2018 at 11:23:04AM +0000, Andrew Cooper wrote:
> diff --git a/xen/arch/x86/x86_64/entry.S b/xen/arch/x86/x86_64/entry.S
> index 58f652d..bd3819a 100644
> --- a/xen/arch/x86/x86_64/entry.S
> +++ b/xen/arch/x86/x86_64/entry.S
> @@ -557,23 +557,9 @@ handle_exception_saved:
>          testb $X86_EFLAGS_IF>>8,UREGS_eflags+1(%rsp)
>          jz    exception_with_ints_disabled
>  
> -.Lcr4_pv32_orig:
> -        jmp   .Lcr4_pv32_done
> -        .skip (.Lcr4_pv32_alt_end - .Lcr4_pv32_alt) - (. - .Lcr4_pv32_orig), 0xcc
> -        .pushsection .altinstr_replacement, "ax"
> -.Lcr4_pv32_alt:
> -        mov   VCPU_domain(%rbx),%rax
> -.Lcr4_pv32_alt_end:
> -        .section .altinstructions, "a"
> -        altinstruction_entry .Lcr4_pv32_orig, .Lcr4_pv32_alt, \
> -                             X86_FEATURE_XEN_SMEP, \
> -                             (.Lcr4_pv32_alt_end - .Lcr4_pv32_alt), \
> -                             (.Lcr4_pv32_alt_end - .Lcr4_pv32_alt)
> -        altinstruction_entry .Lcr4_pv32_orig, .Lcr4_pv32_alt, \
> -                             X86_FEATURE_XEN_SMAP, \
> -                             (.Lcr4_pv32_alt_end - .Lcr4_pv32_alt), \
> -                             (.Lcr4_pv32_alt_end - .Lcr4_pv32_alt)
> -        .popsection
> +        ALTERNATIVE_2 "jmp .Lcr4_pv32_done; .skip 2, 0x90", \

This changed 0xcc to 0x90 but since it is just padding following an
unconditional jmp so it shouldn't matter.

Mostly looks fine. I'm a bit hesitant to give my Rb because reviewing
the diff to assembly is a bit more difficult to than diff to C source.

Do you have a branch somewhere?

Wei.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 4/7] x86/asm: Remove opencoded uses of altinstruction_entry

[Andrew Cooper]

On 12/02/18 12:30, Wei Liu wrote:
> On Mon, Feb 12, 2018 at 11:23:04AM +0000, Andrew Cooper wrote:
>> diff --git a/xen/arch/x86/x86_64/entry.S b/xen/arch/x86/x86_64/entry.S
>> index 58f652d..bd3819a 100644
>> --- a/xen/arch/x86/x86_64/entry.S
>> +++ b/xen/arch/x86/x86_64/entry.S
>> @@ -557,23 +557,9 @@ handle_exception_saved:
>>          testb $X86_EFLAGS_IF>>8,UREGS_eflags+1(%rsp)
>>          jz    exception_with_ints_disabled
>>  
>> -.Lcr4_pv32_orig:
>> -        jmp   .Lcr4_pv32_done
>> -        .skip (.Lcr4_pv32_alt_end - .Lcr4_pv32_alt) - (. - .Lcr4_pv32_orig), 0xcc
>> -        .pushsection .altinstr_replacement, "ax"
>> -.Lcr4_pv32_alt:
>> -        mov   VCPU_domain(%rbx),%rax
>> -.Lcr4_pv32_alt_end:
>> -        .section .altinstructions, "a"
>> -        altinstruction_entry .Lcr4_pv32_orig, .Lcr4_pv32_alt, \
>> -                             X86_FEATURE_XEN_SMEP, \
>> -                             (.Lcr4_pv32_alt_end - .Lcr4_pv32_alt), \
>> -                             (.Lcr4_pv32_alt_end - .Lcr4_pv32_alt)
>> -        altinstruction_entry .Lcr4_pv32_orig, .Lcr4_pv32_alt, \
>> -                             X86_FEATURE_XEN_SMAP, \
>> -                             (.Lcr4_pv32_alt_end - .Lcr4_pv32_alt), \
>> -                             (.Lcr4_pv32_alt_end - .Lcr4_pv32_alt)
>> -        .popsection
>> +        ALTERNATIVE_2 "jmp .Lcr4_pv32_done; .skip 2, 0x90", \
> This changed 0xcc to 0x90 but since it is just padding following an
> unconditional jmp so it shouldn't matter.
>
> Mostly looks fine. I'm a bit hesitant to give my Rb because reviewing
> the diff to assembly is a bit more difficult to than diff to C source.
>
> Do you have a branch somewhere?

http://xenbits.xen.org/gitweb/?p=people/andrewcoop/xen.git;a=shortlog;h=refs/heads/alternatives-v1

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 4/7] x86/asm: Remove opencoded uses of altinstruction_entry

[Jan Beulich]

>>> On 12.02.18 at 13:30, <wei.liu2@citrix.com> wrote:
> On Mon, Feb 12, 2018 at 11:23:04AM +0000, Andrew Cooper wrote:
>> diff --git a/xen/arch/x86/x86_64/entry.S b/xen/arch/x86/x86_64/entry.S
>> index 58f652d..bd3819a 100644
>> --- a/xen/arch/x86/x86_64/entry.S
>> +++ b/xen/arch/x86/x86_64/entry.S
>> @@ -557,23 +557,9 @@ handle_exception_saved:
>>          testb $X86_EFLAGS_IF>>8,UREGS_eflags+1(%rsp)
>>          jz    exception_with_ints_disabled
>>  
>> -.Lcr4_pv32_orig:
>> -        jmp   .Lcr4_pv32_done
>> -        .skip (.Lcr4_pv32_alt_end - .Lcr4_pv32_alt) - (. - .Lcr4_pv32_orig), 0xcc
>> -        .pushsection .altinstr_replacement, "ax"
>> -.Lcr4_pv32_alt:
>> -        mov   VCPU_domain(%rbx),%rax
>> -.Lcr4_pv32_alt_end:
>> -        .section .altinstructions, "a"
>> -        altinstruction_entry .Lcr4_pv32_orig, .Lcr4_pv32_alt, \
>> -                             X86_FEATURE_XEN_SMEP, \
>> -                             (.Lcr4_pv32_alt_end - .Lcr4_pv32_alt), \
>> -                             (.Lcr4_pv32_alt_end - .Lcr4_pv32_alt)
>> -        altinstruction_entry .Lcr4_pv32_orig, .Lcr4_pv32_alt, \
>> -                             X86_FEATURE_XEN_SMAP, \
>> -                             (.Lcr4_pv32_alt_end - .Lcr4_pv32_alt), \
>> -                             (.Lcr4_pv32_alt_end - .Lcr4_pv32_alt)
>> -        .popsection
>> +        ALTERNATIVE_2 "jmp .Lcr4_pv32_done; .skip 2, 0x90", \
> 
> This changed 0xcc to 0x90 but since it is just padding following an
> unconditional jmp so it shouldn't matter.

Well, it was for that very reason that I had picked 0xcc originally:
We better know if some branch mistakenly leads into that region.

I also very much object to the literal 2 passed as an argument to
.skip above: What if the label moves out far enough that a short
branch won't be usable anymore?

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 4/7] x86/asm: Remove opencoded uses of altinstruction_entry

[Andrew Cooper]

On 13/02/2018 09:56, Jan Beulich wrote:
>>>> On 12.02.18 at 13:30, <wei.liu2@citrix.com> wrote:
>> On Mon, Feb 12, 2018 at 11:23:04AM +0000, Andrew Cooper wrote:
>>> diff --git a/xen/arch/x86/x86_64/entry.S b/xen/arch/x86/x86_64/entry.S
>>> index 58f652d..bd3819a 100644
>>> --- a/xen/arch/x86/x86_64/entry.S
>>> +++ b/xen/arch/x86/x86_64/entry.S
>>> @@ -557,23 +557,9 @@ handle_exception_saved:
>>>          testb $X86_EFLAGS_IF>>8,UREGS_eflags+1(%rsp)
>>>          jz    exception_with_ints_disabled
>>>  
>>> -.Lcr4_pv32_orig:
>>> -        jmp   .Lcr4_pv32_done
>>> -        .skip (.Lcr4_pv32_alt_end - .Lcr4_pv32_alt) - (. - .Lcr4_pv32_orig), 0xcc
>>> -        .pushsection .altinstr_replacement, "ax"
>>> -.Lcr4_pv32_alt:
>>> -        mov   VCPU_domain(%rbx),%rax
>>> -.Lcr4_pv32_alt_end:
>>> -        .section .altinstructions, "a"
>>> -        altinstruction_entry .Lcr4_pv32_orig, .Lcr4_pv32_alt, \
>>> -                             X86_FEATURE_XEN_SMEP, \
>>> -                             (.Lcr4_pv32_alt_end - .Lcr4_pv32_alt), \
>>> -                             (.Lcr4_pv32_alt_end - .Lcr4_pv32_alt)
>>> -        altinstruction_entry .Lcr4_pv32_orig, .Lcr4_pv32_alt, \
>>> -                             X86_FEATURE_XEN_SMAP, \
>>> -                             (.Lcr4_pv32_alt_end - .Lcr4_pv32_alt), \
>>> -                             (.Lcr4_pv32_alt_end - .Lcr4_pv32_alt)
>>> -        .popsection
>>> +        ALTERNATIVE_2 "jmp .Lcr4_pv32_done; .skip 2, 0x90", \
>> This changed 0xcc to 0x90 but since it is just padding following an
>> unconditional jmp so it shouldn't matter.
> Well, it was for that very reason that I had picked 0xcc originally:
> We better know if some branch mistakenly leads into that region.

Know how?  At the time you wrote this, Xen silently executed its way
through debug traps, and it took some persuading to get you to ok the
patch which at least printed a line every time we a breakpoint in
hypervisor space.

If you actually want to notice going down the wrong path, then you want
a BUG.

> I also very much object to the literal 2 passed as an argument to
> .skip above: What if the label moves out far enough that a short
> branch won't be usable anymore?

Is the commit message not enough?  a) its not going to change, because
it hasn't changed since you put the code in originally and I don't
expect it to in the future, and b) it is a temporary necessary
requirement to make the series bisectable and reviewable.  This skip is
dropped in patch 6 when the automatic padding calculations work.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 4/7] x86/asm: Remove opencoded uses of altinstruction_entry

[Jan Beulich]

>>> On 13.02.18 at 11:07, <andrew.cooper3@citrix.com> wrote:
> On 13/02/2018 09:56, Jan Beulich wrote:
>>>>> On 12.02.18 at 13:30, <wei.liu2@citrix.com> wrote:
>>> On Mon, Feb 12, 2018 at 11:23:04AM +0000, Andrew Cooper wrote:
>>>> diff --git a/xen/arch/x86/x86_64/entry.S b/xen/arch/x86/x86_64/entry.S
>>>> index 58f652d..bd3819a 100644
>>>> --- a/xen/arch/x86/x86_64/entry.S
>>>> +++ b/xen/arch/x86/x86_64/entry.S
>>>> @@ -557,23 +557,9 @@ handle_exception_saved:
>>>>          testb $X86_EFLAGS_IF>>8,UREGS_eflags+1(%rsp)
>>>>          jz    exception_with_ints_disabled
>>>>  
>>>> -.Lcr4_pv32_orig:
>>>> -        jmp   .Lcr4_pv32_done
>>>> -        .skip (.Lcr4_pv32_alt_end - .Lcr4_pv32_alt) - (. - .Lcr4_pv32_orig), 0xcc
>>>> -        .pushsection .altinstr_replacement, "ax"
>>>> -.Lcr4_pv32_alt:
>>>> -        mov   VCPU_domain(%rbx),%rax
>>>> -.Lcr4_pv32_alt_end:
>>>> -        .section .altinstructions, "a"
>>>> -        altinstruction_entry .Lcr4_pv32_orig, .Lcr4_pv32_alt, \
>>>> -                             X86_FEATURE_XEN_SMEP, \
>>>> -                             (.Lcr4_pv32_alt_end - .Lcr4_pv32_alt), \
>>>> -                             (.Lcr4_pv32_alt_end - .Lcr4_pv32_alt)
>>>> -        altinstruction_entry .Lcr4_pv32_orig, .Lcr4_pv32_alt, \
>>>> -                             X86_FEATURE_XEN_SMAP, \
>>>> -                             (.Lcr4_pv32_alt_end - .Lcr4_pv32_alt), \
>>>> -                             (.Lcr4_pv32_alt_end - .Lcr4_pv32_alt)
>>>> -        .popsection
>>>> +        ALTERNATIVE_2 "jmp .Lcr4_pv32_done; .skip 2, 0x90", \
>>> This changed 0xcc to 0x90 but since it is just padding following an
>>> unconditional jmp so it shouldn't matter.
>> Well, it was for that very reason that I had picked 0xcc originally:
>> We better know if some branch mistakenly leads into that region.
> 
> Know how?  At the time you wrote this, Xen silently executed its way
> through debug traps, and it took some persuading to get you to ok the
> patch which at least printed a line every time we a breakpoint in
> hypervisor space.

Granted I didn't realize at the time that breakpoints would go all
silent.

> If you actually want to notice going down the wrong path, then you want
> a BUG.

I'd be very much in favor of this, if only there was a single byte insn
documented to cause #UD now and forever. Abusing what is INTO or
SALC outside of 64-bit mode doesn't look very attractive.

>> I also very much object to the literal 2 passed as an argument to
>> .skip above: What if the label moves out far enough that a short
>> branch won't be usable anymore?
> 
> Is the commit message not enough?  a) its not going to change, because
> it hasn't changed since you put the code in originally and I don't
> expect it to in the future, and b) it is a temporary necessary
> requirement to make the series bisectable and reviewable.  This skip is
> dropped in patch 6 when the automatic padding calculations work.

Oh, if it goes away by the end of the series, then that's fine.
(When replying here I hadn't looked at the full patch yet, so please
accept my apologies if this is properly explained in the description.)

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 4/7] x86/asm: Remove opencoded uses of altinstruction_entry

[Wei Liu]

On Mon, Feb 12, 2018 at 11:23:04AM +0000, Andrew Cooper wrote:
> With future changes, altinstruction_entry is going to become more complicated
> to use.  Furthermore, there are already ALTERNATIVE* macros which can be used
> to avoid opencoding the creation of replacement information.
> 
> For ASM_STAC, ASM_CLAC and CR4_PV32_RESTORE, this means the removal of all
> hardocded label numbers.  For the cr4_pv32 alternatives, this means hardcoding
> the extra space required in the original patch site, but the hardcoding will
> be removed by a later patch.
> 
> No change to any functionality, but the handling of nops inside the original
> patch sites are a bit different.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Wei Liu <wei.liu2@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 4/7] x86/asm: Remove opencoded uses of altinstruction_entry

[Roger Pau Monné]

On Mon, Feb 12, 2018 at 11:23:04AM +0000, Andrew Cooper wrote:
> With future changes, altinstruction_entry is going to become more complicated
> to use.  Furthermore, there are already ALTERNATIVE* macros which can be used
> to avoid opencoding the creation of replacement information.
> 
> For ASM_STAC, ASM_CLAC and CR4_PV32_RESTORE, this means the removal of all
> hardocded label numbers.  For the cr4_pv32 alternatives, this means hardcoding
> the extra space required in the original patch site, but the hardcoding will
> be removed by a later patch.
> 
> No change to any functionality, but the handling of nops inside the original
> patch sites are a bit different.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

In general I try to align the line breaks '\' of macros, but I don't
think that's used consistently across the code at all.

Again just one nit below.

> diff --git a/xen/arch/x86/x86_64/entry.S b/xen/arch/x86/x86_64/entry.S
> index 58f652d..bd3819a 100644
> --- a/xen/arch/x86/x86_64/entry.S
> +++ b/xen/arch/x86/x86_64/entry.S
> @@ -557,23 +557,9 @@ handle_exception_saved:
>          testb $X86_EFLAGS_IF>>8,UREGS_eflags+1(%rsp)
>          jz    exception_with_ints_disabled
>  
> -.Lcr4_pv32_orig:
> -        jmp   .Lcr4_pv32_done
> -        .skip (.Lcr4_pv32_alt_end - .Lcr4_pv32_alt) - (. - .Lcr4_pv32_orig), 0xcc
> -        .pushsection .altinstr_replacement, "ax"
> -.Lcr4_pv32_alt:
> -        mov   VCPU_domain(%rbx),%rax
> -.Lcr4_pv32_alt_end:
> -        .section .altinstructions, "a"
> -        altinstruction_entry .Lcr4_pv32_orig, .Lcr4_pv32_alt, \
> -                             X86_FEATURE_XEN_SMEP, \
> -                             (.Lcr4_pv32_alt_end - .Lcr4_pv32_alt), \
> -                             (.Lcr4_pv32_alt_end - .Lcr4_pv32_alt)
> -        altinstruction_entry .Lcr4_pv32_orig, .Lcr4_pv32_alt, \
> -                             X86_FEATURE_XEN_SMAP, \
> -                             (.Lcr4_pv32_alt_end - .Lcr4_pv32_alt), \
> -                             (.Lcr4_pv32_alt_end - .Lcr4_pv32_alt)
> -        .popsection
> +        ALTERNATIVE_2 "jmp .Lcr4_pv32_done; .skip 2, 0x90", \
> +            __stringify(mov VCPU_domain(%rbx), %rax), X86_FEATURE_XEN_SMEP, \
> +            __stringify(mov VCPU_domain(%rbx), %rax), X86_FEATURE_XEN_SMAP

What's the point of using __stringify here, isn't it clearer to just
use "mov ..."?

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 4/7] x86/asm: Remove opencoded uses of altinstruction_entry

[Andrew Cooper]

On 12/02/18 17:46, Roger Pau Monné wrote:
> On Mon, Feb 12, 2018 at 11:23:04AM +0000, Andrew Cooper wrote:
>> diff --git a/xen/arch/x86/x86_64/entry.S b/xen/arch/x86/x86_64/entry.S
>> index 58f652d..bd3819a 100644
>> --- a/xen/arch/x86/x86_64/entry.S
>> +++ b/xen/arch/x86/x86_64/entry.S
>> @@ -557,23 +557,9 @@ handle_exception_saved:
>>          testb $X86_EFLAGS_IF>>8,UREGS_eflags+1(%rsp)
>>          jz    exception_with_ints_disabled
>>  
>> -.Lcr4_pv32_orig:
>> -        jmp   .Lcr4_pv32_done
>> -        .skip (.Lcr4_pv32_alt_end - .Lcr4_pv32_alt) - (. - .Lcr4_pv32_orig), 0xcc
>> -        .pushsection .altinstr_replacement, "ax"
>> -.Lcr4_pv32_alt:
>> -        mov   VCPU_domain(%rbx),%rax
>> -.Lcr4_pv32_alt_end:
>> -        .section .altinstructions, "a"
>> -        altinstruction_entry .Lcr4_pv32_orig, .Lcr4_pv32_alt, \
>> -                             X86_FEATURE_XEN_SMEP, \
>> -                             (.Lcr4_pv32_alt_end - .Lcr4_pv32_alt), \
>> -                             (.Lcr4_pv32_alt_end - .Lcr4_pv32_alt)
>> -        altinstruction_entry .Lcr4_pv32_orig, .Lcr4_pv32_alt, \
>> -                             X86_FEATURE_XEN_SMAP, \
>> -                             (.Lcr4_pv32_alt_end - .Lcr4_pv32_alt), \
>> -                             (.Lcr4_pv32_alt_end - .Lcr4_pv32_alt)
>> -        .popsection
>> +        ALTERNATIVE_2 "jmp .Lcr4_pv32_done; .skip 2, 0x90", \
>> +            __stringify(mov VCPU_domain(%rbx), %rax), X86_FEATURE_XEN_SMEP, \
>> +            __stringify(mov VCPU_domain(%rbx), %rax), X86_FEATURE_XEN_SMAP
> What's the point of using __stringify here, isn't it clearer to just
> use "mov ..."?

Assembler macro parameters are miserable.  Spaces and commas are both
separators (unless you've got an integer parameter and some brackets). 
Therefore, the "right" way to do this would be:

ALTERNATIVE_2 "jmp .Lcr4_pv32_done; .skip 2, 0x90", \
    "mov VCPU_domain(%rbx), %rax", X86_FEATURE_XEN_SMEP, \
    "mov VCPU_domain(%rbx), %rax", X86_FEATURE_XEN_SMAP

Except that you also need VCPU_domain to be expanded, hence the
__stringify().

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 4/7] x86/asm: Remove opencoded uses of altinstruction_entry

[Jan Beulich]

>>> On 12.02.18 at 12:23, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/arch/x86/x86_64/compat/entry.S
> +++ b/xen/arch/x86/x86_64/compat/entry.S
> @@ -109,13 +109,10 @@ ENTRY(compat_restore_all_guest)
>          ASSERT_INTERRUPTS_DISABLED
>          mov   $~(X86_EFLAGS_IOPL|X86_EFLAGS_NT|X86_EFLAGS_VM),%r11d
>          and   UREGS_eflags(%rsp),%r11d
> -.Lcr4_orig:
> -        .skip .Lcr4_alt_end - .Lcr4_alt, 0x90
> -.Lcr4_orig_end:
> -        .pushsection .altinstr_replacement, "ax"
> -.Lcr4_alt:
> +
> +.macro alt_cr4_pv32
>          testb $3,UREGS_cs(%rsp)
> -        jpe   .Lcr4_alt_end
> +        jpe   2f
>          mov   CPUINFO_cr4-CPUINFO_guest_cpu_user_regs(%rsp), %rax
>          and   $~XEN_CR4_PV32_BITS, %rax
>  1:
> @@ -133,17 +130,12 @@ ENTRY(compat_restore_all_guest)
>           */
>          cmp   %rax, CPUINFO_cr4-CPUINFO_guest_cpu_user_regs(%rsp)
>          jne   1b
> -.Lcr4_alt_end:
> -        .section .altinstructions, "a"
> -        altinstruction_entry .Lcr4_orig, .Lcr4_orig, X86_FEATURE_ALWAYS, \
> -                             (.Lcr4_orig_end - .Lcr4_orig), 0
> -        altinstruction_entry .Lcr4_orig, .Lcr4_alt, X86_FEATURE_XEN_SMEP, \
> -                             (.Lcr4_orig_end - .Lcr4_orig), \
> -                             (.Lcr4_alt_end - .Lcr4_alt)
> -        altinstruction_entry .Lcr4_orig, .Lcr4_alt, X86_FEATURE_XEN_SMAP, \
> -                             (.Lcr4_orig_end - .Lcr4_orig), \
> -                             (.Lcr4_alt_end - .Lcr4_alt)
> -        .popsection
> +2:
> +.endm
> +	ALTERNATIVE_2 ".skip 45, 0x90", \
> +            alt_cr4_pv32, X86_FEATURE_XEN_SMEP, \
> +            alt_cr4_pv32, X86_FEATURE_XEN_SMAP

I'm not particularly in favor of this, but considering the end result
(after patch 6) it looks reasonable to accept the code duplication.
Down the road we may want to think about ways to re-use
replacement code sequences when they're identical.

Reviewed-by: Jan Beulich <jbeulich@suse.com>



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 5/7] x86/alt: Support for automatic padding calculations

[Andrew Cooper]

The correct amount of padding in an origin patch site can be calculated
automatically, based on the relative lengths of the replacements.

This requires a bit of trickery to calculate correctly, especially in the
ALTENRATIVE_2 case where a branchless max() calculation in needed.  The
calculation is further complicated because GAS's idea of true is -1 rather
than 1, which is why the extra negations are required.

Additionally, have apply_alternatives() attempt to optimise the padding nops.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
 xen/arch/x86/alternative.c            | 32 ++++++++++++++++++++++++----
 xen/include/asm-x86/alternative-asm.h | 40 +++++++++++++++++++++++++++--------
 xen/include/asm-x86/alternative.h     | 39 ++++++++++++++++++++++++++--------
 3 files changed, 89 insertions(+), 22 deletions(-)

diff --git a/xen/arch/x86/alternative.c b/xen/arch/x86/alternative.c
index f8ddab5..ec87ff4 100644
--- a/xen/arch/x86/alternative.c
+++ b/xen/arch/x86/alternative.c
@@ -180,13 +180,37 @@ void init_or_livepatch apply_alternatives(const struct alt_instr *start,
         uint8_t *orig = ALT_ORIG_PTR(a);
         uint8_t *repl = ALT_REPL_PTR(a);
         uint8_t buf[MAX_PATCH_LEN];
+        unsigned int total_len = a->orig_len + a->pad_len;
 
-        BUG_ON(a->repl_len > a->orig_len);
-        BUG_ON(a->orig_len > sizeof(buf));
+        BUG_ON(a->repl_len > total_len);
+        BUG_ON(total_len > sizeof(buf));
         BUG_ON(a->cpuid >= NCAPINTS * 32);
 
         if ( !boot_cpu_has(a->cpuid) )
+        {
+            unsigned int i;
+
+            /* No replacement to make, but try to optimise any padding. */
+            if ( a->pad_len <= 1 )
+                continue;
+
+            /* Search the padding area for any byte which isn't a nop. */
+            for ( i = a->orig_len; i < total_len; ++i )
+                if ( orig[i] != 0x90 )
+                    break;
+
+            /*
+             * Only make any changes if all padding bytes are unoptimised
+             * nops.  With multiple alternatives over the same origin site, we
+             * may have already made a replacement, or optimised the nops.
+             */
+            if ( i != total_len )
+                continue;
+
+            add_nops(buf, a->pad_len);
+            text_poke(orig + a->orig_len, buf, a->pad_len);
             continue;
+        }
 
         memcpy(buf, repl, a->repl_len);
 
@@ -194,8 +218,8 @@ void init_or_livepatch apply_alternatives(const struct alt_instr *start,
         if ( a->repl_len >= 5 && (*buf & 0xfe) == 0xe8 )
             *(s32 *)(buf + 1) += repl - orig;
 
-        add_nops(buf + a->repl_len, a->orig_len - a->repl_len);
-        text_poke(orig, buf, a->orig_len);
+        add_nops(buf + a->repl_len, total_len - a->repl_len);
+        text_poke(orig, buf, total_len);
     }
 }
 
diff --git a/xen/include/asm-x86/alternative-asm.h b/xen/include/asm-x86/alternative-asm.h
index 150bd1a..f7e37cb 100644
--- a/xen/include/asm-x86/alternative-asm.h
+++ b/xen/include/asm-x86/alternative-asm.h
@@ -9,30 +9,41 @@
  * enough information for the alternatives patching code to patch an
  * instruction. See apply_alternatives().
  */
-.macro altinstruction_entry orig repl feature orig_len repl_len
+.macro altinstruction_entry orig repl feature orig_len repl_len pad_len
     .long \orig - .
     .long \repl - .
     .word \feature
     .byte \orig_len
     .byte \repl_len
+    .byte \pad_len
 .endm
 
 #define orig_len               (.L\@_orig_e       -     .L\@_orig_s)
+#define pad_len                (.L\@_orig_p       -     .L\@_orig_e)
+#define total_len              (.L\@_orig_p       -     .L\@_orig_s)
 #define repl_len(nr)           (.L\@_repl_e\()nr  -     .L\@_repl_s\()nr)
 #define decl_repl(insn, nr)     .L\@_repl_s\()nr: insn; .L\@_repl_e\()nr:
+#define gas_max(a, b)          ((a) ^ (((a) ^ (b)) & -(-((a) < (b)))))
 
 .macro ALTERNATIVE oldinstr, newinstr, feature
 .L\@_orig_s:
     \oldinstr
 .L\@_orig_e:
+     .skip (-((repl_len(1) - orig_len) > 0) * (repl_len(1) - orig_len)), 0x90
+.L\@_orig_p:
 
     .pushsection .altinstructions, "a", @progbits
     altinstruction_entry .L\@_orig_s, .L\@_repl_s1, \feature, \
-        orig_len, repl_len(1)
+        orig_len, repl_len(1), pad_len
 
     .section .discard, "a", @progbits
-    /* Assembler-time check that \newinstr isn't longer than \oldinstr. */
-    .byte 0xff + repl_len(1) - orig_len
+    /*
+     * Assembler-time checks:
+     *   - total_len <= 255
+     *   - \newinstr <= total_len
+     */
+    .byte total_len
+    .byte 0xff + repl_len(1) - total_len
 
     .section .altinstr_replacement, "ax", @progbits
 
@@ -45,18 +56,26 @@
 .L\@_orig_s:
     \oldinstr
 .L\@_orig_e:
+    .skip (-((gas_max(repl_len(1), repl_len(2)) - orig_len) > 0) * \
+             (gas_max(repl_len(1), repl_len(2)) - orig_len)), 0x90
+.L\@_orig_p:
 
     .pushsection .altinstructions, "a", @progbits
 
     altinstruction_entry .L\@_orig_s, .L\@_repl_s1, \feature1, \
-        orig_len, repl_len(1)
+        orig_len, repl_len(1), pad_len
     altinstruction_entry .L\@_orig_s, .L\@_repl_s2, \feature2, \
-        orig_len, repl_len(2)
+        orig_len, repl_len(2), pad_len
 
     .section .discard, "a", @progbits
-    /* Assembler-time check that \newinstr{1,2} aren't longer than \oldinstr. */
-    .byte 0xff + repl_len(1) - orig_len
-    .byte 0xff + repl_len(2) - orig_len
+    /*
+     * Assembler-time checks:
+     *   - total_len <= 255
+     *   - \newinstr* <= total_len
+     */
+    .byte total_len
+    .byte 0xff + repl_len(1) - total_len
+    .byte 0xff + repl_len(2) - total_len
 
     .section .altinstr_replacement, "ax", @progbits
 
@@ -66,8 +85,11 @@
     .popsection
 .endm
 
+#undef gas_max
 #undef decl_repl
 #undef repl_len
+#undef total_len
+#undef pad_len
 #undef orig_len
 
 #endif /* __ASSEMBLY__ */
diff --git a/xen/include/asm-x86/alternative.h b/xen/include/asm-x86/alternative.h
index 1e5cfbd..20dea22 100644
--- a/xen/include/asm-x86/alternative.h
+++ b/xen/include/asm-x86/alternative.h
@@ -8,12 +8,13 @@
 #include <xen/stringify.h>
 #include <xen/types.h>
 
-struct alt_instr {
+struct __packed alt_instr {
     int32_t  orig_offset;   /* original instruction */
     int32_t  repl_offset;   /* offset to replacement instruction */
     uint16_t cpuid;         /* cpuid bit set for replacement */
     uint8_t  orig_len;      /* length of original instruction */
-    uint8_t  repl_len;      /* length of new instruction, <= instrlen */
+    uint8_t  repl_len;      /* length of new instruction */
+    uint8_t  pad_len;       /* length of build-time padding */
 };
 
 #define __ALT_PTR(a,f)      ((u8 *)((void *)&(a)->f + (a)->f))
@@ -26,44 +27,64 @@ extern void apply_alternatives(const struct alt_instr *start,
                                const struct alt_instr *end);
 extern void alternative_instructions(void);
 
-#define OLDINSTR(oldinstr)      ".L%=_orig_s:\n\t" oldinstr "\n.L%=_orig_e:\n"
-
 #define repl_s(num)             ".L%=_repl_s"#num
 #define repl_e(num)             ".L%=_repl_e"#num
 
 #define alt_orig_len            "(.L%=_orig_e - .L%=_orig_s)"
+#define alt_pad_len             "(.L%=_orig_p - .L%=_orig_e)"
+#define alt_total_len           "(.L%=_orig_p - .L%=_orig_s)"
 #define alt_repl_len(num)       "(" repl_e(num) " - " repl_s(num) ")"
+#define gas_max(a, b)                                         \
+    "((" a ") ^ (((" a ") ^ (" b ")) & -(-((" a ") < (" b ")))))"
+
+#define OLDINSTR_1(oldinstr, n1)                              \
+    ".L%=_orig_s:\n\t" oldinstr "\n .L%=_orig_e:\n\t"         \
+    ".skip (-(("alt_repl_len(n1)"-"alt_orig_len") > 0) * "    \
+             "("alt_repl_len(n1)"-"alt_orig_len")), 0x90\n\t" \
+    ".L%=_orig_p:\n\t"
+
+#define ALT_PADDING_LEN(n1, n2) \
+    gas_max((alt_repl_len(n1), alt_repl_len(n2))"-"alt_orig_len
+
+#define OLDINSTR_2(oldinstr, n1, n2)                          \
+    ".L%=_orig_s:\n\t" oldinstr "\n .L%=_orig_e:\n\t"         \
+    ".skip (-(("ALT_PADDING_LEN(n1, n2)") > 0) * "            \
+             "("ALT_PADDING_LEN(n1, n2)")), 0x90\n\t"         \
+    ".L%=_orig_p:\n\t"
 
 #define ALTINSTR_ENTRY(feature, num)                                    \
         " .long .L%=_orig_s - .\n"                /* label           */ \
         " .long " repl_s(num)" - .\n"             /* new instruction */ \
         " .word " __stringify(feature) "\n"       /* feature bit     */ \
         " .byte " alt_orig_len "\n"               /* source len      */ \
-        " .byte " alt_repl_len(num) "\n"          /* replacement len */
+        " .byte " alt_repl_len(num) "\n"          /* replacement len */ \
+        " .byte " alt_pad_len "\n"                /* padding len     */
 
-#define DISCARD_ENTRY(num)                        /* repl <= orig */    \
-        " .byte 0xff + (" alt_repl_len(num) ") - (" alt_orig_len ")\n"
+#define DISCARD_ENTRY(num)                        /* repl <= total */   \
+        " .byte 0xff + (" alt_repl_len(num) ") - (" alt_total_len ")\n"
 
 #define ALTINSTR_REPLACEMENT(newinstr, num)       /* replacement */     \
         repl_s(num)":\n\t" newinstr "\n" repl_e(num) ":\n\t"
 
 /* alternative assembly primitive: */
 #define ALTERNATIVE(oldinstr, newinstr, feature)                        \
-        OLDINSTR(oldinstr)                                              \
+        OLDINSTR_1(oldinstr, 1)                                         \
         ".pushsection .altinstructions, \"a\", @progbits\n"             \
         ALTINSTR_ENTRY(feature, 1)                                      \
         ".section .discard, \"a\", @progbits\n"                         \
+        ".byte " alt_total_len "\n" /* total_len <= 255 */              \
         DISCARD_ENTRY(1)                                                \
         ".section .altinstr_replacement, \"ax\", @progbits\n"           \
         ALTINSTR_REPLACEMENT(newinstr, 1)                               \
         ".popsection\n"
 
 #define ALTERNATIVE_2(oldinstr, newinstr1, feature1, newinstr2, feature2) \
-        OLDINSTR(oldinstr)                                              \
+        OLDINSTR_2(oldinstr, 1, 2)                                      \
         ".pushsection .altinstructions, \"a\", @progbits\n"             \
         ALTINSTR_ENTRY(feature1, 1)                                     \
         ALTINSTR_ENTRY(feature2, 2)                                     \
         ".section .discard, \"a\", @progbits\n"                         \
+        ".byte " alt_total_len "\n" /* total_len <= 255 */              \
         DISCARD_ENTRY(1)                                                \
         DISCARD_ENTRY(2)                                                \
         ".section .altinstr_replacement, \"ax\", @progbits\n"           \
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 5/7] x86/alt: Support for automatic padding calculations

[Wei Liu]

On Mon, Feb 12, 2018 at 11:23:05AM +0000, Andrew Cooper wrote:
> The correct amount of padding in an origin patch site can be calculated
> automatically, based on the relative lengths of the replacements.
> 
> This requires a bit of trickery to calculate correctly, especially in the
> ALTENRATIVE_2 case where a branchless max() calculation in needed.  The
> calculation is further complicated because GAS's idea of true is -1 rather
> than 1, which is why the extra negations are required.
> 
> Additionally, have apply_alternatives() attempt to optimise the padding nops.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> ---
>  xen/arch/x86/alternative.c            | 32 ++++++++++++++++++++++++----
>  xen/include/asm-x86/alternative-asm.h | 40 +++++++++++++++++++++++++++--------
>  xen/include/asm-x86/alternative.h     | 39 ++++++++++++++++++++++++++--------
>  3 files changed, 89 insertions(+), 22 deletions(-)
> 
> diff --git a/xen/arch/x86/alternative.c b/xen/arch/x86/alternative.c
> index f8ddab5..ec87ff4 100644
> --- a/xen/arch/x86/alternative.c
> +++ b/xen/arch/x86/alternative.c
> @@ -180,13 +180,37 @@ void init_or_livepatch apply_alternatives(const struct alt_instr *start,
>          uint8_t *orig = ALT_ORIG_PTR(a);
>          uint8_t *repl = ALT_REPL_PTR(a);
>          uint8_t buf[MAX_PATCH_LEN];
> +        unsigned int total_len = a->orig_len + a->pad_len;
>  
> -        BUG_ON(a->repl_len > a->orig_len);
> -        BUG_ON(a->orig_len > sizeof(buf));
> +        BUG_ON(a->repl_len > total_len);
> +        BUG_ON(total_len > sizeof(buf));
>          BUG_ON(a->cpuid >= NCAPINTS * 32);
>  
>          if ( !boot_cpu_has(a->cpuid) )
> +        {
> +            unsigned int i;
> +
> +            /* No replacement to make, but try to optimise any padding. */
> +            if ( a->pad_len <= 1 )
> +                continue;
> +
> +            /* Search the padding area for any byte which isn't a nop. */
> +            for ( i = a->orig_len; i < total_len; ++i )
> +                if ( orig[i] != 0x90 )
> +                    break;
> +
> +            /*
> +             * Only make any changes if all padding bytes are unoptimised
> +             * nops.  With multiple alternatives over the same origin site, we
> +             * may have already made a replacement, or optimised the nops.
> +             */
> +            if ( i != total_len )
> +                continue;
> +
> +            add_nops(buf, a->pad_len);
> +            text_poke(orig + a->orig_len, buf, a->pad_len);
>              continue;
> +        }

Is the expectation here the alternative instructions already contain
optimised paddings (including live patches)? Otherwise why is the same
optimisation no needed when later?

>  
>          memcpy(buf, repl, a->repl_len);
>  
> @@ -194,8 +218,8 @@ void init_or_livepatch apply_alternatives(const struct alt_instr *start,
>          if ( a->repl_len >= 5 && (*buf & 0xfe) == 0xe8 )
>              *(s32 *)(buf + 1) += repl - orig;
>  
> -        add_nops(buf + a->repl_len, a->orig_len - a->repl_len);
> -        text_poke(orig, buf, a->orig_len);
> +        add_nops(buf + a->repl_len, total_len - a->repl_len);
> +        text_poke(orig, buf, total_len);
>      }
>  }
>  
> diff --git a/xen/include/asm-x86/alternative-asm.h b/xen/include/asm-x86/alternative-asm.h
> index 150bd1a..f7e37cb 100644
> --- a/xen/include/asm-x86/alternative-asm.h
> +++ b/xen/include/asm-x86/alternative-asm.h
> @@ -9,30 +9,41 @@
>   * enough information for the alternatives patching code to patch an
>   * instruction. See apply_alternatives().
>   */
> -.macro altinstruction_entry orig repl feature orig_len repl_len
> +.macro altinstruction_entry orig repl feature orig_len repl_len pad_len
>      .long \orig - .
>      .long \repl - .
>      .word \feature
>      .byte \orig_len
>      .byte \repl_len
> +    .byte \pad_len
>  .endm
>  
>  #define orig_len               (.L\@_orig_e       -     .L\@_orig_s)
> +#define pad_len                (.L\@_orig_p       -     .L\@_orig_e)
> +#define total_len              (.L\@_orig_p       -     .L\@_orig_s)
>  #define repl_len(nr)           (.L\@_repl_e\()nr  -     .L\@_repl_s\()nr)
>  #define decl_repl(insn, nr)     .L\@_repl_s\()nr: insn; .L\@_repl_e\()nr:
> +#define gas_max(a, b)          ((a) ^ (((a) ^ (b)) & -(-((a) < (b)))))

What about clang's assembler? At least give it a stub to cause
compilation error?

>  
>  .macro ALTERNATIVE oldinstr, newinstr, feature
>  .L\@_orig_s:
>      \oldinstr
>  .L\@_orig_e:
> +     .skip (-((repl_len(1) - orig_len) > 0) * (repl_len(1) - orig_len)), 0x90

Seeing the negation at the beginning, I suppose this should also be a
gas specific macro?

The rest looks good.

Wei.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 5/7] x86/alt: Support for automatic padding calculations

[Andrew Cooper]

On 12/02/18 14:39, Wei Liu wrote:
> On Mon, Feb 12, 2018 at 11:23:05AM +0000, Andrew Cooper wrote:
>> The correct amount of padding in an origin patch site can be calculated
>> automatically, based on the relative lengths of the replacements.
>>
>> This requires a bit of trickery to calculate correctly, especially in the
>> ALTENRATIVE_2 case where a branchless max() calculation in needed.  The
>> calculation is further complicated because GAS's idea of true is -1 rather
>> than 1, which is why the extra negations are required.
>>
>> Additionally, have apply_alternatives() attempt to optimise the padding nops.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>> CC: Roger Pau Monné <roger.pau@citrix.com>
>> CC: Wei Liu <wei.liu2@citrix.com>
>> ---
>>  xen/arch/x86/alternative.c            | 32 ++++++++++++++++++++++++----
>>  xen/include/asm-x86/alternative-asm.h | 40 +++++++++++++++++++++++++++--------
>>  xen/include/asm-x86/alternative.h     | 39 ++++++++++++++++++++++++++--------
>>  3 files changed, 89 insertions(+), 22 deletions(-)
>>
>> diff --git a/xen/arch/x86/alternative.c b/xen/arch/x86/alternative.c
>> index f8ddab5..ec87ff4 100644
>> --- a/xen/arch/x86/alternative.c
>> +++ b/xen/arch/x86/alternative.c
>> @@ -180,13 +180,37 @@ void init_or_livepatch apply_alternatives(const struct alt_instr *start,
>>          uint8_t *orig = ALT_ORIG_PTR(a);
>>          uint8_t *repl = ALT_REPL_PTR(a);
>>          uint8_t buf[MAX_PATCH_LEN];
>> +        unsigned int total_len = a->orig_len + a->pad_len;
>>  
>> -        BUG_ON(a->repl_len > a->orig_len);
>> -        BUG_ON(a->orig_len > sizeof(buf));
>> +        BUG_ON(a->repl_len > total_len);
>> +        BUG_ON(total_len > sizeof(buf));
>>          BUG_ON(a->cpuid >= NCAPINTS * 32);
>>  
>>          if ( !boot_cpu_has(a->cpuid) )
>> +        {
>> +            unsigned int i;
>> +
>> +            /* No replacement to make, but try to optimise any padding. */
>> +            if ( a->pad_len <= 1 )
>> +                continue;
>> +
>> +            /* Search the padding area for any byte which isn't a nop. */
>> +            for ( i = a->orig_len; i < total_len; ++i )
>> +                if ( orig[i] != 0x90 )
>> +                    break;
>> +
>> +            /*
>> +             * Only make any changes if all padding bytes are unoptimised
>> +             * nops.  With multiple alternatives over the same origin site, we
>> +             * may have already made a replacement, or optimised the nops.
>> +             */
>> +            if ( i != total_len )
>> +                continue;
>> +
>> +            add_nops(buf, a->pad_len);
>> +            text_poke(orig + a->orig_len, buf, a->pad_len);
>>              continue;
>> +        }
> Is the expectation here the alternative instructions already contain
> optimised paddings (including live patches)? Otherwise why is the same
> optimisation no needed when later?

The problem is that we don't store the actual original bytes, so can't
trivially detect whether we've already patched this site before.  We've
a number of cases which are an ALTERNATIVE_2 based on SMEP and SMAP, so
on a fair chunk of hardware, we first make a replacement because of
SMEP, then fail the SMAP check and don't make the second replacement.

Later, we are discarding everything in orig+pad, and replacing it with
repl+any necessary padding, which is made of optimised nops.

>
>>  
>>          memcpy(buf, repl, a->repl_len);
>>  
>> @@ -194,8 +218,8 @@ void init_or_livepatch apply_alternatives(const struct alt_instr *start,
>>          if ( a->repl_len >= 5 && (*buf & 0xfe) == 0xe8 )
>>              *(s32 *)(buf + 1) += repl - orig;
>>  
>> -        add_nops(buf + a->repl_len, a->orig_len - a->repl_len);
>> -        text_poke(orig, buf, a->orig_len);
>> +        add_nops(buf + a->repl_len, total_len - a->repl_len);
>> +        text_poke(orig, buf, total_len);
>>      }
>>  }
>>  
>> diff --git a/xen/include/asm-x86/alternative-asm.h b/xen/include/asm-x86/alternative-asm.h
>> index 150bd1a..f7e37cb 100644
>> --- a/xen/include/asm-x86/alternative-asm.h
>> +++ b/xen/include/asm-x86/alternative-asm.h
>> @@ -9,30 +9,41 @@
>>   * enough information for the alternatives patching code to patch an
>>   * instruction. See apply_alternatives().
>>   */
>> -.macro altinstruction_entry orig repl feature orig_len repl_len
>> +.macro altinstruction_entry orig repl feature orig_len repl_len pad_len
>>      .long \orig - .
>>      .long \repl - .
>>      .word \feature
>>      .byte \orig_len
>>      .byte \repl_len
>> +    .byte \pad_len
>>  .endm
>>  
>>  #define orig_len               (.L\@_orig_e       -     .L\@_orig_s)
>> +#define pad_len                (.L\@_orig_p       -     .L\@_orig_e)
>> +#define total_len              (.L\@_orig_p       -     .L\@_orig_s)
>>  #define repl_len(nr)           (.L\@_repl_e\()nr  -     .L\@_repl_s\()nr)
>>  #define decl_repl(insn, nr)     .L\@_repl_s\()nr: insn; .L\@_repl_e\()nr:
>> +#define gas_max(a, b)          ((a) ^ (((a) ^ (b)) & -(-((a) < (b)))))
> What about clang's assembler? At least give it a stub to cause
> compilation error?
>
>>  
>>  .macro ALTERNATIVE oldinstr, newinstr, feature
>>  .L\@_orig_s:
>>      \oldinstr
>>  .L\@_orig_e:
>> +     .skip (-((repl_len(1) - orig_len) > 0) * (repl_len(1) - orig_len)), 0x90
> Seeing the negation at the beginning, I suppose this should also be a
> gas specific macro?

The build failures are because clang's integrated assembler can't cope
with non-absolute references with .skip, but we already know about this
and have code identical to this in tree.  (I temporarily removed it in
patch 4).

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 5/7] x86/alt: Support for automatic padding calculations

[Roger Pau Monné]

On Mon, Feb 12, 2018 at 03:04:21PM +0000, Andrew Cooper wrote:
> On 12/02/18 14:39, Wei Liu wrote:
> > On Mon, Feb 12, 2018 at 11:23:05AM +0000, Andrew Cooper wrote:
> >>  .macro ALTERNATIVE oldinstr, newinstr, feature
> >>  .L\@_orig_s:
> >>      \oldinstr
> >>  .L\@_orig_e:
> >> +     .skip (-((repl_len(1) - orig_len) > 0) * (repl_len(1) - orig_len)), 0x90
> > Seeing the negation at the beginning, I suppose this should also be a
> > gas specific macro?
> 
> The build failures are because clang's integrated assembler can't cope
> with non-absolute references with .skip, but we already know about this
> and have code identical to this in tree.  (I temporarily removed it in
> patch 4).

Newer clang (6) supports .skip with labels, but doesn't support the
(-(... And it's having some issues with the rest of the expression,
will have to check more closely tomorrow.

I wonder, what's Linux doing in this regard? It seems like clang/llvm
is quite committed to support building Linux, so it might be good to
follow suit in this case.

Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 5/7] x86/alt: Support for automatic padding calculations

[Andrew Cooper]

On 12/02/18 18:41, Roger Pau Monné wrote:
> On Mon, Feb 12, 2018 at 03:04:21PM +0000, Andrew Cooper wrote:
>> On 12/02/18 14:39, Wei Liu wrote:
>>> On Mon, Feb 12, 2018 at 11:23:05AM +0000, Andrew Cooper wrote:
>>>>  .macro ALTERNATIVE oldinstr, newinstr, feature
>>>>  .L\@_orig_s:
>>>>      \oldinstr
>>>>  .L\@_orig_e:
>>>> +     .skip (-((repl_len(1) - orig_len) > 0) * (repl_len(1) - orig_len)), 0x90
>>> Seeing the negation at the beginning, I suppose this should also be a
>>> gas specific macro?
>> The build failures are because clang's integrated assembler can't cope
>> with non-absolute references with .skip, but we already know about this
>> and have code identical to this in tree.  (I temporarily removed it in
>> patch 4).
> Newer clang (6) supports .skip with labels, but doesn't support the
> (-(... And it's having some issues with the rest of the expression,
> will have to check more closely tomorrow.
>
> I wonder, what's Linux doing in this regard? It seems like clang/llvm
> is quite committed to support building Linux, so it might be good to
> follow suit in this case.

This is basically the same as what Linux does.  Linux unconditionally
uses -no-integrated-as.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 5/7] x86/alt: Support for automatic padding calculations

[Roger Pau Monné]

On Mon, Feb 12, 2018 at 11:23:05AM +0000, Andrew Cooper wrote:
> The correct amount of padding in an origin patch site can be calculated
> automatically, based on the relative lengths of the replacements.
> 
> This requires a bit of trickery to calculate correctly, especially in the
> ALTENRATIVE_2 case where a branchless max() calculation in needed.  The
> calculation is further complicated because GAS's idea of true is -1 rather
> than 1, which is why the extra negations are required.
> 
> Additionally, have apply_alternatives() attempt to optimise the padding nops.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

LGTM, just a couple of nits:

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> ---
>  xen/arch/x86/alternative.c            | 32 ++++++++++++++++++++++++----
>  xen/include/asm-x86/alternative-asm.h | 40 +++++++++++++++++++++++++++--------
>  xen/include/asm-x86/alternative.h     | 39 ++++++++++++++++++++++++++--------
>  3 files changed, 89 insertions(+), 22 deletions(-)
> 
> diff --git a/xen/arch/x86/alternative.c b/xen/arch/x86/alternative.c
> index f8ddab5..ec87ff4 100644
> --- a/xen/arch/x86/alternative.c
> +++ b/xen/arch/x86/alternative.c
> @@ -180,13 +180,37 @@ void init_or_livepatch apply_alternatives(const struct alt_instr *start,
>          uint8_t *orig = ALT_ORIG_PTR(a);
>          uint8_t *repl = ALT_REPL_PTR(a);
>          uint8_t buf[MAX_PATCH_LEN];
> +        unsigned int total_len = a->orig_len + a->pad_len;
>  
> -        BUG_ON(a->repl_len > a->orig_len);
> -        BUG_ON(a->orig_len > sizeof(buf));
> +        BUG_ON(a->repl_len > total_len);
> +        BUG_ON(total_len > sizeof(buf));
>          BUG_ON(a->cpuid >= NCAPINTS * 32);
>  
>          if ( !boot_cpu_has(a->cpuid) )
> +        {
> +            unsigned int i;
> +
> +            /* No replacement to make, but try to optimise any padding. */
> +            if ( a->pad_len <= 1 )
> +                continue;
> +
> +            /* Search the padding area for any byte which isn't a nop. */
> +            for ( i = a->orig_len; i < total_len; ++i )
> +                if ( orig[i] != 0x90 )

Maybe better to compare against ASM_NOP1?

> +                    break;
> +
> +            /*
> +             * Only make any changes if all padding bytes are unoptimised
> +             * nops.  With multiple alternatives over the same origin site, we
> +             * may have already made a replacement, or optimised the nops.
> +             */
> +            if ( i != total_len )
> +                continue;
> +
> +            add_nops(buf, a->pad_len);
> +            text_poke(orig + a->orig_len, buf, a->pad_len);
>              continue;
> +        }
>  
>          memcpy(buf, repl, a->repl_len);
>  
> @@ -194,8 +218,8 @@ void init_or_livepatch apply_alternatives(const struct alt_instr *start,
>          if ( a->repl_len >= 5 && (*buf & 0xfe) == 0xe8 )
>              *(s32 *)(buf + 1) += repl - orig;
>  
> -        add_nops(buf + a->repl_len, a->orig_len - a->repl_len);
> -        text_poke(orig, buf, a->orig_len);
> +        add_nops(buf + a->repl_len, total_len - a->repl_len);
> +        text_poke(orig, buf, total_len);
>      }
>  }
>  
> diff --git a/xen/include/asm-x86/alternative-asm.h b/xen/include/asm-x86/alternative-asm.h
> index 150bd1a..f7e37cb 100644
> --- a/xen/include/asm-x86/alternative-asm.h
> +++ b/xen/include/asm-x86/alternative-asm.h
> @@ -9,30 +9,41 @@
>   * enough information for the alternatives patching code to patch an
>   * instruction. See apply_alternatives().
>   */
> -.macro altinstruction_entry orig repl feature orig_len repl_len
> +.macro altinstruction_entry orig repl feature orig_len repl_len pad_len
>      .long \orig - .
>      .long \repl - .
>      .word \feature
>      .byte \orig_len
>      .byte \repl_len
> +    .byte \pad_len
>  .endm
>  
>  #define orig_len               (.L\@_orig_e       -     .L\@_orig_s)
> +#define pad_len                (.L\@_orig_p       -     .L\@_orig_e)
> +#define total_len              (.L\@_orig_p       -     .L\@_orig_s)
>  #define repl_len(nr)           (.L\@_repl_e\()nr  -     .L\@_repl_s\()nr)
>  #define decl_repl(insn, nr)     .L\@_repl_s\()nr: insn; .L\@_repl_e\()nr:
> +#define gas_max(a, b)          ((a) ^ (((a) ^ (b)) & -(-((a) < (b)))))

That seems to work fine at least on newish versions of clang, so I'm
not sure the g prefix is required (as_max).

>  
>  .macro ALTERNATIVE oldinstr, newinstr, feature
>  .L\@_orig_s:
>      \oldinstr
>  .L\@_orig_e:
> +     .skip (-((repl_len(1) - orig_len) > 0) * (repl_len(1) - orig_len)), 0x90
> +.L\@_orig_p:
>  
>      .pushsection .altinstructions, "a", @progbits
>      altinstruction_entry .L\@_orig_s, .L\@_repl_s1, \feature, \
> -        orig_len, repl_len(1)
> +        orig_len, repl_len(1), pad_len
>  
>      .section .discard, "a", @progbits
> -    /* Assembler-time check that \newinstr isn't longer than \oldinstr. */
> -    .byte 0xff + repl_len(1) - orig_len
> +    /*
> +     * Assembler-time checks:
> +     *   - total_len <= 255
> +     *   - \newinstr <= total_len
> +     */
> +    .byte total_len
> +    .byte 0xff + repl_len(1) - total_len
>  
>      .section .altinstr_replacement, "ax", @progbits
>  
> @@ -45,18 +56,26 @@
>  .L\@_orig_s:
>      \oldinstr
>  .L\@_orig_e:
> +    .skip (-((gas_max(repl_len(1), repl_len(2)) - orig_len) > 0) * \
> +             (gas_max(repl_len(1), repl_len(2)) - orig_len)), 0x90
> +.L\@_orig_p:
>  
>      .pushsection .altinstructions, "a", @progbits
>  
>      altinstruction_entry .L\@_orig_s, .L\@_repl_s1, \feature1, \
> -        orig_len, repl_len(1)
> +        orig_len, repl_len(1), pad_len
>      altinstruction_entry .L\@_orig_s, .L\@_repl_s2, \feature2, \
> -        orig_len, repl_len(2)
> +        orig_len, repl_len(2), pad_len
>  
>      .section .discard, "a", @progbits
> -    /* Assembler-time check that \newinstr{1,2} aren't longer than \oldinstr. */
> -    .byte 0xff + repl_len(1) - orig_len
> -    .byte 0xff + repl_len(2) - orig_len
> +    /*
> +     * Assembler-time checks:
> +     *   - total_len <= 255
> +     *   - \newinstr* <= total_len
> +     */
> +    .byte total_len
> +    .byte 0xff + repl_len(1) - total_len
> +    .byte 0xff + repl_len(2) - total_len
>  
>      .section .altinstr_replacement, "ax", @progbits
>  
> @@ -66,8 +85,11 @@
>      .popsection
>  .endm
>  
> +#undef gas_max
>  #undef decl_repl
>  #undef repl_len
> +#undef total_len
> +#undef pad_len
>  #undef orig_len
>  
>  #endif /* __ASSEMBLY__ */
> diff --git a/xen/include/asm-x86/alternative.h b/xen/include/asm-x86/alternative.h
> index 1e5cfbd..20dea22 100644
> --- a/xen/include/asm-x86/alternative.h
> +++ b/xen/include/asm-x86/alternative.h
> @@ -8,12 +8,13 @@
>  #include <xen/stringify.h>
>  #include <xen/types.h>
>  
> -struct alt_instr {
> +struct __packed alt_instr {
>      int32_t  orig_offset;   /* original instruction */
>      int32_t  repl_offset;   /* offset to replacement instruction */
>      uint16_t cpuid;         /* cpuid bit set for replacement */
>      uint8_t  orig_len;      /* length of original instruction */
> -    uint8_t  repl_len;      /* length of new instruction, <= instrlen */
> +    uint8_t  repl_len;      /* length of new instruction */
> +    uint8_t  pad_len;       /* length of build-time padding */
>  };
>  
>  #define __ALT_PTR(a,f)      ((u8 *)((void *)&(a)->f + (a)->f))
> @@ -26,44 +27,64 @@ extern void apply_alternatives(const struct alt_instr *start,
>                                 const struct alt_instr *end);
>  extern void alternative_instructions(void);
>  
> -#define OLDINSTR(oldinstr)      ".L%=_orig_s:\n\t" oldinstr "\n.L%=_orig_e:\n"
> -
>  #define repl_s(num)             ".L%=_repl_s"#num
>  #define repl_e(num)             ".L%=_repl_e"#num
>  
>  #define alt_orig_len            "(.L%=_orig_e - .L%=_orig_s)"
> +#define alt_pad_len             "(.L%=_orig_p - .L%=_orig_e)"
> +#define alt_total_len           "(.L%=_orig_p - .L%=_orig_s)"
>  #define alt_repl_len(num)       "(" repl_e(num) " - " repl_s(num) ")"
> +#define gas_max(a, b)                                         \
> +    "((" a ") ^ (((" a ") ^ (" b ")) & -(-((" a ") < (" b ")))))"
> +
> +#define OLDINSTR_1(oldinstr, n1)                              \
> +    ".L%=_orig_s:\n\t" oldinstr "\n .L%=_orig_e:\n\t"         \
> +    ".skip (-(("alt_repl_len(n1)"-"alt_orig_len") > 0) * "    \
> +             "("alt_repl_len(n1)"-"alt_orig_len")), 0x90\n\t" \

ASM_NOP1 instead of 0x90?

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 5/7] x86/alt: Support for automatic padding calculations

[Roger Pau Monné]

On Mon, Feb 12, 2018 at 11:23:05AM +0000, Andrew Cooper wrote:
>  .macro ALTERNATIVE oldinstr, newinstr, feature
>  .L\@_orig_s:
>      \oldinstr
>  .L\@_orig_e:
> +     .skip (-((repl_len(1) - orig_len) > 0) * (repl_len(1) - orig_len)), 0x90

clang chokes on this expression, because of the negation at the
beginning and I'm also failing to see why are you adding such
negation. AFAICT using:

.skip (((repl_len(1) - orig_len) > 0) * (repl_len(1) - orig_len)), 0x90

Is correct: it adds the right padding if the alternative code is
bigger than the original one, while not adding anything is the
original code is greater than the alternative one.

The negation just turns the 1 to -1, thus converting the result of the
whole expression into a negative value.

Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 5/7] x86/alt: Support for automatic padding calculations

[Andrew Cooper]

On 13/02/2018 09:45, Roger Pau Monné wrote:
> On Mon, Feb 12, 2018 at 11:23:05AM +0000, Andrew Cooper wrote:
>>  .macro ALTERNATIVE oldinstr, newinstr, feature
>>  .L\@_orig_s:
>>      \oldinstr
>>  .L\@_orig_e:
>> +     .skip (-((repl_len(1) - orig_len) > 0) * (repl_len(1) - orig_len)), 0x90
> clang chokes on this expression, because of the negation at the
> beginning and I'm also failing to see why are you adding such
> negation. AFAICT using:
>
> .skip (((repl_len(1) - orig_len) > 0) * (repl_len(1) - orig_len)), 0x90
>
> Is correct: it adds the right padding if the alternative code is
> bigger than the original one, while not adding anything is the
> original code is greater than the alternative one.
>
> The negation just turns the 1 to -1, thus converting the result of the
> whole expression into a negative value.

/sigh so Clang and GAS have different ideas of true.

The reason for this negation is stated in the commit message.  "x > 0"
in GAS yields 0 or -1, rather than the expected 1.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 5/7] x86/alt: Support for automatic padding calculations

[Roger Pau Monné]

On Tue, Feb 13, 2018 at 10:09:15AM +0000, Andrew Cooper wrote:
> On 13/02/2018 09:45, Roger Pau Monné wrote:
> > On Mon, Feb 12, 2018 at 11:23:05AM +0000, Andrew Cooper wrote:
> >>  .macro ALTERNATIVE oldinstr, newinstr, feature
> >>  .L\@_orig_s:
> >>      \oldinstr
> >>  .L\@_orig_e:
> >> +     .skip (-((repl_len(1) - orig_len) > 0) * (repl_len(1) - orig_len)), 0x90
> > clang chokes on this expression, because of the negation at the
> > beginning and I'm also failing to see why are you adding such
> > negation. AFAICT using:
> >
> > .skip (((repl_len(1) - orig_len) > 0) * (repl_len(1) - orig_len)), 0x90
> >
> > Is correct: it adds the right padding if the alternative code is
> > bigger than the original one, while not adding anything is the
> > original code is greater than the alternative one.
> >
> > The negation just turns the 1 to -1, thus converting the result of the
> > whole expression into a negative value.
> 
> /sigh so Clang and GAS have different ideas of true.
> 
> The reason for this negation is stated in the commit message.  "x > 0"
> in GAS yields 0 or -1, rather than the expected 1.

That's unfortunate. What about something along the lines of:

---8<---
diff --git a/xen/arch/x86/Rules.mk b/xen/arch/x86/Rules.mk
index aeae01cd97..db442a45b7 100644
--- a/xen/arch/x86/Rules.mk
+++ b/xen/arch/x86/Rules.mk
@@ -23,6 +23,7 @@ $(call as-insn-check,CFLAGS,CC,"rdseed %eax",-DHAVE_GAS_RDSEED)
 $(call as-insn-check,CFLAGS,CC,".equ \"x\"$$(comma)1", \
                      -U__OBJECT_LABEL__ -DHAVE_GAS_QUOTED_SYM \
                      '-D__OBJECT_LABEL__=$(subst $(BASEDIR)/,,$(CURDIR))/$$@')
+$(call as-insn-check,CFLAGS,CC,".skip (-(1 > 0))$$(comma)0x90",-DAS_NEGATIVE_TRUE)
 
 CFLAGS += -mno-red-zone -fpic -fno-asynchronous-unwind-tables
 
diff --git a/xen/include/asm-x86/alternative-asm.h b/xen/include/asm-x86/alternative-asm.h
index f7e37cb891..6ce6479e5b 100644
--- a/xen/include/asm-x86/alternative-asm.h
+++ b/xen/include/asm-x86/alternative-asm.h
@@ -25,11 +25,18 @@
 #define decl_repl(insn, nr)     .L\@_repl_s\()nr: insn; .L\@_repl_e\()nr:
 #define gas_max(a, b)          ((a) ^ (((a) ^ (b)) & -(-((a) < (b)))))
 
+#ifdef AS_NEGATIVE_TRUE
+#define as_true -
+#else
+#define as_true
+#endif
+
 .macro ALTERNATIVE oldinstr, newinstr, feature
 .L\@_orig_s:
     \oldinstr
 .L\@_orig_e:
-     .skip (-((repl_len(1) - orig_len) > 0) * (repl_len(1) - orig_len)), 0x90
+     .skip (as_true((repl_len(1) - orig_len) > 0) * (repl_len(1) - orig_len)), \
+           0x90
 .L\@_orig_p:
 
     .pushsection .altinstructions, "a", @progbits
@@ -56,8 +63,8 @@
 .L\@_orig_s:
     \oldinstr
 .L\@_orig_e:
-    .skip (-((gas_max(repl_len(1), repl_len(2)) - orig_len) > 0) * \
-             (gas_max(repl_len(1), repl_len(2)) - orig_len)), 0x90
+    .skip (as_true((gas_max(repl_len(1), repl_len(2)) - orig_len) > 0) * \
+           (gas_max(repl_len(1), repl_len(2)) - orig_len)), 0x90
 .L\@_orig_p:
 
     .pushsection .altinstructions, "a", @progbits
diff --git a/xen/include/asm-x86/alternative.h b/xen/include/asm-x86/alternative.h
index 20dea2245a..ea76fa9f8d 100644
--- a/xen/include/asm-x86/alternative.h
+++ b/xen/include/asm-x86/alternative.h
@@ -37,19 +37,25 @@ extern void alternative_instructions(void);
 #define gas_max(a, b)                                         \
     "((" a ") ^ (((" a ") ^ (" b ")) & -(-((" a ") < (" b ")))))"
 
-#define OLDINSTR_1(oldinstr, n1)                              \
-    ".L%=_orig_s:\n\t" oldinstr "\n .L%=_orig_e:\n\t"         \
-    ".skip (-(("alt_repl_len(n1)"-"alt_orig_len") > 0) * "    \
-             "("alt_repl_len(n1)"-"alt_orig_len")), 0x90\n\t" \
+#ifdef AS_NEGATIVE_TRUE
+#define as_true -
+#else
+#define as_true
+#endif
+
+#define OLDINSTR_1(oldinstr, n1)                                        \
+    ".L%=_orig_s:\n\t" oldinstr "\n .L%=_orig_e:\n\t"                   \
+    ".skip ("as_true"(("alt_repl_len(n1)"-"alt_orig_len") > 0) * "      \
+             "("alt_repl_len(n1)"-"alt_orig_len")), 0x90\n\t"           \
     ".L%=_orig_p:\n\t"
 
 #define ALT_PADDING_LEN(n1, n2) \
     gas_max((alt_repl_len(n1), alt_repl_len(n2))"-"alt_orig_len
 
-#define OLDINSTR_2(oldinstr, n1, n2)                          \
-    ".L%=_orig_s:\n\t" oldinstr "\n .L%=_orig_e:\n\t"         \
-    ".skip (-(("ALT_PADDING_LEN(n1, n2)") > 0) * "            \
-             "("ALT_PADDING_LEN(n1, n2)")), 0x90\n\t"         \
+#define OLDINSTR_2(oldinstr, n1, n2)                                    \
+    ".L%=_orig_s:\n\t" oldinstr "\n .L%=_orig_e:\n\t"                   \
+    ".skip ("as_true"(("ALT_PADDING_LEN(n1, n2)") > 0) * "              \
+             "("ALT_PADDING_LEN(n1, n2)")), 0x90\n\t"                   \
     ".L%=_orig_p:\n\t"
 
 #define ALTINSTR_ENTRY(feature, num)                                    \


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 5/7] x86/alt: Support for automatic padding calculations

[Jan Beulich]

>>> On 12.02.18 at 12:23, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/arch/x86/alternative.c
> +++ b/xen/arch/x86/alternative.c
> @@ -180,13 +180,37 @@ void init_or_livepatch apply_alternatives(const struct alt_instr *start,
>          uint8_t *orig = ALT_ORIG_PTR(a);
>          uint8_t *repl = ALT_REPL_PTR(a);
>          uint8_t buf[MAX_PATCH_LEN];
> +        unsigned int total_len = a->orig_len + a->pad_len;
>  
> -        BUG_ON(a->repl_len > a->orig_len);
> -        BUG_ON(a->orig_len > sizeof(buf));
> +        BUG_ON(a->repl_len > total_len);
> +        BUG_ON(total_len > sizeof(buf));
>          BUG_ON(a->cpuid >= NCAPINTS * 32);
>  
>          if ( !boot_cpu_has(a->cpuid) )
> +        {
> +            unsigned int i;
> +
> +            /* No replacement to make, but try to optimise any padding. */

Better move the comment ahead of the declaration?

> @@ -26,44 +27,64 @@ extern void apply_alternatives(const struct alt_instr *start,
>                                 const struct alt_instr *end);
>  extern void alternative_instructions(void);
>  
> -#define OLDINSTR(oldinstr)      ".L%=_orig_s:\n\t" oldinstr 
> "\n.L%=_orig_e:\n"
> -
>  #define repl_s(num)             ".L%=_repl_s"#num
>  #define repl_e(num)             ".L%=_repl_e"#num
>  
>  #define alt_orig_len            "(.L%=_orig_e - .L%=_orig_s)"
> +#define alt_pad_len             "(.L%=_orig_p - .L%=_orig_e)"
> +#define alt_total_len           "(.L%=_orig_p - .L%=_orig_s)"
>  #define alt_repl_len(num)       "(" repl_e(num) " - " repl_s(num) ")"
> +#define gas_max(a, b)                                         \
> +    "((" a ") ^ (((" a ") ^ (" b ")) & -(-((" a ") < (" b ")))))"
> +
> +#define OLDINSTR_1(oldinstr, n1)                              \
> +    ".L%=_orig_s:\n\t" oldinstr "\n .L%=_orig_e:\n\t"         \
> +    ".skip (-(("alt_repl_len(n1)"-"alt_orig_len") > 0) * "    \
> +             "("alt_repl_len(n1)"-"alt_orig_len")), 0x90\n\t" \
> +    ".L%=_orig_p:\n\t"
> +
> +#define ALT_PADDING_LEN(n1, n2) \
> +    gas_max((alt_repl_len(n1), alt_repl_len(n2))"-"alt_orig_len
> +
> +#define OLDINSTR_2(oldinstr, n1, n2)                          \
> +    ".L%=_orig_s:\n\t" oldinstr "\n .L%=_orig_e:\n\t"         \
> +    ".skip (-(("ALT_PADDING_LEN(n1, n2)") > 0) * "            \
> +             "("ALT_PADDING_LEN(n1, n2)")), 0x90\n\t"         \
> +    ".L%=_orig_p:\n\t"
>  
>  #define ALTINSTR_ENTRY(feature, num)                                    \
>          " .long .L%=_orig_s - .\n"                /* label           */ \
>          " .long " repl_s(num)" - .\n"             /* new instruction */ \
>          " .word " __stringify(feature) "\n"       /* feature bit     */ \
>          " .byte " alt_orig_len "\n"               /* source len      */ \
> -        " .byte " alt_repl_len(num) "\n"          /* replacement len */
> +        " .byte " alt_repl_len(num) "\n"          /* replacement len */ \
> +        " .byte " alt_pad_len "\n"                /* padding len     */
>  
> -#define DISCARD_ENTRY(num)                        /* repl <= orig */    \
> -        " .byte 0xff + (" alt_repl_len(num) ") - (" alt_orig_len ")\n"
> +#define DISCARD_ENTRY(num)                        /* repl <= total */   \
> +        " .byte 0xff + (" alt_repl_len(num) ") - (" alt_total_len ")\n"

I don't think this is of much use anymore, now that you add the
padding automatically (same for the respective part of the
check in the assembler macro). Use

        ".byte " alt_total_len "\n" /* total_len <= 255 */

here instead (eliminating their explicit uses below)?

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 6/7] x86/alt: Drop explicit padding of origin sites

[Andrew Cooper]

Now that the alternatives infrastructure can calculate the required padding
automatically, there is no need to hard code it.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
 xen/arch/x86/x86_64/compat/entry.S  |  2 +-
 xen/arch/x86/x86_64/entry.S         |  2 +-
 xen/include/asm-x86/nops.h          |  7 -------
 xen/include/asm-x86/spec_ctrl_asm.h | 19 ++++++++-----------
 4 files changed, 10 insertions(+), 20 deletions(-)

diff --git a/xen/arch/x86/x86_64/compat/entry.S b/xen/arch/x86/x86_64/compat/entry.S
index e93770f..db463c5 100644
--- a/xen/arch/x86/x86_64/compat/entry.S
+++ b/xen/arch/x86/x86_64/compat/entry.S
@@ -132,7 +132,7 @@ ENTRY(compat_restore_all_guest)
         jne   1b
 2:
 .endm
-	ALTERNATIVE_2 ".skip 45, 0x90", \
+	ALTERNATIVE_2 "", \
             alt_cr4_pv32, X86_FEATURE_XEN_SMEP, \
             alt_cr4_pv32, X86_FEATURE_XEN_SMAP
 
diff --git a/xen/arch/x86/x86_64/entry.S b/xen/arch/x86/x86_64/entry.S
index bd3819a..54a3b31 100644
--- a/xen/arch/x86/x86_64/entry.S
+++ b/xen/arch/x86/x86_64/entry.S
@@ -557,7 +557,7 @@ handle_exception_saved:
         testb $X86_EFLAGS_IF>>8,UREGS_eflags+1(%rsp)
         jz    exception_with_ints_disabled
 
-        ALTERNATIVE_2 "jmp .Lcr4_pv32_done; .skip 2, 0x90", \
+        ALTERNATIVE_2 "jmp .Lcr4_pv32_done", \
             __stringify(mov VCPU_domain(%rbx), %rax), X86_FEATURE_XEN_SMEP, \
             __stringify(mov VCPU_domain(%rbx), %rax), X86_FEATURE_XEN_SMAP
 
diff --git a/xen/include/asm-x86/nops.h b/xen/include/asm-x86/nops.h
index 61319cc..1a46b97 100644
--- a/xen/include/asm-x86/nops.h
+++ b/xen/include/asm-x86/nops.h
@@ -65,13 +65,6 @@
 #define ASM_NOP8 _ASM_MK_NOP(P6_NOP8)
 #define ASM_NOP9 _ASM_MK_NOP(P6_NOP9)
 
-#define ASM_NOP17 ASM_NOP8; ASM_NOP7; ASM_NOP2
-#define ASM_NOP21 ASM_NOP8; ASM_NOP8; ASM_NOP5
-#define ASM_NOP24 ASM_NOP8; ASM_NOP8; ASM_NOP8
-#define ASM_NOP29 ASM_NOP8; ASM_NOP8; ASM_NOP8; ASM_NOP5
-#define ASM_NOP32 ASM_NOP8; ASM_NOP8; ASM_NOP8; ASM_NOP8
-#define ASM_NOP40 ASM_NOP8; ASM_NOP8; ASM_NOP8; ASM_NOP8; ASM_NOP8
-
 #define ASM_NOP_MAX 9
 
 #endif /* __X86_ASM_NOPS_H__ */
diff --git a/xen/include/asm-x86/spec_ctrl_asm.h b/xen/include/asm-x86/spec_ctrl_asm.h
index 814f53d..c32c3e6 100644
--- a/xen/include/asm-x86/spec_ctrl_asm.h
+++ b/xen/include/asm-x86/spec_ctrl_asm.h
@@ -216,9 +216,8 @@
 
 /* Use after a VMEXIT from an HVM guest. */
 #define SPEC_CTRL_ENTRY_FROM_VMEXIT                                     \
-    ALTERNATIVE __stringify(ASM_NOP40),                                 \
-        DO_OVERWRITE_RSB, X86_FEATURE_RSB_VMEXIT;                       \
-    ALTERNATIVE_2 __stringify(ASM_NOP32),                               \
+    ALTERNATIVE "", DO_OVERWRITE_RSB, X86_FEATURE_RSB_VMEXIT;           \
+    ALTERNATIVE_2 "",                                                   \
         __stringify(DO_SPEC_CTRL_ENTRY_FROM_VMEXIT                      \
                     ibrs_val=SPEC_CTRL_IBRS),                           \
         X86_FEATURE_XEN_IBRS_SET,                                       \
@@ -228,9 +227,8 @@
 
 /* Use after an entry from PV context (syscall/sysenter/int80/int82/etc). */
 #define SPEC_CTRL_ENTRY_FROM_PV                                         \
-    ALTERNATIVE __stringify(ASM_NOP40),                                 \
-        DO_OVERWRITE_RSB, X86_FEATURE_RSB_NATIVE;                       \
-    ALTERNATIVE_2 __stringify(ASM_NOP21),                               \
+    ALTERNATIVE "", DO_OVERWRITE_RSB, X86_FEATURE_RSB_NATIVE;           \
+    ALTERNATIVE_2 "",                                                   \
         __stringify(DO_SPEC_CTRL_ENTRY maybexen=0                       \
                     ibrs_val=SPEC_CTRL_IBRS),                           \
         X86_FEATURE_XEN_IBRS_SET,                                       \
@@ -239,9 +237,8 @@
 
 /* Use in interrupt/exception context.  May interrupt Xen or PV context. */
 #define SPEC_CTRL_ENTRY_FROM_INTR                                       \
-    ALTERNATIVE __stringify(ASM_NOP40),                                 \
-        DO_OVERWRITE_RSB, X86_FEATURE_RSB_NATIVE;                       \
-    ALTERNATIVE_2 __stringify(ASM_NOP29),                               \
+    ALTERNATIVE "", DO_OVERWRITE_RSB, X86_FEATURE_RSB_NATIVE;           \
+    ALTERNATIVE_2 "",                                                   \
         __stringify(DO_SPEC_CTRL_ENTRY maybexen=1                       \
                     ibrs_val=SPEC_CTRL_IBRS),                           \
         X86_FEATURE_XEN_IBRS_SET,                                       \
@@ -250,13 +247,13 @@
 
 /* Use when exiting to Xen context. */
 #define SPEC_CTRL_EXIT_TO_XEN                                           \
-    ALTERNATIVE_2 __stringify(ASM_NOP17),                               \
+    ALTERNATIVE_2 "",                                                   \
         DO_SPEC_CTRL_EXIT_TO_XEN, X86_FEATURE_XEN_IBRS_SET,             \
         DO_SPEC_CTRL_EXIT_TO_XEN, X86_FEATURE_XEN_IBRS_CLEAR
 
 /* Use when exiting to guest context. */
 #define SPEC_CTRL_EXIT_TO_GUEST                                         \
-    ALTERNATIVE_2 __stringify(ASM_NOP24),                               \
+    ALTERNATIVE_2 "",                                                   \
         DO_SPEC_CTRL_EXIT_TO_GUEST, X86_FEATURE_XEN_IBRS_SET,           \
         DO_SPEC_CTRL_EXIT_TO_GUEST, X86_FEATURE_XEN_IBRS_CLEAR
 
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 6/7] x86/alt: Drop explicit padding of origin sites

[Wei Liu]

On Mon, Feb 12, 2018 at 11:23:06AM +0000, Andrew Cooper wrote:
> Now that the alternatives infrastructure can calculate the required padding
> automatically, there is no need to hard code it.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Wei Liu <wei.liu2@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 6/7] x86/alt: Drop explicit padding of origin sites

[Roger Pau Monné]

On Mon, Feb 12, 2018 at 11:23:06AM +0000, Andrew Cooper wrote:
> Now that the alternatives infrastructure can calculate the required padding
> automatically, there is no need to hard code it.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 6/7] x86/alt: Drop explicit padding of origin sites

[Jan Beulich]

>>> On 12.02.18 at 12:23, <andrew.cooper3@citrix.com> wrote:
> Now that the alternatives infrastructure can calculate the required padding
> automatically, there is no need to hard code it.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 7/7] x86/build: Use new .nop directive when available

[Andrew Cooper]

Newer versions of binutils are capable of emitting an exact number bytes worth
of optimised nops.  Use this in preference to .skip when available.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>

RFC until support is actually committed to binutils mainline.
---
 xen/arch/x86/Rules.mk                 |  1 +
 xen/include/asm-x86/alternative-asm.h | 16 +++++++++++++---
 xen/include/asm-x86/alternative.h     | 17 ++++++++++++-----
 3 files changed, 26 insertions(+), 8 deletions(-)

diff --git a/xen/arch/x86/Rules.mk b/xen/arch/x86/Rules.mk
index 56b2ea8..c3b726c 100644
--- a/xen/arch/x86/Rules.mk
+++ b/xen/arch/x86/Rules.mk
@@ -20,6 +20,7 @@ $(call as-insn-check,CFLAGS,CC,"invept (%rax)$$(comma)%rax",-DHAVE_GAS_EPT)
 $(call as-insn-check,CFLAGS,CC,"rdrand %eax",-DHAVE_GAS_RDRAND)
 $(call as-insn-check,CFLAGS,CC,"rdfsbase %rax",-DHAVE_GAS_FSGSBASE)
 $(call as-insn-check,CFLAGS,CC,"rdseed %eax",-DHAVE_GAS_RDSEED)
+$(call as-insn-check,CFLAGS,CC,".nop 0$$(comma)9",-DHAVE_GAS_LONG_NOPS)
 $(call as-insn-check,CFLAGS,CC,".equ \"x\"$$(comma)1", \
                      -U__OBJECT_LABEL__ -DHAVE_GAS_QUOTED_SYM \
                      '-D__OBJECT_LABEL__=$(subst $(BASEDIR)/,,$(CURDIR))/$$@')
diff --git a/xen/include/asm-x86/alternative-asm.h b/xen/include/asm-x86/alternative-asm.h
index f7e37cb..a4893e7 100644
--- a/xen/include/asm-x86/alternative-asm.h
+++ b/xen/include/asm-x86/alternative-asm.h
@@ -1,6 +1,8 @@
 #ifndef _ASM_X86_ALTERNATIVE_ASM_H_
 #define _ASM_X86_ALTERNATIVE_ASM_H_
 
+#include <asm/nops.h>
+
 #ifdef __ASSEMBLY__
 
 /*
@@ -18,6 +20,14 @@
     .byte \pad_len
 .endm
 
+.macro mknops nr_bytes
+#ifdef HAVE_GAS_LONG_NOPS
+    .nop \nr_bytes, ASM_NOP_MAX
+#else
+    .skip \nr_bytes, 0x90
+#endif
+.endm
+
 #define orig_len               (.L\@_orig_e       -     .L\@_orig_s)
 #define pad_len                (.L\@_orig_p       -     .L\@_orig_e)
 #define total_len              (.L\@_orig_p       -     .L\@_orig_s)
@@ -29,7 +39,7 @@
 .L\@_orig_s:
     \oldinstr
 .L\@_orig_e:
-     .skip (-((repl_len(1) - orig_len) > 0) * (repl_len(1) - orig_len)), 0x90
+     mknops (-((repl_len(1) - orig_len) > 0) * (repl_len(1) - orig_len))
 .L\@_orig_p:
 
     .pushsection .altinstructions, "a", @progbits
@@ -56,8 +66,8 @@
 .L\@_orig_s:
     \oldinstr
 .L\@_orig_e:
-    .skip (-((gas_max(repl_len(1), repl_len(2)) - orig_len) > 0) * \
-             (gas_max(repl_len(1), repl_len(2)) - orig_len)), 0x90
+    mknops (-((gas_max(repl_len(1), repl_len(2)) - orig_len) > 0) * \
+              (gas_max(repl_len(1), repl_len(2)) - orig_len))
 .L\@_orig_p:
 
     .pushsection .altinstructions, "a", @progbits
diff --git a/xen/include/asm-x86/alternative.h b/xen/include/asm-x86/alternative.h
index 20dea22..076d700 100644
--- a/xen/include/asm-x86/alternative.h
+++ b/xen/include/asm-x86/alternative.h
@@ -2,7 +2,6 @@
 #define __X86_ALTERNATIVE_H__
 
 #include <asm/alternative-asm.h>
-#include <asm/nops.h>
 
 #ifndef __ASSEMBLY__
 #include <xen/stringify.h>
@@ -27,6 +26,14 @@ extern void apply_alternatives(const struct alt_instr *start,
                                const struct alt_instr *end);
 extern void alternative_instructions(void);
 
+asm ( ".macro mknops nr_bytes\n\t"
+#ifdef HAVE_GAS_LONG_NOPS
+      ".nop \\nr_bytes, " __stringify(ASM_NOP_MAX) "\n\t"
+#else
+      ".skip \\nr_bytes, 0x90\n\t"
+#endif
+      ".endm\n\t" );
+
 #define repl_s(num)             ".L%=_repl_s"#num
 #define repl_e(num)             ".L%=_repl_e"#num
 
@@ -39,8 +46,8 @@ extern void alternative_instructions(void);
 
 #define OLDINSTR_1(oldinstr, n1)                              \
     ".L%=_orig_s:\n\t" oldinstr "\n .L%=_orig_e:\n\t"         \
-    ".skip (-(("alt_repl_len(n1)"-"alt_orig_len") > 0) * "    \
-             "("alt_repl_len(n1)"-"alt_orig_len")), 0x90\n\t" \
+    "mknops (-(("alt_repl_len(n1)"-"alt_orig_len") > 0) * "   \
+              "("alt_repl_len(n1)"-"alt_orig_len"))\n\t"      \
     ".L%=_orig_p:\n\t"
 
 #define ALT_PADDING_LEN(n1, n2) \
@@ -48,8 +55,8 @@ extern void alternative_instructions(void);
 
 #define OLDINSTR_2(oldinstr, n1, n2)                          \
     ".L%=_orig_s:\n\t" oldinstr "\n .L%=_orig_e:\n\t"         \
-    ".skip (-(("ALT_PADDING_LEN(n1, n2)") > 0) * "            \
-             "("ALT_PADDING_LEN(n1, n2)")), 0x90\n\t"         \
+    "mknops (-(("ALT_PADDING_LEN(n1, n2)") > 0) * "           \
+              "("ALT_PADDING_LEN(n1, n2)"))\n\t"              \
     ".L%=_orig_p:\n\t"
 
 #define ALTINSTR_ENTRY(feature, num)                                    \
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 7/7] x86/build: Use new .nop directive when available

[Wei Liu]

On Mon, Feb 12, 2018 at 11:23:07AM +0000, Andrew Cooper wrote:
> Newer versions of binutils are capable of emitting an exact number bytes worth
> of optimised nops.  Use this in preference to .skip when available.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Wei Liu <wei.liu2@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 7/7] x86/build: Use new .nop directive when available

[Roger Pau Monné]

On Mon, Feb 12, 2018 at 11:23:07AM +0000, Andrew Cooper wrote:
> Newer versions of binutils are capable of emitting an exact number bytes worth
> of optimised nops.  Use this in preference to .skip when available.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> 
> RFC until support is actually committed to binutils mainline.

Do you think you could reference the documentation? I can guess what
the arguments are, but would be better if I can have a reference.

> ---
>  xen/arch/x86/Rules.mk                 |  1 +
>  xen/include/asm-x86/alternative-asm.h | 16 +++++++++++++---
>  xen/include/asm-x86/alternative.h     | 17 ++++++++++++-----
>  3 files changed, 26 insertions(+), 8 deletions(-)
> 
> diff --git a/xen/arch/x86/Rules.mk b/xen/arch/x86/Rules.mk
> index 56b2ea8..c3b726c 100644
> --- a/xen/arch/x86/Rules.mk
> +++ b/xen/arch/x86/Rules.mk
> @@ -20,6 +20,7 @@ $(call as-insn-check,CFLAGS,CC,"invept (%rax)$$(comma)%rax",-DHAVE_GAS_EPT)
>  $(call as-insn-check,CFLAGS,CC,"rdrand %eax",-DHAVE_GAS_RDRAND)
>  $(call as-insn-check,CFLAGS,CC,"rdfsbase %rax",-DHAVE_GAS_FSGSBASE)
>  $(call as-insn-check,CFLAGS,CC,"rdseed %eax",-DHAVE_GAS_RDSEED)
> +$(call as-insn-check,CFLAGS,CC,".nop 0$$(comma)9",-DHAVE_GAS_LONG_NOPS)

Since .nop is going to be used with labels, might be better to have a
test like:

.L1:
.L2:
.nop (.L2 - .L1),9

At least with clang assembler some expressions used in directives
allow labels, while others not (ie: .skip and .fill allow labels,
.rept doesn't)

>  $(call as-insn-check,CFLAGS,CC,".equ \"x\"$$(comma)1", \
>                       -U__OBJECT_LABEL__ -DHAVE_GAS_QUOTED_SYM \
>                       '-D__OBJECT_LABEL__=$(subst $(BASEDIR)/,,$(CURDIR))/$$@')
> diff --git a/xen/include/asm-x86/alternative-asm.h b/xen/include/asm-x86/alternative-asm.h
> index f7e37cb..a4893e7 100644
> --- a/xen/include/asm-x86/alternative-asm.h
> +++ b/xen/include/asm-x86/alternative-asm.h
> @@ -1,6 +1,8 @@
>  #ifndef _ASM_X86_ALTERNATIVE_ASM_H_
>  #define _ASM_X86_ALTERNATIVE_ASM_H_
>  
> +#include <asm/nops.h>
> +
>  #ifdef __ASSEMBLY__
>  
>  /*
> @@ -18,6 +20,14 @@
>      .byte \pad_len
>  .endm
>  
> +.macro mknops nr_bytes
> +#ifdef HAVE_GAS_LONG_NOPS
> +    .nop \nr_bytes, ASM_NOP_MAX
> +#else
> +    .skip \nr_bytes, 0x90
> +#endif
> +.endm

Out of curiosity, is there any reason this is an assembly macro
instead of a define?

Easier to integrate with existing code?

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel