[PATCH] x86/domain: restrict context switch hooks for the idle domain

[PATCH] x86/domain: restrict context switch hooks for the idle domain

[Roger Pau Monne]

From: Grygorii Strashko <grygorii_strashko@epam.com>

The arch_init_idle_domain() initializes struct arch_csw idle_csw as:

    static const struct arch_csw idle_csw = {
        .from = paravirt_ctxt_switch_from,
        .to   = paravirt_ctxt_switch_to,
        .tail = idle_loop,
    };

but only place where d->arch.ctxt_switch->{to,from}() are used is
__context_switch() where they are both guarded by !is_idle_domain(pd)
which makes the callbacks in idle_csw unreachable.

Drop to/from() callbacks initialization in idle_csw.

Signed-off-by: Grygorii Strashko <grygorii_strashko@epam.com>
Signed-off-by: Roger Pau Monné <roger@xenproject.org>
---
I don't think there's a suitable Fixes candidate here.  The idle domain
hooks used to piggyback on the PV ones, and commit ab914e04a6272 split
those out into it's own structure.  At that point the to/from hooks could
have been left zeroed, but that wasn't the point of that commit anyway.
---
 xen/arch/x86/domain.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 996b50af7a17..464ca6f39026 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -813,8 +813,6 @@ static bool emulation_flags_ok(const struct domain *d, uint32_t emflags)
 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,
     };
 
-- 
2.55.0

Re: [PATCH] x86/domain: restrict context switch hooks for the idle domain

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 811 bytes --]

On 01.10.26 16:27, Roger Pau Monne wrote:
> From: Grygorii Strashko <grygorii_strashko@epam.com>
> 
> The arch_init_idle_domain() initializes struct arch_csw idle_csw as:
> 
>      static const struct arch_csw idle_csw = {
>          .from = paravirt_ctxt_switch_from,
>          .to   = paravirt_ctxt_switch_to,
>          .tail = idle_loop,
>      };
> 
> but only place where d->arch.ctxt_switch->{to,from}() are used is
> __context_switch() where they are both guarded by !is_idle_domain(pd)
> which makes the callbacks in idle_csw unreachable.
> 
> Drop to/from() callbacks initialization in idle_csw.
> 
> Signed-off-by: Grygorii Strashko <grygorii_strashko@epam.com>
> Signed-off-by: Roger Pau Monné <roger@xenproject.org>

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH] x86/domain: restrict context switch hooks for the idle domain

[Jason Andryuk]

On 2026-10-01 10:27, Roger Pau Monne wrote:
> From: Grygorii Strashko <grygorii_strashko@epam.com>
> 
> The arch_init_idle_domain() initializes struct arch_csw idle_csw as:
> 
>      static const struct arch_csw idle_csw = {
>          .from = paravirt_ctxt_switch_from,
>          .to   = paravirt_ctxt_switch_to,
>          .tail = idle_loop,
>      };
> 
> but only place where d->arch.ctxt_switch->{to,from}() are used is
> __context_switch() where they are both guarded by !is_idle_domain(pd)
> which makes the callbacks in idle_csw unreachable.
> 
> Drop to/from() callbacks initialization in idle_csw.
> 
> Signed-off-by: Grygorii Strashko <grygorii_strashko@epam.com>
> Signed-off-by: Roger Pau Monné <roger@xenproject.org>
Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>

Thanks,
Jason

Re: [PATCH] x86/domain: restrict context switch hooks for the idle domain

[Jan Beulich]

On 01.10.2026 16:27, Roger Pau Monne wrote:
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -813,8 +813,6 @@ static bool emulation_flags_ok(const struct domain *d, uint32_t emflags)
>  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,
>      };

Leaving NULL pointers around isn't a good thing, though. May I suggest to
at least poison the pointers then?

Jan

Re: [PATCH] x86/domain: restrict context switch hooks for the idle domain

[Roger Pau Monné]

