[PATCH] xen: Use VCPUOP_register_{runstate,vcpu_time}_phys_area when available

[PATCH] xen: Use VCPUOP_register_{runstate,vcpu_time}_phys_area when available

[Teddy Astie]

[-- Attachment #1: Type: text/plain, Size: 7695 bytes --]

Updating these area can be expensive when done through virtual addresses
(especially for runstate one), in particular for HVM/PVH guests.
Xen 4.18 introduced physical address alternatives to address those
short-commings, but they aren't used currently.

Fix it by using these alternatives when available (when reported in
XENFEAT_{runstate,vcpu_time}_phys_area, but keep the current logic
in the PV case.

Assisted-by: LLM
[guidance and testing]
Signed-off-by: Teddy Astie <teddy.astie@vates.tech>
---
When using phys_area ABI, the struct is not allowed to cross a page
boundary anymore, hence we need to be careful with per-cpu struct
alignment.

I've deliberately put the PV domain (!XENFEAT_auto_translated_physmap)
case aside as it profits less from the physical address ABI, and it
would make things more complicated as "physical address" doesn't have
a obvious meaning there.

CC: Juergen Gross <jgross@suse.com>
CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
CC: David Woodhouse <dwmw2@infradead.org>
CC: George Dunlap <gwd@xenproject.org>
CC: x86@kernel.org
CC: linux-kernel@vger.kernel.org

 arch/x86/xen/time.c              | 35 ++++++++++++++++++--------------
 drivers/xen/time.c               | 24 +++++++++++++++++-----
 include/xen/interface/features.h |  9 ++++++++
 include/xen/interface/vcpu.h     | 17 ++++++++++++++++
 4 files changed, 65 insertions(+), 20 deletions(-)

diff --git a/arch/x86/xen/time.c b/arch/x86/xen/time.c
index 5c7822254a01..19ace1b78ae4 100644
--- a/arch/x86/xen/time.c
+++ b/arch/x86/xen/time.c
@@ -37,6 +37,22 @@
 
 static u64 xen_sched_clock_offset __read_mostly;
 
+static int xen_register_time_area(struct pvclock_vcpu_time_info *pvti)
+{
+	struct vcpu_register_time_memory_area t;
+	bool phys = xen_feature(XENFEAT_vcpu_time_phys_area) &&
+		    xen_feature(XENFEAT_auto_translated_physmap);
+	int op = phys ? VCPUOP_register_vcpu_time_phys_area
+		      : VCPUOP_register_vcpu_time_memory_area;
+
+	if (phys)
+		t.addr.p = pvti ? virt_to_phys(pvti) : ~0LLU;
+	else
+		t.addr.v = pvti;
+
+	return HYPERVISOR_vcpu_op(op, 0, &t);
+}
+
 /* Get the TSC speed from Xen */
 static unsigned long xen_tsc_khz(void)
 {
@@ -395,7 +411,6 @@ static u64 xen_clock_value_saved;
 
 void xen_save_time_memory_area(void)
 {
-	struct vcpu_register_time_memory_area t;
 	int ret;
 
 	xen_clock_value_saved = xen_clocksource_read() - xen_sched_clock_offset;
@@ -403,9 +418,7 @@ void xen_save_time_memory_area(void)
 	if (!xen_clock)
 		return;
 
-	t.addr.v = NULL;
-
-	ret = HYPERVISOR_vcpu_op(VCPUOP_register_vcpu_time_memory_area, 0, &t);
+	ret = xen_register_time_area(NULL);
 	if (ret != 0)
 		pr_notice("Cannot save secondary vcpu_time_info (err %d)",
 			  ret);
@@ -415,15 +428,12 @@ void xen_save_time_memory_area(void)
 
 void xen_restore_time_memory_area(void)
 {
-	struct vcpu_register_time_memory_area t;
 	int ret;
 
 	if (!xen_clock)
 		goto out;
 
-	t.addr.v = &xen_clock->pvti;
-
-	ret = HYPERVISOR_vcpu_op(VCPUOP_register_vcpu_time_memory_area, 0, &t);
+	ret = xen_register_time_area(&xen_clock->pvti);
 
 	/*
 	 * We don't disable VDSO_CLOCKMODE_PVCLOCK entirely if it fails to
@@ -446,7 +456,6 @@ void xen_restore_time_memory_area(void)
 
 static void xen_setup_vsyscall_time_info(void)
 {
-	struct vcpu_register_time_memory_area t;
 	struct pvclock_vsyscall_time_info *ti;
 	int ret;
 
@@ -454,9 +463,7 @@ static void xen_setup_vsyscall_time_info(void)
 	if (!ti)
 		return;
 
-	t.addr.v = &ti->pvti;
-
-	ret = HYPERVISOR_vcpu_op(VCPUOP_register_vcpu_time_memory_area, 0, &t);
+	ret = xen_register_time_area(&ti->pvti);
 	if (ret) {
 		pr_notice("xen: VDSO_CLOCKMODE_PVCLOCK not supported (err %d)\n", ret);
 		free_page((unsigned long)ti);
@@ -469,9 +476,7 @@ static void xen_setup_vsyscall_time_info(void)
 	 * still check it in case hypervisor is buggy.
 	 */
 	if (!(ti->pvti.flags & PVCLOCK_TSC_STABLE_BIT)) {
-		t.addr.v = NULL;
-		ret = HYPERVISOR_vcpu_op(VCPUOP_register_vcpu_time_memory_area,
-					 0, &t);
+		ret = xen_register_time_area(NULL);
 		if (!ret)
 			free_page((unsigned long)ti);
 
diff --git a/drivers/xen/time.c b/drivers/xen/time.c
index a2be0a4d45b0..6b6ddc7a5822 100644
--- a/drivers/xen/time.c
+++ b/drivers/xen/time.c
@@ -19,8 +19,12 @@
 #include <xen/interface/vcpu.h>
 #include <xen/xen-ops.h>
 
-/* runstate info updated by Xen */
-static DEFINE_PER_CPU(struct vcpu_runstate_info, xen_runstate);
+/*
+ * Runstate info updated by Xen. The phys-area ABI doesn't allow it to cross
+ * a page boundary; the struct is 48 bytes, so 64 is the smallest alignment
+ * that guarantees this.
+ */
+static DEFINE_PER_CPU(struct vcpu_runstate_info, xen_runstate) __aligned(64);
 
 static DEFINE_PER_CPU(u64[4], old_runstate_time);
 
@@ -152,11 +156,21 @@ u64 xen_steal_clock(int cpu)
 void xen_setup_runstate_info(int cpu)
 {
 	struct vcpu_register_runstate_memory_area area;
+	unsigned int op;
 
-	area.addr.v = &per_cpu(xen_runstate, cpu);
+	if (xen_feature(XENFEAT_runstate_phys_area) &&
+	    xen_feature(XENFEAT_auto_translated_physmap))
+	{
+		op = VCPUOP_register_runstate_phys_area;
+		area.addr.p = per_cpu_ptr_to_phys(&per_cpu(xen_runstate, cpu));
+	}
+	else
+	{
+		op = VCPUOP_register_runstate_memory_area;
+		area.addr.v = &per_cpu(xen_runstate, cpu);
+	}
 
-	if (HYPERVISOR_vcpu_op(VCPUOP_register_runstate_memory_area,
-			       xen_vcpu_nr(cpu), &area))
+	if (HYPERVISOR_vcpu_op(op, xen_vcpu_nr(cpu), &area))
 		BUG();
 }
 
diff --git a/include/xen/interface/features.h b/include/xen/interface/features.h
index 53f760378e39..f00144a7c790 100644
--- a/include/xen/interface/features.h
+++ b/include/xen/interface/features.h
@@ -97,6 +97,15 @@
 #define XENFEAT_not_direct_mapped         16
 #define XENFEAT_direct_mapped             17
 
+/*
+ * Signal whether the domain is able to use the following hypercalls:
+ *
+ * VCPUOP_register_runstate_phys_area
+ * VCPUOP_register_vcpu_time_phys_area
+ */
+#define XENFEAT_runstate_phys_area        18
+#define XENFEAT_vcpu_time_phys_area       19
+
 #define XENFEAT_NR_SUBMAPS 1
 
 #endif /* __XEN_PUBLIC_FEATURES_H__ */
diff --git a/include/xen/interface/vcpu.h b/include/xen/interface/vcpu.h
index c7cc28ad8d63..d3858233c32f 100644
--- a/include/xen/interface/vcpu.h
+++ b/include/xen/interface/vcpu.h
@@ -203,4 +203,21 @@ struct vcpu_register_time_memory_area {
 };
 DEFINE_GUEST_HANDLE_STRUCT(vcpu_register_time_memory_area);
 
+/*
+ * Like the respective VCPUOP_register_*_memory_area, just using the "addr.p"
+ * field of the supplied struct as a guest physical address (i.e. in GFN space).
+ * The respective area may not cross a page boundary.  Pass ~0 to unregister an
+ * area.  Note that as long as an area is registered by physical address, the
+ * linear address based area will not be serviced (updated) by the hypervisor.
+ *
+ * Note that the area registered via VCPUOP_register_runstate_phys_area will
+ * be updated in the same manner as the one registered via virtual address PLUS
+ * VMASST_TYPE_runstate_update_flag engaged by the domain.
+ *
+ * XENFEAT_{runstate,vcpu_time}_phys_area feature bits signal the availability
+ * of these ops.
+ */
+#define VCPUOP_register_runstate_phys_area      14
+#define VCPUOP_register_vcpu_time_phys_area     15
+
 #endif /* __XEN_PUBLIC_VCPU_H__ */
-- 
2.56.0



--
Teddy Astie | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

Re: [PATCH] xen: Use VCPUOP_register_{runstate,vcpu_time}_phys_area when available

[Andrew Cooper]

On 16/10/2026 11:22 am, Teddy Astie wrote:
> Updating these area can be expensive when done through virtual addresses
> (especially for runstate one), in particular for HVM/PVH guests.
> Xen 4.18 introduced physical address alternatives to address those
> short-commings, but they aren't used currently.
>
> Fix it by using these alternatives when available (when reported in
> XENFEAT_{runstate,vcpu_time}_phys_area, but keep the current logic
> in the PV case.
>
> Assisted-by: LLM
> [guidance and testing]
> Signed-off-by: Teddy Astie <teddy.astie@vates.tech>
> ---
> When using phys_area ABI, the struct is not allowed to cross a page
> boundary anymore, hence we need to be careful with per-cpu struct
> alignment.
>
> I've deliberately put the PV domain (!XENFEAT_auto_translated_physmap)
> case aside as it profits less from the physical address ABI, and it
> would make things more complicated as "physical address" doesn't have
> a obvious meaning there.

It should always have been a guest physical registration, even for PV
guests.

The virtual address registered with Xen doesn't exist in the user
pagetables, and the way we discover that is taking a pagefault in Xen at
the point we need to update things.

> diff --git a/arch/x86/xen/time.c b/arch/x86/xen/time.c
> index 5c7822254a01..19ace1b78ae4 100644
> --- a/arch/x86/xen/time.c
> +++ b/arch/x86/xen/time.c
> @@ -37,6 +37,22 @@
>  
>  static u64 xen_sched_clock_offset __read_mostly;
>  
> +static int xen_register_time_area(struct pvclock_vcpu_time_info *pvti)

This needs a comment explaining what's going on (by GPA, falling back to
VA), including that passing NULL means unregister.

> +{
> +	struct vcpu_register_time_memory_area t;
> +	bool phys = xen_feature(XENFEAT_vcpu_time_phys_area) &&
> +		    xen_feature(XENFEAT_auto_translated_physmap);

Not related to your patch, but the xen_feature() infrastructure is idiotic.

It takes a bitmap from Xen, and transforms it into a u8 array of 1's or 0's.

It wants storing as a proper Linux bitmap, with xen_feature() being a
trivial wrapper around test_bit(), at which point the compiler will able
to collapse expressions such as the one above into a single operation. 
(except, you need to remove the XENFEAT_auto_translated_physmap aspect
to include PV guests.)

> +	int op = phys ? VCPUOP_register_vcpu_time_phys_area
> +		      : VCPUOP_register_vcpu_time_memory_area;
> +
> +	if (phys)
> +		t.addr.p = pvti ? virt_to_phys(pvti) : ~0LLU;
> +	else
> +		t.addr.v = pvti;
> +
> +	return HYPERVISOR_vcpu_op(op, 0, &t);
> +}
> +
>  /* Get the TSC speed from Xen */
>  static unsigned long xen_tsc_khz(void)
>  {
> @@ -395,7 +411,6 @@ static u64 xen_clock_value_saved;
>  
>  void xen_save_time_memory_area(void)
>  {
> -	struct vcpu_register_time_memory_area t;
>  	int ret;
>  
>  	xen_clock_value_saved = xen_clocksource_read() - xen_sched_clock_offset;
> @@ -403,9 +418,7 @@ void xen_save_time_memory_area(void)
>  	if (!xen_clock)
>  		return;
>  
> -	t.addr.v = NULL;
> -
> -	ret = HYPERVISOR_vcpu_op(VCPUOP_register_vcpu_time_memory_area, 0, &t);
> +	ret = xen_register_time_area(NULL);
>  	if (ret != 0)
>  		pr_notice("Cannot save secondary vcpu_time_info (err %d)",
>  			  ret);

How does this work?  Answer is it doesn't, because the print message is
bogus.  This is a failure to unregister the time area, not a failure to
save the time value.

At lest the error message wants fixing, and in a prep patch.

> diff --git a/drivers/xen/time.c b/drivers/xen/time.c
> index a2be0a4d45b0..6b6ddc7a5822 100644
> --- a/drivers/xen/time.c
> +++ b/drivers/xen/time.c
> @@ -152,11 +156,21 @@ u64 xen_steal_clock(int cpu)
>  void xen_setup_runstate_info(int cpu)
>  {
>  	struct vcpu_register_runstate_memory_area area;
> +	unsigned int op;
>  
> -	area.addr.v = &per_cpu(xen_runstate, cpu);
> +	if (xen_feature(XENFEAT_runstate_phys_area) &&
> +	    xen_feature(XENFEAT_auto_translated_physmap))

Again, PV guests want to use phys when available.

> +	{
> +		op = VCPUOP_register_runstate_phys_area;
> +		area.addr.p = per_cpu_ptr_to_phys(&per_cpu(xen_runstate, cpu));
> +	}
> +	else
> +	{
> +		op = VCPUOP_register_runstate_memory_area;
> +		area.addr.v = &per_cpu(xen_runstate, cpu);

I'd suggest factoring out the &per_cpu(xen_runstate, cpu) calculation. 
It's not free, and can't be hoisted by the compiler.

~Andrew

Re: [PATCH] xen: Use VCPUOP_register_{runstate,vcpu_time}_phys_area when available

[Teddy Astie]

[-- Attachment #1.1: Type: text/plain, Size: 4956 bytes --]

Le 16/10/2026 à 13:20, Andrew Cooper a écrit :
> On 16/10/2026 11:22 am, Teddy Astie wrote:
>> Updating these area can be expensive when done through virtual addresses
>> (especially for runstate one), in particular for HVM/PVH guests.
>> Xen 4.18 introduced physical address alternatives to address those
>> short-commings, but they aren't used currently.
>>
>> Fix it by using these alternatives when available (when reported in
>> XENFEAT_{runstate,vcpu_time}_phys_area, but keep the current logic
>> in the PV case.
>>
>> Assisted-by: LLM
>> [guidance and testing]
>> Signed-off-by: Teddy Astie <teddy.astie@vates.tech>
>> ---
>> When using phys_area ABI, the struct is not allowed to cross a page
>> boundary anymore, hence we need to be careful with per-cpu struct
>> alignment.
>>
>> I've deliberately put the PV domain (!XENFEAT_auto_translated_physmap)
>> case aside as it profits less from the physical address ABI, and it
>> would make things more complicated as "physical address" doesn't have
>> a obvious meaning there.
> 
> It should always have been a guest physical registration, even for PV
> guests.
> 
> The virtual address registered with Xen doesn't exist in the user
> pagetables, and the way we discover that is taking a pagefault in Xen at
> the point we need to update things.
> 

I wasn't aware of that specific case. Will try to make the code 
compatible with PV guests.

>> diff --git a/arch/x86/xen/time.c b/arch/x86/xen/time.c
>> index 5c7822254a01..19ace1b78ae4 100644
>> --- a/arch/x86/xen/time.c
>> +++ b/arch/x86/xen/time.c
>> @@ -37,6 +37,22 @@
>>   
>>   static u64 xen_sched_clock_offset __read_mostly;
>>   
>> +static int xen_register_time_area(struct pvclock_vcpu_time_info *pvti)
> 
> This needs a comment explaining what's going on (by GPA, falling back to
> VA), including that passing NULL means unregister.
> 

Will add, I guess we also want to mention that this does stuff on vCPU 0.

>> +{
>> +	struct vcpu_register_time_memory_area t;
>> +	bool phys = xen_feature(XENFEAT_vcpu_time_phys_area) &&
>> +		    xen_feature(XENFEAT_auto_translated_physmap);
> 
> Not related to your patch, but the xen_feature() infrastructure is idiotic.
> 
> It takes a bitmap from Xen, and transforms it into a u8 array of 1's or 0's.
> 
> It wants storing as a proper Linux bitmap, with xen_feature() being a
> trivial wrapper around test_bit(), at which point the compiler will able
> to collapse expressions such as the one above into a single operation.
> (except, you need to remove the XENFEAT_auto_translated_physmap aspect
> to include PV guests.)
> 

Looks good to me.

>> +	int op = phys ? VCPUOP_register_vcpu_time_phys_area
>> +		      : VCPUOP_register_vcpu_time_memory_area;
>> +
>> +	if (phys)
>> +		t.addr.p = pvti ? virt_to_phys(pvti) : ~0LLU;
>> +	else
>> +		t.addr.v = pvti;
>> +
>> +	return HYPERVISOR_vcpu_op(op, 0, &t);
>> +}
>> +
>>   /* Get the TSC speed from Xen */
>>   static unsigned long xen_tsc_khz(void)
>>   {
>> @@ -395,7 +411,6 @@ static u64 xen_clock_value_saved;
>>   
>>   void xen_save_time_memory_area(void)
>>   {
>> -	struct vcpu_register_time_memory_area t;
>>   	int ret;
>>   
>>   	xen_clock_value_saved = xen_clocksource_read() - xen_sched_clock_offset;
>> @@ -403,9 +418,7 @@ void xen_save_time_memory_area(void)
>>   	if (!xen_clock)
>>   		return;
>>   
>> -	t.addr.v = NULL;
>> -
>> -	ret = HYPERVISOR_vcpu_op(VCPUOP_register_vcpu_time_memory_area, 0, &t);
>> +	ret = xen_register_time_area(NULL);
>>   	if (ret != 0)
>>   		pr_notice("Cannot save secondary vcpu_time_info (err %d)",
>>   			  ret);
> 
> How does this work?  Answer is it doesn't, because the print message is
> bogus.  This is a failure to unregister the time area, not a failure to
> save the time value.
> 
> At lest the error message wants fixing, and in a prep patch.
> 

Yes.

>> diff --git a/drivers/xen/time.c b/drivers/xen/time.c
>> index a2be0a4d45b0..6b6ddc7a5822 100644
>> --- a/drivers/xen/time.c
>> +++ b/drivers/xen/time.c
>> @@ -152,11 +156,21 @@ u64 xen_steal_clock(int cpu)
>>   void xen_setup_runstate_info(int cpu)
>>   {
>>   	struct vcpu_register_runstate_memory_area area;
>> +	unsigned int op;
>>   
>> -	area.addr.v = &per_cpu(xen_runstate, cpu);
>> +	if (xen_feature(XENFEAT_runstate_phys_area) &&
>> +	    xen_feature(XENFEAT_auto_translated_physmap))
> 
> Again, PV guests want to use phys when available.
> 
>> +	{
>> +		op = VCPUOP_register_runstate_phys_area;
>> +		area.addr.p = per_cpu_ptr_to_phys(&per_cpu(xen_runstate, cpu));
>> +	}
>> +	else
>> +	{
>> +		op = VCPUOP_register_runstate_memory_area;
>> +		area.addr.v = &per_cpu(xen_runstate, cpu);
> 
> I'd suggest factoring out the &per_cpu(xen_runstate, cpu) calculation.
> It's not free, and can't be hoisted by the compiler.
> 

Done locally for both.

> ~Andrew

Teddy

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 665 bytes --]

Re: [PATCH] xen: Use VCPUOP_register_{runstate,vcpu_time}_phys_area when available

[Andrew Cooper]

On 16/10/2026 3:10 pm, Teddy Astie wrote:
> Le 16/10/2026 à 13:20, Andrew Cooper a écrit :
>> On 16/10/2026 11:22 am, Teddy Astie wrote:
>>> diff --git a/arch/x86/xen/time.c b/arch/x86/xen/time.c
>>> index 5c7822254a01..19ace1b78ae4 100644
>>> --- a/arch/x86/xen/time.c
>>> +++ b/arch/x86/xen/time.c
>>> @@ -37,6 +37,22 @@
>>>     static u64 xen_sched_clock_offset __read_mostly;
>>>   +static int xen_register_time_area(struct pvclock_vcpu_time_info
>>> *pvti)
>>
>> This needs a comment explaining what's going on (by GPA, falling back to
>> VA), including that passing NULL means unregister.
>>
>
> Will add, I guess we also want to mention that this does stuff on vCPU 0.

I was puzzled at why it only ever uses vCPU 0 for time handling.

Xen does allow there to be a time area per vCPU, and I have a suspicion
that this might matter for older systems lacking INVARIANT_TSC.

Either way, I don't suggest opening that particular can of worms now.

~Andrew