Re: [PATCH] x86/paging: Delete update_cr3()'s do_locking parameter

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 21/09/2023 1:38 pm, Jan Beulich wrote:
> On 20.09.2023 21:21, Andrew Cooper wrote:
>> Nicola reports that the XSA-438 fix introduced new MISRA violations because 
>> of
>> some incidental tidying it tried to do.  The parameter is useless, so resolve
>> the MISRA regression by removing it.
>>
>> hap_update_cr3() discards the parameter entirely, while sh_update_cr3() uses
>> it to distinguish internal and external callers and therefore whether the
>> paging lock should be taken.
>>
>> However, we have paging_lock_recursive() for this purpose, which also avoids
>> the ability for the shadow internal callers to accidentally not hold the 
>> lock.
>>
>> Fixes: fb0ff49fe9f7 ("x86/shadow: defer releasing of PV's top-level shadow 
>> reference")
>> Reported-by: Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> ---
>> CC: Jan Beulich <JBeulich@xxxxxxxx>
>> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>> CC: Wei Liu <wl@xxxxxxx>
>> CC: George Dunlap <george.dunlap@xxxxxxxxxxxxx>
>> CC: Tim Deegan <tim@xxxxxxx>
>> CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
>> CC: Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>
>>
>> Slightly RFC.  Only compile tested so far.
> With shadow/none.c also suitably edited
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Ah yes - I did forget about none.c.  Thanks.

> I'm a little surprised you introduce new uses of the (kind of odd) recursive 
> lock,
> when previously you voiced your dislike for our use of such. ("Kind of odd" 
> because
> unlike spin_lock_recursive(), only the potentially inner caller needs to use 
> the
> recursive form of the acquire.)

I do very much dislike recursive locks, and I do think that an
alternative universe without them would be better code.  But a stream of
int/bool params are a similarly bad antipattern too.

As paging_lock_recursive() is used for this exact purpose elsewhere,
it's silly not to use fix one of the problems when it doesn't really
make the other problem worse.

~Andrew