Re: [RFC PATCH 4/4] x86/hap: Migrate tlb flush logic to p2m->flush_tlb

[Jan Beulich <jbeulich@xxxxxxxx>]

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 <teddy.astie@xxxxxxxxxx>

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).

> --- 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? 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.

Also, why would hap_p2m_tlb_flush() lose static and cf_check that the prior hook
function validly had?

Jan