Re: [PATCH v2 5/5] xen/arm: Support ARM standard PV time for domains created via toolstack

[Koichiro Den <den@xxxxxxxxxxxxx>]

On Mon, Jul 07, 2025 at 10:01:47AM +0200, Jan Beulich wrote:
> On 05.07.2025 16:27, Koichiro Den wrote:
> > --- a/xen/arch/arm/mm.c
> > +++ b/xen/arch/arm/mm.c
> > @@ -180,7 +180,21 @@ int xenmem_add_to_physmap_one(
> >      case XENMAPSPACE_dev_mmio:
> >          rc = map_dev_mmio_page(d, gfn, _mfn(idx));
> >          return rc;
> > +    case XENMAPSPACE_pv_time:
> > +#ifdef CONFIG_ARM_64
> 
> These two lines want to change places, I think.

Will fix it, thank you.

> 
> > +        ASSERT(IS_POWER_OF_TWO(sizeof(struct pv_time_region)));
> 
> BUILD_BUG_ON() please, so that an issue here can be spotted at build time
> rather than only at runtime.
> 
> > +        if ( idx >= DIV_ROUND_UP(d->max_vcpus * sizeof(struct 
> > pv_time_region),
> > +                                 PAGE_SIZE) )
> > +            return -EINVAL;
> > +
> > +        if ( idx == 0 )
> > +            d->arch.pv_time_regions_gfn = gfn;
> 
> This looks fragile, as it'll break once d->max_vcpus can grow large enough to
> permit a non-zero idx by way of the earlier if() falling through. Imo you'll
> need at least one further BUILD_BUG_ON() here.

get_pv_region() can legitimately call xc_domain_add_to_physmap(..,
XENMAPSPACE_pv_time, ..) with idx > 0, but only if the preceding call with
idx == 0 succeeded. So while this may look odd at first glance, I think
this is not broken. What do you think?

> 
> >  
> > +        mfn = virt_to_mfn(d->arch.pv_time_regions[idx]);
> > +        t = p2m_ram_ro;
> 
> Is this the correct type to use here? That is, do you really mean guest write
> attempts to be silently dropped, rather than being reported to the guest as a
> fault? Then again I can't see such behavior being implemented on Arm, despite
> the comment on the enumerator saying so (likely inherited from x86).

No I didn't intend the "silently drop" behavior. IIUC, we may as well
correct the comment on the enum for Arm:

    diff --git a/xen/arch/arm/include/asm/p2m.h b/xen/arch/arm/include/asm/p2m.h
    index 2d53bf9b6177..927c588dbcb0 100644
    --- a/xen/arch/arm/include/asm/p2m.h
    +++ b/xen/arch/arm/include/asm/p2m.h
    @@ -123,7 +123,7 @@ struct p2m_domain {
     typedef enum {
         p2m_invalid = 0,    /* Nothing mapped here */
         p2m_ram_rw,         /* Normal read/write guest RAM */
    -    p2m_ram_ro,         /* Read-only; writes are silently dropped */
    +    p2m_ram_ro,         /* Read-only */

> 
> > +        break;
> > +#endif
> >      default:
> >          return -ENOSYS;
> >      }
> 
> As to style: Please, rather than absorbing the blank line that was there, make
> sure non-fall-through case blocks are separated from adjacent ones by a blank
> line.

Will do so in the next take.

> 
> > --- a/xen/include/public/memory.h
> > +++ b/xen/include/public/memory.h
> > @@ -217,6 +217,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_machphys_mapping_t);
> >                                        Stage-2 using the Normal Memory
> >                                        Inner/Outer Write-Back Cacheable
> >                                        memory attribute. */
> > +#define XENMAPSPACE_pv_time      6 /* PV time shared region (ARM64 only) */
> 
> The comment isn't specific enough. As per the struct declaration in patch 4,
> this interface is solely about stolen time. There's a wider PV interface,
> which at least x86 Linux also uses, and which has been adopted by KVM as
> well iirc. Hence this new type wants to clarify what exactly it's used for
> right now, while leaving open other uses on other architectures.

That sounds reasonable, I'll update it in the next iteration.

Thanks for the review.
-Koichiro

> 
> Jan