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

[Jan Beulich <jbeulich@xxxxxxxx>]

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