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@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien@xen.org>
CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
CC: Bertrand Marquis <bertrand.marquis@arm.com>
CC: Michal Orzel <michal.orzel@amd.com>
CC: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
 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);
 
         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

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@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien@xen.org>
> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
> CC: Bertrand Marquis <bertrand.marquis@arm.com>
> CC: Michal Orzel <michal.orzel@amd.com>
> CC: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> ---
>  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
> 

On 31/07/2024 12:44 am, Stefano Stabellini wrote:
> 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@citrix.com>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Roger Pau Monné <roger.pau@citrix.com>
>> CC: Stefano Stabellini <sstabellini@kernel.org>
>> CC: Julien Grall <julien@xen.org>
>> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
>> CC: Bertrand Marquis <bertrand.marquis@arm.com>
>> CC: Michal Orzel <michal.orzel@amd.com>
>> CC: Oleksii Kurochko <oleksii.kurochko@gmail.com>
>> ---
>>  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.

It's stale from an older version of the series.  I'll drop it, which
will reduce the churn in this as well as patch 4.

> 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.

See the thread on patch 4 with Jan.  It's intentional.

This was a safety check which is dubious (it's not special enough to
warrant it on its own), and has never tripped.

I'll note it's intentional omission in the commit message.

~Andrew