| Message ID | 20240718215744.3892072-5-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | xen/domain: Idle domain creation improvements | expand |
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
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
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
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 >
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 )
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(-)