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.
> --- 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
