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

[Alejandro Vallejo <alejandro.garciavallejo@xxxxxxx>]

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.

Cheers,
Alejandro