Re: [PATCH 01/23] xen: introduce hardware domain create flag

[Jason Andryuk <jason.andryuk@xxxxxxx>]

On 2025-03-06 17:39, Andrew Cooper wrote:
> On 06/03/2025 10:03 pm, Jason Andryuk wrote:
> > From: "Daniel P. Smith" <dpsmith@xxxxxxxxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxxxxxxx>
> > Signed-off-by: Jason Andryuk <jason.andryuk@xxxxxxx>
> 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