Re: [PATCH v2 4/6] xen/arm: dom0less seed xenstore grant table entry

[Jan Beulich <jbeulich@xxxxxxxx>]

On 31.03.2025 23:43, Jason Andryuk wrote:
> xenstored maps other domains' xenstore pages.  Currently this relies on
> init-dom0less or xl to seed the grants from Dom0.  With split
> hardware/control/xenstore domains, this is problematic since we don't
> want the hardware domain to be able to map other domains' resources
> without their permission.  Instead have the hypervisor seed the grant
> table entry for every dom0less domain.  The grant is then accessible as
> normal.

Yet aiui the original idea was to specifically not do this in the hypervisor.
I agree it shouldn't be the hardware domain, but what's wrong with having the
control domain do that? It is what is responsible for creating new domains as
well. The question of where to do that when there's no control domain must
also have been solved already (without me knowing the answer), as that's
where init-dom0less must be running.

> This works with C xenstored.  OCaml xenstored does not use grants and
> would fail to foreign map the page.

>From the sentence it's not clear whether this is unchanged behavior or
a deliberate regression.

> --- a/xen/arch/arm/dom0less-build.c
> +++ b/xen/arch/arm/dom0less-build.c
> @@ -865,6 +865,10 @@ static void __init initialize_domU_xenstore(void)
>          rc = alloc_xenstore_evtchn(d);
>          if ( rc < 0 )
>              panic("%pd: Failed to allocate xenstore_evtchn\n", d);
> +
> +        if ( gfn != ~0ULL )

Is this an odd open-coding of INVALID_GFN? And even if not - why ULL when
"gfn" is unsigned long? The way you have it the condition will always be
false on Arm32, if I'm not mistaken.

> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -4346,6 +4346,21 @@ static void gnttab_usage_print(struct domain *rd)
>          printk("no active grant table entries\n");
>  }
>  
> +#ifdef CONFIG_DOM0LESS_BOOT
> +void __init gnttab_seed_entry(const struct domain *d, unsigned int idx,
> +                              domid_t be_domid, uint64_t frame,
> +                              unsigned int flags)
> +{
> +    const struct grant_table *gt = d->grant_table;
> +
> +    ASSERT(!d->creation_finished);

While I don't mind the assertion, it's a little funny to see such in an
__init function.

> +    ASSERT(gt->gt_version == 1);
> +    shared_entry_v1(gt, idx).flags = flags;

Does this really need to be a function parameter? The sole caller passes
a constant (GTF_permit_access).

> --- a/xen/include/xen/grant_table.h
> +++ b/xen/include/xen/grant_table.h
> @@ -45,6 +45,11 @@ void grant_table_destroy(
>      struct domain *d);
>  void grant_table_init_vcpu(struct vcpu *v);
>  
> +/* Seed a gnttab entry for Hyperlaunch/dom0less. */
> +void __init gnttab_seed_entry(const struct domain *d, unsigned int idx,

No __init on declarations please. They have no effect (as long as the definition
properly has the attribute) and hence are only at risk of going stale.

Jan