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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

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

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

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

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

Jan