[PATCH] x86/CPUID: don't shrink hypervisor leaves

[PATCH] x86/CPUID: don't shrink hypervisor leaves

[Jan Beulich]

This is a partial revert of 540d911c2813 ("x86/CPUID: shrink
max_{,sub}leaf fields according to actual leaf contents"). Andrew points
out that XXX.

Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Obviously the XXX wants filling in. So far I did not really understand
what bad consequences there might be, but I can agree with the undoing
of this part of the original change along the lines of why the Viridian
side adjustment was also requested to be dropped (before the patch went
in).

--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -964,15 +964,13 @@ void cpuid_hypervisor_leaves(const struc
     uint32_t base = is_viridian_domain(d) ? 0x40000100 : 0x40000000;
     uint32_t idx  = leaf - base;
     unsigned int limit = is_viridian_domain(d) ? p->hv2_limit : p->hv_limit;
-    unsigned int dflt = is_pv_domain(d) ? XEN_CPUID_MAX_PV_NUM_LEAVES
-                                        : XEN_CPUID_MAX_HVM_NUM_LEAVES;
 
     if ( limit == 0 )
         /* Default number of leaves */
-        limit = dflt;
+        limit = XEN_CPUID_MAX_NUM_LEAVES;
     else
         /* Clamp toolstack value between 2 and MAX_NUM_LEAVES. */
-        limit = min(max(limit, 2u), dflt);
+        limit = min(max(limit, 2u), XEN_CPUID_MAX_NUM_LEAVES + 0u);
 
     if ( idx > limit )
         return;
--- a/xen/include/public/arch-x86/cpuid.h
+++ b/xen/include/public/arch-x86/cpuid.h
@@ -113,10 +113,6 @@
 /* Max. address width in bits taking memory hotplug into account. */
 #define XEN_CPUID_MACHINE_ADDRESS_WIDTH_MASK (0xffu << 0)
 
-#define XEN_CPUID_MAX_PV_NUM_LEAVES 5
-#define XEN_CPUID_MAX_HVM_NUM_LEAVES 4
-#define XEN_CPUID_MAX_NUM_LEAVES \
-    (XEN_CPUID_MAX_PV_NUM_LEAVES > XEN_CPUID_MAX_HVM_NUM_LEAVES ? \
-     XEN_CPUID_MAX_PV_NUM_LEAVES : XEN_CPUID_MAX_HVM_NUM_LEAVES)
+#define XEN_CPUID_MAX_NUM_LEAVES 5
 
 #endif /* __XEN_PUBLIC_ARCH_X86_CPUID_H__ */

Ping: [PATCH] x86/CPUID: don't shrink hypervisor leaves

[Jan Beulich]

Andrew,

On 07.05.2021 10:40, Jan Beulich wrote:
> This is a partial revert of 540d911c2813 ("x86/CPUID: shrink
> max_{,sub}leaf fields according to actual leaf contents"). Andrew points
> out that XXX.

if you still think the original change was wrong, would you please take
the time to fill in the XXX above. It was you who asked for the revert,
so I hope you can explain the reasons (I'm sorry for not being able to
deduce these from your [informal iirc] revert request), and I expect
you can find this much of time when I've already taken care of
everything else. If I don't hear back within a couple of days, I'll
assume you've changed your mind, and I'd then drop this patch.

Thanks, Jan

> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> Obviously the XXX wants filling in. So far I did not really understand
> what bad consequences there might be, but I can agree with the undoing
> of this part of the original change along the lines of why the Viridian
> side adjustment was also requested to be dropped (before the patch went
> in).
> 
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -964,15 +964,13 @@ void cpuid_hypervisor_leaves(const struc
>      uint32_t base = is_viridian_domain(d) ? 0x40000100 : 0x40000000;
>      uint32_t idx  = leaf - base;
>      unsigned int limit = is_viridian_domain(d) ? p->hv2_limit : p->hv_limit;
> -    unsigned int dflt = is_pv_domain(d) ? XEN_CPUID_MAX_PV_NUM_LEAVES
> -                                        : XEN_CPUID_MAX_HVM_NUM_LEAVES;
>  
>      if ( limit == 0 )
>          /* Default number of leaves */
> -        limit = dflt;
> +        limit = XEN_CPUID_MAX_NUM_LEAVES;
>      else
>          /* Clamp toolstack value between 2 and MAX_NUM_LEAVES. */
> -        limit = min(max(limit, 2u), dflt);
> +        limit = min(max(limit, 2u), XEN_CPUID_MAX_NUM_LEAVES + 0u);
>  
>      if ( idx > limit )
>          return;
> --- a/xen/include/public/arch-x86/cpuid.h
> +++ b/xen/include/public/arch-x86/cpuid.h
> @@ -113,10 +113,6 @@
>  /* Max. address width in bits taking memory hotplug into account. */
>  #define XEN_CPUID_MACHINE_ADDRESS_WIDTH_MASK (0xffu << 0)
>  
> -#define XEN_CPUID_MAX_PV_NUM_LEAVES 5
> -#define XEN_CPUID_MAX_HVM_NUM_LEAVES 4
> -#define XEN_CPUID_MAX_NUM_LEAVES \
> -    (XEN_CPUID_MAX_PV_NUM_LEAVES > XEN_CPUID_MAX_HVM_NUM_LEAVES ? \
> -     XEN_CPUID_MAX_PV_NUM_LEAVES : XEN_CPUID_MAX_HVM_NUM_LEAVES)
> +#define XEN_CPUID_MAX_NUM_LEAVES 5
>  
>  #endif /* __XEN_PUBLIC_ARCH_X86_CPUID_H__ */
> 

Ping²: [PATCH] x86/CPUID: don't shrink hypervisor leaves

[Jan Beulich]

On 06.07.2021 09:48, Jan Beulich wrote:
> Andrew,
> 
> On 07.05.2021 10:40, Jan Beulich wrote:
>> This is a partial revert of 540d911c2813 ("x86/CPUID: shrink
>> max_{,sub}leaf fields according to actual leaf contents"). Andrew points
>> out that XXX.
> 
> if you still think the original change was wrong, would you please take
> the time to fill in the XXX above. It was you who asked for the revert,
> so I hope you can explain the reasons (I'm sorry for not being able to
> deduce these from your [informal iirc] revert request), and I expect
> you can find this much of time when I've already taken care of
> everything else. If I don't hear back within a couple of days, I'll
> assume you've changed your mind, and I'd then drop this patch.
> 
> Thanks, Jan

I notice this still is pending, despite what I've said above, since it
seems unlikely to me that you've changed your mind and didn't care about
saying so. Could you please clarify things one way or another?

Jan

>> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> Obviously the XXX wants filling in. So far I did not really understand
>> what bad consequences there might be, but I can agree with the undoing
>> of this part of the original change along the lines of why the Viridian
>> side adjustment was also requested to be dropped (before the patch went
>> in).
>>
>> --- a/xen/arch/x86/traps.c
>> +++ b/xen/arch/x86/traps.c
>> @@ -964,15 +964,13 @@ void cpuid_hypervisor_leaves(const struc
>>      uint32_t base = is_viridian_domain(d) ? 0x40000100 : 0x40000000;
>>      uint32_t idx  = leaf - base;
>>      unsigned int limit = is_viridian_domain(d) ? p->hv2_limit : p->hv_limit;
>> -    unsigned int dflt = is_pv_domain(d) ? XEN_CPUID_MAX_PV_NUM_LEAVES
>> -                                        : XEN_CPUID_MAX_HVM_NUM_LEAVES;
>>  
>>      if ( limit == 0 )
>>          /* Default number of leaves */
>> -        limit = dflt;
>> +        limit = XEN_CPUID_MAX_NUM_LEAVES;
>>      else
>>          /* Clamp toolstack value between 2 and MAX_NUM_LEAVES. */
>> -        limit = min(max(limit, 2u), dflt);
>> +        limit = min(max(limit, 2u), XEN_CPUID_MAX_NUM_LEAVES + 0u);
>>  
>>      if ( idx > limit )
>>          return;
>> --- a/xen/include/public/arch-x86/cpuid.h
>> +++ b/xen/include/public/arch-x86/cpuid.h
>> @@ -113,10 +113,6 @@
>>  /* Max. address width in bits taking memory hotplug into account. */
>>  #define XEN_CPUID_MACHINE_ADDRESS_WIDTH_MASK (0xffu << 0)
>>  
>> -#define XEN_CPUID_MAX_PV_NUM_LEAVES 5
>> -#define XEN_CPUID_MAX_HVM_NUM_LEAVES 4
>> -#define XEN_CPUID_MAX_NUM_LEAVES \
>> -    (XEN_CPUID_MAX_PV_NUM_LEAVES > XEN_CPUID_MAX_HVM_NUM_LEAVES ? \
>> -     XEN_CPUID_MAX_PV_NUM_LEAVES : XEN_CPUID_MAX_HVM_NUM_LEAVES)
>> +#define XEN_CPUID_MAX_NUM_LEAVES 5
>>  
>>  #endif /* __XEN_PUBLIC_ARCH_X86_CPUID_H__ */
>>
> 

Re: [PATCH] x86/CPUID: don't shrink hypervisor leaves

[Roger Pau Monné]

On Fri, May 07, 2021 at 10:40:34AM +0200, Jan Beulich wrote:
> This is a partial revert of 540d911c2813 ("x86/CPUID: shrink
> max_{,sub}leaf fields according to actual leaf contents"). Andrew points
> out that XXX.
> 
> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> Obviously the XXX wants filling in. So far I did not really understand
> what bad consequences there might be, but I can agree with the undoing
> of this part of the original change along the lines of why the Viridian
> side adjustment was also requested to be dropped (before the patch went
> in).

I have to admit I'm confused about this. Here the maximum leaf
reported only changes between PV and HVM, but never as a result of the
features exposed, which I think should be fine?

Ie: I recall the problem being the maximum leaf possibly shrinking
when migrating, but that's not the case.

Thanks, Roger.

Re: [PATCH] x86/CPUID: don't shrink hypervisor leaves

[Jan Beulich]

On 18.10.2021 13:45, Roger Pau Monné wrote:
> On Fri, May 07, 2021 at 10:40:34AM +0200, Jan Beulich wrote:
>> This is a partial revert of 540d911c2813 ("x86/CPUID: shrink
>> max_{,sub}leaf fields according to actual leaf contents"). Andrew points
>> out that XXX.
>>
>> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> Obviously the XXX wants filling in. So far I did not really understand
>> what bad consequences there might be, but I can agree with the undoing
>> of this part of the original change along the lines of why the Viridian
>> side adjustment was also requested to be dropped (before the patch went
>> in).
> 
> I have to admit I'm confused about this. Here the maximum leaf
> reported only changes between PV and HVM, but never as a result of the
> features exposed, which I think should be fine?
> 
> Ie: I recall the problem being the maximum leaf possibly shrinking
> when migrating, but that's not the case.

I assume that's a question to Andrew? Because if I knew the answer,
I could probably have written a proper / complete description.

Jan