| Message ID | 20250331214321.205331-5-jason.andryuk@amd.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | ARM split hardware and control domains | expand |
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
On 2025-04-01 08:16, Jan Beulich wrote: > 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. dom0less does not allow configuring a domU to have xenstore without using dom0 today. This series changes the dependency to the xenstore domain. I want to move away from relying on the control domain to set up xenstore connections. Domain boot can achieve more parallelism if xenstore domain can independently configure its connections. If xenstore connections need to wait until dom0/control userspace runs, that is a measurable delay. From the permission side, with a preseeded grant, and the event channel places in the xenstore_domain_interface page, a xenstore domain can connect and introduce existing domains when it starts up without needing additional permissions. Generally, I'm looking at making xenstored perform more configuration on its own. It doesn't matter for dom0, but it will help for dom0less/Hyperlaunch. Seeding grants from the hypervisor is a small change, but it helps remove dependencies for xenstore. >> 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. I was trying to highlight existing compatibility. xenstored uses grants, so it can take advantage of the pre-seeded grant and therefore does not need privilege to map foreign pages. OCaml does not use grants, so this seeding is irrelevant. With a combined hardware|xenstore domain, C xenstored using grants works, but Ocaml does not. >> --- 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. I'll have to double check that. Thanks. >> --- 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. This check was added in response to v1 comment. Yes, I too considered just relying on __init, but the ASSERT explicitly checks the desired property holds. >> + 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). I suppose it could be dropped. The toolstack side passes flags, which I mirrored when writing this. >> --- 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. Oh, okay. I thought they were necessary, but clear I am wrong. Thanks, Jason
On 01.04.2025 15:29, Jason Andryuk wrote: > On 2025-04-01 08:16, Jan Beulich wrote: >> On 31.03.2025 23:43, Jason Andryuk wrote: >>> 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. > > I was trying to highlight existing compatibility. xenstored uses > grants, so it can take advantage of the pre-seeded grant and therefore > does not need privilege to map foreign pages. OCaml does not use > grants, so this seeding is irrelevant. With a combined > hardware|xenstore domain, C xenstored using grants works, but Ocaml does > not. IOW oxenstored works in all-mighty Dom0 only now, and that doesn't change? This would be fine. Re-wording the sentence to become unambiguous (towards possibly admitting to deliberately introduce a regression) would seem desirable though. Jan
On 2025-04-01 08:16, Jan Beulich wrote: > On 31.03.2025 23:43, Jason Andryuk wrote: >> --- 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. The gfn is pulled out of the HVM_PARAMS, which is a uint64_t. It is set like: d->arch.hvm.params[HVM_PARAM_STORE_PFN] = ~0ULL; But pulled out by: unsigned long gfn = d->arch.hvm.params[HVM_PARAM_STORE_PFN]; So your comment highlights that unsigned long is incorrect for ARM32. While I realize fixed types are discouraged, I'd prefer to use uint64_t for the replacement. That is the type of HVM_PARAMS, and uint64_t is used on the init-dom0less side as well. Using unsigned long long to get a 64bit value on ARM32 seems less clear to me. Thanks, Jason
On Tue, 1 Apr 2025, Jason Andryuk wrote: > On 2025-04-01 08:16, Jan Beulich wrote: > > On 31.03.2025 23:43, Jason Andryuk wrote: > > > > --- 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. > > The gfn is pulled out of the HVM_PARAMS, which is a uint64_t. It is set like: > > d->arch.hvm.params[HVM_PARAM_STORE_PFN] = ~0ULL; > > But pulled out by: > > unsigned long gfn = d->arch.hvm.params[HVM_PARAM_STORE_PFN]; > > So your comment highlights that unsigned long is incorrect for ARM32. > > While I realize fixed types are discouraged, I'd prefer to use uint64_t for > the replacement. That is the type of HVM_PARAMS, and uint64_t is used on the > init-dom0less side as well. Using unsigned long long to get a 64bit value on > ARM32 seems less clear to me. The types that correspond to hypercall struct field types should match the hypercall struct field types. I think gfn should be uint64_t to match the definition of params. Similarly among the arguments of gnttab_seed_entry, flags should be uint16_t and I think frame should be uint32_t. This last one I am confused why you defined it as uint64_t, maybe I am missing something.
On 2025-04-01 19:50, Stefano Stabellini wrote: > On Tue, 1 Apr 2025, Jason Andryuk wrote: >> On 2025-04-01 08:16, Jan Beulich wrote: >>> On 31.03.2025 23:43, Jason Andryuk wrote: >> >>>> --- 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. >> >> The gfn is pulled out of the HVM_PARAMS, which is a uint64_t. It is set like: >> >> d->arch.hvm.params[HVM_PARAM_STORE_PFN] = ~0ULL; >> >> But pulled out by: >> >> unsigned long gfn = d->arch.hvm.params[HVM_PARAM_STORE_PFN]; >> >> So your comment highlights that unsigned long is incorrect for ARM32. >> >> While I realize fixed types are discouraged, I'd prefer to use uint64_t for >> the replacement. That is the type of HVM_PARAMS, and uint64_t is used on the >> init-dom0less side as well. Using unsigned long long to get a 64bit value on >> ARM32 seems less clear to me. > > The types that correspond to hypercall struct field types should match > the hypercall struct field types. > > I think gfn should be uint64_t to match the definition of params. > > Similarly among the arguments of gnttab_seed_entry, flags should be > uint16_t and I think frame should be uint32_t. This last one I am > confused why you defined it as uint64_t, maybe I am missing something. With Jan's suggestion, I am dropping flags. Yes, frame should be uin32_t since it is filling in a grant table v1 entry, and that is limited to 32bit frame numbers. If the frame number is >32bits, then the grant can't be seeded. I guess I'll put a check in the caller to ensure that. Looking at the supported guest memory maximums, 32bit gfns should be enough to cover all the limits. > 16TiB would be needed to exceed a 32bit frame number. Regards, Jason
diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c index a46f292c1f..fc515c9852 100644 --- 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 ) + gnttab_seed_entry(d, GNTTAB_RESERVED_XENSTORE, xs_domid, + gfn, GTF_permit_access); } } diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c index 6c77867f8c..2fb3679447 100644 --- 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); + ASSERT(gt->gt_version == 1); + shared_entry_v1(gt, idx).flags = flags; + shared_entry_v1(gt, idx).domid = be_domid; + shared_entry_v1(gt, idx).frame = frame; +} +#endif + 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..a17f1787da 100644 --- 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, + domid_t be_domid, uint64_t frame, + unsigned int flags); + /* * Check if domain has active grants and log first 10 of them. */ @@ -85,6 +90,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; }
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. 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> --- v2: Tweak commit message Mark gnttab_seed_entry() __init and put inside CONFIG_DOM0LESS_BOOT Add ASSERT(!d->creation_finished) and ASSERT(gt->gt_version == 1); const struct domain & struct grant_table --- xen/arch/arm/dom0less-build.c | 4 ++++ xen/common/grant_table.c | 15 +++++++++++++++ xen/include/xen/grant_table.h | 9 +++++++++ 3 files changed, 28 insertions(+)