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

[Koichiro Den <den@xxxxxxxxxxxxx>]

On Mon, Jun 23, 2025 at 09:41:47AM +0100, Julien Grall wrote:
> Hi Koichiro,
> 
> I haven't checked all the details. I will mainly comment on the Xen
> internals.

Hi Julien,

Thank you for the review, and apologies for my late response.

> 
---(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. Do you have any recommendations how to
reliably and dynamically allocating the shared region PFN(s) when using xl?

In any case, I agree that conditional allocation would be preferable.

> 
> Lastly, you probably only want to allocate the memory for 64-bit domain as
> this is unusable for 32-bit domains.
> 
> > +    d->arch.pv_time_regions = alloc_xenheap_pages(order, 0);
> > +    if ( !d->arch.pv_time_regions ) {> +        rc = -ENOMEM;
> > +        goto fail;
> > +    }
> > +    memset(d->arch.pv_time_regions, 0, PAGE_SIZE << order);
> > +>       return 0;
> >   fail:
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index 85b6909e2b0e..1c51b53d9c6b 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -1561,8 +1561,80 @@ int __init make_chosen_node(const struct kernel_info 
> > *kinfo)
> >       return res;
> >   }
> > -int __init make_resv_memory_node(const struct kernel_info *kinfo, int 
> > addrcells,
> > -                                 int sizecells)
> > +int __init make_pv_time_resv_memory_node(struct domain *d,
> > +                                         const struct kernel_info *kinfo,
> > +                                         int addrcells, int sizecells)
> > +{
> > +    __be32 reg[GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS];
> > +    unsigned int len = (addrcells + sizecells) * sizeof(__be32);
> > +    struct p2m_domain *p2m = p2m_get_hostp2m(d);
> > +    struct membanks *unused_banks = NULL;
> > +    void *fdt = kinfo->fdt;
> > +    unsigned regions_len;
> > +    gfn_t pv_time_gfn;
> > +    mfn_t pv_time_mfn;
> > +    paddr_t aligned;
> > +    paddr_t avail;
> > +    __be32 *cells;
> > +    int res;
> > +    int i;
> > +
> > +    /* Find unused regions */
> > +    regions_len = PAGE_ALIGN(d->max_vcpus * 64);
> 
> I would be best to avoid hardcoding the size of the region and use the size
> of struct pv_time_region.

Thanks for catching it, I'll fix this in v2.

> 
> > +    unused_banks = membanks_xzalloc(NR_MEM_BANKS, MEMORY);
> > +    if ( !unused_banks )
> > +        return -ENOMEM;
> > +
> > +    res = find_unused_regions(d, kinfo, unused_banks);
> > +    if ( res ) {
> > +        printk(XENLOG_WARNING "%pd: failed to find unused regions\n", d);
> > +        goto fail;
> > +    }
> > +    for ( i = 0; i < unused_banks->nr_banks; i++ ) {
> > +        const struct membank *bank = &unused_banks->bank[i];
> > +        aligned = PAGE_ALIGN(bank->start);
> > +        avail = bank->size - (aligned - bank->start);
> > +        if ( avail >= regions_len )
> > +            break;
> > +    }
> > +    if ( i == unused_banks->nr_banks ) {
> > +        res = -ENOSPC;
> > +        goto fail;
> > +    }
> > +
> > +    /* Insert P2M entry */
> > +    pv_time_mfn = virt_to_mfn(d->arch.pv_time_regions);
> > +    pv_time_gfn = gaddr_to_gfn(aligned);
> > +    p2m_write_lock(p2m);
> > +    res = p2m_set_entry(p2m, pv_time_gfn, regions_len / PAGE_SIZE,
> > +                        pv_time_mfn, p2m_ram_rw, p2m_access_r);
> 
> p2m_access_* are used for restricting temporarily permission by memaccess.
> So on a data abort, we will call p2m_mem_access_check() which will then
> forward the fault to the memaccess agent.
> 
> If you want to restrict permission forever, then you need to use a different
> p2m_type_t. In this case, I am guessing you want p2m_ram_ro.

Thanks for pointing that out, I'll fix this in v2.

> 
> [...]
> 
> > 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!

> 
> Cheers,
> 
> -- 
> Julien Grall
>