| Message ID | 20250331214321.205331-2-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: > --- a/xen/common/domain.c > +++ b/xen/common/domain.c > @@ -820,11 +820,15 @@ struct domain *domain_create(domid_t domid, > d->is_privileged = flags & CDF_privileged; > > /* Sort out our idea of is_hardware_domain(). */ > - if ( domid == 0 || domid == hardware_domid ) > + if ( (flags & CDF_hardware) || domid == hardware_domid ) Since it's || here ... > { > if ( hardware_domid < 0 || hardware_domid >= DOMID_FIRST_RESERVED ) > panic("The value of hardware_dom must be a valid domain ID\n"); > > + /* late_hwdom is only allowed for dom0. */ > + if ( hardware_domain && hardware_domain->domain_id ) > + return ERR_PTR(-EINVAL); > + > old_hwdom = hardware_domain; > hardware_domain = d; > } ... doesn't this code then also need to set CDF_hardware if it's unset in the function argument? Then: Reviewed-by: Jan Beulich <jbeulich@suse.com> Jan
On 2025-04-01 08:00, Jan Beulich wrote: > On 31.03.2025 23:43, Jason Andryuk wrote: >> --- a/xen/common/domain.c >> +++ b/xen/common/domain.c >> @@ -820,11 +820,15 @@ struct domain *domain_create(domid_t domid, >> d->is_privileged = flags & CDF_privileged; >> >> /* Sort out our idea of is_hardware_domain(). */ >> - if ( domid == 0 || domid == hardware_domid ) >> + if ( (flags & CDF_hardware) || domid == hardware_domid ) > > Since it's || here ... > >> { >> if ( hardware_domid < 0 || hardware_domid >= DOMID_FIRST_RESERVED ) >> panic("The value of hardware_dom must be a valid domain ID\n"); >> >> + /* late_hwdom is only allowed for dom0. */ >> + if ( hardware_domain && hardware_domain->domain_id ) >> + return ERR_PTR(-EINVAL); >> + >> old_hwdom = hardware_domain; >> hardware_domain = d; >> } > > ... doesn't this code then also need to set CDF_hardware if it's unset > in the function argument? I don't think it matters today - later construction depends on the value of hardware_domain. Which is also used for the check underlying is_hardware_domain(). But I agree that it makes sense to set it here in case the use of CDF_hardware expands in the future. > Then: > Reviewed-by: Jan Beulich <jbeulich@suse.com> Thanks, Jason
On 01.04.2025 14:39, Jason Andryuk wrote: > On 2025-04-01 08:00, Jan Beulich wrote: >> On 31.03.2025 23:43, Jason Andryuk wrote: >>> --- a/xen/common/domain.c >>> +++ b/xen/common/domain.c >>> @@ -820,11 +820,15 @@ struct domain *domain_create(domid_t domid, >>> d->is_privileged = flags & CDF_privileged; >>> >>> /* Sort out our idea of is_hardware_domain(). */ >>> - if ( domid == 0 || domid == hardware_domid ) >>> + if ( (flags & CDF_hardware) || domid == hardware_domid ) >> >> Since it's || here ... >> >>> { >>> if ( hardware_domid < 0 || hardware_domid >= DOMID_FIRST_RESERVED ) >>> panic("The value of hardware_dom must be a valid domain ID\n"); >>> >>> + /* late_hwdom is only allowed for dom0. */ >>> + if ( hardware_domain && hardware_domain->domain_id ) >>> + return ERR_PTR(-EINVAL); >>> + >>> old_hwdom = hardware_domain; >>> hardware_domain = d; >>> } >> >> ... doesn't this code then also need to set CDF_hardware if it's unset >> in the function argument? > > I don't think it matters today - later construction depends on the value > of hardware_domain. Which is also used for the check underlying > is_hardware_domain(). > > But I agree that it makes sense to set it here in case the use of > CDF_hardware expands in the future. If we don't do this now, it'll be quite likely that it'll be forgotten later. Hardly anyone actually tests late-hwdom these days, afaict. It may also matter if someone looks at e.g. a dump of Xen after a crash. Jan
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index 2b5b433183..051c48329a 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -2366,7 +2366,7 @@ void __init create_dom0(void) .max_maptrack_frames = -1, .grant_opts = XEN_DOMCTL_GRANT_version(opt_gnttab_max_version), }; - unsigned int flags = CDF_privileged; + unsigned int flags = CDF_privileged | CDF_hardware; int rc; /* The vGIC for DOM0 is exactly emulating the hardware GIC */ diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c index d70abb7e0c..67d399c469 100644 --- a/xen/arch/x86/setup.c +++ b/xen/arch/x86/setup.c @@ -1018,7 +1018,8 @@ static struct domain *__init create_dom0(struct boot_info *bi) /* Create initial domain. Not d0 for pvshim. */ domid = get_initial_domain_id(); - d = domain_create(domid, &dom0_cfg, pv_shim ? 0 : CDF_privileged); + d = domain_create(domid, &dom0_cfg, + pv_shim ? 0 : CDF_privileged | CDF_hardware); if ( IS_ERR(d) ) panic("Error creating d%u: %ld\n", domid, PTR_ERR(d)); diff --git a/xen/common/domain.c b/xen/common/domain.c index 585fd726a9..b3eb589872 100644 --- a/xen/common/domain.c +++ b/xen/common/domain.c @@ -820,11 +820,15 @@ struct domain *domain_create(domid_t domid, d->is_privileged = flags & CDF_privileged; /* Sort out our idea of is_hardware_domain(). */ - if ( domid == 0 || domid == hardware_domid ) + if ( (flags & CDF_hardware) || domid == hardware_domid ) { if ( hardware_domid < 0 || hardware_domid >= DOMID_FIRST_RESERVED ) panic("The value of hardware_dom must be a valid domain ID\n"); + /* late_hwdom is only allowed for dom0. */ + if ( hardware_domain && hardware_domain->domain_id ) + return ERR_PTR(-EINVAL); + old_hwdom = hardware_domain; hardware_domain = d; } diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h index 83069de501..9be18d16b9 100644 --- a/xen/include/xen/domain.h +++ b/xen/include/xen/domain.h @@ -52,6 +52,8 @@ domid_t get_initial_domain_id(void); #else #define CDF_staticmem 0 #endif +/* This is the hardware domain. Only 1 allowed. */ +#define CDF_hardware (1U << 3) #define is_domain_direct_mapped(d) ((d)->cdf & CDF_directmap) #define is_domain_using_staticmem(d) ((d)->cdf & CDF_staticmem)