Re: [RFC PATCH 4/4] xen/arm: Implement standard PV time interface as per ARM DEN 0057A

[Julien Grall <julien@xxxxxxx>]

Hi,

On 28/06/2025 14:58, Koichiro Den wrote:
> On Mon, Jun 23, 2025 at 09:41:47AM +0100, Julien Grall wrote:
> ---(snip)---
> > > @@ -707,6 +723,7 @@ int arch_domain_create(struct domain *d,
> > >                           unsigned int flags)
> > >    {
> > >        unsigned int count = 0;
> > > +    int order;
> > >        int rc;
> > >        BUILD_BUG_ON(GUEST_MAX_VCPUS < MAX_VIRT_CPUS);
> > > @@ -791,6 +808,19 @@ int arch_domain_create(struct domain *d,
> > >        d->arch.sve_vl = config->arch.sve_vl;
> > >    #endif
> > > +    /*
> > > +     * Preallocate the stolen time shared memory regions for all the
> > > +     * possible vCPUs.
> > > +     */
> > > +    order = get_order_from_bytes(d->max_vcpus * sizeof(struct pv_time_region));
> > As this is an order, we could end up to waste memory fairly quickly. So we
> > should try to free the unused pages from the order. That said, the maximum
> > number of virtual CPUs we currently support is 128. If I am not mistaken,
> > this could fit in 2 4KB pages. So I would also be ok with a
> > BUILD_BUG_ON(MAX_VIRT_CPUS <= 128) and we defer this work.
> I'll go with the former in the next iteration. Thanks!
>
> > > +    d->arch.pv_time_regions_gfn = INVALID_GFN;
> > Does this mean PV time is optional? If so, shouldn't we allocate the memory
> > conditionally?
> >
> > Also, looking at the code below, you seem to cater domains created via
> > dom0less but not from the toolstack. I think both should be supported for
> > the PV time.
> Yes, that was intentional. I should've mentioned that this RFC series only
> caters the dom0less case. For domains created dynamically via xl create, the
> only viable solution I've found so far is to reserve PFN range(s) large enough
> to cover the maximum possible number of toolstack-created domains on boot,
> which I think would be too wasteful.
AFAICT, with current MAX_VIRT_CPUS (128), we would only need to reserve 
8KB in the guest address space. We still have plenty of space that we 
can afford reserve 8KB (the layout is described in 
xen/include/public/arch-arm.h). I would suggest to allocate the region 
just before the grant-table (see GUEST_GNTTAB_BASE).
> In any case, I agree that conditional allocation would be preferable.
For XL, I would suggest to introduce a field flags in 
xen_arch_domainconfig and use one bit for enabling the PV time 
interface. The other bits would be reserved for the future (so we would 
need to check they are zeroes). You can have a look how "flags" in 
xen_domctl_createdomain is handled.
[...]

> > > diff --git a/xen/arch/arm/include/asm/domain.h 
> > > b/xen/arch/arm/include/asm/domain.h
> > > index a3487ca71303..c231c45fe40f 100644
> > > --- a/xen/arch/arm/include/asm/domain.h
> > > +++ b/xen/arch/arm/include/asm/domain.h
> > > @@ -59,6 +59,18 @@ struct paging_domain {
> > >        unsigned long p2m_total_pages;
> > >    };
> > > +/* Stolen time shared memory region (ARM DEN 0057A.b) */
> > > +struct pv_time_region {
> > > +    /* This field must be 0 as per ARM DEN 0057A.b */
> > > +    uint32_t revision;
> > > +
> > > +    /* This field must be 0 as per ARM DEN 0057A.b */
> > > +    uint32_t attribute;
> > > +
> > > +    /* Total stolen time in nanoseconds */
> > > +    uint64_t stolen_time;
> > > +} __aligned(64);
> > > +
> > >    struct arch_domain
> > >    {
> > >    #ifdef CONFIG_ARM_64
> > > @@ -121,6 +133,9 @@ struct arch_domain
> > >        void *tee;
> > >    #endif
> > > +    struct pv_time_region *pv_time_regions;
> > > +    gfn_t pv_time_regions_gfn;
> > Given the feature is 32-bit specific, shouldn't the field be protected with
> > #define CONFIG_ARM_32?
> Is this typo s/32/64/? Assuming so, I'll do so (=protect them with #ifdef
> CONFIG_ARM_64) in the next iteration. Thanks!
Yes this is a typo.

Cheers,

--
Julien Grall