Re: [PATCH] x86/shadow: Delete the none.c dummy file

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, Feb 09, 2026 at 05:04:55PM +0100, Jan Beulich wrote:
> On 09.02.2026 16:55, Roger Pau Monné wrote:
> > On Mon, Feb 09, 2026 at 04:35:04PM +0100, Alejandro Vallejo wrote:
> >> On Mon Feb 9, 2026 at 3:42 PM CET, Roger Pau Monné wrote:
> >>> On Mon, Feb 09, 2026 at 11:41:02AM +0100, Alejandro Vallejo wrote:
> >>>> It only has 2 callers, both of which can be conditionally removed.
> >>>>
> >>>> Signed-off-by: Alejandro Vallejo <alejandro.garciavallejo@xxxxxxx>
> >>>> ---
> >>>> I'd be ok conditionalising the else branch on...
> >>>>
> >>>>     IS_ENABLED(CONFIG_SHADOW_PAGING )|| IS_ENABLED(CONFIG_LOG_DIRTY)
> >>>>
> >>>> logdirty patch: 
> >>>> https://lore.kernel.org/xen-devel/20260209103118.5885-1-alejandro.garciavallejo@xxxxxxx
> >>>>
> >>>> ... to avoid the danger of stale pointers, with required changes 
> >>>> elsewhere so
> >>>> none.c is only compiled out in that case.
> >>>>
> >>>> I'm not sure how much it matters seeing how they are all unreachable.
> >>>> ---
> >>>>  xen/arch/x86/mm/Makefile        |  2 +-
> >>>>  xen/arch/x86/mm/paging.c        |  4 +-
> >>>>  xen/arch/x86/mm/shadow/Makefile |  4 --
> >>>>  xen/arch/x86/mm/shadow/none.c   | 77 ---------------------------------
> >>>>  4 files changed, 3 insertions(+), 84 deletions(-)
> >>>>  delete mode 100644 xen/arch/x86/mm/shadow/none.c
> >>>>
> >>>> diff --git a/xen/arch/x86/mm/Makefile b/xen/arch/x86/mm/Makefile
> >>>> index 960f6e8409..066c4caff3 100644
> >>>> --- a/xen/arch/x86/mm/Makefile
> >>>> +++ b/xen/arch/x86/mm/Makefile
> >>>> @@ -1,4 +1,4 @@
> >>>> -obj-y += shadow/
> >>>> +obj-$(CONFIG_SHADOW_PAGING) += shadow/
> >>>>  obj-$(CONFIG_HVM) += hap/
> >>>>  
> >>>>  obj-$(CONFIG_ALTP2M) += altp2m.o
> >>>> diff --git a/xen/arch/x86/mm/paging.c b/xen/arch/x86/mm/paging.c
> >>>> index 2396f81ad5..5f70254cec 100644
> >>>> --- a/xen/arch/x86/mm/paging.c
> >>>> +++ b/xen/arch/x86/mm/paging.c
> >>>> @@ -634,7 +634,7 @@ int paging_domain_init(struct domain *d)
> >>>>       */
> >>>>      if ( hap_enabled(d) )
> >>>>          hap_domain_init(d);
> >>>> -    else
> >>>> +    else if ( IS_ENABLED(CONFIG_SHADOW_PAGING) )
> >>>>          rc = shadow_domain_init(d);
> >>>
> >>> If you want to go this route you will need to set rc = -EOPNOTSUPP;
> >>> prior to the `if ... else if` on the HVM case.
> >>
> >> Maybe this instead
> >>
> >>     else
> >>         rc = IS_ENABLED(PV) ? shadow_domain_init(d) : -EOPNOTSUPP;
> > 
> > But even for the PV case we cannot call shadow_domain_init() if shadow
> > is compiled out?  I think you want:
> > 
> >     if ( hap_enabled(d) )
> >         hap_domain_init(d);
> >     else if ( IS_ENABLED(CONFIG_SHADOW_PAGING) )
> >         rc = shadow_domain_init(d);
> >     else
> >         rc = is_hvm_domain(d) ? -EOPNOTSUPP : 0;
> 
> Wouldn't this still leave NULL pointers in places where they can be rather
> dangerous with PV guests?

Possibly, I didn't look much further on this patch, as returning rc ==
0 and not having initialized neither HAP nor shadow is likely to
already lead to NULL pointer dereferences.

Regards, Roger.