Re: [Xen-devel] [PATCH v3 5/7] x86/alt: Support for automatic padding calculations

[Andrew Cooper]

On 08/03/18 16:49, Jan Beulich wrote:
 On 08.03.18 at 17:23,  wrote:
>> On 08/03/18 15:42, Jan Beulich wrote:
>> On 07.03.18 at 16:51,  wrote:
 @@ -175,18 +175,47 @@ void init_or_livepatch apply_alternatives(const 
 struct alt_instr *start,
   * So be careful if you want to change the scan order to any other
   * order.
   */
 -for ( a = start; a < end; a++ )
 +for ( a = base = start; a < end; a++ )
  {
  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);
  
 +/*
 + * Detect sequences of alt_instr's patching the same origin site, 
 and
 + * keep base pointing at the first alt_instr entry.  This is so 
 we can
 + * refer to a single ->priv field for patching decisions.
 + *
 + * ->priv being nonzero means that the origin site has already 
 been
 + * modified, and we shouldn't try to optimise the nops again.
 + */
 +if ( ALT_ORIG_PTR(base) != orig )
 +base = a;
>>> I don't understand why you need the new "priv" field - have a
>>> boolean local variable which you reset instead of base here, and
>>> which you check/set instead of base->priv below.
>> That can break in a "corrupted instruction stream" kind of way if we
>> perform two passes over the same set of alt_instr's, e.g. after loading
>> microcode, finding some new features, and rerunning alternatives.
> Well, we don't do anything like that today (which first of all would
> require all that stuff to come out of .init.*). But yes, if you want
> the code be prepared for that, fine with me:
>
> Reviewed-by: Jan Beulich 
>
> But it would be nice if you called out that extra aspect in the
> description.

Sure - I'll update this comment in the code.

FWIW, I chose this specific example because alternatives handling for
livepatching Spectre mitigations was a particularly sore area for some
people.

~Andrew

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

Re: [Xen-devel] [PATCH v3 5/7] x86/alt: Support for automatic padding calculations

[Jan Beulich]

>>> On 08.03.18 at 17:23,  wrote:
> On 08/03/18 15:42, Jan Beulich wrote:
> On 07.03.18 at 16:51,  wrote:
>>> @@ -175,18 +175,47 @@ void init_or_livepatch apply_alternatives(const 
>>> struct alt_instr *start,
>>>   * So be careful if you want to change the scan order to any other
>>>   * order.
>>>   */
>>> -for ( a = start; a < end; a++ )
>>> +for ( a = base = start; a < end; a++ )
>>>  {
>>>  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);
>>>  
>>> +/*
>>> + * Detect sequences of alt_instr's patching the same origin site, 
>>> and
>>> + * keep base pointing at the first alt_instr entry.  This is so we 
>>> can
>>> + * refer to a single ->priv field for patching decisions.
>>> + *
>>> + * ->priv being nonzero means that the origin site has already been
>>> + * modified, and we shouldn't try to optimise the nops again.
>>> + */
>>> +if ( ALT_ORIG_PTR(base) != orig )
>>> +base = a;
>> I don't understand why you need the new "priv" field - have a
>> boolean local variable which you reset instead of base here, and
>> which you check/set instead of base->priv below.
> 
> That can break in a "corrupted instruction stream" kind of way if we
> perform two passes over the same set of alt_instr's, e.g. after loading
> microcode, finding some new features, and rerunning alternatives.

Well, we don't do anything like that today (which first of all would
require all that stuff to come out of .init.*). But yes, if you want
the code be prepared for that, fine with me:

Reviewed-by: Jan Beulich 

But it would be nice if you called out that extra aspect in the
description.

Jan


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

Re: [Xen-devel] [PATCH v3 5/7] x86/alt: Support for automatic padding calculations

[Andrew Cooper]

On 08/03/18 15:42, Jan Beulich wrote:
 On 07.03.18 at 16:51,  wrote:
