Re: [PATCH 2/2] x86: Remove stubs from asm/pv/domain.h

[Grygorii Strashko <grygorii_strashko@xxxxxxxx>]

On 12.11.25 19:21, Alejandro Vallejo wrote:
> On Wed Nov 12, 2025 at 4:56 PM CET, Grygorii Strashko wrote:
> > On 12.11.25 17:22, Alejandro Vallejo wrote:
> > > They are unnecessary. The only two cases with link-time failures are
> > > fallback else branches that can just as well be adjusted with explicit
> > > IS_ENABLED(CONFIG_PV). HVM has no equivalent stubs and there's no reason
> > > to keep the asymmetry.
> > >
> > > No functional change.
> > >
> > > Signed-off-by: Alejandro Vallejo <alejandro.garciavallejo@xxxxxxx>
> > > ---
> > > I'd rather remove the "rc = -EOPNOTSUPP" branch altogether, or replace
> > > it with ASSERT_UNREACHABLE(), but I kept it here to preserve prior behaviour.
> > >
> > > Thoughts?
> > >
> > > ---
> > >    xen/arch/x86/domain.c                | 10 ++++++----
> > >    xen/arch/x86/include/asm/pv/domain.h | 25 -------------------------
> > >    2 files changed, 6 insertions(+), 29 deletions(-)
> > >
> > > diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> > > index 19fd86ce88..0977d9323d 100644
> > > --- a/xen/arch/x86/domain.c
> > > +++ b/xen/arch/x86/domain.c
> > > @@ -571,15 +571,17 @@ int arch_vcpu_create(struct vcpu *v)
> > >    
> > >        if ( is_hvm_domain(d) )
> > >            rc = hvm_vcpu_initialise(v);
> > > -    else if ( !is_idle_domain(d) )
> > > -        rc = pv_vcpu_initialise(v);
> > > -    else
> > > +    else if ( is_idle_domain(d) )
> > >        {
> > The is_idle_domain() wants to go first here, i think.
> > [1] https://patchwork.kernel.org/comment/26646246/
> I'm not sure I follow. I inverted the condition in order for the PV case to
> become the fallback "else" and be thus eliminable through DCE.
>
> > >            /* Idle domain */
> > >            v->arch.cr3 = __pa(idle_pg_table);
> > >            rc = 0;
> > >            v->arch.msrs = ZERO_BLOCK_PTR; /* Catch stray misuses */
> > >        }
> > > +    else if ( IS_ENABLED(CONFIG_PV) )
> > > +        rc = pv_vcpu_initialise(v);
> > > +    else
> > > +        rc = -EOPNOTSUPP;
> > >    
> > >        if ( rc )
> > >            goto fail;
> > Actually, if you are here and have time and inspiration :)
> I may find at least one of those two :)
>
> > - if ( is_idle_domain(d) ) staff can be consolidated at the
> >     beginning of arch_vcpu_create() which will make it much more readable and 
> > simple.
> Good point
>
> Though it's subtle because the idle domain has wacky matching semantics
> and there's many exit paths. Let me see if I can make a clearer version
> with that sort of consolidation that is not a functional change.
>
> > - mapcache_vcpu_init() is PV only (->pv_vcpu_initialise()?)
> This I shouldn't do. It's PV-only only temporarily. The directmap removal series
> (in-flight for a while now, but ought to make it upstream sooner or later) makes
> it also usable for HVM when the directmap is sparsely populated. I'd rather not
> generate more churn than I have to for that series.
It's all up to you.

--
Best regards,
-grygorii