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

[Koichiro Den <den@xxxxxxxxxxxxx>]

On Wed, Jul 09, 2025 at 11:16:02AM +0200, Jan Beulich wrote:
> On 09.07.2025 10:04, Koichiro Den wrote:
> > 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.
> 
> Can it? What's the intended effect then, when only the idx == 0 case is fully
> handled here?

GFNs are made always contiguous in this implementation (while MFNs might
not) so it can register the whole range in one go when recognizing the
starting GFN (when idx == 0). It's ugly indeed, as it implicitly requires
the caller firstly invokes it with idx == 0 and aborts when it fails.

That said, after reading another feedback, I'm thinking of simplifying
the whole patch series (as I just said in [1]) and I believe this
unnecessarily complicated/confusing stuff will disappear.

[1] 
https://lore.kernel.org/xen-devel/20250705142703.2769819-1-den@xxxxxxxxxxxxx/T/#medaa074cd863c05606bfdb6280cd4ccb88803bc7

> 
> > So while this may look odd at first glance, I think
> > this is not broken. What do you think?
> 
> The GFN not being recorded anywhere means the call has no effect, while giving
> the caller the impression that there was one (by way of returning success).
> 
> >>> +        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 */
> 
> Don't know whether that's a good idea, as it'll diverge Arm from the same-
> name P2M type that x86 has. (Arguably x86'es type may better be named
> p2m_ram_write_ignore or some such.)

Thanks for sharing your thought. Incidentally, there seems to be the same
comment in ppc's p2m.h as well. I'm not sure at all but I'm guessing that
"writes are silently dropped" line would make sense only when PoD and/or VM
forking is to be supported by arm/ppc, and leaving it as it is for arm/ppc
might be acceptable at the moment.

> 
> Jan