On Fri, Oct 02, 2026 at 07:45:07AM +0200, Jan Beulich wrote:
> On 01.10.2026 16:27, Roger Pau Monne wrote:
> > --- a/xen/arch/x86/domain.c
> > +++ b/xen/arch/x86/domain.c
> > @@ -813,8 +813,6 @@ static bool emulation_flags_ok(const struct domain *d, uint32_t emflags)
> >  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,
> >      };
> 
> Leaving NULL pointers around isn't a good thing, though. May I suggest to
> at least poison the pointers then?

I don't mind doing so, but I don't think we usually do that for other
hook structures, ie: hvm_function_table for example doesn't poison
unset hooks (and it possibly can't, because there are .hook != NULL
checks).

TBH I'm missing a benefit of the poisoning here.  Using NULL or a
poisoned value will both trigger a page-fault, and NULL has the
benefit of callers easily checking whether the hook is set.

Thanks, Roger.

Re: [PATCH] x86/domain: restrict context switch hooks for the idle domain

[Jan Beulich]

On 02.10.2026 09:48, Roger Pau Monné wrote:
> On Fri, Oct 02, 2026 at 07:45:07AM +0200, Jan Beulich wrote:
>> On 01.10.2026 16:27, Roger Pau Monne wrote:
>>> --- a/xen/arch/x86/domain.c
>>> +++ b/xen/arch/x86/domain.c
>>> @@ -813,8 +813,6 @@ static bool emulation_flags_ok(const struct domain *d, uint32_t emflags)
>>>  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,
>>>      };
>>
>> Leaving NULL pointers around isn't a good thing, though. May I suggest to
>> at least poison the pointers then?
> 
> I don't mind doing so, but I don't think we usually do that for other
> hook structures, ie: hvm_function_table for example doesn't poison
> unset hooks (and it possibly can't, because there are .hook != NULL
> checks).
> 
> TBH I'm missing a benefit of the poisoning here.  Using NULL or a
> poisoned value will both trigger a page-fault, and NULL has the
> benefit of callers easily checking whether the hook is set.

NULL in the context of a HVM vCPU will #PF. NULL in the context of a PV
one may not, depending on what the guest may have put there.

Jan

Re: [PATCH] x86/domain: restrict context switch hooks for the idle domain

[Roger Pau Monné]

On Fri, Oct 02, 2026 at 09:55:09AM +0200, Jan Beulich wrote:
> On 02.10.2026 09:48, Roger Pau Monné wrote:
> > On Fri, Oct 02, 2026 at 07:45:07AM +0200, Jan Beulich wrote:
> >> On 01.10.2026 16:27, Roger Pau Monne wrote:
> >>> --- a/xen/arch/x86/domain.c
> >>> +++ b/xen/arch/x86/domain.c
> >>> @@ -813,8 +813,6 @@ static bool emulation_flags_ok(const struct domain *d, uint32_t emflags)
> >>>  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,
> >>>      };
> >>
> >> Leaving NULL pointers around isn't a good thing, though. May I suggest to
> >> at least poison the pointers then?
> > 
> > I don't mind doing so, but I don't think we usually do that for other
> > hook structures, ie: hvm_function_table for example doesn't poison
> > unset hooks (and it possibly can't, because there are .hook != NULL
> > checks).
> > 
> > TBH I'm missing a benefit of the poisoning here.  Using NULL or a
> > poisoned value will both trigger a page-fault, and NULL has the
> > benefit of callers easily checking whether the hook is set.
> 
> NULL in the context of a HVM vCPU will #PF. NULL in the context of a PV
> one may not, depending on what the guest may have put there.

It still feels a bit arbitrary to do here but not in other places, but
never mind, I will adjust and send v2.

Thanks, Roger.

Re: [PATCH] x86/domain: restrict context switch hooks for the idle domain

[Jan Beulich]

