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

[Wei Liu <wl@xen.org>]

On Thu, Apr 23, 2020 at 12:57:44PM +0200, Roger Pau Monné wrote:
> On Thu, Apr 23, 2020 at 12:41:49PM +0200, Jan Beulich wrote:
> > On 23.04.2020 12:30, Wei Liu wrote:
> > > On Wed, Apr 22, 2020 at 06:33:38PM +0200, Roger Pau Monné wrote:
> > >> On Thu, Apr 16, 2020 at 03:59:07PM +0200, Roger Pau Monne wrote:
> > >>> @@ -254,3 +257,14 @@ unsigned int flush_area_local(const void *va, unsigned int flags)
> > >>>  
> > >>>      return flags;
> > >>>  }
> > >>> +
> > >>> +void guest_flush_tlb_mask(const struct domain *d, const cpumask_t *mask)
> > >>> +{
> > >>> +    unsigned int flags = (is_pv_domain(d) || paging_mode_shadow(d) ? FLUSH_TLB
> > >>> +                                                                   : 0) |
> > >>> +                         (is_hvm_domain(d) && cpu_has_svm ? FLUSH_HVM_ASID_CORE
> > >>> +                                                          : 0);
> > >>
> > >> Maybe I'm getting confused, but I think the above is wrong and ASID
> > >> should _always_ be flushed when running a HVM domain in shadow mode
> > >> regardless of whether the underlying hw is Intel or AMD, ie:
> > >>
> > >> bool shadow = paging_mode_shadow(d);
> > >> unsigned int flags = (shadow ? FLUSH_TLB : 0) |
> > >>                      (is_hvm_domain(d) &&
> > >>                       (cpu_has_svm || shadow) ? FLUSH_HVM_ASID_CORE : 0);
> > > 
> > > This sort of long expression is prone to error. See XSA-316.
> > 
> > To be honest I consider it quite fine. XSA-316 was in particular
> > because of successive closing parentheses, of which there are
> > none here. (This isn't to say I would strictly mind splitting,
> > but I fear this would result in (multiple?) single use local
> > variables.)
> 
> Right now it's exactly (including the indentation):
> 
>     bool shadow = paging_mode_shadow(d);
> 
>     return (shadow ? FLUSH_TLB : 0) |
>            (is_hvm_domain(d) && (cpu_has_svm || shadow) ? FLUSH_HVM_ASID_CORE
>                                                         : 0);
> 
> I could change it to:
> 
>     bool shadow = paging_mode_shadow(d);
>     bool asid = is_hvm_domain(d) && (cpu_has_svm || shadow);
> 
>     return (shadow ? FLUSH_TLB : 0) | (asid ? FLUSH_HVM_ASID_CORE : 0);

IMHO this is much clearer. I merely made a suggestion and it is up to
you and Jan to decide. :-)

Wei.