Re: [PATCH 4/5] x86/PV: restrict TLB flushing after mod_l[234]_entry()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 11.01.2021 14:00, Roger Pau Monné wrote:
> On Tue, Nov 03, 2020 at 11:57:33AM +0100, Jan Beulich wrote:
>> Just like we avoid to invoke remote root pt flushes when all uses of an
>> L4 table can be accounted for locally, the same can be done for all of
>> L[234] for the linear pt flush when the table is a "free floating" one,
>> i.e. it is pinned but not hooked up anywhere. While this situation
>> doesn't occur very often, it can be observed.
>>
>> Since this breaks one of the implications of the XSA-286 fix, drop the
>> flush_root_pt_local variable again and set ->root_pgt_changed directly,
>> just like it was before that change.
>>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> LGTM, so:
> 
> Acked-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Thanks.

> It would be good however if Andrew can give is opinion also, since he
> was the one to introduce such logic, and it's not trivial.

I can certainly wait some, also to hopefully get a comment on

>> ---
>> While adjusting the big comment that was added for XSA-286 I wondered
>> why it talks about the "construction of 32bit PV guests". How are 64-bit
>> PV guests different in this regard?

... this, but his email troubles make it hard to judge for how
long to wait.

>> @@ -4054,7 +4053,9 @@ long do_mmu_update(
>>                          break;
>>                      rc = mod_l2_entry(va, l2e_from_intpte(req.val), mfn,
>>                                        cmd == MMU_PT_UPDATE_PRESERVE_AD, v);
>> -                    if ( !rc )
>> +                    if ( !rc &&
>> +                         (page->u.inuse.type_info & PGT_count_mask) >
>> +                         1 + !!(page->u.inuse.type_info & PGT_pinned) )
> 
> I think adding a macro to encapsulate the added condition would make
> the code clearer, maybe PAGETABLE_IN_USE, _LOADED or _ACTIVE?

A macro or inline function may certainly be nice (indeed I did
consider adding one), but I think here more than in some other
cases it is crucial that the name properly reflect what it
does. Unfortunately none of the ones you suggest nor any of the
ones I did consider will meet this requirement. In particular,
the check is about "is there any _other_ use". Hence I went
with the not very fortunate redundant open-coding.

Jan