Re: [PATCH 1/2] x86: Conditionalise PV-only fallback branches on CONFIG_PV

[Alejandro Vallejo <alejandro.garciavallejo@xxxxxxx>]

On Thu Nov 13, 2025 at 12:42 PM CET, Jan Beulich wrote:
> On 12.11.2025 16:22, Alejandro Vallejo wrote:
>> --- a/xen/arch/x86/cpuid.c
>> +++ b/xen/arch/x86/cpuid.c
>> @@ -297,7 +297,7 @@ void guest_cpuid(const struct vcpu *v, uint32_t leaf,
>>              if ( v->arch.hvm.guest_cr[4] & X86_CR4_OSXSAVE )
>>                  res->c |= cpufeat_mask(X86_FEATURE_OSXSAVE);
>>          }
>> -        else /* PV domain */
>> +        else if ( IS_ENABLED(CONFIG_PV) )
>>          {
>>              regs = guest_cpu_user_regs();
>>  
>> @@ -509,7 +509,7 @@ void guest_cpuid(const struct vcpu *v, uint32_t leaf,
>>              if ( !hap_enabled(d) && !hvm_pae_enabled(v) )
>>                  res->d &= ~cpufeat_mask(X86_FEATURE_PSE36);
>>          }
>> -        else /* PV domain */
>> +        else if ( IS_ENABLED(CONFIG_PV) )
>
> Maybe better leave the "else"-s as is and, ahead of them, insert
>
>         else if ( !IS_ENABLED(CONFIG_PV) )
>             ASSERT_UNREACHABLE();
>
> Happy to make the adjustment while committing, provided you'd be happy with me
> doing so.
>
> Jan

Should I understand that as an Acked-by?

I think it'd be marginally clearer with the assert added to my code as an else
branch at the end, but either form works. I'm fine with it being committed
in the form I originally sent, what you proposed, or the ASSERT as an else
branch.

They all have the same effect, after all.

Cheers,
Alejandro