Re: [PATCH 6/8] xen/livepatch: Support new altcall scheme

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.cooper3@xxxxxxxxxx>
> Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Thanks.

>
> One nit/comment below.
>
>> ---
>> CC: Jan Beulich <JBeulich@xxxxxxxx>
>> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>> CC: Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>
>> ---
>>  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