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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 13.11.2025 13:20, Alejandro Vallejo wrote:
> 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.
> 
> Should I understand that as an Acked-by?

You may, yes (implicitly).

Jan

> 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