* [PATCH] livepatch: apply_alternatives() is only used for livepatch
@ 2023-06-06 17:23 Roger Pau Monne
2023-06-06 18:10 ` Julien Grall
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Roger Pau Monne @ 2023-06-06 17:23 UTC (permalink / raw)
To: xen-devel
Cc: Roger Pau Monne, Stefano Stabellini, Julien Grall,
Bertrand Marquis, Volodymyr Babchuk, Jan Beulich, Andrew Cooper,
Wei Liu
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(®ion, 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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] livepatch: apply_alternatives() is only used for livepatch
2023-06-06 17:23 [PATCH] livepatch: apply_alternatives() is only used for livepatch Roger Pau Monne
@ 2023-06-06 18:10 ` Julien Grall
2023-06-07 8:13 ` Jan Beulich
2023-06-07 8:56 ` Roger Pau Monné
2023-06-06 18:16 ` Andrew Cooper
2023-06-07 8:14 ` Jan Beulich
2 siblings, 2 replies; 7+ messages in thread
From: Julien Grall @ 2023-06-06 18:10 UTC (permalink / raw)
To: Roger Pau Monne, xen-devel
Cc: Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk,
Jan Beulich, Andrew Cooper, Wei Liu
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(®ion, 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] livepatch: apply_alternatives() is only used for livepatch
2023-06-06 17:23 [PATCH] livepatch: apply_alternatives() is only used for livepatch Roger Pau Monne
2023-06-06 18:10 ` Julien Grall
@ 2023-06-06 18:16 ` Andrew Cooper
2023-06-07 8:14 ` Jan Beulich
2 siblings, 0 replies; 7+ messages in thread
From: Andrew Cooper @ 2023-06-06 18:16 UTC (permalink / raw)
To: Roger Pau Monne, xen-devel
Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk, Jan Beulich, Wei Liu
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] livepatch: apply_alternatives() is only used for livepatch
2023-06-06 18:10 ` Julien Grall
@ 2023-06-07 8:13 ` Jan Beulich
2023-06-07 8:56 ` Roger Pau Monné
1 sibling, 0 replies; 7+ messages in thread
From: Jan Beulich @ 2023-06-07 8:13 UTC (permalink / raw)
To: Julien Grall
Cc: Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk,
Andrew Cooper, Wei Liu, Roger Pau Monne, xen-devel
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] livepatch: apply_alternatives() is only used for livepatch
2023-06-06 17:23 [PATCH] livepatch: apply_alternatives() is only used for livepatch Roger Pau Monne
2023-06-06 18:10 ` Julien Grall
2023-06-06 18:16 ` Andrew Cooper
@ 2023-06-07 8:14 ` Jan Beulich
2023-06-07 8:47 ` Roger Pau Monné
2 siblings, 1 reply; 7+ messages in thread
From: Jan Beulich @ 2023-06-07 8:14 UTC (permalink / raw)
To: Roger Pau Monne
Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk, Andrew Cooper, Wei Liu, xen-devel
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] livepatch: apply_alternatives() is only used for livepatch
2023-06-07 8:14 ` Jan Beulich
@ 2023-06-07 8:47 ` Roger Pau Monné
0 siblings, 0 replies; 7+ messages in thread
From: Roger Pau Monné @ 2023-06-07 8:47 UTC (permalink / raw)
To: Jan Beulich
Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk, Andrew Cooper, Wei Liu, xen-devel
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] livepatch: apply_alternatives() is only used for livepatch
2023-06-06 18:10 ` Julien Grall
2023-06-07 8:13 ` Jan Beulich
@ 2023-06-07 8:56 ` Roger Pau Monné
1 sibling, 0 replies; 7+ messages in thread
From: Roger Pau Monné @ 2023-06-07 8:56 UTC (permalink / raw)
To: Julien Grall
Cc: xen-devel, Stefano Stabellini, Bertrand Marquis,
Volodymyr Babchuk, Jan Beulich, Andrew Cooper, Wei Liu
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(®ion, 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.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2023-06-07 8:57 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-06-06 17:23 [PATCH] livepatch: apply_alternatives() is only used for livepatch Roger Pau Monne
2023-06-06 18:10 ` Julien Grall
2023-06-07 8:13 ` Jan Beulich
2023-06-07 8:56 ` Roger Pau Monné
2023-06-06 18:16 ` Andrew Cooper
2023-06-07 8:14 ` Jan Beulich
2023-06-07 8:47 ` Roger Pau Monné
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).