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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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>

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.

> ---
> 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?
> 
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -3903,8 +3903,7 @@ long do_mmu_update(
>      struct vcpu *curr = current, *v = curr;
>      struct domain *d = v->domain, *pt_owner = d, *pg_owner;
>      mfn_t map_mfn = INVALID_MFN, mfn;
> -    bool flush_linear_pt = false, flush_root_pt_local = false,
> -        flush_root_pt_others = false;
> +    bool flush_linear_pt = false, flush_root_pt_others = false;
>      uint32_t xsm_needed = 0;
>      uint32_t xsm_checked = 0;
>      int rc = put_old_guest_table(curr);
> @@ -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?

Thanks, Roger.