Re: [Xen-devel] [PATCH v4 10/16] livepatch: x86, ARM, alternative: Expose FEATURE_LIVEPATCH

2016-09-19 Thread Julien Grall 

Hi Konrad,

On 16/09/2016 18:38, Konrad Rzeszutek Wilk wrote:

To use as a common way of testing alternative patching for
livepatches. Both architectures have this FEATURE and the
test-cases can piggyback on that.

Suggested-by: Julien Grall 
Signed-off-by: Konrad Rzeszutek Wilk 

---
Cc: Julien Grall 
Cc: Stefano Stabellini 
Cc: Jan Beulich 
Cc: Andrew Cooper 

v3: New submission
v4: Move the LIVEPATCH_FEATURE to asm-x86/livepatch.h
---
 xen/arch/arm/livepatch.c  | 3 +++
 xen/include/asm-arm/alternative.h | 2 ++
 xen/include/asm-arm/cpufeature.h  | 5 +
 xen/include/asm-x86/livepatch.h   | 1 +
 xen/test/livepatch/xen_hello_world_func.c | 8 +---
 5 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/xen/arch/arm/livepatch.c b/xen/arch/arm/livepatch.c
index 13d6c10..b771cb7 100644
--- a/xen/arch/arm/livepatch.c
+++ b/xen/arch/arm/livepatch.c
@@ -8,6 +8,7 @@
 #include 
 #include 

+#include 
 #include 
 #include 

@@ -175,6 +176,8 @@ void __init arch_livepatch_init(void)
 end = (void *)LIVEPATCH_VMAP_END;

 vm_init_type(VMAP_XEN, start, end);
+
+cpus_set_cap(LIVEPATCH_FEATURE);
 }

 /*
diff --git a/xen/include/asm-arm/alternative.h 
b/xen/include/asm-arm/alternative.h
index 6851217..cca5f17 100644
--- a/xen/include/asm-arm/alternative.h
+++ b/xen/include/asm-arm/alternative.h
@@ -165,6 +165,8 @@ static inline int apply_alternatives(void *start, size_t 
lenght)
 return 0;
 }

+#define ALTERNATIVE(oldinstr, newinstr, ...) ""
+


Why this change? Nobody should ever use ALTERNATIVE when 
CONFIG_ALTERNATIVE is not enabled. This is a call to have a Xen not 
patched for a buggy hardware.


Not to mention that a void ALTERNATIVE is really buggy. We use 
ALTERNATIVE to switch between two set of instructions. So we always need 
at least one integrated in Xen.



 #endif

 #endif /* __ASM_ALTERNATIVE_H */
diff --git a/xen/include/asm-arm/cpufeature.h b/xen/include/asm-arm/cpufeature.h
index 66e930f..19e768b 100644
--- a/xen/include/asm-arm/cpufeature.h
+++ b/xen/include/asm-arm/cpufeature.h
@@ -40,7 +40,12 @@
 #define ARM32_WORKAROUND_766422 2
 #define ARM64_WORKAROUND_834220 3

+#ifdef CONFIG_LIVEPATCH
+#define LIVEPATCH_FEATURE   4
+#define ARM_NCAPS   5
+#else
 #define ARM_NCAPS   4
+#endif


I would rather define the feature livepatch unconditionally to avoid the 
definition of ARM_NCAPS twice.



diff --git a/xen/test/livepatch/xen_hello_world_func.c 
b/xen/test/livepatch/xen_hello_world_func.c
index 6f53ab4..765a871 100644
--- a/xen/test/livepatch/xen_hello_world_func.c
+++ b/xen/test/livepatch/xen_hello_world_func.c
@@ -20,7 +20,6 @@ const char *xen_hello_world(void)
 unsigned long tmp;
 int rc;

-alternative(ASM_NOP8, ASM_NOP1, X86_FEATURE_LM);


