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

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



On 31.03.2020 18:45, Roger Pau Monné wrote:
> On Tue, Mar 31, 2020 at 05:40:59PM +0200, Jan Beulich wrote:
>> On 20.03.2020 19:42, Roger Pau Monne wrote:
>>> --- a/xen/arch/x86/mm/hap/hap.c
>>> +++ b/xen/arch/x86/mm/hap/hap.c
>>> @@ -118,7 +118,7 @@ int hap_track_dirty_vram(struct domain *d,
>>>              p2m_change_type_range(d, begin_pfn, begin_pfn + nr,
>>>                                    p2m_ram_rw, p2m_ram_logdirty);
>>>  
>>> -            flush_tlb_mask(d->dirty_cpumask);
>>> +            flush_mask(d->dirty_cpumask, FLUSH_TLB | FLUSH_HVM_ASID_CORE);
>>>  
>>>              memset(dirty_bitmap, 0xff, size); /* consider all pages dirty 
>>> */
>>>          }
>>> @@ -205,7 +205,7 @@ static int hap_enable_log_dirty(struct domain *d, 
>>> bool_t log_global)
>>>           * to be read-only, or via hardware-assisted log-dirty.
>>>           */
>>>          p2m_change_entry_type_global(d, p2m_ram_rw, p2m_ram_logdirty);
>>> -        flush_tlb_mask(d->dirty_cpumask);
>>> +        flush_mask(d->dirty_cpumask, FLUSH_TLB | FLUSH_HVM_ASID_CORE);
>>>      }
>>>      return 0;
>>>  }
>>> @@ -234,7 +234,7 @@ static void hap_clean_dirty_bitmap(struct domain *d)
>>>       * be read-only, or via hardware-assisted log-dirty.
>>>       */
>>>      p2m_change_entry_type_global(d, p2m_ram_rw, p2m_ram_logdirty);
>>> -    flush_tlb_mask(d->dirty_cpumask);
>>> +    flush_mask(d->dirty_cpumask, FLUSH_TLB | FLUSH_HVM_ASID_CORE);
>>>  }
>>>  
>>>  /************************************************/
>>> @@ -798,7 +798,7 @@ hap_write_p2m_entry(struct p2m_domain *p2m, unsigned 
>>> long gfn, l1_pgentry_t *p,
>>>  
>>>      safe_write_pte(p, new);
>>>      if ( old_flags & _PAGE_PRESENT )
>>> -        flush_tlb_mask(d->dirty_cpumask);
>>> +        flush_mask(d->dirty_cpumask, FLUSH_TLB | FLUSH_HVM_ASID_CORE);
>>
>> For all four - why FLUSH_TLB? Doesn't the flushing here solely care
>> about guest translations?
> 
> Not on AMD, at least to my understanding, the AMD SDM states:
> 
> "If a hypervisor modifies a nested page table by decreasing permission
> levels, clearing present bits, or changing address translations and
> intends to return to the same ASID, it should use either TLB command
> 011b or 001b."
> 
> It's in section 15.16.1.
> 
> This to my understanding implies that on AMD hardware modifications to
> the NPT require an ASID flush. I assume that on AMD ASIDs also cache
> combined translations, guest linear -> host physical.

I guess I don't follow - I asked about FLUSH_TLB. I agree there needs
to be FLUSH_HVM_ASID_CORE here.

>>> --- a/xen/arch/x86/mm/hap/nested_hap.c
>>> +++ b/xen/arch/x86/mm/hap/nested_hap.c
>>> @@ -84,7 +84,7 @@ nestedp2m_write_p2m_entry(struct p2m_domain *p2m, 
>>> unsigned long gfn,
>>>      safe_write_pte(p, new);
>>>  
>>>      if (old_flags & _PAGE_PRESENT)
>>> -        flush_tlb_mask(p2m->dirty_cpumask);
>>> +        flush_mask(p2m->dirty_cpumask, FLUSH_TLB | FLUSH_HVM_ASID_CORE);
>>
>> Same here then I guess.
>>
>>> --- a/xen/arch/x86/mm/p2m-pt.c
>>> +++ b/xen/arch/x86/mm/p2m-pt.c
>>> @@ -896,7 +896,8 @@ static void p2m_pt_change_entry_type_global(struct 
>>> p2m_domain *p2m,
>>>      unmap_domain_page(tab);
>>>  
>>>      if ( changed )
>>> -         flush_tlb_mask(p2m->domain->dirty_cpumask);
>>> +         flush_mask(p2m->domain->dirty_cpumask,
>>> +                    FLUSH_TLB | FLUSH_HVM_ASID_CORE);
>>
>> Given that this code is used in shadow mode as well, perhaps
>> better to keep it here. Albeit maybe FLUSH_TLB could be dependent
>> upon !hap_enabled()?
>>
>>> --- a/xen/arch/x86/mm/paging.c
>>> +++ b/xen/arch/x86/mm/paging.c
>>> @@ -613,7 +613,7 @@ void paging_log_dirty_range(struct domain *d,
>>>  
>>>      p2m_unlock(p2m);
>>>  
>>> -    flush_tlb_mask(d->dirty_cpumask);
>>> +    flush_mask(d->dirty_cpumask, FLUSH_TLB | FLUSH_HVM_ASID_CORE);
>>
>> Same here?
> 
> I'm fine with doing further refinements, but I would like to be on the
> conservative side and keep such flushes.

Well, if hap.c had FLUSH_TLB dropped, for consistency it should
become conditional here, imo.

>>> @@ -993,7 +993,7 @@ static void shadow_blow_tables(struct domain *d)
>>>                                 pagetable_get_mfn(v->arch.shadow_table[i]), 
>>> 0);
>>>  
>>>      /* Make sure everyone sees the unshadowings */
>>> -    flush_tlb_mask(d->dirty_cpumask);
>>> +    flush_mask(d->dirty_cpumask, FLUSH_TLB | FLUSH_HVM_ASID_CORE);
>>
>> Taking this as example, wouldn't it be more consistent overall if
>> paths not being HVM-only would specify FLUSH_HVM_ASID_CORE only
>> for HVM domains?
> 
> I think there's indeed room for improvement here, as it's likely
> possible to drop some of the ASID/VPID flushes. Given that previous to
> this patch we would flush ASIDs on every TLB flush I think the current
> approach is safe, and as said above further improvements can be done
> afterwards.

There's no safety implication from my suggestion. Needless
FLUSH_HVM_ASID_CORE for non-HVM will result in a call to
hvm_flush_guest_tlbs(), with it then causing the generation
to be incremented without there being any vCPU to consume
this, as there's not going to be a VM entry without a prior
context switch on the specific CPU.

>> Also, seeing the large number of conversions, perhaps have another
>> wrapper, e.g. flush_tlb_mask_hvm(), at least for the cases where
>> both flags get specified unconditionally?
> 
> That's fine for me, if you agree with the proposed naming
> (flush_tlb_mask_hvm) I'm happy to introduce the helper.

Well, I couldn't (and still can't) think of a better (yet not
overly long) name, yet I'm also not fully happy with it.

Jan



 


Rackspace

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