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

[Andrew Cooper <andrew.cooper3@citrix.com>]

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