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

[Teddy Astie]

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@xxxxxxxxxx>
> > ---
> > 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: OpenPGP_signature.asc
Description: OpenPGP digital signature