Re: [PATCH v9 1/3] x86/tlb: introduce a flush HVM ASIDs flag

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

On Thu, Apr 09, 2020 at 01:16:57PM +0200, Jan Beulich wrote:
> On 08.04.2020 17:10, Roger Pau Monné wrote:
> > On Wed, Apr 08, 2020 at 01:25:14PM +0200, Jan Beulich wrote:
> >> On 06.04.2020 12:57, Roger Pau Monne wrote:
> >>> --- a/xen/arch/x86/mm/paging.c
> >>> +++ b/xen/arch/x86/mm/paging.c
> >>> @@ -613,7 +613,8 @@ void paging_log_dirty_range(struct domain *d,
> >>>  
> >>>      p2m_unlock(p2m);
> >>>  
> >>> -    flush_tlb_mask(d->dirty_cpumask);
> >>> +    flush_mask(d->dirty_cpumask, (!hap_enabled(d) ? FLUSH_TLB : 0) |
> >>> +                                 FLUSH_HVM_ASID_CORE);
> >>
> >> In cases where one case is assumed to be more likely than the other
> >> putting the more likely one first can be viewed as a mild hint to
> >> the compiler, and hence an extra ! may be warranted in an if() or
> >> a conditional expression. Here, however, I don't think we can
> >> really consider one case more likely than the other, and hence I'd
> >> suggest to avoid the !, flipping the other two expressions
> >> accordingly. I may take the liberty to adjust this while committing
> >> (if I'm to be the one).
> > 
> > That's fine, thanks. Somehow '!hap -> flush' was clearer in my mind.
> 
> Thinking about it with the other HVM-related changes in v9, shouldn't
> this then be
> 
>     flush_mask(d->dirty_cpumask, (hap_enabled(d) ? 0 : FLUSH_TLB) |
>                                  (is_hvm_domain(d) ? FLUSH_HVM_ASID_CORE : 
> 0));
> 
> Or wait - the only caller lives in hap.c. As a result the FLUSH_TLB
> part can be dropped altogether. And I question the need of flushing
> guest TLBs - this is purely a p2m operation. I'll go look at the
> history of this function, but for now I think the call should be
> dropped (albeit then maybe better in a separate patch).

The ASID flush needs to stay unless it's moved into p2m_pt_set_entry,
as p2m_pt_set_entry itself doesn't perform any ASID flush and won't
work correctly.

I think it's safe to remove the TLB flush, as the code is only called
from HAP, and hence is not used by shadow (which is what would require
a plain TLB flush). The placement of this function seems misleading to
me, as it looks like it's used by both shadow and HAP. It might be
better to move it to hap.c if it's only to be used by HAP code.

Thanks, Roger.