>> @@ -175,18 +175,47 @@ void init_or_livepatch apply_alternatives(const struct 
>> alt_instr *start,
>>   * So be careful if you want to change the scan order to any other
>>   * order.
>>   */
>> -for ( a = start; a < end; a++ )
>> +for ( a = base = start; a < end; a++ )
>>  {
>>  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);
>>  
>> +/*
>> + * Detect sequences of alt_instr's patching the same origin site, 
>> and
>> + * keep base pointing at the first alt_instr entry.  This is so we 
>> can
>> + * refer to a single ->priv field for patching decisions.
>> + *
>> + * ->priv being nonzero means that the origin site has already been
>> + * modified, and we shouldn't try to optimise the nops again.
>> + */
>> +if ( ALT_ORIG_PTR(base) != orig )
>> +base = a;
> I don't understand why you need the new "priv" field - have a
> boolean local variable which you reset instead of base here, and
> which you check/set instead of base->priv below.

That can break in a "corrupted instruction stream" kind of way if we
perform two passes over the same set of alt_instr's, e.g. after loading
microcode, finding some new features, and rerunning alternatives.

~Andrew

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

Re: [Xen-devel] [PATCH v3 5/7] x86/alt: Support for automatic padding calculations

[Jan Beulich]

>>> On 07.03.18 at 16:51,  wrote:
> @@ -175,18 +175,47 @@ void init_or_livepatch apply_alternatives(const struct 
> alt_instr *start,
>   * So be careful if you want to change the scan order to any other
>   * order.
>   */
> -for ( a = start; a < end; a++ )
> +for ( a = base = start; a < end; a++ )
>  {
>  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);
>  
> +/*
> + * Detect sequences of alt_instr's patching the same origin site, and
> + * keep base pointing at the first alt_instr entry.  This is so we 
> can
> + * refer to a single ->priv field for patching decisions.
> + *
> + * ->priv being nonzero means that the origin site has already been
> + * modified, and we shouldn't try to optimise the nops again.
> + */
> +if ( ALT_ORIG_PTR(base) != orig )
> +base = a;

I don't understand why you need the new "priv" field - have a
boolean local variable which you reset instead of base here, and
which you check/set instead of base->priv below.

Everything else looks fine to me.

Jan


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

[Xen-devel] [PATCH v3 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.
This is complicated by the fact that we must not attempt to optimise nops over
an origin site which has already been modified.

To keep track of this, add a priv field to struct alt_instr, which gets
modified by apply_alternatives().  One extra requirement is that alt_instr's
referring to the same origin site must now be consecutive, but we already have
this property.

Signed-off-by: Andrew Cooper 
---
CC: Jan Beulich 
CC: Konrad Rzeszutek Wilk 
CC: Roger Pau Monné 
CC: Wei Liu 

v2:
 * Fix build with Clang
v3:
 * s/GCC/GAS/ for the NEGATIVE_TRUE comments
 * Factor out OLDINSTR() generation on the C side
 * Introduce alt_instr->priv and use it to avoid repeatedly optimising the
   alternatives.
---
 xen/arch/x86/Rules.mk |  4 +++
 xen/arch/x86/alternative.c| 45 +++-
 xen/include/asm-x86/alternative-asm.h | 56 +++
 xen/include/asm-x86/alternative.h | 50 ---
 4 files changed, 124 insertions(+), 31 deletions(-)

diff --git a/xen/arch/x86/Rules.mk b/xen/arch/x86/Rules.mk
index a29ab22..ac585a3 100644
--- a/xen/arch/x86/Rules.mk
+++ b/xen/arch/x86/Rules.mk
@@ -25,6 +25,10 @@ $(call as-option-add,CFLAGS,CC,".equ \"x\"$$(comma)1", \
  '-D__OBJECT_LABEL__=$(subst $(BASEDIR)/,,$(CURDIR))/$$@')
 $(call as-option-add,CFLAGS,CC,"invpcid (%rax)$$(comma)%rax",-DHAVE_AS_INVPCID)
 
+# GAS's idea of true is -1.  Clang's idea is 1
+$(call as-option-add,CFLAGS,CC,\
+".if ((1 > 0) < 0); .error \"\";.endif",,-DHAVE_AS_NEGATIVE_TRUE)
+
 CFLAGS += -mno-red-zone -fpic -fno-asynchronous-unwind-tables
 
 # Xen doesn't use SSE interally.  If the compiler supports it, also skip the
diff --git a/xen/arch/x86/alternative.c b/xen/arch/x86/alternative.c
index 51ca53e..93adf56 100644
--- a/xen/arch/x86/alternative.c
+++ b/xen/arch/x86/alternative.c
@@ -159,10 +159,10 @@ text_poke(void *addr, const void *opcode, size_t len)
  * APs have less capabilities than the boot processor are not handled.
  * Tough. Make sure you disable such features by hand.
  */
-void init_or_livepatch apply_alternatives(const struct alt_instr *start,
-  const struct alt_instr *end)
+void init_or_livepatch apply_alternatives(struct alt_instr *start,
+  struct alt_instr *end)
 {
-const struct alt_instr *a;
+struct alt_instr *a, *base;
 
 printk(KERN_INFO "alt table %p -> %p\n", start, end);
 
@@ -175,18 +175,47 @@ void init_or_livepatch apply_alternatives(const struct 
alt_instr *start,
  * So be careful if you want to change the scan order to any other
  * order.
  */
-for ( a = start; a < end; a++ )
+for ( a = base = start; a < end; a++ )
 {
 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);
 
+/*
+ * Detect sequences of alt_instr's patching the same origin site, and
+ * keep base pointing at the first alt_instr entry.  This is so we can
+ * refer to a single ->priv field for patching decisions.
+ *
+ * ->priv being nonzero means that the origin site has already been
+ * modified, and we shouldn't try to optimise the nops again.
+ */
+if ( ALT_ORIG_PTR(base) != orig )
+base = a;
+
+/* If there is no replacement to make, see about optimising the nops. 
*/
 if ( !boot_cpu_has(a->cpuid) )
+{
+/* Origin site site already touched?  Don't nop anything. */
+if ( base->priv )
+continue;
+
+base->priv = 1;
+
+/* Nothing useful to do? */
+if ( a->pad_len <= 1 )
+continue;
+
+add_nops(buf, a->pad_len);
+text_poke(orig + a->orig_len, buf, a->pad_len);
 continue;
+}
+
+base->priv = 1;
 
 memcpy(buf, repl, a->repl_len);
 
@@ -194,8 +223,8 @@ void init_or_livepatch apply_alternatives(const struct 
alt_instr *start,
 if (