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
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);
return rc;
@@ -645,7 +645,7 @@ void paging_vcpu_init(struct vcpu *v)
{
if ( hap_enabled(v->domain) )
hap_vcpu_init(v);
- else
+ else if ( IS_ENABLED(CONFIG_SHADOW_PAGING) )
shadow_vcpu_init(v);
}
diff --git a/xen/arch/x86/mm/shadow/Makefile b/xen/arch/x86/mm/shadow/Makefile
index 3012fa127d..119989ca4d 100644
--- a/xen/arch/x86/mm/shadow/Makefile
+++ b/xen/arch/x86/mm/shadow/Makefile
@@ -1,7 +1,3 @@
-ifeq ($(CONFIG_SHADOW_PAGING),y)
obj-y += common.o set.o
obj-$(CONFIG_HVM) += hvm.o guest_2.o guest_3.o guest_4.o oos.o
obj-$(CONFIG_PV) += pv.o guest_4.o
-else
-obj-y += none.o
-endif
diff --git a/xen/arch/x86/mm/shadow/none.c b/xen/arch/x86/mm/shadow/none.c
deleted file mode 100644
index 2a4005a795..0000000000
--- a/xen/arch/x86/mm/shadow/none.c
+++ /dev/null
@@ -1,77 +0,0 @@
-#include <xen/mm.h>
-#include <asm/shadow.h>
-
-static int cf_check _toggle_log_dirty(struct domain *d)
-{
- ASSERT(is_pv_domain(d));
- return -EOPNOTSUPP;
-}
-
-static void cf_check _clean_dirty_bitmap(struct domain *d)
-{
- ASSERT(is_pv_domain(d));
-}
-
-static void cf_check _update_paging_modes(struct vcpu *v)
-{
- ASSERT_UNREACHABLE();
-}
-
-int shadow_domain_init(struct domain *d)
-{
- /* For HVM set up pointers for safety, then fail. */
- static const struct log_dirty_ops sh_none_ops = {
- .enable = _toggle_log_dirty,
- .disable = _toggle_log_dirty,
- .clean = _clean_dirty_bitmap,
- };
-
- paging_log_dirty_init(d, &sh_none_ops);
-
- d->arch.paging.update_paging_modes = _update_paging_modes;
-
- return is_hvm_domain(d) ? -EOPNOTSUPP : 0;
-}
-
-static int cf_check _page_fault(
- struct vcpu *v, unsigned long va, struct cpu_user_regs *regs)
-{
- ASSERT_UNREACHABLE();
- return 0;
-}
-
-static bool cf_check _invlpg(struct vcpu *v, unsigned long linear)
-{
- ASSERT_UNREACHABLE();
- return true;
-}
-
-#ifdef CONFIG_HVM
-static unsigned long cf_check _gva_to_gfn(
- struct vcpu *v, struct p2m_domain *p2m, unsigned long va, uint32_t *pfec)
-{
- ASSERT_UNREACHABLE();
- return gfn_x(INVALID_GFN);
-}
-#endif
-
-static pagetable_t cf_check _update_cr3(struct vcpu *v, bool noflush)
-{
- ASSERT_UNREACHABLE();
- return pagetable_null();
-}
-
-static const struct paging_mode sh_paging_none = {
- .page_fault = _page_fault,
- .invlpg = _invlpg,
-#ifdef CONFIG_HVM
- .gva_to_gfn = _gva_to_gfn,
-#endif
- .update_cr3 = _update_cr3,
-};
-
-void shadow_vcpu_init(struct vcpu *v)
-{
- ASSERT(is_pv_vcpu(v));
- v->arch.paging.mode = &sh_paging_none;
-}
base-commit: 1ee8b11c1106dca6b8fe0d54c0e79146bb6545f0
--
2.43.0On 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. Otherwise when shadow 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. Thanks, Roger.
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;
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.
Cheers,
Alejandro
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;
> 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.
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. >> 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? > HVM can use shadow or HAP, and our default is HAP. For regular use or migrations? > 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? Cheers, Alejandro
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.
On 09.02.2026 17:20, 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: >>>> 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. There's no such overload, I don't think. Shadow paging is _needed_ for certain operations. E.g. to put a PV guest in log-dirty mode. >> 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? > >> HVM can use shadow or HAP, and our default is HAP. > > For regular use or migrations? HVM guests always have paging enabled - either HAP or shadow. Hence this distinction makes sense only for PV. However, the answer is still: Both. Because besides for log-dirty tracking, shadow mode is also used there to mitigate L1TF for guests not doing so on their own. Jan
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@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; Wouldn't this still leave NULL pointers in places where they can be rather dangerous with PV guests? Jan
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@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; > > 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.
On 09.02.2026 11:41, 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 I understand this remark. Is this about something in the other
patch (which I haven't looked at yet), or ...
> --- 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);
>
> return rc;
> @@ -645,7 +645,7 @@ void paging_vcpu_init(struct vcpu *v)
> {
> if ( hap_enabled(v->domain) )
> hap_vcpu_init(v);
> - else
> + else if ( IS_ENABLED(CONFIG_SHADOW_PAGING) )
> shadow_vcpu_init(v);
> }
... these two hunks? In this latter case, I don't think the bigger conditional
would be correct.
> --- a/xen/arch/x86/mm/shadow/none.c
> +++ /dev/null
> @@ -1,77 +0,0 @@
> -#include <xen/mm.h>
> -#include <asm/shadow.h>
> -
> -static int cf_check _toggle_log_dirty(struct domain *d)
> -{
> - ASSERT(is_pv_domain(d));
> - return -EOPNOTSUPP;
> -}
> -
> -static void cf_check _clean_dirty_bitmap(struct domain *d)
> -{
> - ASSERT(is_pv_domain(d));
> -}
> -
> -static void cf_check _update_paging_modes(struct vcpu *v)
> -{
> - ASSERT_UNREACHABLE();
> -}
> -
> -int shadow_domain_init(struct domain *d)
> -{
> - /* For HVM set up pointers for safety, then fail. */
> - static const struct log_dirty_ops sh_none_ops = {
> - .enable = _toggle_log_dirty,
> - .disable = _toggle_log_dirty,
> - .clean = _clean_dirty_bitmap,
> - };
> -
> - paging_log_dirty_init(d, &sh_none_ops);
How do you avoid d->arch.paging.log_dirty.ops remaining NULL with this
removed?
> - d->arch.paging.update_paging_modes = _update_paging_modes;
Same question for this function pointer.
> - return is_hvm_domain(d) ? -EOPNOTSUPP : 0;
> -}
> -
> -static int cf_check _page_fault(
> - struct vcpu *v, unsigned long va, struct cpu_user_regs *regs)
> -{
> - ASSERT_UNREACHABLE();
> - return 0;
> -}
> -
> -static bool cf_check _invlpg(struct vcpu *v, unsigned long linear)
> -{
> - ASSERT_UNREACHABLE();
> - return true;
> -}
> -
> -#ifdef CONFIG_HVM
> -static unsigned long cf_check _gva_to_gfn(
> - struct vcpu *v, struct p2m_domain *p2m, unsigned long va, uint32_t *pfec)
> -{
> - ASSERT_UNREACHABLE();
> - return gfn_x(INVALID_GFN);
> -}
> -#endif
> -
> -static pagetable_t cf_check _update_cr3(struct vcpu *v, bool noflush)
> -{
> - ASSERT_UNREACHABLE();
> - return pagetable_null();
> -}
> -
> -static const struct paging_mode sh_paging_none = {
> - .page_fault = _page_fault,
> - .invlpg = _invlpg,
> -#ifdef CONFIG_HVM
> - .gva_to_gfn = _gva_to_gfn,
> -#endif
> - .update_cr3 = _update_cr3,
> -};
> -
> -void shadow_vcpu_init(struct vcpu *v)
> -{
> - ASSERT(is_pv_vcpu(v));
> - v->arch.paging.mode = &sh_paging_none;
And the same question yet again for this pointer.
Jan
On Mon Feb 9, 2026 at 3:36 PM CET, Jan Beulich wrote:
> On 09.02.2026 11:41, 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 I understand this remark. Is this about something in the other
> patch (which I haven't looked at yet), or ...
>
>> --- 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);
>>
>> return rc;
>> @@ -645,7 +645,7 @@ void paging_vcpu_init(struct vcpu *v)
>> {
>> if ( hap_enabled(v->domain) )
>> hap_vcpu_init(v);
>> - else
>> + else if ( IS_ENABLED(CONFIG_SHADOW_PAGING) )
>> shadow_vcpu_init(v);
>> }
>
> ... these two hunks? In this latter case, I don't think the bigger conditional
> would be correct.
It'd be about these hunks and the inclusion condition for shadow/. I suggest that
because...
>
>> --- a/xen/arch/x86/mm/shadow/none.c
>> +++ /dev/null
>> @@ -1,77 +0,0 @@
>> -#include <xen/mm.h>
>> -#include <asm/shadow.h>
>> -
>> -static int cf_check _toggle_log_dirty(struct domain *d)
>> -{
>> - ASSERT(is_pv_domain(d));
>> - return -EOPNOTSUPP;
>> -}
>> -
>> -static void cf_check _clean_dirty_bitmap(struct domain *d)
>> -{
>> - ASSERT(is_pv_domain(d));
>> -}
>> -
>> -static void cf_check _update_paging_modes(struct vcpu *v)
>> -{
>> - ASSERT_UNREACHABLE();
>> -}
>> -
>> -int shadow_domain_init(struct domain *d)
>> -{
>> - /* For HVM set up pointers for safety, then fail. */
>> - static const struct log_dirty_ops sh_none_ops = {
>> - .enable = _toggle_log_dirty,
>> - .disable = _toggle_log_dirty,
>> - .clean = _clean_dirty_bitmap,
>> - };
>> -
>> - paging_log_dirty_init(d, &sh_none_ops);
>
> How do you avoid d->arch.paging.log_dirty.ops remaining NULL with this
> removed?
... as you point out, the ops don't get initialised. Adding the log-dirty
condition ensures there's no uninitialised ops (even when unreachable).
>
>> - d->arch.paging.update_paging_modes = _update_paging_modes;
>
> Same question for this function pointer.
>
>> - return is_hvm_domain(d) ? -EOPNOTSUPP : 0;
>> -}
Oh. This was a hard miss, true that.
>> -
>> -static int cf_check _page_fault(
>> - struct vcpu *v, unsigned long va, struct cpu_user_regs *regs)
>> -{
>> - ASSERT_UNREACHABLE();
>> - return 0;
>> -}
>> -
>> -static bool cf_check _invlpg(struct vcpu *v, unsigned long linear)
>> -{
>> - ASSERT_UNREACHABLE();
>> - return true;
>> -}
>> -
>> -#ifdef CONFIG_HVM
>> -static unsigned long cf_check _gva_to_gfn(
>> - struct vcpu *v, struct p2m_domain *p2m, unsigned long va, uint32_t *pfec)
>> -{
>> - ASSERT_UNREACHABLE();
>> - return gfn_x(INVALID_GFN);
>> -}
>> -#endif
>> -
>> -static pagetable_t cf_check _update_cr3(struct vcpu *v, bool noflush)
>> -{
>> - ASSERT_UNREACHABLE();
>> - return pagetable_null();
>> -}
>> -
>> -static const struct paging_mode sh_paging_none = {
>> - .page_fault = _page_fault,
>> - .invlpg = _invlpg,
>> -#ifdef CONFIG_HVM
>> - .gva_to_gfn = _gva_to_gfn,
>> -#endif
>> - .update_cr3 = _update_cr3,
>> -};
>> -
>> -void shadow_vcpu_init(struct vcpu *v)
>> -{
>> - ASSERT(is_pv_vcpu(v));
>> - v->arch.paging.mode = &sh_paging_none;
>
> And the same question yet again for this pointer.
>
> Jan
However, on the whole. Under what circumstances are these handlers invoked?
They are only compiled in for !CONFIG_SHADOW. But these are only applied with
HAP disabled. Are they for PV or something?
Cheers,
Alejandro
On 09.02.2026 16:06, Alejandro Vallejo wrote:
> On Mon Feb 9, 2026 at 3:36 PM CET, Jan Beulich wrote:
>> On 09.02.2026 11:41, 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 I understand this remark. Is this about something in the other
>> patch (which I haven't looked at yet), or ...
>>
>>> --- 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);
>>>
>>> return rc;
>>> @@ -645,7 +645,7 @@ void paging_vcpu_init(struct vcpu *v)
>>> {
>>> if ( hap_enabled(v->domain) )
>>> hap_vcpu_init(v);
>>> - else
>>> + else if ( IS_ENABLED(CONFIG_SHADOW_PAGING) )
>>> shadow_vcpu_init(v);
>>> }
>>
>> ... these two hunks? In this latter case, I don't think the bigger conditional
>> would be correct.
>
> It'd be about these hunks and the inclusion condition for shadow/. I suggest that
> because...
>
>>
>>> --- a/xen/arch/x86/mm/shadow/none.c
>>> +++ /dev/null
>>> @@ -1,77 +0,0 @@
>>> -#include <xen/mm.h>
>>> -#include <asm/shadow.h>
>>> -
>>> -static int cf_check _toggle_log_dirty(struct domain *d)
>>> -{
>>> - ASSERT(is_pv_domain(d));
>>> - return -EOPNOTSUPP;
>>> -}
>>> -
>>> -static void cf_check _clean_dirty_bitmap(struct domain *d)
>>> -{
>>> - ASSERT(is_pv_domain(d));
>>> -}
>>> -
>>> -static void cf_check _update_paging_modes(struct vcpu *v)
>>> -{
>>> - ASSERT_UNREACHABLE();
>>> -}
>>> -
>>> -int shadow_domain_init(struct domain *d)
>>> -{
>>> - /* For HVM set up pointers for safety, then fail. */
>>> - static const struct log_dirty_ops sh_none_ops = {
>>> - .enable = _toggle_log_dirty,
>>> - .disable = _toggle_log_dirty,
>>> - .clean = _clean_dirty_bitmap,
>>> - };
>>> -
>>> - paging_log_dirty_init(d, &sh_none_ops);
>>
>> How do you avoid d->arch.paging.log_dirty.ops remaining NULL with this
>> removed?
>
> ... as you point out, the ops don't get initialised. Adding the log-dirty
> condition ensures there's no uninitialised ops (even when unreachable).
IOW the remark is kind of (but not quite) making that other change a prereq?
(See my remark there as to typing together SHADOW_PAGING and LOG_DIRTY.)
>>> - d->arch.paging.update_paging_modes = _update_paging_modes;
>>
>> Same question for this function pointer.
>>
>>> - return is_hvm_domain(d) ? -EOPNOTSUPP : 0;
>>> -}
>
> Oh. This was a hard miss, true that.
>
>>> -
>>> -static int cf_check _page_fault(
>>> - struct vcpu *v, unsigned long va, struct cpu_user_regs *regs)
>>> -{
>>> - ASSERT_UNREACHABLE();
>>> - return 0;
>>> -}
>>> -
>>> -static bool cf_check _invlpg(struct vcpu *v, unsigned long linear)
>>> -{
>>> - ASSERT_UNREACHABLE();
>>> - return true;
>>> -}
>>> -
>>> -#ifdef CONFIG_HVM
>>> -static unsigned long cf_check _gva_to_gfn(
>>> - struct vcpu *v, struct p2m_domain *p2m, unsigned long va, uint32_t *pfec)
>>> -{
>>> - ASSERT_UNREACHABLE();
>>> - return gfn_x(INVALID_GFN);
>>> -}
>>> -#endif
>>> -
>>> -static pagetable_t cf_check _update_cr3(struct vcpu *v, bool noflush)
>>> -{
>>> - ASSERT_UNREACHABLE();
>>> - return pagetable_null();
>>> -}
>>> -
>>> -static const struct paging_mode sh_paging_none = {
>>> - .page_fault = _page_fault,
>>> - .invlpg = _invlpg,
>>> -#ifdef CONFIG_HVM
>>> - .gva_to_gfn = _gva_to_gfn,
>>> -#endif
>>> - .update_cr3 = _update_cr3,
>>> -};
>>> -
>>> -void shadow_vcpu_init(struct vcpu *v)
>>> -{
>>> - ASSERT(is_pv_vcpu(v));
>>> - v->arch.paging.mode = &sh_paging_none;
>>
>> And the same question yet again for this pointer.
>
> However, on the whole. Under what circumstances are these handlers invoked?
>
> They are only compiled in for !CONFIG_SHADOW. But these are only applied with
> HAP disabled. Are they for PV or something?
The .gva_to_gfn hook is clearly HVM-only. We still want to be sure to have no
NULL pointers around that we could stumble across, especially as long as PV=y.
Jan
© 2016 - 2026 Red Hat, Inc.