On Wed, Sep 25, 2024 at 11:53:30AM +0100, Andrew Cooper wrote:
> 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....@citrix.com>
>
> 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 don't think this will work as-is for livepatches. The call to
register the virtual region is currently done in livepatch_upload(),
after load_payload_data() has completed.
We could see about registering the virtual region earlier (no
volunteering to do that work right now).
> 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.
Sure.
> I can adjust on commit. Everything else is fine.
If you are comfortable with doing the adjustments on commit, please go
ahead.
Thanks, Roger.
