diff mbox series

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

Message ID 20230607090120.49597-1-roger.pau@citrix.com (mailing list archive)
State New, archived
Headers show
Series [v2] livepatch: apply_alternatives() is only used for livepatch | expand

Commit Message

Roger Pau Monné June 7, 2023, 9:01 a.m. UTC 
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>
---
Changes since v1:
 - Do not guard function prototypes.
---
Andrew raised a valid point of moving the prototype to a common
header, but since I'm not longer touching the prototypes I'm not
introducing such header here.
---
 xen/arch/arm/alternative.c | 2 ++
 xen/arch/x86/alternative.c | 5 +++--
 2 files changed, 5 insertions(+), 2 deletions(-)

Comments

Jan Beulich June 7, 2023, 9:10 a.m. UTC | #1 
On 07.06.2023 11:01, 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>

Reviewed-by: Jan Beulich <jbeulich@suse.com>
albeit the implicit ack therein is only on the assumption that (apart
from me) it is generally deemed better ...

> --- 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

... to have the #ifdef than the init_or_livepatch attribute.

Jan

> +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;
Roger Pau Monné June 7, 2023, 9:17 a.m. UTC | #2 
On Wed, Jun 07, 2023 at 11:10:27AM +0200, Jan Beulich wrote:
> On 07.06.2023 11:01, 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>
> 
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> albeit the implicit ack therein is only on the assumption that (apart
> from me) it is generally deemed better ...
> 
> > --- 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
> 
> ... to have the #ifdef than the init_or_livepatch attribute.

But the init_or_livepatch attribute doesn't remove the function when
!CONFIG_LIVEPATCH, so it's not a replacement for the ifdef.

IOW: it's my understanding that the purpose of init_or_livepatch is to
add the __init attribute if livepatch is not enabled, but
apply_alternatives() should never have the __init attribute because
it's solely used by livepatch, it's not used at boot.

Thanks, Roger.
Andrew Cooper June 7, 2023, 9:19 a.m. UTC | #3 
On 07/06/2023 10:17 am, Roger Pau Monné wrote:
> On Wed, Jun 07, 2023 at 11:10:27AM +0200, Jan Beulich wrote:
>> On 07.06.2023 11:01, 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>
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>> albeit the implicit ack therein is only on the assumption that (apart
>> from me) it is generally deemed better ...
>>
>>> --- 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
>> ... to have the #ifdef than the init_or_livepatch attribute.
> But the init_or_livepatch attribute doesn't remove the function when
> !CONFIG_LIVEPATCH, so it's not a replacement for the ifdef.
>
> IOW: it's my understanding that the purpose of init_or_livepatch is to
> add the __init attribute if livepatch is not enabled, but
> apply_alternatives() should never have the __init attribute because
> it's solely used by livepatch, it's not used at boot.

For context, Jan you missed the MISRA call yesterday where this was
identified as an emitted-but-undeferenced function.

~Andrew
Jan Beulich June 7, 2023, 10:15 a.m. UTC | #4 
On 07.06.2023 11:19, Andrew Cooper wrote:
> On 07/06/2023 10:17 am, Roger Pau Monné wrote:
>> On Wed, Jun 07, 2023 at 11:10:27AM +0200, Jan Beulich wrote:
>>> On 07.06.2023 11:01, 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>
>>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>> albeit the implicit ack therein is only on the assumption that (apart
>>> from me) it is generally deemed better ...
>>>
>>>> --- 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
>>> ... to have the #ifdef than the init_or_livepatch attribute.
>> But the init_or_livepatch attribute doesn't remove the function when
>> !CONFIG_LIVEPATCH, so it's not a replacement for the ifdef.
>>
>> IOW: it's my understanding that the purpose of init_or_livepatch is to
>> add the __init attribute if livepatch is not enabled, but
>> apply_alternatives() should never have the __init attribute because
>> it's solely used by livepatch, it's not used at boot.
> 
> For context, Jan you missed the MISRA call yesterday where this was
> identified as an emitted-but-undeferenced function.

Ah, this helps.

Thanks, Jan
Jan Beulich June 7, 2023, 10:19 a.m. UTC | #5 
On 07.06.2023 11:17, Roger Pau Monné wrote:
> On Wed, Jun 07, 2023 at 11:10:27AM +0200, Jan Beulich wrote:
>> On 07.06.2023 11:01, 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>
>>
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>> albeit the implicit ack therein is only on the assumption that (apart
>> from me) it is generally deemed better ...
>>
>>> --- 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
>>
>> ... to have the #ifdef than the init_or_livepatch attribute.
> 
> But the init_or_livepatch attribute doesn't remove the function when
> !CONFIG_LIVEPATCH,

Yes up to here.

> so it's not a replacement for the ifdef.

That depends how you look at it. We don't meaningfully care about a
few extra bytes in .init.text, I think. So it really is the Misra
requirement of not having unreferenced symbols which makes the
difference here.

Jan
Andrew Cooper June 7, 2023, 10:27 a.m. UTC | #6 
On 07/06/2023 11:19 am, Jan Beulich wrote:
> On 07.06.2023 11:17, Roger Pau Monné wrote:
>> On Wed, Jun 07, 2023 at 11:10:27AM +0200, Jan Beulich wrote:
>>> On 07.06.2023 11:01, 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>
>>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>> albeit the implicit ack therein is only on the assumption that (apart
>>> from me) it is generally deemed better ...
>>>
>>>> --- 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
>>> ... to have the #ifdef than the init_or_livepatch attribute.
>> But the init_or_livepatch attribute doesn't remove the function when
>> !CONFIG_LIVEPATCH,
> Yes up to here.
>
>> so it's not a replacement for the ifdef.
> That depends how you look at it. We don't meaningfully care about a
> few extra bytes in .init.text, I think. So it really is the Misra
> requirement of not having unreferenced symbols which makes the
> difference here.

A different option, which I've recommended before for other reasons, is
to build with function/data sections (already supported for livepatch),
and use --gc-sections at link time.

That would remove examples like this from the final binary, and there's
probably wiggle-room at the MISRA level to say "we comply with this
because the linker deletes it".

~Andrew
diff mbox series

Patch

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/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;