Re: [PATCH v2 10/35] xen/domain: add get_initial_domain_id()

[Denis Mukhin <dmkhn@xxxxxxxxx>]

On Wednesday, December 11th, 2024 at 8:50 AM, Roger Pau Monné 
<roger.pau@xxxxxxxxxx> wrote:

>
>
> On Thu, Dec 05, 2024 at 08:41:40PM -0800, Denis Mukhin via B4 Relay wrote:
>
> > From: Denis Mukhin dmukhin@xxxxxxxx
> >
> > Move get_initial_domain_id() to a public API and enable for all 
> > architectures.
> > That is pre-requisite change for console focus switch logic cleanup.
> >
> > Signed-off-by: Denis Mukhin dmukhin@xxxxxxxx
> > ---
> > xen/arch/x86/include/asm/pv/shim.h | 4 ++--
> > xen/arch/x86/pv/shim.c | 4 ++--
> > xen/common/domain.c | 10 ++++++++++
> > xen/include/xen/domain.h | 2 ++
> > 4 files changed, 16 insertions(+), 4 deletions(-)
> >
> > diff --git a/xen/arch/x86/include/asm/pv/shim.h 
> > b/xen/arch/x86/include/asm/pv/shim.h
> > index 
> > 6153e27005986881ad87e9db0b555b30edc59fc0..1515ad1b0680aa11ab91a152a1944fc1bb477a79
> >  100644
> > --- a/xen/arch/x86/include/asm/pv/shim.h
> > +++ b/xen/arch/x86/include/asm/pv/shim.h
> > @@ -31,7 +31,7 @@ long cf_check pv_shim_cpu_up(void *data);
> > long cf_check pv_shim_cpu_down(void *data);
> > void pv_shim_online_memory(unsigned int nr, unsigned int order);
> > void pv_shim_offline_memory(unsigned int nr, unsigned int order);
> > -domid_t get_initial_domain_id(void);
> > +domid_t pv_shim_initial_domain_id(void);
> > uint64_t pv_shim_mem(uint64_t avail);
> > void pv_shim_fixup_e820(void);
> > const struct platform_bad_page *pv_shim_reserved_pages(unsigned int *size);
> > @@ -76,7 +76,7 @@ static inline void pv_shim_offline_memory(unsigned int 
> > nr, unsigned int order)
> > {
> > ASSERT_UNREACHABLE();
> > }
> > -static inline domid_t get_initial_domain_id(void)
> > +static inline domid_t pv_shim_initial_domain_id(void)
> > {
> > return 0;
> > }
> > diff --git a/xen/arch/x86/pv/shim.c b/xen/arch/x86/pv/shim.c
> > index 
> > 81e4a0516d18b359561f471f1d96e38977661ca7..17cb30620290c76cf42251f70cfa4199c0e165d1
> >  100644
> > --- a/xen/arch/x86/pv/shim.c
> > +++ b/xen/arch/x86/pv/shim.c
> > @@ -328,7 +328,7 @@ int pv_shim_shutdown(uint8_t reason)
> > }
> >
> > /* Update domain id. */
> > - d->domain_id = get_initial_domain_id();
> > + d->domain_id = pv_shim_initial_domain_id();
>
>
> Can't you leave this instance using get_initial_domain_id(), it should
> DTRT when running in pv-shim mode.

Reverted.

>
> > /* Clean the iomem range. */
> > BUG_ON(iomem_deny_access(d, 0, ~0UL));
> > @@ -1016,7 +1016,7 @@ void pv_shim_offline_memory(unsigned int nr, unsigned 
> > int order)
> > }
> > }
> >
> > -domid_t get_initial_domain_id(void)
> > +domid_t pv_shim_initial_domain_id(void)
> > {
> > uint32_t eax, ebx, ecx, edx;
> >
> > diff --git a/xen/common/domain.c b/xen/common/domain.c
> > index 
> > 92263a4fbdc57159b4a32d9d4ee038f9f37804ed..2f67aa06ed50e69c27cedc8d7f6eb0b469fe81cd
> >  100644
> > --- a/xen/common/domain.c
> > +++ b/xen/common/domain.c
> > @@ -45,6 +45,7 @@
> >
> > #ifdef CONFIG_X86
> > #include <asm/guest.h>
> > +#include <asm/pv/shim.h>
> > #endif
> >
> > /* Linux config option: propageted to domain0 */
> > @@ -2229,6 +2230,15 @@ int continue_hypercall_on_cpu(
> > return 0;
> > }
> >
> > +domid_t get_initial_domain_id(void)
> > +{
> > +#ifdef CONFIG_X86
> > + return pv_shim_initial_domain_id();
> > +#else
> > + return 0;
> > +#endif
> > +}
>
>
> Maybe there are further changes that make this a not suitable option,
> but won't it be better to maybe do something like:
>
> #ifndef HAS_ARCH_INITIAL_DOMID
> static inline domid_t get_initial_domain_id(void) { return 0; }
> #else
> domid_t get_initial_domain_id(void);
> #endif
>
> In a generic header, and then in an x86 header you just
>
> #define HAS_ARCH_INITIAL_DOMID
>
> The ifdefary in get_initial_domain_id() if other arches need different
> implementations seems undesirable.

IMO, existing implementation of get_initial_domain_id() looks like
a layering violation: global get_initial_domain_id() does not belong to PV
shim layer.

The current equivalent of HAS_ARCH_INITIAL_DOMID is CONFIG_PV_SHIM, so I think
there's no need to define another config flag.

After addressing Jan's feedback and then moving the code around,
I kept pv_shim_get_initial_domain_id() in PV shim and #ifdefs
in common/domain.c (similar to domain_shutdown()).

Also, I switched to using hardware_domid instead of open-coded 0.

>
> Thanks, Roger.