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 <[email protected]>
>> ---
>> 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/[email protected]
>>
>> ... 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
