1. Patchew
  2. Xen
  3. [PATCH 0/2] Code removal for !CONFIG_PV
  4. View patch

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

Alejandro Vallejo posted 2 patches 6 days, 10 hours ago
[PATCH 1/2] x86: Conditionalise PV-only fallback branches on CONFIG_PV
Posted by Alejandro Vallejo 6 days, 10 hours ago 
A couple of branches rely on PV being the "else" branch, making it
be compiled even when PV support is not itself compiled-in.

Add a explicit conditional on CONFIG_PV in those cases to remove the
code in !CONFIG_PV builds.

Not a functional change.

Signed-off-by: Alejandro Vallejo <alejandro.garciavallejo@amd.com>
---
 xen/arch/x86/cpuid.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c
index b63a82dd38..2e24c84708 100644
--- 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) )
         {
             /*
              * MTRR used to unconditionally leak into PV guests.  They cannot
-- 
2.43.0
Re: [PATCH 1/2] x86: Conditionalise PV-only fallback branches on CONFIG_PV
Posted by Jan Beulich 5 days, 14 hours ago 
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
Re: [PATCH 1/2] x86: Conditionalise PV-only fallback branches on CONFIG_PV
Posted by Alejandro Vallejo 5 days, 13 hours ago 
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
Re: [PATCH 1/2] x86: Conditionalise PV-only fallback branches on CONFIG_PV
Posted by Jan Beulich 5 days, 13 hours ago 
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
Re: [PATCH 1/2] x86: Conditionalise PV-only fallback branches on CONFIG_PV
Posted by Teddy Astie 6 days, 10 hours ago 
Le 12/11/2025 à 16:25, Alejandro Vallejo a écrit :
> A couple of branches rely on PV being the "else" branch, making it
> be compiled even when PV support is not itself compiled-in.
> 
> Add a explicit conditional on CONFIG_PV in those cases to remove the
> code in !CONFIG_PV builds.
> 
> Not a functional change.
> 
> Signed-off-by: Alejandro Vallejo <alejandro.garciavallejo@amd.com>
> ---
>   xen/arch/x86/cpuid.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c
> index b63a82dd38..2e24c84708 100644
> --- 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) )
>           {
>               /*
>                * MTRR used to unconditionally leak into PV guests.  They cannot

I don't like having IS_ENABLED(CONFIG_PV) here, but I don't think of a 
better solution as using e.g is_pv_domain isn't really better as it 
would make us evaluate d->options twice.

That said,
Reviewed-by: Teddy Astie <teddy.astie@vates.tech>

Teddy


--
Teddy Astie | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.