On 23/04/2025 12:16 pm, Roger Pau Monné wrote:
> On Wed, Apr 23, 2025 at 02:02:35AM +0100, Andrew Cooper wrote:
>> The new altcall scheme uses an .alt_call_sites section. Wire this up in very
>> much the same way as the .altinstructions section, although there is less
>> sanity checking necessary.
>>
>> Signed-off-by: Andrew Cooper <andrew.coop...@citrix.com>
> Reviewed-by: Roger Pau Monné <roger....@citrix.com>
Thanks.
>
> One nit/comment below.
>
>> ---
>> CC: Jan Beulich <jbeul...@suse.com>
>> CC: Roger Pau Monné <roger....@citrix.com>
>> CC: Ross Lagerwall <ross.lagerw...@citrix.com>
>> ---
>> xen/arch/x86/alternative.c | 6 ++++
>> xen/common/livepatch.c | 58 ++++++++++++++++++++++++++++++
>> xen/include/xen/alternative-call.h | 8 +++--
>> 3 files changed, 70 insertions(+), 2 deletions(-)
>>
>> diff --git a/xen/arch/x86/alternative.c b/xen/arch/x86/alternative.c
>> index f6594e21a14c..22af224f08f7 100644
>> --- a/xen/arch/x86/alternative.c
>> +++ b/xen/arch/x86/alternative.c
>> @@ -479,6 +479,12 @@ int apply_alternatives(struct alt_instr *start, struct
>> alt_instr *end)
>> {
>> return _apply_alternatives(start, end, true);
>> }
>> +
>> +int livepatch_apply_alt_calls(const struct alt_call *start,
>> + const struct alt_call *end)
>> +{
>> + return apply_alt_calls(start, end);
>> +}
>> #endif
>>
>> #define ALT_INSNS (1U << 0)
>> diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
>> index 6ce77bf021b7..be9b7e367553 100644
>> --- a/xen/common/livepatch.c
>> +++ b/xen/common/livepatch.c
>> @@ -905,6 +905,64 @@ static int prepare_payload(struct payload *payload,
>> #endif
>> }
>>
>> + sec = livepatch_elf_sec_by_name(elf, ".alt_call_sites");
>> + if ( sec )
>> + {
>> +#ifdef CONFIG_ALTERNATIVE_CALL
>> + const struct alt_call *a, *start, *end;
>> +
>> + if ( !section_ok(elf, sec, sizeof(*a)) )
>> + return -EINVAL;
>> +
>> + /* Tolerate an empty .alt_call_sites section... */
>> + if ( sec->sec->sh_size == 0 )
> You could possibly move this check to the outer `if` condition, and
> avoid the alt_call_done label?
>
> As even in the !CONFIG_ALTERNATIVE_CALL case skipping an empty section
> would be OK.
.altinstructions is like this. It was put in as part of e74360e4ba4a, I
believe because it was decided that an empty section wasn't wanted.
We can revisit the decision, but the logic should be consistent.
~Andrew
