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

["Roger Pau Monné" <roger.pau@citrix.com>]

On Mon, Feb 09, 2026 at 05:20:40PM +0100, Alejandro Vallejo wrote:
> On Mon Feb 9, 2026 at 4:55 PM CET, 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@amd.com>
> >> >> ---
> >> >> 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@amd.com
> >> >> 
> >> >> ... 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;
> >
> 
> none.c would need to stay in PV for what I proposed. For what you proposed PV
> would return 0, but all the hooks would be gone. And I really don't know if
> they would be triggered or not.

Oh, OK, so that would already diverge form the current patch - in this
proposal you completely remove none.c.  I see at least two of the
hooks look to be reachable from PV seeing the ASSERTs in there, or at
least that was the expectation.

> >> 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.
> 
> I'm not sure _I_ follow exactly what I mean. Part of the confusion is the
> overloaded use of "shadow" to mean "shadow paging" and "fault-and-track"
> of logdirty behaviour.
> 
> > Some rants below which
> > might or might not be along the lines of what you suggest.
> 
> Thanks.
> 
> >
> > PV needs shadow for migration.
> 
> shadow in the sense of shadow paging? So PV-only + !SHADOW means migrations are
> impossible? Why can't Xen operate on the PV pagetables rather than using shadow?

At the time it was likely seen as easier to re-use the shadow code
rather than add more complexity to the PV one?

> > HVM can use shadow or HAP, and our default is HAP.
> 
> For regular use or migrations?

HAP doesn't need shadow to perform migrations, so in HVM there's no
dependency between HAP and shadow.

> > 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.
> 
> What do you mean by "enabling shadow"? compiling it in? Running HVM without HAP?

I meant not compiling it in.  If the only PV guest running on the
system is dom0, and the domUs are all HVM + HAP, you don't need
shadow for neither of them.  PV dom0 will never migrate, and HVM + HAP
domUs won't use the shadow code for migration (or anything).

Thanks, Roger.