With a split hardware and control domain, the control domain may still
want and xenstore access.  Currently this relies on init-dom0less to
seed the grants.  This is problematic since we don't want hardware
domain to be able to map the control domain's resources.  Instead have
the hypervisor see the grant table entry.  The grant is then accessible
as normal.

This is also useful with a xenstore stubdom to setup the xenbus page
much earlier.

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

Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>
---
 xen/arch/arm/dom0less-build.c |  9 +++++++++
 xen/common/grant_table.c      | 10 ++++++++++
 xen/include/xen/grant_table.h |  8 ++++++++
 3 files changed, 27 insertions(+)

diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c
index 068bf99294..f1d5bbb097 100644
--- a/xen/arch/arm/dom0less-build.c
+++ b/xen/arch/arm/dom0less-build.c
@@ -21,6 +21,8 @@
 #include <asm/static-memory.h>
 #include <asm/static-shmem.h>
 
+static domid_t __initdata xs_domid = DOMID_INVALID;
+
 bool __init is_dom0less_mode(void)
 {
     struct bootmodules *mods = &bootinfo.modules;
@@ -753,6 +755,10 @@ static int __init alloc_xenstore_page(struct domain *d)
     interface->connection = XENSTORE_RECONNECT;
     unmap_domain_page(interface);
 
+    if ( xs_domid != DOMID_INVALID )
+        gnttab_seed_entry(d, GNTTAB_RESERVED_XENSTORE, xs_domid,
+                          gfn_x(gfn), GTF_permit_access);
+
     return 0;
 }
 
@@ -1173,6 +1179,9 @@ void __init create_domUs(void)
         if ( rc )
             panic("Could not set up domain %s (rc = %d)\n",
                   dt_node_name(node), rc);
+
+        if ( d_cfg.flags & XEN_DOMCTL_CDF_xs_domain )
+            xs_domid = d->domain_id;
     }
 }
 
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index 6c77867f8c..ba93cdcbca 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -4346,6 +4346,16 @@ static void gnttab_usage_print(struct domain *rd)
         printk("no active grant table entries\n");
 }
 
