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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

Jan