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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 25/09/2024 11:02 am, Jan Beulich wrote:
> On 25.09.2024 11:28, Roger Pau Monné wrote:
>> On Wed, Sep 25, 2024 at 11:01:08AM +0200, Jan Beulich wrote:
>>> On 25.09.2024 10:42, 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>
>>> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
>>>
>>> Albeit ...
>>>
>>>> @@ -394,8 +418,10 @@ static int __init cf_check nmi_apply_alternatives(
>>>>                                   PAGE_HYPERVISOR_RWX);
>>>>          flush_local(FLUSH_TLB_GLOBAL);
>>>>  
>>>> -        _apply_alternatives(__alt_instructions, __alt_instructions_end,
>>>> -                            alt_done);
>>>> +        rc = _apply_alternatives(__alt_instructions, 
>>>> __alt_instructions_end,
>>>> +                                 alt_done);
>>>> +        if ( rc )
>>>> +            panic("Unable to apply alternatives: %d\n", rc);
>>> ... I see little value in logging rc here - the other log message will
>>> provide better detail anyway.
>> Current log messages do indeed provide better detail, but maybe we end
>> up adding new return error paths to _apply_alternatives() in the
>> future.
> I'd expect such error paths to then also have dedicated logging.
>
>>  I see no harm in printing the error code if there's one.
> Well, it's not much harm indeed, yet imo extra data logged also normally
> wants to have a reason for the logging. After if you look at the log,
> you'd expect every detail to tell you something (useful; in some certain
> cases at least). Anyway - I don't mean to insist on the removal, it just
> looked pretty useless to me.

People reading the logs can ignore bits they don't think are useful.

What they cannot do is read data which should have been there but wasn't.

~Andrew