Re: [PATCH 2/4] x86/domain: Implement arch_init_idle_domain()

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 18 Jul 2024, Andrew Cooper wrote:
> The idle domain needs d->arch.ctxt_switch initialised on x86.  Implement the
> new arch_init_idle_domain() in order to do this.
> 
> Right now this double-initalises the ctxt_switch pointer, but it's safe and
> will stop happening when domain_create() is cleaned up as a consequence.
> 
> No practical change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> CC: Julien Grall <julien@xxxxxxx>
> CC: Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
> CC: Bertrand Marquis <bertrand.marquis@xxxxxxx>
> CC: Michal Orzel <michal.orzel@xxxxxxx>
> CC: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> ---
>  xen/arch/x86/domain.c             | 19 ++++++++++++-------
>  xen/arch/x86/include/asm/domain.h |  3 +++
>  2 files changed, 15 insertions(+), 7 deletions(-)
> 
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index ccadfe0c9e70..eff905c6c6e5 100644
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -768,6 +768,17 @@ static bool emulation_flags_ok(const struct domain *d, 
> uint32_t emflags)
>      return true;
>  }
>  
> +void __init arch_init_idle_domain(struct domain *d)
> +{
> +    static const struct arch_csw idle_csw = {
> +        .from = paravirt_ctxt_switch_from,
> +        .to   = paravirt_ctxt_switch_to,
> +        .tail = idle_loop,
> +    };
> +
> +    d->arch.ctxt_switch = &idle_csw;
> +}
> +
>  int arch_domain_create(struct domain *d,
>                         struct xen_domctl_createdomain *config,
>                         unsigned int flags)
> @@ -783,13 +794,7 @@ int arch_domain_create(struct domain *d,
>      /* Minimal initialisation for the idle domain. */
>      if ( unlikely(is_idle_domain(d)) )
>      {
> -        static const struct arch_csw idle_csw = {
> -            .from = paravirt_ctxt_switch_from,
> -            .to   = paravirt_ctxt_switch_to,
> -            .tail = idle_loop,
> -        };
> -
> -        d->arch.ctxt_switch = &idle_csw;
> +        arch_init_idle_domain(d);

I don't understand why you need this call to arch_init_idle_domain(d)
here given the other call to arch_init_idle_domain added by patch #1.

Also I am not sure why you didn't move the line below (cpu_policy =
ZERO_BLOCK_PTR) to arch_init_idle_domain as well but I admit I don't
know this code

I realize you are removing all this code in patch #4... but still why
the line below is not needed anymore? And why do we need to add an extra
call to arch_init_idle_domain temporarily in this patch?


>          d->arch.cpu_policy = ZERO_BLOCK_PTR; /* Catch stray misuses. */
>  
> diff --git a/xen/arch/x86/include/asm/domain.h 
> b/xen/arch/x86/include/asm/domain.h
> index f5daeb182baa..bca3258d69ac 100644
> --- a/xen/arch/x86/include/asm/domain.h
> +++ b/xen/arch/x86/include/asm/domain.h
> @@ -779,6 +779,9 @@ struct arch_vcpu_io {
>  /* Maxphysaddr supportable by the paging infrastructure. */
>  unsigned int domain_max_paddr_bits(const struct domain *d);
>  
> +#define arch_init_idle_domain arch_init_idle_domain
> +void arch_init_idle_domain(struct domain *d);
> +
>  #endif /* __ASM_DOMAIN_H__ */
>  
>  /*
> -- 
> 2.39.2
>