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