On 02.10.2026 10:05, Roger Pau Monné wrote:
> On Fri, Oct 02, 2026 at 09:55:09AM +0200, Jan Beulich wrote:
>> On 02.10.2026 09:48, Roger Pau Monné wrote:
>>> On Fri, Oct 02, 2026 at 07:45:07AM +0200, Jan Beulich wrote:
>>>> On 01.10.2026 16:27, Roger Pau Monne wrote:
>>>>> --- a/xen/arch/x86/domain.c
>>>>> +++ b/xen/arch/x86/domain.c
>>>>> @@ -813,8 +813,6 @@ static bool emulation_flags_ok(const struct domain *d, uint32_t emflags)
>>>>>  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,
>>>>>      };
>>>>
>>>> Leaving NULL pointers around isn't a good thing, though. May I suggest to
>>>> at least poison the pointers then?
>>>
>>> I don't mind doing so, but I don't think we usually do that for other
>>> hook structures, ie: hvm_function_table for example doesn't poison
>>> unset hooks (and it possibly can't, because there are .hook != NULL
>>> checks).
>>>
>>> TBH I'm missing a benefit of the poisoning here.  Using NULL or a
>>> poisoned value will both trigger a page-fault, and NULL has the
>>> benefit of callers easily checking whether the hook is set.
>>
>> NULL in the context of a HVM vCPU will #PF. NULL in the context of a PV
>> one may not, depending on what the guest may have put there.
> 
> It still feels a bit arbitrary to do here but not in other places,

We should imo resolve this by replacing NULLs wherever hooks could, due
to other issues, end up "in sight" on paths reachable for PV guests. This
would then also cover potential speculation along such paths.

> but never mind, I will adjust and send v2.

Thanks.

Jan

Re: [PATCH] x86/domain: restrict context switch hooks for the idle domain

[Roger Pau Monné]

On Fri, Oct 02, 2026 at 10:09:02AM +0200, Jan Beulich wrote:
> On 02.10.2026 10:05, Roger Pau Monné wrote:
> > On Fri, Oct 02, 2026 at 09:55:09AM +0200, Jan Beulich wrote:
> >> On 02.10.2026 09:48, Roger Pau Monné wrote:
> >>> On Fri, Oct 02, 2026 at 07:45:07AM +0200, Jan Beulich wrote:
> >>>> On 01.10.2026 16:27, Roger Pau Monne wrote:
> >>>>> --- a/xen/arch/x86/domain.c
> >>>>> +++ b/xen/arch/x86/domain.c
> >>>>> @@ -813,8 +813,6 @@ static bool emulation_flags_ok(const struct domain *d, uint32_t emflags)
> >>>>>  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,
> >>>>>      };
> >>>>
> >>>> Leaving NULL pointers around isn't a good thing, though. May I suggest to
> >>>> at least poison the pointers then?
> >>>
> >>> I don't mind doing so, but I don't think we usually do that for other
> >>> hook structures, ie: hvm_function_table for example doesn't poison
> >>> unset hooks (and it possibly can't, because there are .hook != NULL
> >>> checks).
> >>>
> >>> TBH I'm missing a benefit of the poisoning here.  Using NULL or a
> >>> poisoned value will both trigger a page-fault, and NULL has the
> >>> benefit of callers easily checking whether the hook is set.
> >>
> >> NULL in the context of a HVM vCPU will #PF. NULL in the context of a PV
> >> one may not, depending on what the guest may have put there.
> > 
> > It still feels a bit arbitrary to do here but not in other places,
> 
> We should imo resolve this by replacing NULLs wherever hooks could, due
> to other issues, end up "in sight" on paths reachable for PV guests. This
> would then also cover potential speculation along such paths.

FTAOD, I plan to introduce a poison value using a non-canonical
address, ie:

#define POINTER_POISON ((void *)0xDEAD0000DEAD0000UL)

in asm/config.h.

Would that be acceptable?

Thanks, Roger.

Re: [PATCH] x86/domain: restrict context switch hooks for the idle domain

[Roger Pau Monné]

