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

[Jan Beulich <jbeulich@xxxxxxxx>]

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?

> 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.)

Jan