[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



On 23.04.2020 12:57, 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);
> 
> But would result in a single use asid local variable.
> 
> I think XSA-316 was slightly different because the issue arose from
> assigning and comparing a variable inside of an if condition, which is
> not the case here. I however don't mind changing it if it's regarded
> as potentially insecure, or hard to read.

I'd be okay to ack either, but would still somewhat prefer the original
variant.

Jan



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.