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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.)

Jan