| Message ID | 20250306220343.203047-2-jason.andryuk@amd.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | ARM split hardware and control domains | expand |
On 06/03/2025 10:03 pm, 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> I definitely like the removal of the late_hwdom bodges. However, there are several things to be aware of here. First, CDF_privileged probably wants renaming to CDF_control, now that CDF_hardware is being split out. Second, you've created a case where we can make multiple hardware domains, yet it is very much a singleton object from Xen's point of view. This might be ok it's addressed by later in the series. One especially nasty bit of late_hwdom was how dom0 started as both, then the late_hwdom stole dom0's hw-ness. I expect untangling this is more complicated than a single patch. But, by the end, I think we do need to have reasonable confidence that only a single domain can be constructed as the hardware domain. > diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h > index 3de5635291..b5e82578c3 100644 > --- a/xen/include/xen/domain.h > +++ b/xen/include/xen/domain.h > @@ -50,6 +50,8 @@ void arch_get_domain_info(const struct domain *d, > #else > #define CDF_staticmem 0 > #endif > +/* Is this the hardware? */ > +#define CDF_hardware (1U << 3) No, this one CDF flag isn't the hardware. That would be the CPU we're running on. ~Andrew
On 2025-03-06 17:39, Andrew Cooper wrote: > On 06/03/2025 10:03 pm, 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> > > I definitely like the removal of the late_hwdom bodges. > > However, there are several things to be aware of here. > > First, CDF_privileged probably wants renaming to CDF_control, now that > CDF_hardware is being split out. > > Second, you've created a case where we can make multiple hardware > domains, yet it is very much a singleton object from Xen's point of view. hardware_domain still remains the check for is_hardware_domain(), so it's still a singleton. A later ARM patch for the dom0less code adds a panic() if the device tree defines a second hardware domains. > This might be ok it's addressed by later in the series. One especially > nasty bit of late_hwdom was how dom0 started as both, then the > late_hwdom stole dom0's hw-ness. I expect untangling this is more > complicated than a single patch. I didn't address the late_hwdom path. I don't think I broken anything. But this hardware stealing is why I added the 2nd hwdom check in the ARM code. > But, by the end, I think we do need to have reasonable confidence that > only a single domain can be constructed as the hardware domain. What do you think about multiple control/privileged domains? >> diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h >> index 3de5635291..b5e82578c3 100644 >> --- a/xen/include/xen/domain.h >> +++ b/xen/include/xen/domain.h >> @@ -50,6 +50,8 @@ void arch_get_domain_info(const struct domain *d, >> #else >> #define CDF_staticmem 0 >> #endif >> +/* Is this the hardware? */ >> +#define CDF_hardware (1U << 3) > > No, this one CDF flag isn't the hardware. That would be the CPU we're > running on. :) -Jason
On 07/03/2025 2:55 pm, Jason Andryuk wrote: > On 2025-03-06 17:39, Andrew Cooper wrote: >> Second, you've created a case where we can make multiple hardware >> domains, yet it is very much a singleton object from Xen's point of >> view. > > hardware_domain still remains the check for is_hardware_domain(), so > it's still a singleton. Multiple domains can pass in CDF_hardware and latest-takes-precedence for hardware_domain. That only exists because late_hwdom is a bodge and relies on stealing. > A later ARM patch for the dom0less code adds a panic() if the device > tree defines a second hardware domains. Another option might be to strip out late_hwdom, and do this more nicely. I have little confidence that it works, seeing as it only gets touched to fix build issues. Either way, I think the common code wants to be ultimately responsible for refusing to create multiple hardware domains. > >> But, by the end, I think we do need to have reasonable confidence that >> only a single domain can be constructed as the hardware domain. > > What do you think about multiple control/privileged domains? Well, I am the author of https://github.com/xenserver/xen.pg/blob/XS-8.2.x/patches/xen-domctl-set-privileged-domain.patch and this is deployed in production for XenServer+HVMI. "Works on my hypervisor". ~Andrew
On 06.03.2025 23:03, Jason Andryuk wrote: > --- a/xen/common/domain.c > +++ b/xen/common/domain.c > @@ -699,7 +699,7 @@ 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 ) Nit: If the rhs of the || needs to stay for the time being (does it?), parentheses please around the & expression. Jan
On 2025-03-07 11:26, Andrew Cooper wrote: > On 07/03/2025 2:55 pm, Jason Andryuk wrote: >> On 2025-03-06 17:39, Andrew Cooper wrote: >>> Second, you've created a case where we can make multiple hardware >>> domains, yet it is very much a singleton object from Xen's point of >>> view. >> >> hardware_domain still remains the check for is_hardware_domain(), so >> it's still a singleton. > > Multiple domains can pass in CDF_hardware and latest-takes-precedence > for hardware_domain. > > That only exists because late_hwdom is a bodge and relies on stealing. > >> A later ARM patch for the dom0less code adds a panic() if the device >> tree defines a second hardware domains. > > Another option might be to strip out late_hwdom, and do this more > nicely. I have little confidence that it works, seeing as it only gets > touched to fix build issues. I don't want late_hwdom to hold up ARM side changes. I'm not using late_hwdom, so I'd be fine removing it. But until Hyperlaunch is merged, removal seems a little premature. > Either way, I think the common code wants to be ultimately responsible > for refusing to create multiple hardware domains. > >> >>> But, by the end, I think we do need to have reasonable confidence that >>> only a single domain can be constructed as the hardware domain. Would just an addition check be okay? Only allow the late_hwdom behaviour for dom0. --- i/xen/common/domain.c +++ w/xen/common/domain.c @@ -704,6 +704,10 @@ struct domain *domain_create(domid_t 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 -EINVAL; + old_hwdom = hardware_domain; hardware_domain = d; } CDF_hardware is a Xen-internal flag, so it's not something the toolstack can pass in. Regards, Jason
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index d4570bc0b4..6784ee6f6d 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -2358,7 +2358,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 143749e5da..fa18b9caf2 100644 --- a/xen/arch/x86/setup.c +++ b/xen/arch/x86/setup.c @@ -1017,7 +1017,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 0c4cc77111..c170597410 100644 --- a/xen/common/domain.c +++ b/xen/common/domain.c @@ -699,7 +699,7 @@ 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"); diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h index 3de5635291..b5e82578c3 100644 --- a/xen/include/xen/domain.h +++ b/xen/include/xen/domain.h @@ -50,6 +50,8 @@ void arch_get_domain_info(const struct domain *d, #else #define CDF_staticmem 0 #endif +/* Is this the hardware? */ +#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)