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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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

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