[v1] livepatch: apply_alternatives() is only used for livepatch

[PATCH] livepatch: apply_alternatives() is only used for livepatch

Posted by Roger Pau Monne 11 months, 1 week ago

Guard it with CONFIG_LIVEPATCH.  Note alternatives are applied at boot
using _apply_alternatives().

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/arm/alternative.c             | 2 ++
 xen/arch/arm/include/asm/alternative.h | 2 ++
 xen/arch/x86/alternative.c             | 5 +++--
 xen/arch/x86/include/asm/alternative.h | 2 ++
 4 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/alternative.c b/xen/arch/arm/alternative.c
index 7366af4ea646..016e66978b6d 100644
--- a/xen/arch/arm/alternative.c
+++ b/xen/arch/arm/alternative.c
@@ -223,6 +223,7 @@ void __init apply_alternatives_all(void)
     vunmap(xenmap);
 }
 
+#ifdef CONFIG_LIVEPATCH
 int apply_alternatives(const struct alt_instr *start, const struct alt_instr *end)
 {
     const struct alt_region region = {
@@ -232,6 +233,7 @@ int apply_alternatives(const struct alt_instr *start, const struct alt_instr *en
 
     return __apply_alternatives(&region, 0);
 }
+#endif
 
 /*
  * Local variables:
diff --git a/xen/arch/arm/include/asm/alternative.h b/xen/arch/arm/include/asm/alternative.h
index d3210e82f9e5..ce82dc1ca0d2 100644
--- a/xen/arch/arm/include/asm/alternative.h
+++ b/xen/arch/arm/include/asm/alternative.h
@@ -29,7 +29,9 @@ typedef void (*alternative_cb_t)(const struct alt_instr *alt,
 				 int nr_inst);
 
 void apply_alternatives_all(void);
+#ifdef CONFIG_LIVEPATCH
 int apply_alternatives(const struct alt_instr *start, const struct alt_instr *end);
+#endif
 
 #define ALTINSTR_ENTRY(feature, cb)					      \
 	" .word 661b - .\n"				/* label           */ \
diff --git a/xen/arch/x86/alternative.c b/xen/arch/x86/alternative.c
index 99482766b51f..21af0e825822 100644
--- a/xen/arch/x86/alternative.c
+++ b/xen/arch/x86/alternative.c
@@ -358,11 +358,12 @@ static void init_or_livepatch _apply_alternatives(struct alt_instr *start,
     }
 }
 
-void init_or_livepatch apply_alternatives(struct alt_instr *start,
-                                          struct alt_instr *end)
+#ifdef CONFIG_LIVEPATCH
+void apply_alternatives(struct alt_instr *start, struct alt_instr *end)
 {
     _apply_alternatives(start, end, true);
 }
+#endif
 
 static unsigned int __initdata alt_todo;
 static unsigned int __initdata alt_done;
diff --git a/xen/arch/x86/include/asm/alternative.h b/xen/arch/x86/include/asm/alternative.h
index a1cd6a9fe5b8..688b554099b3 100644
--- a/xen/arch/x86/include/asm/alternative.h
+++ b/xen/arch/x86/include/asm/alternative.h
@@ -24,7 +24,9 @@ struct __packed alt_instr {
 
 extern void add_nops(void *insns, unsigned int len);
 /* Similar to alternative_instructions except it can be run with IRQs enabled. */
+#ifdef CONFIG_LIVEPATCH
 extern void apply_alternatives(struct alt_instr *start, struct alt_instr *end);
+#endif
 extern void alternative_instructions(void);
 extern void alternative_branches(void);
 
-- 
2.40.0

Re: [PATCH] livepatch: apply_alternatives() is only used for livepatch

Posted by Jan Beulich 11 months, 1 week ago

On 06.06.2023 19:23, Roger Pau Monne wrote:
> --- a/xen/arch/x86/include/asm/alternative.h
> +++ b/xen/arch/x86/include/asm/alternative.h
> @@ -24,7 +24,9 @@ struct __packed alt_instr {
>  
>  extern void add_nops(void *insns, unsigned int len);
>  /* Similar to alternative_instructions except it can be run with IRQs enabled. */
> +#ifdef CONFIG_LIVEPATCH
>  extern void apply_alternatives(struct alt_instr *start, struct alt_instr *end);
> +#endif

I don't see the need for an #ifdef on the declaration. We avoid such
in a fair number of other cases, keeping the code better readable.

Jan

Re: [PATCH] livepatch: apply_alternatives() is only used for livepatch

Posted by Roger Pau Monné 11 months, 1 week ago

On Wed, Jun 07, 2023 at 10:14:35AM +0200, Jan Beulich wrote:
> On 06.06.2023 19:23, Roger Pau Monne wrote:
> > --- a/xen/arch/x86/include/asm/alternative.h
> > +++ b/xen/arch/x86/include/asm/alternative.h
> > @@ -24,7 +24,9 @@ struct __packed alt_instr {
> >  
> >  extern void add_nops(void *insns, unsigned int len);
> >  /* Similar to alternative_instructions except it can be run with IRQs enabled. */
> > +#ifdef CONFIG_LIVEPATCH
> >  extern void apply_alternatives(struct alt_instr *start, struct alt_instr *end);
> > +#endif
> 
> I don't see the need for an #ifdef on the declaration. We avoid such
> in a fair number of other cases, keeping the code better readable.

Hm, yes, we will get a linker error anyway if attempted to use without
livepatch enabled.

Thanks, Roger.

Re: [PATCH] livepatch: apply_alternatives() is only used for livepatch

Posted by Andrew Cooper 11 months, 1 week ago

On 06/06/2023 6:23 pm, Roger Pau Monne wrote:
> diff --git a/xen/arch/x86/include/asm/alternative.h b/xen/arch/x86/include/asm/alternative.h
> index a1cd6a9fe5b8..688b554099b3 100644
> --- a/xen/arch/x86/include/asm/alternative.h
> +++ b/xen/arch/x86/include/asm/alternative.h
> @@ -24,7 +24,9 @@ struct __packed alt_instr {
>  
>  extern void add_nops(void *insns, unsigned int len);
>  /* Similar to alternative_instructions except it can be run with IRQs enabled. */
> +#ifdef CONFIG_LIVEPATCH
>  extern void apply_alternatives(struct alt_instr *start, struct alt_instr *end);
> +#endif

Given that this is called by common code, it shouldn't live in an
arch-specific header, and it absolutely shouldn't live identically in 2
different arch's header files.

As this is a cleanup patch, we should gain a xen/alternative.h which
depending on CONFIG_ALTERNATIVE includes asm/alternative.h

This will help RISC-V too (a little).

~Andrew

Re: [PATCH] livepatch: apply_alternatives() is only used for livepatch

Posted by Julien Grall 11 months, 1 week ago

Hi Roger,

On 06/06/2023 18:23, Roger Pau Monne wrote:
> Guard it with CONFIG_LIVEPATCH.  Note alternatives are applied at boot
> using _apply_alternatives().
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Julien Grall <jgrall@amazon.com>

> ---
>   xen/arch/arm/alternative.c             | 2 ++
>   xen/arch/arm/include/asm/alternative.h | 2 ++
>   xen/arch/x86/alternative.c             | 5 +++--
>   xen/arch/x86/include/asm/alternative.h | 2 ++
>   4 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/alternative.c b/xen/arch/arm/alternative.c
> index 7366af4ea646..016e66978b6d 100644
> --- a/xen/arch/arm/alternative.c
> +++ b/xen/arch/arm/alternative.c
> @@ -223,6 +223,7 @@ void __init apply_alternatives_all(void)
>       vunmap(xenmap);
>   }
>   
> +#ifdef CONFIG_LIVEPATCH
>   int apply_alternatives(const struct alt_instr *start, const struct alt_instr *end)
>   {
>       const struct alt_region region = {
> @@ -232,6 +233,7 @@ int apply_alternatives(const struct alt_instr *start, const struct alt_instr *en
>   
>       return __apply_alternatives(&region, 0);
>   }
> +#endif
>   
>   /*
>    * Local variables:
> diff --git a/xen/arch/arm/include/asm/alternative.h b/xen/arch/arm/include/asm/alternative.h
> index d3210e82f9e5..ce82dc1ca0d2 100644
> --- a/xen/arch/arm/include/asm/alternative.h
> +++ b/xen/arch/arm/include/asm/alternative.h
> @@ -29,7 +29,9 @@ typedef void (*alternative_cb_t)(const struct alt_instr *alt,
>   				 int nr_inst);
>   
>   void apply_alternatives_all(void);
> +#ifdef CONFIG_LIVEPATCH
>   int apply_alternatives(const struct alt_instr *start, const struct alt_instr *end);
> +#endif
>   
>   #define ALTINSTR_ENTRY(feature, cb)					      \
>   	" .word 661b - .\n"				/* label           */ \
> diff --git a/xen/arch/x86/alternative.c b/xen/arch/x86/alternative.c
> index 99482766b51f..21af0e825822 100644
> --- a/xen/arch/x86/alternative.c
> +++ b/xen/arch/x86/alternative.c
> @@ -358,11 +358,12 @@ static void init_or_livepatch _apply_alternatives(struct alt_instr *start,
>       }
>   }
>   
> -void init_or_livepatch apply_alternatives(struct alt_instr *start,
> -                                          struct alt_instr *end)

NIT: It sounds like the init_* was a left-over after commit 67d01cdb5518 
("x86: infrastructure to allow converting certain indirect calls to 
direct ones").

Cheers,

-- 
Julien Grall

Re: [PATCH] livepatch: apply_alternatives() is only used for livepatch

Posted by Roger Pau Monné 11 months, 1 week ago

On Tue, Jun 06, 2023 at 07:10:05PM +0100, Julien Grall wrote:
> Hi Roger,
> 
> On 06/06/2023 18:23, Roger Pau Monne wrote:
> > Guard it with CONFIG_LIVEPATCH.  Note alternatives are applied at boot
> > using _apply_alternatives().
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Reviewed-by: Julien Grall <jgrall@amazon.com>

Thanks!

> > ---
> >   xen/arch/arm/alternative.c             | 2 ++
> >   xen/arch/arm/include/asm/alternative.h | 2 ++
> >   xen/arch/x86/alternative.c             | 5 +++--
> >   xen/arch/x86/include/asm/alternative.h | 2 ++
> >   4 files changed, 9 insertions(+), 2 deletions(-)
> > 
> > diff --git a/xen/arch/arm/alternative.c b/xen/arch/arm/alternative.c
> > index 7366af4ea646..016e66978b6d 100644
> > --- a/xen/arch/arm/alternative.c
> > +++ b/xen/arch/arm/alternative.c
> > @@ -223,6 +223,7 @@ void __init apply_alternatives_all(void)
> >       vunmap(xenmap);
> >   }
> > +#ifdef CONFIG_LIVEPATCH
> >   int apply_alternatives(const struct alt_instr *start, const struct alt_instr *end)
> >   {
> >       const struct alt_region region = {
> > @@ -232,6 +233,7 @@ int apply_alternatives(const struct alt_instr *start, const struct alt_instr *en
> >       return __apply_alternatives(&region, 0);
> >   }
> > +#endif
> >   /*
> >    * Local variables:
> > diff --git a/xen/arch/arm/include/asm/alternative.h b/xen/arch/arm/include/asm/alternative.h
> > index d3210e82f9e5..ce82dc1ca0d2 100644
> > --- a/xen/arch/arm/include/asm/alternative.h
> > +++ b/xen/arch/arm/include/asm/alternative.h
> > @@ -29,7 +29,9 @@ typedef void (*alternative_cb_t)(const struct alt_instr *alt,
> >   				 int nr_inst);
> >   void apply_alternatives_all(void);
> > +#ifdef CONFIG_LIVEPATCH
> >   int apply_alternatives(const struct alt_instr *start, const struct alt_instr *end);
> > +#endif
> >   #define ALTINSTR_ENTRY(feature, cb)					      \
> >   	" .word 661b - .\n"				/* label           */ \
> > diff --git a/xen/arch/x86/alternative.c b/xen/arch/x86/alternative.c
> > index 99482766b51f..21af0e825822 100644
> > --- a/xen/arch/x86/alternative.c
> > +++ b/xen/arch/x86/alternative.c
> > @@ -358,11 +358,12 @@ static void init_or_livepatch _apply_alternatives(struct alt_instr *start,
> >       }
> >   }
> > -void init_or_livepatch apply_alternatives(struct alt_instr *start,
> > -                                          struct alt_instr *end)
> 
> NIT: It sounds like the init_* was a left-over after commit 67d01cdb5518
> ("x86: infrastructure to allow converting certain indirect calls to direct
> ones").

I think it doesn't warrant a fixes tag in this case.  The commit you
point out should also have added the livepatch guards in x86 at least,
since that's the only caller of apply_alternatives() after the
patch.

Thanks, Roger.

Re: [PATCH] livepatch: apply_alternatives() is only used for livepatch

Posted by Jan Beulich 11 months, 1 week ago

On 06.06.2023 20:10, Julien Grall wrote:
> On 06/06/2023 18:23, Roger Pau Monne wrote:
>> --- a/xen/arch/x86/alternative.c
>> +++ b/xen/arch/x86/alternative.c
>> @@ -358,11 +358,12 @@ static void init_or_livepatch _apply_alternatives(struct alt_instr *start,
>>       }
>>   }
>>   
>> -void init_or_livepatch apply_alternatives(struct alt_instr *start,
>> -                                          struct alt_instr *end)
> 
> NIT: It sounds like the init_* was a left-over after commit 67d01cdb5518 
> ("x86: infrastructure to allow converting certain indirect calls to 
> direct ones").

Iirc this was intentional, to avoid the need for an #ifdef.

Jan