Le 27/11/2025 à 15:32, Jan Beulich a écrit :
> On 27.11.2025 14:39, Teddy Astie wrote:
>> Now that p2m->need_flush is set when modifying the pagetable in a way that
>> requires it. We can move the tlb flush logic to p2m->tlb_flush.
>>
>> Introduce hap_tlb_flush to do it, which is called by main p2m logic (e.g
>> p2m_unlock,
>> p2m_tlb_flush_sync, ...). Drop inline calls of guest_flush_tlb_mask which
>> are now
>> redundant with what now does p2m->flush_tlb, allowing us to drop
>> guest_flush_tlb_*
>> as it is now unused.
>>
>> No function change intended.
>>
>> Signed-off-by: Teddy Astie <[email protected]>
>
> I think the safety / correctness of the change needs explaining in the
> description. TLB flushing is often delicate, so it wants to be made pretty
> clear that no necessary flush is now omitted anywhere. It also wants to be
> made clear, from a performance angle, no new excess flushing is added (see
> below for a respective question towards EPT).
>
Overall, all places where there is was a guest_flush_tlb_mask, there is
a implicit call to p2m->tlb_flush (usually through p2m_unlock() or
something that calls it); but that indeed wants to be explained.
>> --- a/xen/arch/x86/flushtlb.c
>> +++ b/xen/arch/x86/flushtlb.c
>> @@ -320,18 +320,3 @@ void cache_writeback(const void *addr, unsigned int
>> size)
>> asm volatile ("sfence" ::: "memory");
>> }
>>
>> -unsigned int guest_flush_tlb_flags(const struct domain *d)
>> -{
>> - bool shadow = paging_mode_shadow(d);
>> - bool asid = is_hvm_domain(d) && (cpu_has_svm || shadow);
>
> There's an SVM dependency here, which ...
>
>> --- a/xen/arch/x86/mm/hap/hap.c
>> +++ b/xen/arch/x86/mm/hap/hap.c
>> @@ -105,8 +105,6 @@ int hap_track_dirty_vram(struct domain *d,
>> p2m_change_type_range(d, begin_pfn, begin_pfn + nr_frames,
>> p2m_ram_rw, p2m_ram_logdirty);
>>
>> - guest_flush_tlb_mask(d, d->dirty_cpumask);
>> -
>> memset(dirty_bitmap, 0xff, size); /* consider all pages dirty
>> */
>> }
>> else
>> @@ -191,7 +189,6 @@ static int cf_check hap_enable_log_dirty(struct domain
>> *d)
>> * to be read-only, or via hardware-assisted log-dirty.
>> */
>> p2m_change_entry_type_global(d, p2m_ram_rw, p2m_ram_logdirty);
>> - guest_flush_tlb_mask(d, d->dirty_cpumask);
>>
>> return 0;
>> }
>> @@ -220,7 +217,6 @@ static void cf_check 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);
>> - guest_flush_tlb_mask(d, d->dirty_cpumask);
>> }
>>
>> /************************************************/
>> @@ -806,18 +802,14 @@ static void cf_check hap_update_paging_modes(struct
>> vcpu *v)
>> put_gfn(d, cr3_gfn);
>> }
>>
>> -static void cf_check
>> -hap_write_p2m_entry_post(struct p2m_domain *p2m, unsigned int oflags)
>> +void hap_p2m_tlb_flush(struct p2m_domain *p2m)
>> {
>> - struct domain *d = p2m->domain;
>> -
>> - if ( oflags & _PAGE_PRESENT )
>> - guest_flush_tlb_mask(d, d->dirty_cpumask);
>> + flush_mask(p2m->domain->dirty_cpumask, FLUSH_HVM_ASID_CORE);
>> }
>>
>> void hap_p2m_init(struct p2m_domain *p2m)
>> {
>> - p2m->write_p2m_entry_post = hap_write_p2m_entry_post;
>> + p2m->tlb_flush = hap_p2m_tlb_flush;
>> }
>
> ... is entirely lost throughout the hap.c changes. Are we now doing excess
> flushing
> for EPT?
Probably not as EPT uses its own tlb_flush function (ept_tlb_flush)
instead. Most changes are isolated to p2m-pt.c, and here only SVM with HAP.
> I guess the relevant point here is that hap_p2m_init(), despite its name
> suggesting otherwise, doesn't come into play when EPT is in use. (This could
> do
> with cleaning up, as right now it then has to be the case that in a AMD_SVM=n
> configuration that function is unreachable, violating a Misra rule.
>
I would like in the end that NPT and EPT being similar to use the same
tlb flushing logic thus hap_p2m_tlb_flush() (with some changes), but
that requires additional work.
> Also, why would hap_p2m_tlb_flush() lose static and cf_check that the prior
> hook
> function validly had?
>
that's not intended indeed
> Jan
>
--
Teddy Astie | Vates XCP-ng Developer
XCP-ng & Xen Orchestra - Vates solutions
web: https://vates.tech
