* [RFC PATCH 4/4] x86/hap: Migrate tlb flush logic to p2m->flush_tlb
2025-11-27 13:39 [RFC PATCH 0/4] x86/p2m: Some P2M refactoring Teddy Astie
@ 2025-11-27 13:39 ` Teddy Astie
2025-11-27 14:30 ` Jan Beulich
2025-11-27 13:39 ` [RFC PATCH 1/4] x86/ept: Drop shadow mode check in ept_sync_domain() Teddy Astie
` (3 subsequent siblings)
4 siblings, 1 reply; 13+ messages in thread
From: Teddy Astie @ 2025-11-27 13:39 UTC (permalink / raw)
To: xen-devel; +Cc: Teddy Astie, Jan Beulich, Andrew Cooper, Roger Pau Monné
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.tech
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [RFC PATCH 4/4] x86/hap: Migrate tlb flush logic to p2m->flush_tlb
2025-11-27 13:39 ` [RFC PATCH 4/4] x86/hap: Migrate tlb flush logic to p2m->flush_tlb Teddy Astie
@ 2025-11-27 14:30 ` Jan Beulich
2025-11-27 17:44 ` Teddy Astie
0 siblings, 1 reply; 13+ messages in thread
From: Jan Beulich @ 2025-11-27 14:30 UTC (permalink / raw)
To: Teddy Astie; +Cc: Andrew Cooper, Roger Pau Monné, xen-devel
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).
> --- 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
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [RFC PATCH 4/4] x86/hap: Migrate tlb flush logic to p2m->flush_tlb
2025-11-27 14:30 ` Jan Beulich
@ 2025-11-27 17:44 ` Teddy Astie
0 siblings, 0 replies; 13+ messages in thread
From: Teddy Astie @ 2025-11-27 17:44 UTC (permalink / raw)
To: Jan Beulich; +Cc: Andrew Cooper, Roger Pau Monné, xen-devel
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* [RFC PATCH 1/4] x86/ept: Drop shadow mode check in ept_sync_domain()
2025-11-27 13:39 [RFC PATCH 0/4] x86/p2m: Some P2M refactoring Teddy Astie
2025-11-27 13:39 ` [RFC PATCH 4/4] x86/hap: Migrate tlb flush logic to p2m->flush_tlb Teddy Astie
@ 2025-11-27 13:39 ` Teddy Astie
2025-11-27 13:50 ` Jan Beulich
2025-11-27 13:39 ` [RFC PATCH 3/4] x86/p2m-pt: Set p2m->need_flush if page was present before Teddy Astie
` (2 subsequent siblings)
4 siblings, 1 reply; 13+ messages in thread
From: Teddy Astie @ 2025-11-27 13:39 UTC (permalink / raw)
To: xen-devel; +Cc: Teddy Astie, Jan Beulich, Andrew Cooper, Roger Pau Monné
This function can only be reached from EPT-related code which is inherently
HAP. Thus it is not useful to check for shadow_paging (or lack of HAP) there.
Moreover, it is an error to call this function in the non-EPT cases.
Not a functional change.
Signed-off-by: Teddy Astie <teddy.astie@vates.tech>
---
This function is only called through EPT code and by vmx_domain_update_eptp()
called by EPT log-dirty logic, and doesn't look reachable from shadow paging
code.
I think the original reason of this check was for eventually allowing guests to
use both shadow paging and HAP and switch between the 2 dynamically.
xen/arch/x86/mm/p2m-ept.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
index ce4ef632ae..dfdbfa0afe 100644
--- a/xen/arch/x86/mm/p2m-ept.c
+++ b/xen/arch/x86/mm/p2m-ept.c
@@ -1268,9 +1268,10 @@ static void ept_sync_domain_mask(struct p2m_domain *p2m, const cpumask_t *mask)
void ept_sync_domain(struct p2m_domain *p2m)
{
struct domain *d = p2m->domain;
+ ASSERT(hap_enabled(d));
- /* Only if using EPT and this domain has some VCPUs to dirty. */
- if ( paging_mode_shadow(d) || !d->vcpu || !d->vcpu[0] )
+ /* Only if this domain has some VCPUs to dirty. */
+ if ( !d->vcpu || !d->vcpu[0] )
return;
ept_sync_domain_prepare(p2m);
--
2.51.2
--
Teddy Astie | Vates XCP-ng Developer
XCP-ng & Xen Orchestra - Vates solutions
web: https://vates.tech
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [RFC PATCH 1/4] x86/ept: Drop shadow mode check in ept_sync_domain()
2025-11-27 13:39 ` [RFC PATCH 1/4] x86/ept: Drop shadow mode check in ept_sync_domain() Teddy Astie
@ 2025-11-27 13:50 ` Jan Beulich
0 siblings, 0 replies; 13+ messages in thread
From: Jan Beulich @ 2025-11-27 13:50 UTC (permalink / raw)
To: Teddy Astie; +Cc: Andrew Cooper, Roger Pau Monné, xen-devel
On 27.11.2025 14:39, Teddy Astie wrote:
> This function can only be reached from EPT-related code which is inherently
> HAP. Thus it is not useful to check for shadow_paging (or lack of HAP) there.
>
> Moreover, it is an error to call this function in the non-EPT cases.
>
> Not a functional change.
>
> Signed-off-by: Teddy Astie <teddy.astie@vates.tech>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
> ---
> This function is only called through EPT code and by vmx_domain_update_eptp()
> called by EPT log-dirty logic, and doesn't look reachable from shadow paging
> code.
>
> I think the original reason of this check was for eventually allowing guests to
> use both shadow paging and HAP and switch between the 2 dynamically.
I don't think there ever was such a plan. The function originally lived in vmx.c,
and there - even if just as a safeguard - having the extra check may have made
sense.
Jan
^ permalink raw reply [flat|nested] 13+ messages in thread
* [RFC PATCH 3/4] x86/p2m-pt: Set p2m->need_flush if page was present before
2025-11-27 13:39 [RFC PATCH 0/4] x86/p2m: Some P2M refactoring Teddy Astie
2025-11-27 13:39 ` [RFC PATCH 4/4] x86/hap: Migrate tlb flush logic to p2m->flush_tlb Teddy Astie
2025-11-27 13:39 ` [RFC PATCH 1/4] x86/ept: Drop shadow mode check in ept_sync_domain() Teddy Astie
@ 2025-11-27 13:39 ` Teddy Astie
2025-11-27 14:08 ` Jan Beulich
2025-11-27 13:39 ` [RFC PATCH 2/4] x86/shadow: Replace guest_tlb_flush_mask with sh_flush_tlb_mask Teddy Astie
2025-11-27 19:49 ` [RFC PATCH 0/4] x86/p2m: Some P2M refactoring Andrew Cooper
4 siblings, 1 reply; 13+ messages in thread
From: Teddy Astie @ 2025-11-27 13:39 UTC (permalink / raw)
To: xen-devel; +Cc: Teddy Astie, Jan Beulich, Andrew Cooper, Roger Pau Monné
Update p2m->need_flush if page was present before (requiring a tlb flush).
This causes p2m->flush_tlb() to be now be reachable, make sure we call it
only when set.
Signed-off-by: Teddy Astie <teddy.astie@vates.tech>
---
This change is important for the next patch.
Would it be better to merge it with the next patch ?
xen/arch/x86/mm/p2m-pt.c | 3 +++
xen/arch/x86/mm/p2m.c | 6 ++++--
2 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/xen/arch/x86/mm/p2m-pt.c b/xen/arch/x86/mm/p2m-pt.c
index 5a6ce2f8bc..1fea3884be 100644
--- a/xen/arch/x86/mm/p2m-pt.c
+++ b/xen/arch/x86/mm/p2m-pt.c
@@ -143,6 +143,9 @@ static int write_p2m_entry(struct p2m_domain *p2m, unsigned long gfn,
if ( p2m->write_p2m_entry_post )
p2m->write_p2m_entry_post(p2m, oflags);
+ if ( oflags & _PAGE_PRESENT )
+ p2m->need_flush = true;
+
paging_unlock(d);
if ( nestedhvm_enabled(d) && !p2m_is_nestedp2m(p2m) &&
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index e2a00a0efd..98f8272270 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -274,7 +274,8 @@ void p2m_tlb_flush_sync(struct p2m_domain *p2m)
if ( p2m->need_flush )
{
p2m->need_flush = 0;
- p2m->tlb_flush(p2m);
+ if ( p2m->tlb_flush )
+ p2m->tlb_flush(p2m);
}
}
@@ -287,7 +288,8 @@ void p2m_unlock_and_tlb_flush(struct p2m_domain *p2m)
{
p2m->need_flush = 0;
mm_write_unlock(&p2m->lock);
- p2m->tlb_flush(p2m);
+ if ( p2m->tlb_flush )
+ p2m->tlb_flush(p2m);
} else
mm_write_unlock(&p2m->lock);
}
--
2.51.2
--
Teddy Astie | Vates XCP-ng Developer
XCP-ng & Xen Orchestra - Vates solutions
web: https://vates.tech
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [RFC PATCH 3/4] x86/p2m-pt: Set p2m->need_flush if page was present before
2025-11-27 13:39 ` [RFC PATCH 3/4] x86/p2m-pt: Set p2m->need_flush if page was present before Teddy Astie
@ 2025-11-27 14:08 ` Jan Beulich
2025-11-27 16:37 ` Teddy Astie
0 siblings, 1 reply; 13+ messages in thread
From: Jan Beulich @ 2025-11-27 14:08 UTC (permalink / raw)
To: Teddy Astie; +Cc: Andrew Cooper, Roger Pau Monné, xen-devel
On 27.11.2025 14:39, Teddy Astie wrote:
> Update p2m->need_flush if page was present before (requiring a tlb flush).
> This causes p2m->flush_tlb() to be now be reachable, make sure we call it
> only when set.
I don't follow. Why would p2m->flush_tlb() not have been reachable? There are
other places where ->need_flush is set to true.
> Signed-off-by: Teddy Astie <teddy.astie@vates.tech>
> ---
> This change is important for the next patch.
> Would it be better to merge it with the next patch ?
I'm inclined to say yes, but without understanding the description here I
cannot be sure.
Hmm, wait. The sole place in x86 where the flag is set is in EPT code. So
do you perhaps mean that for NPT or shadow those call sites can now be
reached, and hence - with the hook being NULL there - checks need adding?
Then, by its title, the next patch actually may be making use of the hook,
so the checks then can be dropped again?
Jan
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH 3/4] x86/p2m-pt: Set p2m->need_flush if page was present before
2025-11-27 14:08 ` Jan Beulich
@ 2025-11-27 16:37 ` Teddy Astie
0 siblings, 0 replies; 13+ messages in thread
From: Teddy Astie @ 2025-11-27 16:37 UTC (permalink / raw)
To: Jan Beulich; +Cc: Andrew Cooper, Roger Pau Monné, xen-devel
Le 27/11/2025 à 15:08, Jan Beulich a écrit :
> On 27.11.2025 14:39, Teddy Astie wrote:
>> Update p2m->need_flush if page was present before (requiring a tlb flush).
>> This causes p2m->flush_tlb() to be now be reachable, make sure we call it
>> only when set.
>
> I don't follow. Why would p2m->flush_tlb() not have been reachable? There are
> other places where ->need_flush is set to true.
>
>> Signed-off-by: Teddy Astie <teddy.astie@vates.tech>
>> ---
>> This change is important for the next patch.
>> Would it be better to merge it with the next patch ?
>
> I'm inclined to say yes, but without understanding the description here I
> cannot be sure.
>
> Hmm, wait. The sole place in x86 where the flag is set is in EPT code. So
> do you perhaps mean that for NPT or shadow those call sites can now be
> reached, and hence - with the hook being NULL there - checks need adding?
> Then, by its title, the next patch actually may be making use of the hook,
> so the checks then can be dropped again?
>
Even with the next patch, in the shadow case, p2m->flush_tlb is still
NULL as only HAP-NPT uses hap_p2m_tlb_flush().
> Jan
--
Teddy Astie | Vates XCP-ng Developer
XCP-ng & Xen Orchestra - Vates solutions
web: https://vates.tech
^ permalink raw reply [flat|nested] 13+ messages in thread
* [RFC PATCH 2/4] x86/shadow: Replace guest_tlb_flush_mask with sh_flush_tlb_mask
2025-11-27 13:39 [RFC PATCH 0/4] x86/p2m: Some P2M refactoring Teddy Astie
` (2 preceding siblings ...)
2025-11-27 13:39 ` [RFC PATCH 3/4] x86/p2m-pt: Set p2m->need_flush if page was present before Teddy Astie
@ 2025-11-27 13:39 ` Teddy Astie
2025-11-27 13:57 ` Jan Beulich
2025-11-27 19:49 ` [RFC PATCH 0/4] x86/p2m: Some P2M refactoring Andrew Cooper
4 siblings, 1 reply; 13+ messages in thread
From: Teddy Astie @ 2025-11-27 13:39 UTC (permalink / raw)
To: xen-devel
Cc: Teddy Astie, Jan Beulich, Andrew Cooper, Tim Deegan,
Roger Pau Monné
Introduce sh_flush_tlb_{mask,local} variants used to flush the
tlb from within the shadow paging code. This is meant to decouple
shadow code from the more general guest_tlb_flush_mask.
Not a functional change.
Signed-off-by: Teddy Astie <teddy.astie@vates.tech>
---
xen/arch/x86/mm/shadow/common.c | 12 ++++++------
xen/arch/x86/mm/shadow/hvm.c | 8 ++++----
xen/arch/x86/mm/shadow/multi.c | 18 ++++++------------
xen/arch/x86/mm/shadow/private.h | 22 ++++++++++++++++++++++
4 files changed, 38 insertions(+), 22 deletions(-)
diff --git a/xen/arch/x86/mm/shadow/common.c b/xen/arch/x86/mm/shadow/common.c
index 0176e33bc9..8511da5c7f 100644
--- a/xen/arch/x86/mm/shadow/common.c
+++ b/xen/arch/x86/mm/shadow/common.c
@@ -421,7 +421,7 @@ static int oos_remove_write_access(struct vcpu *v, mfn_t gmfn,
}
if ( ftlb )
- guest_flush_tlb_mask(d, d->dirty_cpumask);
+ sh_flush_tlb_mask(d, d->dirty_cpumask);
return 0;
}
@@ -969,7 +969,7 @@ static bool __must_check _shadow_prealloc(struct domain *d, unsigned int pages)
/* See if that freed up enough space */
if ( d->arch.paging.free_pages >= pages )
{
- guest_flush_tlb_mask(d, d->dirty_cpumask);
+ sh_flush_tlb_mask(d, d->dirty_cpumask);
return true;
}
}
@@ -984,7 +984,7 @@ static bool __must_check _shadow_prealloc(struct domain *d, unsigned int pages)
ASSERT_UNREACHABLE();
- guest_flush_tlb_mask(d, d->dirty_cpumask);
+ sh_flush_tlb_mask(d, d->dirty_cpumask);
return false;
}
@@ -1052,7 +1052,7 @@ void shadow_blow_tables(struct domain *d)
0);
/* Make sure everyone sees the unshadowings */
- guest_flush_tlb_mask(d, d->dirty_cpumask);
+ sh_flush_tlb_mask(d, d->dirty_cpumask);
}
void shadow_blow_tables_per_domain(struct domain *d)
@@ -1157,7 +1157,7 @@ mfn_t shadow_alloc(struct domain *d,
if ( unlikely(!cpumask_empty(&mask)) )
{
perfc_incr(shadow_alloc_tlbflush);
- guest_flush_tlb_mask(d, &mask);
+ sh_flush_tlb_mask(d, &mask);
}
/* Now safe to clear the page for reuse */
clear_domain_page(page_to_mfn(sp));
@@ -2276,7 +2276,7 @@ void sh_remove_shadows(struct domain *d, mfn_t gmfn, int fast, int all)
/* Need to flush TLBs now, so that linear maps are safe next time we
* take a fault. */
- guest_flush_tlb_mask(d, d->dirty_cpumask);
+ sh_flush_tlb_mask(d, d->dirty_cpumask);
paging_unlock(d);
}
diff --git a/xen/arch/x86/mm/shadow/hvm.c b/xen/arch/x86/mm/shadow/hvm.c
index 114957a3e1..b558ed82e8 100644
--- a/xen/arch/x86/mm/shadow/hvm.c
+++ b/xen/arch/x86/mm/shadow/hvm.c
@@ -594,7 +594,7 @@ static void validate_guest_pt_write(struct vcpu *v, mfn_t gmfn,
if ( rc & SHADOW_SET_FLUSH )
/* Need to flush TLBs to pick up shadow PT changes */
- guest_flush_tlb_mask(d, d->dirty_cpumask);
+ sh_flush_tlb_mask(d, d->dirty_cpumask);
if ( rc & SHADOW_SET_ERROR )
{
@@ -744,7 +744,7 @@ bool cf_check shadow_flush_tlb(const unsigned long *vcpu_bitmap)
}
/* Flush TLBs on all CPUs with dirty vcpu state. */
- guest_flush_tlb_mask(d, mask);
+ sh_flush_tlb_mask(d, mask);
/* Done. */
for_each_vcpu ( d, v )
@@ -978,7 +978,7 @@ static void cf_check sh_unshadow_for_p2m_change(
}
if ( flush )
- guest_flush_tlb_mask(d, d->dirty_cpumask);
+ sh_flush_tlb_mask(d, d->dirty_cpumask);
}
#if (SHADOW_OPTIMIZATIONS & SHOPT_FAST_FAULT_PATH)
@@ -1196,7 +1196,7 @@ int shadow_track_dirty_vram(struct domain *d,
}
}
if ( flush_tlb )
- guest_flush_tlb_mask(d, d->dirty_cpumask);
+ sh_flush_tlb_mask(d, d->dirty_cpumask);
goto out;
out_sl1ma:
diff --git a/xen/arch/x86/mm/shadow/multi.c b/xen/arch/x86/mm/shadow/multi.c
index 03be61e225..3924ff4da6 100644
--- a/xen/arch/x86/mm/shadow/multi.c
+++ b/xen/arch/x86/mm/shadow/multi.c
@@ -79,12 +79,6 @@ const char *const fetch_type_names[] = {
# define for_each_shadow_table(v, i) for ( (i) = 0; (i) < 1; ++(i) )
#endif
-/* Helper to perform a local TLB flush. */
-static void sh_flush_local(const struct domain *d)
-{
- flush_local(guest_flush_tlb_flags(d));
-}
-
#if GUEST_PAGING_LEVELS >= 4 && defined(CONFIG_PV32)
#define ASSERT_VALID_L2(t) \
ASSERT((t) == SH_type_l2_shadow || (t) == SH_type_l2h_shadow)
@@ -2429,7 +2423,7 @@ static int cf_check sh_page_fault(
perfc_incr(shadow_rm_write_flush_tlb);
smp_wmb();
atomic_inc(&d->arch.paging.shadow.gtable_dirty_version);
- guest_flush_tlb_mask(d, d->dirty_cpumask);
+ sh_flush_tlb_mask(d, d->dirty_cpumask);
}
#if (SHADOW_OPTIMIZATIONS & SHOPT_OUT_OF_SYNC)
@@ -3243,7 +3237,7 @@ static pagetable_t cf_check sh_update_cr3(struct vcpu *v, bool noflush)
* (old) shadow linear maps in the writeable mapping heuristics. */
#if GUEST_PAGING_LEVELS == 4
if ( sh_remove_write_access(d, gmfn, 4, 0) != 0 )
- guest_flush_tlb_mask(d, d->dirty_cpumask);
+ sh_flush_tlb_mask(d, d->dirty_cpumask);
old_entry = sh_set_toplevel_shadow(v, 0, gmfn, SH_type_l4_shadow,
sh_make_shadow);
if ( unlikely(pagetable_is_null(v->arch.paging.shadow.shadow_table[0])) )
@@ -3284,7 +3278,7 @@ static pagetable_t cf_check sh_update_cr3(struct vcpu *v, bool noflush)
}
}
if ( flush )
- guest_flush_tlb_mask(d, d->dirty_cpumask);
+ sh_flush_tlb_mask(d, d->dirty_cpumask);
/* Now install the new shadows. */
for ( i = 0; i < 4; i++ )
{
@@ -3309,7 +3303,7 @@ static pagetable_t cf_check sh_update_cr3(struct vcpu *v, bool noflush)
}
#elif GUEST_PAGING_LEVELS == 2
if ( sh_remove_write_access(d, gmfn, 2, 0) != 0 )
- guest_flush_tlb_mask(d, d->dirty_cpumask);
+ sh_flush_tlb_mask(d, d->dirty_cpumask);
old_entry = sh_set_toplevel_shadow(v, 0, gmfn, SH_type_l2_shadow,
sh_make_shadow);
ASSERT(pagetable_is_null(old_entry));
@@ -3747,7 +3741,7 @@ static void cf_check sh_pagetable_dying(paddr_t gpa)
}
}
if ( flush )
- guest_flush_tlb_mask(d, d->dirty_cpumask);
+ sh_flush_tlb_mask(d, d->dirty_cpumask);
/* Remember that we've seen the guest use this interface, so we
* can rely on it using it in future, instead of guessing at
@@ -3786,7 +3780,7 @@ static void cf_check sh_pagetable_dying(paddr_t gpa)
mfn_to_page(gmfn)->pagetable_dying = true;
shadow_unhook_mappings(d, smfn, 1/* user pages only */);
/* Now flush the TLB: we removed toplevel mappings. */
- guest_flush_tlb_mask(d, d->dirty_cpumask);
+ sh_flush_tlb_mask(d, d->dirty_cpumask);
}
/* Remember that we've seen the guest use this interface, so we
diff --git a/xen/arch/x86/mm/shadow/private.h b/xen/arch/x86/mm/shadow/private.h
index cef9dbef2e..565a334bc0 100644
--- a/xen/arch/x86/mm/shadow/private.h
+++ b/xen/arch/x86/mm/shadow/private.h
@@ -15,6 +15,7 @@
// been included...
#include <asm/page.h>
#include <xen/domain_page.h>
+#include <asm/flushtlb.h>
#include <asm/x86_emulate.h>
#include <asm/hvm/support.h>
#include <asm/atomic.h>
@@ -910,6 +911,27 @@ static inline int sh_check_page_has_no_refs(struct page_info *page)
((count & PGC_allocated) ? 1 : 0) );
}
+/* Helper to perform a local TLB flush. */
+static inline void sh_flush_local(const struct domain *d)
+{
+ unsigned int flags = FLUSH_TLB;
+
+ if ( is_hvm_domain(d) )
+ flags |= FLUSH_HVM_ASID_CORE;
+
+ flush_local(flags);
+}
+
+static inline void sh_flush_tlb_mask(const struct domain *d, const cpumask_t *mask)
+{
+ unsigned int flags = FLUSH_TLB;
+
+ if ( is_hvm_domain(d) )
+ flags |= FLUSH_HVM_ASID_CORE;
+
+ flush_mask(mask, flags);
+}
+
/* Flush the TLB of the selected vCPUs. */
bool cf_check shadow_flush_tlb(const unsigned long *vcpu_bitmap);
--
2.51.2
--
Teddy Astie | Vates XCP-ng Developer
XCP-ng & Xen Orchestra - Vates solutions
web: https://vates.tech
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [RFC PATCH 0/4] x86/p2m: Some P2M refactoring
2025-11-27 13:39 [RFC PATCH 0/4] x86/p2m: Some P2M refactoring Teddy Astie
` (3 preceding siblings ...)
2025-11-27 13:39 ` [RFC PATCH 2/4] x86/shadow: Replace guest_tlb_flush_mask with sh_flush_tlb_mask Teddy Astie
@ 2025-11-27 19:49 ` Andrew Cooper
2025-11-28 11:14 ` Teddy Astie
4 siblings, 1 reply; 13+ messages in thread
From: Andrew Cooper @ 2025-11-27 19:49 UTC (permalink / raw)
To: Teddy Astie, xen-devel
Cc: andrew.cooper3, Jan Beulich, Roger Pau Monné, Tim Deegan
On 27/11/2025 1:39 pm, Teddy Astie wrote:
> First patch removes a shadow mode check in a function that can't be
> called with shadow mode (only called with EPT hence HAP).
>
> 3 other patches drops guest_tlb_flush_mask by removing all its users :
> in the shadow paging case by migrating it a shadow variant of it and
> in the hap case by moving it to p2m->flush_tlb logic.
>
> One of the goal is to prepare adding HAP-specific optimizations to TLB
> flushing code without risking regressions in the shadow paging code.
You need a clearer background to try and explain the changes. I've
discussed some of it with you, but it needs describing here for everyone
else.
From memory, encrypted AMD VMs cannot use "asid++" to flush TLBs, and
must used VMCB->tlb_ctrl instead.
On top of that, Xen does not have a correct abstraction for "flush guest
VA space" vs "flush guest PA space" vs "flush Xen's mappings of guest VA
space". This comes about because of the shadow code originally had all
3 things together, and HAP didn't clean this up when the need first arose.
This causes over-flushing (Tamas found and reported this on Intel), and
FLUSH_HVM_ASID_CORE isn't an adequate abstraction either.
All of that said, it would help to have a sketch of what you want the
logic to look like in the end.
"flush guest VA space" and "flush guest PA space" originate from
different actions. VA flushes always from emulation of a guest action,
whereas PA flushes originate from modifications to the P2M. When shadow
is in use, both of these escalate into a full local flush because of
Xen's use of shadow linear mappings, but this escalation should be
inside the shadow code, not the top level primitive.
Have I missed anything else?
~Andrew
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [RFC PATCH 0/4] x86/p2m: Some P2M refactoring
2025-11-27 19:49 ` [RFC PATCH 0/4] x86/p2m: Some P2M refactoring Andrew Cooper
@ 2025-11-28 11:14 ` Teddy Astie
0 siblings, 0 replies; 13+ messages in thread
From: Teddy Astie @ 2025-11-28 11:14 UTC (permalink / raw)
To: Andrew Cooper, xen-devel; +Cc: Jan Beulich, Roger Pau Monné, Tim Deegan
Le 27/11/2025 à 20:52, Andrew Cooper a écrit :
> On 27/11/2025 1:39 pm, Teddy Astie wrote:
>> First patch removes a shadow mode check in a function that can't be
>> called with shadow mode (only called with EPT hence HAP).
>>
>> 3 other patches drops guest_tlb_flush_mask by removing all its users :
>> in the shadow paging case by migrating it a shadow variant of it and
>> in the hap case by moving it to p2m->flush_tlb logic.
>>
>> One of the goal is to prepare adding HAP-specific optimizations to TLB
>> flushing code without risking regressions in the shadow paging code.
>
> You need a clearer background to try and explain the changes. I've
> discussed some of it with you, but it needs describing here for everyone
> else.
>
> From memory, encrypted AMD VMs cannot use "asid++" to flush TLBs, and
> must used VMCB->tlb_ctrl instead.
>
Not only for encrypted VMs, but also for broadcast TLB invalidations
(like AMD INVLPGB and Intel RAR) which also requires this.
I'm also wondering if the current model works well when Xen is running
as a nested guest (as the L0 may get confused about the ASID changing
constantly).
> On top of that, Xen does not have a correct abstraction for "flush guest
> VA space" vs "flush guest PA space" vs "flush Xen's mappings of guest VA
> space". This comes about because of the shadow code originally had all
> 3 things together, and HAP didn't clean this up when the need first arose.
>
> This causes over-flushing (Tamas found and reported this on Intel), and
> FLUSH_HVM_ASID_CORE isn't an adequate abstraction either.
>
I guess that also wants to have a way to optionally specify the address
we want to flush (whether it is gva or gpa). So that it can lower to
things like single-address invalidation (instead of flushing everything)
if possible and meaningful.
Having a clearer model would definetely help clarifying
p2m_tlb_flush_sync vs paging_flush_tlb (which sounds like it does the
same thing, but actually not really).
>
> All of that said, it would help to have a sketch of what you want the
> logic to look like in the end.
>
Yes, that's something I planned to do.
> "flush guest VA space" and "flush guest PA space" originate from
> different actions. VA flushes always from emulation of a guest action,
> whereas PA flushes originate from modifications to the P2M. When shadow
> is in use, both of these escalate into a full local flush because of
> Xen's use of shadow linear mappings, but this escalation should be
> inside the shadow code, not the top level primitive.
> > Have I missed anything else?
>
Nested virtualization will also wants clearly defined p2m semantics to
avoid falling into obscure corner cases.
> ~Andrew
>
--
Teddy Astie | Vates XCP-ng Developer
XCP-ng & Xen Orchestra - Vates solutions
web: https://vates.tech
^ permalink raw reply [flat|nested] 13+ messages in thread