Any reason to move the code later on?


 /*
  * Any BUG, or WARN_ON will contain symbol and payload name. Furthermore
  * exceptions will be caught and processed properly.
@@ -28,8 +27,11 @@ const char *xen_hello_world(void)
 rc = __get_user(tmp, non_canonical_addr);
 BUG_ON(rc != -EFAULT);
 #endif
-#ifdef CONFIG_ARM_64
-asm(ALTERNATIVE("nop", "nop", ARM64_WORKAROUND_CLEAN_CACHE));
+#ifdef CONFIG_ARM
+asm(ALTERNATIVE("nop", "nop", LIVEPATCH_FEATURE));


Ah, this is why you need a nop ALTERNATIVE. My comment above still 
stands, this definition of ALTERNATIVE maybe be valid for your use case, 
but it is invalid for most of the others.



+#endif
+#ifdef CONFIG_X86
+asm(ALTERNATIVE(ASM_NOP8, ASM_NOP1, LIVEPATCH_FEATURE));
 #endif
 return "Hello World";
 }



Regards,

--
Julien Grall

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

[Xen-devel] [PATCH v4 10/16] livepatch: x86, ARM, alternative: Expose FEATURE_LIVEPATCH

2016-09-16 Thread Konrad Rzeszutek Wilk 
To use as a common way of testing alternative patching for
livepatches. Both architectures have this FEATURE and the
test-cases can piggyback on that.

Suggested-by: Julien Grall 
Signed-off-by: Konrad Rzeszutek Wilk 

---
Cc: Julien Grall 
Cc: Stefano Stabellini 
Cc: Jan Beulich 
Cc: Andrew Cooper 

v3: New submission
v4: Move the LIVEPATCH_FEATURE to asm-x86/livepatch.h
---
 xen/arch/arm/livepatch.c  | 3 +++
 xen/include/asm-arm/alternative.h | 2 ++
 xen/include/asm-arm/cpufeature.h  | 5 +
 xen/include/asm-x86/livepatch.h   | 1 +
 xen/test/livepatch/xen_hello_world_func.c | 8 +---
 5 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/xen/arch/arm/livepatch.c b/xen/arch/arm/livepatch.c
index 13d6c10..b771cb7 100644
--- a/xen/arch/arm/livepatch.c
+++ b/xen/arch/arm/livepatch.c
@@ -8,6 +8,7 @@
 #include 
 #include 
 
+#include 
 #include 
 #include 
 
@@ -175,6 +176,8 @@ void __init arch_livepatch_init(void)
 end = (void *)LIVEPATCH_VMAP_END;
 
 vm_init_type(VMAP_XEN, start, end);
+
+cpus_set_cap(LIVEPATCH_FEATURE);
 }
 
 /*
diff --git a/xen/include/asm-arm/alternative.h 
b/xen/include/asm-arm/alternative.h
index 6851217..cca5f17 100644
--- a/xen/include/asm-arm/alternative.h
+++ b/xen/include/asm-arm/alternative.h
@@ -165,6 +165,8 @@ static inline int apply_alternatives(void *start, size_t 
lenght)
 return 0;
 }
 
+#define ALTERNATIVE(oldinstr, newinstr, ...) ""
+
 #endif
 
 #endif /* __ASM_ALTERNATIVE_H */
diff --git a/xen/include/asm-arm/cpufeature.h b/xen/include/asm-arm/cpufeature.h
index 66e930f..19e768b 100644
--- a/xen/include/asm-arm/cpufeature.h
+++ b/xen/include/asm-arm/cpufeature.h
@@ -40,7 +40,12 @@
 #define ARM32_WORKAROUND_766422 2
 #define ARM64_WORKAROUND_834220 3
 
+#ifdef CONFIG_LIVEPATCH
+#define LIVEPATCH_FEATURE   4
+#define ARM_NCAPS   5
+#else
 #define ARM_NCAPS   4
+#endif
 
 #ifndef __ASSEMBLY__
 
diff --git a/xen/include/asm-x86/livepatch.h b/xen/include/asm-x86/livepatch.h
index 7dfc2e7..00aefd2 100644
--- a/xen/include/asm-x86/livepatch.h
+++ b/xen/include/asm-x86/livepatch.h
@@ -10,6 +10,7 @@
 
 #define ARCH_PATCH_INSN_SIZE 5
 #define ARCH_LIVEPATCH_RANGE SZ_2G
+#define LIVEPATCH_FEATUREX86_FEATURE_ALWAYS
 
 #endif /* __XEN_X86_LIVEPATCH_H__ */
 
diff --git a/xen/test/livepatch/xen_hello_world_func.c 
b/xen/test/livepatch/xen_hello_world_func.c
index 6f53ab4..765a871 100644
--- a/xen/test/livepatch/xen_hello_world_func.c
+++ b/xen/test/livepatch/xen_hello_world_func.c
@@ -20,7 +20,6 @@ const char *xen_hello_world(void)
 unsigned long tmp;
 int rc;
 
-alternative(ASM_NOP8, ASM_NOP1, X86_FEATURE_LM);
 /*
  * Any BUG, or WARN_ON will contain symbol and payload name. Furthermore
  * exceptions will be caught and processed properly.
@@ -28,8 +27,11 @@ const char *xen_hello_world(void)
 rc = __get_user(tmp, non_canonical_addr);
 BUG_ON(rc != -EFAULT);
 #endif
-#ifdef CONFIG_ARM_64
-asm(ALTERNATIVE("nop", "nop", ARM64_WORKAROUND_CLEAN_CACHE));
+#ifdef CONFIG_ARM
+asm(ALTERNATIVE("nop", "nop", LIVEPATCH_FEATURE));
+#endif
+#ifdef CONFIG_X86
+asm(ALTERNATIVE(ASM_NOP8, ASM_NOP1, LIVEPATCH_FEATURE));
 #endif
 return "Hello World";
 }
-- 
2.5.5


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