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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 14.04.2020 09:52, Roger Pau Monné wrote:
> On Thu, Apr 09, 2020 at 01:54:40PM +0200, Jan Beulich wrote:
>> On 06.04.2020 12:57, 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);
>>> +            hap_flush_tlb_mask(d->dirty_cpumask);
>>>  
>>>              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);
>>> +        hap_flush_tlb_mask(d->dirty_cpumask);
>>>      }
>>>      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);
>>> +    hap_flush_tlb_mask(d->dirty_cpumask);
>>>  }
>>>  
>>>  /************************************************/
>>> @@ -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);
>>> +        hap_flush_tlb_mask(d->dirty_cpumask);
>>>  
>>>      paging_unlock(d);
>>>  
>>
>> Following up on my earlier mail about paging_log_dirty_range(), I'm
>> now of the opinion that all of these flushes should go away too. I
>> can only assume that they got put where they are when HAP code was
>> cloned from the shadow one. These are only p2m operations, and hence
>> p2m level TLB flushing is all that's needed here.
> 
> IIRC without those ASID flushes NPT won't work correctly, as I think
> it relies on those flushes when updating the p2m.

Hmm, yes - at least for this last one (in patch context) I definitely
agree: NPT's TLB invalidation is quite different from EPT's (and I
was too focused on the latter when writing the earlier reply).
Therefore how about keeping hap_flush_tlb_mask() (and its uses), but
teaching it to do nothing for EPT, while doing an ASID based flush
for NPT?

There may then even be the option to have a wider purpose helper,
dealing with most (all?) of the flushes needed from underneath
x86/mm/, setting the TLB and HVM_ASID_CORE flags as appropriate. In
the EPT case the function would still be a no-op (afaict).

> We could see about moving those flushes inside the NPT functions that
> modify the p2m, AFAICT p2m_pt_set_entry which calls
> p2m->write_p2m_entry relies on the later doing the ASID flushes, as it
> doesn't perform any.

I don't think we want to go this far at this point.

Jan