On Fri, Oct 02, 2026 at 11:23:17AM +0200, Roger Pau Monné wrote:
> On Fri, Oct 02, 2026 at 10:09:02AM +0200, Jan Beulich wrote:
> > On 02.10.2026 10:05, Roger Pau Monné wrote:
> > > On Fri, Oct 02, 2026 at 09:55:09AM +0200, Jan Beulich wrote:
> > >> On 02.10.2026 09:48, Roger Pau Monné wrote:
> > >>> On Fri, Oct 02, 2026 at 07:45:07AM +0200, Jan Beulich wrote:
> > >>>> On 01.10.2026 16:27, Roger Pau Monne wrote:
> > >>>>> --- a/xen/arch/x86/domain.c
> > >>>>> +++ b/xen/arch/x86/domain.c
> > >>>>> @@ -813,8 +813,6 @@ static bool emulation_flags_ok(const struct domain *d, uint32_t emflags)
> > >>>>>  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,
> > >>>>>      };
> > >>>>
> > >>>> Leaving NULL pointers around isn't a good thing, though. May I suggest to
> > >>>> at least poison the pointers then?
> > >>>
> > >>> I don't mind doing so, but I don't think we usually do that for other
> > >>> hook structures, ie: hvm_function_table for example doesn't poison
> > >>> unset hooks (and it possibly can't, because there are .hook != NULL
> > >>> checks).
> > >>>
> > >>> TBH I'm missing a benefit of the poisoning here.  Using NULL or a
> > >>> poisoned value will both trigger a page-fault, and NULL has the
> > >>> benefit of callers easily checking whether the hook is set.
> > >>
> > >> NULL in the context of a HVM vCPU will #PF. NULL in the context of a PV
> > >> one may not, depending on what the guest may have put there.
> > > 
> > > It still feels a bit arbitrary to do here but not in other places,
> > 
> > We should imo resolve this by replacing NULLs wherever hooks could, due
> > to other issues, end up "in sight" on paths reachable for PV guests. This
> > would then also cover potential speculation along such paths.
> 
> FTAOD, I plan to introduce a poison value using a non-canonical
> address, ie:
> 
> #define POINTER_POISON ((void *)0xDEAD0000DEAD0000UL)

s/POINTER_POISON/FUNCTION_POISON/

Re: [PATCH] x86/domain: restrict context switch hooks for the idle domain

[Jan Beulich]

On 02.10.2026 11:29, Roger Pau Monné wrote:
> On Fri, Oct 02, 2026 at 11:23:17AM +0200, Roger Pau Monné wrote:
>> On Fri, Oct 02, 2026 at 10:09:02AM +0200, Jan Beulich wrote:
>>> On 02.10.2026 10:05, Roger Pau Monné wrote:
>>>> On Fri, Oct 02, 2026 at 09:55:09AM +0200, Jan Beulich wrote:
>>>>> On 02.10.2026 09:48, Roger Pau Monné wrote:
>>>>>> On Fri, Oct 02, 2026 at 07:45:07AM +0200, Jan Beulich wrote:
>>>>>>> On 01.10.2026 16:27, Roger Pau Monne wrote:
>>>>>>>> --- a/xen/arch/x86/domain.c
>>>>>>>> +++ b/xen/arch/x86/domain.c
>>>>>>>> @@ -813,8 +813,6 @@ static bool emulation_flags_ok(const struct domain *d, uint32_t emflags)
>>>>>>>>  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,
>>>>>>>>      };
>>>>>>>
>>>>>>> Leaving NULL pointers around isn't a good thing, though. May I suggest to
>>>>>>> at least poison the pointers then?
>>>>>>
>>>>>> I don't mind doing so, but I don't think we usually do that for other
>>>>>> hook structures, ie: hvm_function_table for example doesn't poison
>>>>>> unset hooks (and it possibly can't, because there are .hook != NULL
>>>>>> checks).
>>>>>>
>>>>>> TBH I'm missing a benefit of the poisoning here.  Using NULL or a
>>>>>> poisoned value will both trigger a page-fault, and NULL has the
>>>>>> benefit of callers easily checking whether the hook is set.
>>>>>
>>>>> NULL in the context of a HVM vCPU will #PF. NULL in the context of a PV
>>>>> one may not, depending on what the guest may have put there.
>>>>
>>>> It still feels a bit arbitrary to do here but not in other places,
>>>
>>> We should imo resolve this by replacing NULLs wherever hooks could, due
>>> to other issues, end up "in sight" on paths reachable for PV guests. This
>>> would then also cover potential speculation along such paths.
>>
>> FTAOD, I plan to introduce a poison value using a non-canonical
>> address, ie:
>>
>> #define POINTER_POISON ((void *)0xDEAD0000DEAD0000UL)
> 
> s/POINTER_POISON/FUNCTION_POISON/

