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@vates.tech>
---
I would like hap_p2m_tlb_flush() to use p2m->dirty_cpumask instead of
p2m->domain->dirty_cpumask. But p2m->dirty_cpumask is updated nowhere
in the current logic, so that doesn't work and p2m->domain->dirty_cpumask
is used instead (which works, even though it is less efficient with np2m).
xen/arch/x86/flushtlb.c | 15 ---------------
xen/arch/x86/include/asm/flushtlb.h | 3 ---
xen/arch/x86/include/asm/p2m.h | 3 ---
xen/arch/x86/mm/hap/hap.c | 14 +++-----------
xen/arch/x86/mm/hap/nested_hap.c | 7 +------
xen/arch/x86/mm/nested.c | 2 +-
xen/arch/x86/mm/p2m-pt.c | 10 ++--------
xen/arch/x86/mm/p2m.c | 2 --
8 files changed, 7 insertions(+), 49 deletions(-)
diff --git a/xen/arch/x86/flushtlb.c b/xen/arch/x86/flushtlb.c
index 09e676c151..48e0142848 100644
--- 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);
-
- return (shadow ? FLUSH_TLB : 0) | (asid ? FLUSH_HVM_ASID_CORE : 0);
-}
-
-void guest_flush_tlb_mask(const struct domain *d, const cpumask_t *mask)
-{
- unsigned int flags = guest_flush_tlb_flags(d);
-
- if ( flags )
- flush_mask(mask, flags);
-}
diff --git a/xen/arch/x86/include/asm/flushtlb.h b/xen/arch/x86/include/asm/flushtlb.h
index 7bcbca2b7f..5be6c4e08d 100644
--- a/xen/arch/x86/include/asm/flushtlb.h
+++ b/xen/arch/x86/include/asm/flushtlb.h
@@ -190,7 +190,4 @@ void flush_area_mask(const cpumask_t *mask, const void *va,
static inline void flush_page_to_ram(unsigned long mfn, bool sync_icache) {}
-unsigned int guest_flush_tlb_flags(const struct domain *d);
-void guest_flush_tlb_mask(const struct domain *d, const cpumask_t *mask);
-
#endif /* __FLUSHTLB_H__ */
diff --git a/xen/arch/x86/include/asm/p2m.h b/xen/arch/x86/include/asm/p2m.h
index 9016e88411..9ee79c9d39 100644
--- a/xen/arch/x86/include/asm/p2m.h
+++ b/xen/arch/x86/include/asm/p2m.h
@@ -886,9 +886,6 @@ void np2m_flush_base(struct vcpu *v, unsigned long np2m_base);
void hap_p2m_init(struct p2m_domain *p2m);
void shadow_p2m_init(struct p2m_domain *p2m);
-void cf_check nestedp2m_write_p2m_entry_post(
- struct p2m_domain *p2m, unsigned int oflags);
-
#ifdef CONFIG_ALTP2M
/*
diff --git a/xen/arch/x86/mm/hap/hap.c b/xen/arch/x86/mm/hap/hap.c
index 2f69ff9c7b..58254c3039 100644
--- 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;
}
static unsigned long cf_check hap_gva_to_gfn_real_mode(
diff --git a/xen/arch/x86/mm/hap/nested_hap.c b/xen/arch/x86/mm/hap/nested_hap.c
index 255fba7e1c..2b4ccc0c81 100644
--- a/xen/arch/x86/mm/hap/nested_hap.c
+++ b/xen/arch/x86/mm/hap/nested_hap.c
@@ -58,12 +58,7 @@
/* NESTED VIRT P2M FUNCTIONS */
/********************************************/
-void cf_check
-nestedp2m_write_p2m_entry_post(struct p2m_domain *p2m, unsigned int oflags)
-{
- if ( oflags & _PAGE_PRESENT )
- guest_flush_tlb_mask(p2m->domain, p2m->dirty_cpumask);
-}
+/* None */
/********************************************/
/* NESTED VIRT FUNCTIONS */
diff --git a/xen/arch/x86/mm/nested.c b/xen/arch/x86/mm/nested.c
index 03741ffae4..ac5d990c6c 100644
--- a/xen/arch/x86/mm/nested.c
+++ b/xen/arch/x86/mm/nested.c
@@ -38,7 +38,7 @@ int p2m_init_nestedp2m(struct domain *d)
}
p2m->p2m_class = p2m_nested;
p2m->write_p2m_entry_pre = NULL;
- p2m->write_p2m_entry_post = nestedp2m_write_p2m_entry_post;
+ p2m->write_p2m_entry_post = NULL;
list_add(&p2m->np2m_list, &p2m_get_hostp2m(d)->np2m_list);
}
diff --git a/xen/arch/x86/mm/p2m-pt.c b/xen/arch/x86/mm/p2m-pt.c
index 1fea3884be..24918d09e6 100644
--- a/xen/arch/x86/mm/p2m-pt.c
+++ b/xen/arch/x86/mm/p2m-pt.c
@@ -941,7 +941,7 @@ static void cf_check p2m_pt_change_entry_type_global(
{
l1_pgentry_t *tab;
unsigned long gfn = 0;
- unsigned int i, changed;
+ unsigned int i;
const struct domain *d = p2m->domain;
if ( pagetable_get_pfn(p2m_get_pagetable(p2m)) == 0 )
@@ -950,7 +950,7 @@ static void cf_check p2m_pt_change_entry_type_global(
ASSERT(hap_enabled(d));
tab = map_domain_page(pagetable_get_mfn(p2m_get_pagetable(p2m)));
- for ( changed = i = 0; i < (1 << PAGETABLE_ORDER); ++i )
+ for ( i = 0; i < (1 << PAGETABLE_ORDER); ++i )
{
l1_pgentry_t e = tab[i];
@@ -966,14 +966,10 @@ static void cf_check p2m_pt_change_entry_type_global(
ASSERT_UNREACHABLE();
break;
}
- ++changed;
}
gfn += 1UL << (L4_PAGETABLE_SHIFT - PAGE_SHIFT);
}
unmap_domain_page(tab);
-
- if ( changed )
- guest_flush_tlb_mask(d, d->dirty_cpumask);
}
static int cf_check p2m_pt_change_entry_type_range(
@@ -1185,5 +1181,3 @@ void p2m_pt_init(struct p2m_domain *p2m)
p2m->audit_p2m = p2m_pt_audit_p2m;
#endif
}
-
-
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 98f8272270..bbdc20cbd9 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -2187,8 +2187,6 @@ void p2m_log_dirty_range(struct domain *d, unsigned long begin_pfn,
dirty_bitmap[i >> 3] |= (1 << (i & 7));
p2m_unlock(p2m);
-
- guest_flush_tlb_mask(d, d->dirty_cpumask);
}
/*
--
2.51.2
--
Teddy Astie | Vates XCP-ng Developer
XCP-ng & Xen Orchestra - Vates solutions
web: https://vates.techOn 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@vates.tech>
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
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 <teddy.astie@vates.tech>
>
> 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
© 2016 - 2025 Red Hat, Inc.