Re: [PATCH v2 5/6] x86/alternatives: do not BUG during apply

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 25/09/2024 9:42 am, Roger Pau Monne wrote:
> alternatives is used both at boot time, and when loading livepatch payloads.
> While for the former it makes sense to panic, it's not useful for the later, 
> as
> for livepatches it's possible to fail to load the livepatch if alternatives
> cannot be resolved and continue operating normally.
>
> Relax the BUGs in _apply_alternatives() to instead return an error code.  The
> caller will figure out whether the failures are fatal and panic.
>
> Print an error message to provide some user-readable information about what
> went wrong.
>
> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Much nicer.  A few more diagnostic comments.

> diff --git a/xen/arch/x86/alternative.c b/xen/arch/x86/alternative.c
> index 7824053c9d33..c8848ba6006e 100644
> --- a/xen/arch/x86/alternative.c
> +++ b/xen/arch/x86/alternative.c
> @@ -198,9 +198,29 @@ static void init_or_livepatch _apply_alternatives(struct 
> alt_instr *start,
>          uint8_t buf[MAX_PATCH_LEN];
>          unsigned int total_len = a->orig_len + a->pad_len;
>  
> -        BUG_ON(a->repl_len > total_len);
> -        BUG_ON(total_len > sizeof(buf));
> -        BUG_ON(a->cpuid >= NCAPINTS * 32);
> +        if ( a->repl_len > total_len )
> +        {
> +            printk(XENLOG_ERR
> +                   "alt replacement size (%#x) bigger than destination 
> (%#x)\n",

These all say "some alternative went wrong", without identifying which. 
For x86_decode_lite(), debugging was far easier when using:

"Alternative for %ps ...", ALT_ORIG_PTR(a)

If we get the order of loading correct, then %ps will even work for a
livepatch, but that's secondary - even the raw number is slightly useful
given the livepatch load address.

I could be talked down to just "Alt for %ps" as you've got it here.  I
think it's clear enough in context.  So, I'd recommend:

"Alt for %ps, replacement size %#x larger than origin %#x\n".

Here, I think origin is better than destination, when discussing
alternatives.

I can adjust on commit.  Everything else is fine.

~Andrew