Sure, if you think none of the present poisoning values we have is good to
reuse here.

Jan

Re: [PATCH] x86/domain: restrict context switch hooks for the idle domain

[Roger Pau Monné]

On Fri, Oct 02, 2026 at 11:35:40AM +0200, Jan Beulich wrote:
> On 02.10.2026 11:29, Roger Pau Monné wrote:
> > On Fri, Oct 02, 2026 at 11:23:17AM +0200, Roger Pau Monné wrote:
> >> On Fri, Oct 02, 2026 at 10:09:02AM +0200, Jan Beulich wrote:
> >>> On 02.10.2026 10:05, Roger Pau Monné wrote:
> >>>> On Fri, Oct 02, 2026 at 09:55:09AM +0200, Jan Beulich wrote:
> >>>>> On 02.10.2026 09:48, Roger Pau Monné wrote:
> >>>>>> On Fri, Oct 02, 2026 at 07:45:07AM +0200, Jan Beulich wrote:
> >>>>>>> On 01.10.2026 16:27, Roger Pau Monne wrote:
> >>>>>>>> --- a/xen/arch/x86/domain.c
> >>>>>>>> +++ b/xen/arch/x86/domain.c
> >>>>>>>> @@ -813,8 +813,6 @@ static bool emulation_flags_ok(const struct domain *d, uint32_t emflags)
> >>>>>>>>  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,
> >>>>>>>>      };
> >>>>>>>
> >>>>>>> Leaving NULL pointers around isn't a good thing, though. May I suggest to
> >>>>>>> at least poison the pointers then?
> >>>>>>
> >>>>>> I don't mind doing so, but I don't think we usually do that for other
> >>>>>> hook structures, ie: hvm_function_table for example doesn't poison
> >>>>>> unset hooks (and it possibly can't, because there are .hook != NULL
> >>>>>> checks).
> >>>>>>
> >>>>>> TBH I'm missing a benefit of the poisoning here.  Using NULL or a
> >>>>>> poisoned value will both trigger a page-fault, and NULL has the
> >>>>>> benefit of callers easily checking whether the hook is set.
> >>>>>
> >>>>> NULL in the context of a HVM vCPU will #PF. NULL in the context of a PV
> >>>>> one may not, depending on what the guest may have put there.
> >>>>
> >>>> It still feels a bit arbitrary to do here but not in other places,
> >>>
> >>> We should imo resolve this by replacing NULLs wherever hooks could, due
> >>> to other issues, end up "in sight" on paths reachable for PV guests. This
> >>> would then also cover potential speculation along such paths.
> >>
> >> FTAOD, I plan to introduce a poison value using a non-canonical
> >> address, ie:
> >>
> >> #define POINTER_POISON ((void *)0xDEAD0000DEAD0000UL)
> > 
> > s/POINTER_POISON/FUNCTION_POISON/
> 
> Sure, if you think none of the present poisoning values we have is good to
> reuse here.

They maybe could, but I assumed we wanted differentiation between this
usage and ZERO_BLOCK_PTR for example.

Thanks, Roger.