| Message ID | 20250403214608.152954-2-jason.andryuk@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ARM split hardware and control domains | expand |
On Thu, 3 Apr 2025, Jason Andryuk wrote: > From: "Daniel P. Smith" <dpsmith@apertussolutions.com> > > Add and use a new internal create domain flag to specify the hardware > domain. This removes the hardcoding of domid 0 as the hardware domain. > > This allows more flexibility with domain creation. > > Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com> > Signed-off-by: Jason Andryuk <jason.andryuk@amd.com> > Reviewed-by: Jan Beulich <jbeulich@suse.com> ARM: Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> > --- > v3: > Or-in CDF_hardware for late hwdom case > Add Jan's R-b > > v2: > () around binary & > Only allow late_hwdom for dom0 > --- > xen/arch/arm/domain_build.c | 2 +- > xen/arch/x86/setup.c | 3 ++- > xen/common/domain.c | 7 ++++++- > xen/include/xen/domain.h | 2 ++ > 4 files changed, 11 insertions(+), 3 deletions(-) > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > index 634333cdde..b8f282ff10 100644 > --- a/xen/arch/arm/domain_build.c > +++ b/xen/arch/arm/domain_build.c > @@ -2369,7 +2369,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..da74f815f4 100644 > --- a/xen/common/domain.c > +++ b/xen/common/domain.c > @@ -820,13 +820,18 @@ 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; > + flags |= CDF_hardware; > } > > TRACE_TIME(TRC_DOM0_DOM_ADD, d->domain_id); > diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h > index a34daa7d10..e10baf2615 100644 > --- a/xen/include/xen/domain.h > +++ b/xen/include/xen/domain.h > @@ -53,6 +53,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) > -- > 2.49.0 >
On 03.04.2025 23:46, Jason Andryuk wrote: > From: "Daniel P. Smith" <dpsmith@apertussolutions.com> > > Add and use a new internal create domain flag to specify the hardware > domain. This removes the hardcoding of domid 0 as the hardware domain. > > This allows more flexibility with domain creation. > > Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com> > Signed-off-by: Jason Andryuk <jason.andryuk@amd.com> > Reviewed-by: Jan Beulich <jbeulich@suse.com> > --- > v3: > Or-in CDF_hardware for late hwdom case Except that ... > --- a/xen/common/domain.c > +++ b/xen/common/domain.c > @@ -820,13 +820,18 @@ 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; > + flags |= CDF_hardware; > } ... this isn't quite enough. You're only modifying what will go out of scope when returning from the function. What's at least equally important to OR into is d->cdf. Jan
On 2025-04-04 03:38, Jan Beulich wrote: > On 03.04.2025 23:46, Jason Andryuk wrote: >> From: "Daniel P. Smith" <dpsmith@apertussolutions.com> >> >> Add and use a new internal create domain flag to specify the hardware >> domain. This removes the hardcoding of domid 0 as the hardware domain. >> >> This allows more flexibility with domain creation. >> >> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com> >> Signed-off-by: Jason Andryuk <jason.andryuk@amd.com> >> Reviewed-by: Jan Beulich <jbeulich@suse.com> >> --- >> v3: >> Or-in CDF_hardware for late hwdom case > > Except that ... > >> --- a/xen/common/domain.c >> +++ b/xen/common/domain.c >> @@ -820,13 +820,18 @@ 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; >> + flags |= CDF_hardware; >> } > > ... this isn't quite enough. You're only modifying what will go out of scope > when returning from the function. What's at least equally important to OR into > is d->cdf. Yes, thanks for catching that. I'll move d->cdf assignment to after here instead of or-ing in a second time. With that, it seems like it should also be removed from old_hwdom? old_hwdom->cdf &= ~CDF_hardware Regards, Jason
On 07.04.2025 20:16, Jason Andryuk wrote: > On 2025-04-04 03:38, Jan Beulich wrote: >> On 03.04.2025 23:46, Jason Andryuk wrote: >>> From: "Daniel P. Smith" <dpsmith@apertussolutions.com> >>> >>> Add and use a new internal create domain flag to specify the hardware >>> domain. This removes the hardcoding of domid 0 as the hardware domain. >>> >>> This allows more flexibility with domain creation. >>> >>> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com> >>> Signed-off-by: Jason Andryuk <jason.andryuk@amd.com> >>> Reviewed-by: Jan Beulich <jbeulich@suse.com> >>> --- >>> v3: >>> Or-in CDF_hardware for late hwdom case >> >> Except that ... >> >>> --- a/xen/common/domain.c >>> +++ b/xen/common/domain.c >>> @@ -820,13 +820,18 @@ 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; >>> + flags |= CDF_hardware; >>> } >> >> ... this isn't quite enough. You're only modifying what will go out of scope >> when returning from the function. What's at least equally important to OR into >> is d->cdf. > > Yes, thanks for catching that. > > I'll move d->cdf assignment to after here instead of or-ing in a second > time. Not sure that'll be good to do - intermediate code (in particular passing d to other functions) may need to have that set already. And even if not now, then maybe going forward. > With that, it seems like it should also be removed from old_hwdom? > > old_hwdom->cdf &= ~CDF_hardware Oh, indeed. Thanks in turn for catching this further aspect. Jan
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index 634333cdde..b8f282ff10 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -2369,7 +2369,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..da74f815f4 100644 --- a/xen/common/domain.c +++ b/xen/common/domain.c @@ -820,13 +820,18 @@ 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; + flags |= CDF_hardware; } TRACE_TIME(TRC_DOM0_DOM_ADD, d->domain_id); diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h index a34daa7d10..e10baf2615 100644 --- a/xen/include/xen/domain.h +++ b/xen/include/xen/domain.h @@ -53,6 +53,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)