[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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |