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

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

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;

> And gate none.c on PV && !SHADOW_PAGING, which seems to be the only use.
> 
> It's a lot easier to see the safety on the HVM-only case, particularly with...
> 
> > is compiled out, and the toolstack has not specified the HAP flag at
> > domain creation you will end up with a domain that doesn't have the
> > paging operations initialized as paging_domain_init() would return 0
> > with neither HAP nor shadow having been setup.  That's likely to
> > trigger NULL pointer dereferences inside of Xen.
> >
> > Also, seeing the code in arch_sanitise_domain_config() we possibly
> > want to return an error at that point if toolstack attempts to create
> > an HVM guest without HAP enabled, and shadow is build time disabled.
> > I've sent a patch to that end.
> 
> ... this patch you meantion. Thanks.
> 
> I'm guessing it's still a hot potato in for non-shadow PV, which strongly 
> hints
> at our being better off leaving it in that case. On HVM-only configurations it
> seems rather silly.

I'm not sure I follow exactly what you mean.  Some rants below which
might or might not be along the lines of what you suggest.

PV needs shadow for migration.  HVM can use shadow or HAP, and our
default is HAP.  For HVM only builds it could be possible to not
recommend enabling shadow.  Even for deployments where only dom0 is
using PV mode, it does still make sense to possible recommend not
enabling shadow for attack surface reduction.

Thanks, Roger.