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>
---
 xen/arch/arm/domain_build.c | 2 +-
 xen/arch/x86/setup.c        | 3 ++-
 xen/common/domain.c         | 2 +-
 xen/include/xen/domain.h    | 2 ++
 4 files changed, 6 insertions(+), 3 deletions(-)

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)
-- 
2.48.1

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