| Message ID | 20250306220343.203047-9-jason.andryuk@amd.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | ARM split hardware and control domains | expand |
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
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,
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 06.03.2025 23:03, Jason Andryuk wrote: > --- 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, Can idx be negative? If not, unsigned int please. Furthermore, as with any additions to common code, please keep in mind Misra: We better wouldn't gain new unreachable code for certain configs, even if - didn't check - the respective rule continues to be non-blocking. > + uint64_t frame, unsigned int flags) > +{ > + struct grant_table *gt = d->grant_table; This looks like it could be pointer-to-const. Same apparently for the d function parameter. Question is - would it perhaps make sense to have const struct grant_table *gt be the function parameter in the first place? > + shared_entry_v1(gt, idx).flags = flags; > + shared_entry_v1(gt, idx).domid = be_domid; > + shared_entry_v1(gt, idx).frame = frame; > +} In common code there shouldn't be an assumption that gnttab v1 is in use. Jan
On 10.03.25 09:01, Jan Beulich wrote: > On 06.03.2025 23:03, Jason Andryuk wrote: >> + shared_entry_v1(gt, idx).flags = flags; >> + shared_entry_v1(gt, idx).domid = be_domid; >> + shared_entry_v1(gt, idx).frame = frame; >> +} > > In common code there shouldn't be an assumption that gnttab v1 is in use. But isn't the grant table in V1 format until the guest potentially switches to V2? Juergen
On 10.03.2025 09:17, Juergen Gross wrote: > On 10.03.25 09:01, Jan Beulich wrote: >> On 06.03.2025 23:03, Jason Andryuk wrote: >>> + shared_entry_v1(gt, idx).flags = flags; >>> + shared_entry_v1(gt, idx).domid = be_domid; >>> + shared_entry_v1(gt, idx).frame = frame; >>> +} >> >> In common code there shouldn't be an assumption that gnttab v1 is in use. > > But isn't the grant table in V1 format until the guest potentially switches > to V2? Strictly speaking it's in v0 format initially. But yes, I see your point. Provided this function is made clear that it may only ever be used on a domain that wasn't started yet (perhaps proven by way of an assertion). Jan
Hi Jason, 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. > > 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 ) Looking at this patch again, is this guarantee that the xenstore domain will be created first? If not, then I think your series needs to be re-ordered so patch #10 is before this patch. > + 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; What if there is multiple domain with XEN_DOMCTL_CDF_xs_domain? Should we throw an error? Cheers,
On 2025-03-10 05:32, Julien Grall wrote: > Hi Jason, > > 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. >> >> 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 ) > > Looking at this patch again, is this guarantee that the xenstore domain > will be created first? If not, then I think your series needs to be re- > ordered so patch #10 is before this patch. Yes, you are right. >> + 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; > > What if there is multiple domain with XEN_DOMCTL_CDF_xs_domain? Should > we throw an error? Before this series, there was no restriction on its use, but only init-xenstore-domain used it. The XEN_DOMCTL_CDF_xs_domain flag allows the domain XSM_XS_PRIV, which grants a few more operations to an otherwise unprivileged domU With the use of xs_domid (only during construction), maybe it should be limited to just 1 to avoid surprises. Otherwise the last built xenstore domain would be configured as the backend. Nothing would break - it would just be surprising. I'll restrict to just 1. Thanks, Jason
On 2025-03-10 04:21, Jan Beulich wrote: > On 10.03.2025 09:17, Juergen Gross wrote: >> On 10.03.25 09:01, Jan Beulich wrote: >>> On 06.03.2025 23:03, Jason Andryuk wrote: >>>> + shared_entry_v1(gt, idx).flags = flags; >>>> + shared_entry_v1(gt, idx).domid = be_domid; >>>> + shared_entry_v1(gt, idx).frame = frame; >>>> +} >>> >>> In common code there shouldn't be an assumption that gnttab v1 is in use. >> >> But isn't the grant table in V1 format until the guest potentially switches >> to V2? > > Strictly speaking it's in v0 format initially. But yes, I see your point. > Provided this function is made clear that it may only ever be used on a > domain that wasn't started yet (perhaps proven by way of an assertion). Yes, this is what I was relying on. If d is still passed in, we could do ASSERT(!d->creation_finished); Hmmm, the function might could be marked __init, too, I think. struct grant_table and shared_entry_v1 are defined in xen/common/grant_table.c, which is why the function lives there (and it'll be useful for hyperlaunch). To avoid unreachable code, I guess I'll wrap it inside #ifdef CONFIG_DOM0LESS_BOOT. Regards, Jason
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; }
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(+)