xen/domain: Idle domain creation improvements

[PATCH 4/4] arch/domain: Clean up the idle domain remnants in arch_domain_create()

Posted by Andrew Cooper 1 month, 4 weeks ago

With arch_domain_create() no longer being called with the idle domain, drop
the last remaining logic.

No functional 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/arm/domain.c |  6 ------
 xen/arch/x86/domain.c | 17 -----------------
 2 files changed, 23 deletions(-)

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 7cfcefd27944..3ba959f86633 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -706,12 +706,6 @@ int arch_domain_create(struct domain *d,
 
     BUILD_BUG_ON(GUEST_MAX_VCPUS < MAX_VIRT_CPUS);
 
-    /* Idle domains do not need this setup */
-    if ( is_idle_domain(d) )
-        return 0;
-
-    ASSERT(config != NULL);
-
 #ifdef CONFIG_IOREQ_SERVER
     ioreq_domain_init(d);
 #endif
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index eff905c6c6e5..c71b9023cb1a 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -791,23 +791,6 @@ int arch_domain_create(struct domain *d,
 
     spin_lock_init(&d->arch.e820_lock);
 
-    /* Minimal initialisation for the idle domain. */
-    if ( unlikely(is_idle_domain(d)) )
-    {
-        arch_init_idle_domain(d);
-
-        d->arch.cpu_policy = ZERO_BLOCK_PTR; /* Catch stray misuses. */
-
-        return 0;
-    }
-
-    if ( !config )
-    {
-        /* Only IDLE is allowed with no config. */
-        ASSERT_UNREACHABLE();
-        return -EINVAL;
-    }
-
     if ( d->domain_id && cpu_has_amd_erratum(&boot_cpu_data, AMD_ERRATUM_121) )
     {
         if ( !opt_allow_unsafe )
-- 
2.39.2

Re: [PATCH 4/4] arch/domain: Clean up the idle domain remnants in arch_domain_create()

Posted by Stefano Stabellini 1 month, 2 weeks ago

On Thu, 18 Jul 2024, Andrew Cooper wrote:
> With arch_domain_create() no longer being called with the idle domain, drop
> the last remaining logic.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

but one question below


> ---
> 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/arm/domain.c |  6 ------
>  xen/arch/x86/domain.c | 17 -----------------
>  2 files changed, 23 deletions(-)
> 
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 7cfcefd27944..3ba959f86633 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -706,12 +706,6 @@ int arch_domain_create(struct domain *d,
>  
>      BUILD_BUG_ON(GUEST_MAX_VCPUS < MAX_VIRT_CPUS);
>  
> -    /* Idle domains do not need this setup */
> -    if ( is_idle_domain(d) )
> -        return 0;
> -
> -    ASSERT(config != NULL);
> -
>  #ifdef CONFIG_IOREQ_SERVER
>      ioreq_domain_init(d);
>  #endif
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index eff905c6c6e5..c71b9023cb1a 100644
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -791,23 +791,6 @@ int arch_domain_create(struct domain *d,
>  
>      spin_lock_init(&d->arch.e820_lock);
>  
> -    /* Minimal initialisation for the idle domain. */
> -    if ( unlikely(is_idle_domain(d)) )
> -    {
> -        arch_init_idle_domain(d);
> -
> -        d->arch.cpu_policy = ZERO_BLOCK_PTR; /* Catch stray misuses. */

Are you sure you don't want to keep it?


> -        return 0;
> -    }
> -
> -    if ( !config )
> -    {
> -        /* Only IDLE is allowed with no config. */
> -        ASSERT_UNREACHABLE();
> -        return -EINVAL;
> -    }
> -
>      if ( d->domain_id && cpu_has_amd_erratum(&boot_cpu_data, AMD_ERRATUM_121) )
>      {
>          if ( !opt_allow_unsafe )
> -- 
> 2.39.2
>

Re: [PATCH 4/4] arch/domain: Clean up the idle domain remnants in arch_domain_create()

Posted by Jan Beulich 1 month, 3 weeks ago

On 18.07.2024 23:57, Andrew Cooper wrote:
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -791,23 +791,6 @@ int arch_domain_create(struct domain *d,
>  
>      spin_lock_init(&d->arch.e820_lock);
>  
> -    /* Minimal initialisation for the idle domain. */
> -    if ( unlikely(is_idle_domain(d)) )
> -    {
> -        arch_init_idle_domain(d);
> -
> -        d->arch.cpu_policy = ZERO_BLOCK_PTR; /* Catch stray misuses. */

This line looks to be lost in the process. Already in an earlier patch it
should move to arch_init_idle_domain(), shouldn't it? With that adjustment
for the entire series:
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan

Re: [PATCH 4/4] arch/domain: Clean up the idle domain remnants in arch_domain_create()

Posted by Andrew Cooper 1 month, 3 weeks ago

On 22/07/2024 8:23 am, Jan Beulich wrote:
> On 18.07.2024 23:57, Andrew Cooper wrote:
>> --- a/xen/arch/x86/domain.c
>> +++ b/xen/arch/x86/domain.c
>> @@ -791,23 +791,6 @@ int arch_domain_create(struct domain *d,
>>  
>>      spin_lock_init(&d->arch.e820_lock);
>>  
>> -    /* Minimal initialisation for the idle domain. */
>> -    if ( unlikely(is_idle_domain(d)) )
>> -    {
>> -        arch_init_idle_domain(d);
>> -
>> -        d->arch.cpu_policy = ZERO_BLOCK_PTR; /* Catch stray misuses. */
> This line looks to be lost in the process. Already in an earlier patch it
> should move to arch_init_idle_domain(), shouldn't it? With that adjustment
> for the entire series:
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

It was semi-intentional.  cpu_policy is just one of many pointers, and I
don't see a good reason for it to be treated specially.

It's been around for years now, and never one triggered, not to mention
the fact that there's a dwindling set of machines where a dereference of
0 isn't instantly fatal anyway.

~Andrew

Re: [PATCH 4/4] arch/domain: Clean up the idle domain remnants in arch_domain_create()

Posted by Jan Beulich 1 month, 3 weeks ago

On 22.07.2024 19:46, Andrew Cooper wrote:
> On 22/07/2024 8:23 am, Jan Beulich wrote:
>> On 18.07.2024 23:57, Andrew Cooper wrote:
>>> --- a/xen/arch/x86/domain.c
>>> +++ b/xen/arch/x86/domain.c
>>> @@ -791,23 +791,6 @@ int arch_domain_create(struct domain *d,
>>>  
>>>      spin_lock_init(&d->arch.e820_lock);
>>>  
>>> -    /* Minimal initialisation for the idle domain. */
>>> -    if ( unlikely(is_idle_domain(d)) )
>>> -    {
>>> -        arch_init_idle_domain(d);
>>> -
>>> -        d->arch.cpu_policy = ZERO_BLOCK_PTR; /* Catch stray misuses. */
>> This line looks to be lost in the process. Already in an earlier patch it
>> should move to arch_init_idle_domain(), shouldn't it? With that adjustment
>> for the entire series:
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 
> It was semi-intentional.  cpu_policy is just one of many pointers, and I
> don't see a good reason for it to be treated specially.
> 
> It's been around for years now, and never one triggered, not to mention
> the fact that there's a dwindling set of machines where a dereference of
> 0 isn't instantly fatal anyway.

Okay, yet would you mind mentioning this removal as intentional then in
the description?

Jan