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

[Jan Beulich <jbeulich@xxxxxxxx>]

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