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

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