+void gnttab_seed_entry(struct domain *d, int idx, domid_t be_domid,
+                       uint64_t frame, unsigned int flags)
+{
+    struct grant_table *gt = d->grant_table;
+
+    shared_entry_v1(gt, idx).flags = flags;
+    shared_entry_v1(gt, idx).domid = be_domid;
+    shared_entry_v1(gt, idx).frame = frame;
+}
+
 static void cf_check gnttab_usage_print_all(unsigned char key)
 {
     struct domain *d;
diff --git a/xen/include/xen/grant_table.h b/xen/include/xen/grant_table.h
index 50edfecfb6..63150fa497 100644
--- a/xen/include/xen/grant_table.h
+++ b/xen/include/xen/grant_table.h
@@ -45,6 +45,10 @@ void grant_table_destroy(
     struct domain *d);
 void grant_table_init_vcpu(struct vcpu *v);
 
+/* Seed a gnttab entry for Hyperlaunch/dom0less. */
+void gnttab_seed_entry(struct domain *d, int idx, domid_t be_domid,
+                       uint64_t frame, unsigned int flags);
+
 /*
  * Check if domain has active grants and log first 10 of them.
  */
@@ -85,6 +89,10 @@ static inline void grant_table_destroy(struct domain *d) {}
 
 static inline void grant_table_init_vcpu(struct vcpu *v) {}
 
+static inline void gnttab_seed_entry(struct domain *d, int idx,
+                                     domid_t be_domid, uint64_t frame,
+                                     unsigned int flags) {}
+
 static inline void grant_table_warn_active_grants(struct domain *d) {}
 
 static inline int gnttab_release_mappings(struct domain *d) { return 0; }
-- 
2.48.1

Hi,

On 06/03/2025 22:03, Jason Andryuk wrote:
> With a split hardware and control domain, the control domain may still
> want and xenstore access.  Currently this relies on init-dom0less to
> seed the grants.  This is problematic since we don't want hardware
> domain to be able to map the control domain's resources.  Instead have
> the hypervisor see the grant table entry.  The grant is then accessible
> as normal.

I am probably missing something, but why would run xenstored in the 
hardware domain rather than the control domain? Isn't xenstored more 
related to the VM management than HW?

Cheers,

-- 
Julien Grall

On 2025-03-07 16:24, Julien Grall wrote:
> Hi,
> 
> On 06/03/2025 22:03, Jason Andryuk wrote:
>> With a split hardware and control domain, the control domain may still
>> want and xenstore access.  Currently this relies on init-dom0less to
>> seed the grants.  This is problematic since we don't want hardware
>> domain to be able to map the control domain's resources.  Instead have
>> the hypervisor see the grant table entry.  The grant is then accessible
>> as normal.
> 
> I am probably missing something, but why would run xenstored in the 
> hardware domain rather than the control domain? Isn't xenstored more 
> related to the VM management than HW?

I addressed this in my other email.  You're probably right that 
xenstored should run in control, but implementation details prevent that 
in the short term.

Regardless, of the xenstored placement, I think it's a better design for 
Xen to seed the grants.  With Xen allocating the xenstore page and event 
channel, and now seeding the grant table, they can just be used.  A 
xenstore stubdom can just establish all the connections without relying 
on another domain to perform an action.

I tested that with hyperlaunching the xenstore stubdom.  That is where 
the two XS_PRIV changes later in the series come from.  xenstore stubdom 
iterates the domains, reads the hvm_param for the event channel, and 
then runs introduce to set up the connection.

Regards,
Jason

On Fri, 7 Mar 2025, Jason Andryuk wrote:
> On 2025-03-07 16:24, Julien Grall wrote:
> > Hi,
> > 
> > On 06/03/2025 22:03, Jason Andryuk wrote:
> > > With a split hardware and control domain, the control domain may still
> > > want and xenstore access.  Currently this relies on init-dom0less to
> > > seed the grants.  This is problematic since we don't want hardware
> > > domain to be able to map the control domain's resources.  Instead have
> > > the hypervisor see the grant table entry.  The grant is then accessible
> > > as normal.
> > 
> > I am probably missing something, but why would run xenstored in the hardware
> > domain rather than the control domain? Isn't xenstored more related to the
> > VM management than HW?
> 
> I addressed this in my other email.  You're probably right that xenstored
> should run in control, but implementation details prevent that in the short
> term.

I wrote a longer reply here:
https://marc.info/?l=xen-devel&m=174139414000462

I think there are valid reasons to run xenstored in either the control
domain or the hardware domain, so it should be configurable. If no
specific preference is indicated, I would place it in the hardware
domain because the control domain must remain free from interference.
Given that I don't think we know for sure today whether the Xenstore
protocol could be a potential vector for interference, it is safer to
avoid placing it in the control domain by default.


> Regardless, of the xenstored placement, I think it's a better design for Xen
> to seed the grants.  With Xen allocating the xenstore page and event channel,
> and now seeding the grant table, they can just be used.  A xenstore stubdom
> can just establish all the connections without relying on another domain to
> perform an action.

+1


> I tested that with hyperlaunching the xenstore stubdom.  That is where the two
> XS_PRIV changes later in the series come from.  xenstore stubdom iterates the
> domains, reads the hvm_param for the event channel, and then runs introduce to
> set up the connection.

On Thu, 6 Mar 2025, Jason Andryuk wrote:
> With a split hardware and control domain, the control domain may still
> want and xenstore access.  Currently this relies on init-dom0less to
> seed the grants.  This is problematic since we don't want hardware
> domain to be able to map the control domain's resources.  Instead have
> the hypervisor see the grant table entry.  The grant is then accessible
> as normal.
> 
> This is also useful with a xenstore stubdom to setup the xenbus page
> much earlier.

Reading the patch, it seems that what is doing is letting the xenstore
domain map the domU's grant table page. Is that correct?

If so, I would suggest to update the commit message as follows:

With split hardware/control/xenstore domains, the xenstore domain may
still want to access other domains' xenstore page. Currently this relies
on init-dom0less to seed the grants from Dom0.  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.


> This works with C xenstored.  OCaml xenstored does not use grants and
> would fail to foreign map the page.
> 
> Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>
> ---
>  xen/arch/arm/dom0less-build.c |  9 +++++++++
>  xen/common/grant_table.c      | 10 ++++++++++
>  xen/include/xen/grant_table.h |  8 ++++++++
>  3 files changed, 27 insertions(+)
> 
> diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c
> index 068bf99294..f1d5bbb097 100644
> --- a/xen/arch/arm/dom0less-build.c
> +++ b/xen/arch/arm/dom0less-build.c
> @@ -21,6 +21,8 @@
>  #include <asm/static-memory.h>
>  #include <asm/static-shmem.h>
>  
> +static domid_t __initdata xs_domid = DOMID_INVALID;
> +
>  bool __init is_dom0less_mode(void)
>  {
>      struct bootmodules *mods = &bootinfo.modules;
> @@ -753,6 +755,10 @@ static int __init alloc_xenstore_page(struct domain *d)
>      interface->connection = XENSTORE_RECONNECT;
>      unmap_domain_page(interface);
>  
> +    if ( xs_domid != DOMID_INVALID )
> +        gnttab_seed_entry(d, GNTTAB_RESERVED_XENSTORE, xs_domid,
> +                          gfn_x(gfn), GTF_permit_access);
> +
>      return 0;
>  }
>  
> @@ -1173,6 +1179,9 @@ void __init create_domUs(void)
>          if ( rc )
>              panic("Could not set up domain %s (rc = %d)\n",
>                    dt_node_name(node), rc);
> +
> +        if ( d_cfg.flags & XEN_DOMCTL_CDF_xs_domain )
> +            xs_domid = d->domain_id;
>      }
>  }
>  
> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
> index 6c77867f8c..ba93cdcbca 100644
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -4346,6 +4346,16 @@ static void gnttab_usage_print(struct domain *rd)
>          printk("no active grant table entries\n");
>  }
>  
> +void gnttab_seed_entry(struct domain *d, int idx, domid_t be_domid,
> +                       uint64_t frame, unsigned int flags)
> +{
> +    struct grant_table *gt = d->grant_table;
> +
> +    shared_entry_v1(gt, idx).flags = flags;
> +    shared_entry_v1(gt, idx).domid = be_domid;
> +    shared_entry_v1(gt, idx).frame = frame;
> +}
> +
>  static void cf_check gnttab_usage_print_all(unsigned char key)
>  {
>      struct domain *d;
> diff --git a/xen/include/xen/grant_table.h b/xen/include/xen/grant_table.h
> index 50edfecfb6..63150fa497 100644
> --- a/xen/include/xen/grant_table.h
> +++ b/xen/include/xen/grant_table.h
> @@ -45,6 +45,10 @@ void grant_table_destroy(
>      struct domain *d);
>  void grant_table_init_vcpu(struct vcpu *v);
>  
> +/* Seed a gnttab entry for Hyperlaunch/dom0less. */
> +void gnttab_seed_entry(struct domain *d, int idx, domid_t be_domid,
> +                       uint64_t frame, unsigned int flags);
> +
>  /*
>   * Check if domain has active grants and log first 10 of them.
>   */
> @@ -85,6 +89,10 @@ static inline void grant_table_destroy(struct domain *d) {}
>  
>  static inline void grant_table_init_vcpu(struct vcpu *v) {}
>  
> +static inline void gnttab_seed_entry(struct domain *d, int idx,
> +                                     domid_t be_domid, uint64_t frame,
> +                                     unsigned int flags) {}
> +
>  static inline void grant_table_warn_active_grants(struct domain *d) {}
>  
>  static inline int gnttab_release_mappings(struct domain *d) { return 0; }
> -- 
> 2.48.1
> 
>

On 2025-03-06 20:47, Stefano Stabellini wrote:
> On Thu, 6 Mar 2025, Jason Andryuk wrote:
>> With a split hardware and control domain, the control domain may still
>> want and xenstore access.  Currently this relies on init-dom0less to
>> seed the grants.  This is problematic since we don't want hardware
>> domain to be able to map the control domain's resources.  Instead have
>> the hypervisor see the grant table entry.  The grant is then accessible
>> as normal.
>>
>> This is also useful with a xenstore stubdom to setup the xenbus page
>> much earlier.
> 
> Reading the patch, it seems that what is doing is letting the xenstore
> domain map the domU's grant table page. Is that correct?

The end result is everything is setup for xenstored to map 
GNTTAB_RESERVED_XENSTORE at some time later.

> If so, I would suggest to update the commit message as follows:
> 
> With split hardware/control/xenstore domains, the xenstore domain may
> still want to access other domains' xenstore page. Currently this relies
> on init-dom0less to seed the grants from Dom0.  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.

I'll go with a tweaked version of yours:
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.

Regards,
Jason