In order to be compatible with previous Xen versions, and not change
max hypervisor leaf as a result of a migration, keep the clamping of
the maximum leaf value provided to XEN_CPUID_MAX_NUM_LEAVES, instead
of doing it based on the domain type. Also set the default maximum
leaf without taking the domain type into account. The maximum
hypervisor leaf is not migrated, so we need the default to not regress
beyond what might already be reported to a guest by existing Xen
versions.

This is a partial revert of 540d911c28 and restores the previous
behaviour and assures that HVM guests won't see it's maximum
hypervisor leaf reduced from 5 to 4 as a result of a migration.

Fixes: 540d911c28 ('x86/CPUID: shrink max_{,sub}leaf fields according to actual leaf contents')
Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Ian Jackson <iwj@xenproject.org>
---
Regarding release risks:

This is a partial revert of a commit.  The main issues could be that a
partial revert could break the build or leave the remaining code in a
non-working condition.

Breaking the build will be easily discovered by our automated testing,
while leaving the remaining code in a broken state is unlikely, as the
chunks reverted are isolated from the rest of the change in
540d911c28.
---
 xen/arch/x86/traps.c                | 6 ++----
 xen/include/public/arch-x86/cpuid.h | 6 +-----
 2 files changed, 3 insertions(+), 9 deletions(-)

diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index a1c2adb7ad..79fd276a41 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -1086,15 +1086,13 @@ void cpuid_hypervisor_leaves(const struct vcpu *v, uint32_t leaf,
     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;
diff --git a/xen/include/public/arch-x86/cpuid.h b/xen/include/public/arch-x86/cpuid.h
index 00926b1fef..ce46305bee 100644
--- 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__ */
-- 
2.33.0

On 27/10/2021 15:00, Roger Pau Monne wrote:
> In order to be compatible with previous Xen versions, and not change
> max hypervisor leaf as a result of a migration, keep the clamping of
> the maximum leaf value provided to XEN_CPUID_MAX_NUM_LEAVES, instead
> of doing it based on the domain type. Also set the default maximum
> leaf without taking the domain type into account. The maximum
> hypervisor leaf is not migrated, so we need the default to not regress
> beyond what might already be reported to a guest by existing Xen
> versions.
>
> This is a partial revert of 540d911c28 and restores the previous
> behaviour and assures that HVM guests won't see it's maximum
> hypervisor leaf reduced from 5 to 4 as a result of a migration.
>
> Fixes: 540d911c28 ('x86/CPUID: shrink max_{,sub}leaf fields according to actual leaf contents')
> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

> ---
> Cc: Ian Jackson <iwj@xenproject.org>
> ---
> Regarding release risks:
>
> This is a partial revert of a commit.  The main issues could be that a
> partial revert could break the build or leave the remaining code in a
> non-working condition.
>
> Breaking the build will be easily discovered by our automated testing,
> while leaving the remaining code in a broken state is unlikely, as the
> chunks reverted are isolated from the rest of the change in
> 540d911c28.

This fixes a regression vs 4.15.  Furthermore, the changes to the
hypervisor leaves don't even interact with the rest of the patch.

Failure to compile is about the only risk, and this is easy to prove one
way or another.

~Andrew

Roger Pau Monne writes ("[PATCH] x86/cpuid: prevent decreasing of hypervisor max leaf on migration"):
> In order to be compatible with previous Xen versions, and not change
> max hypervisor leaf as a result of a migration, keep the clamping of
> the maximum leaf value provided to XEN_CPUID_MAX_NUM_LEAVES, instead
> of doing it based on the domain type. Also set the default maximum
> leaf without taking the domain type into account. The maximum
> hypervisor leaf is not migrated, so we need the default to not regress
> beyond what might already be reported to a guest by existing Xen
> versions.
> 
> This is a partial revert of 540d911c28 and restores the previous
> behaviour and assures that HVM guests won't see it's maximum
> hypervisor leaf reduced from 5 to 4 as a result of a migration.
> 
> Fixes: 540d911c28 ('x86/CPUID: shrink max_{,sub}leaf fields according to actual leaf contents')
> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> Cc: Ian Jackson <iwj@xenproject.org>
> ---
> Regarding release risks:
> 
> This is a partial revert of a commit.  The main issues could be that a
> partial revert could break the build or leave the remaining code in a
> non-working condition.
> 
> Breaking the build will be easily discovered by our automated testing,
> while leaving the remaining code in a broken state is unlikely, as the
> chunks reverted are isolated from the rest of the change in
> 540d911c28.

Release-Acked-by: Ian Jackson <iwj@xenproject.org>

Thanks

On 27.10.2021 16:00, Roger Pau Monne wrote:
> In order to be compatible with previous Xen versions, and not change
> max hypervisor leaf as a result of a migration, keep the clamping of
> the maximum leaf value provided to XEN_CPUID_MAX_NUM_LEAVES, instead
> of doing it based on the domain type. Also set the default maximum
> leaf without taking the domain type into account. The maximum
> hypervisor leaf is not migrated, so we need the default to not regress
> beyond what might already be reported to a guest by existing Xen
> versions.

While this is the missing description to the patch I had submitted
back in May upon Andrew's request, I have to admit that I don't
consider this a satisfactory explanation. Shouldn't hypervisor
leaves undergo similar leveling as other leaves? I.e. upon
migration leaves or individual bits should neither disappear nor
appear?

I continue to consider it at least suspicious that HVM guests get
5 leaves reported when only 4 are really meaningful to them. I
see this has gone in, so I'm likely to trip up on this again in
the future. Might result in the same patch again then if I don't
end up doing archeology at that point ...

Jan

> This is a partial revert of 540d911c28 and restores the previous
> behaviour and assures that HVM guests won't see it's maximum
> hypervisor leaf reduced from 5 to 4 as a result of a migration.
> 
> Fixes: 540d911c28 ('x86/CPUID: shrink max_{,sub}leaf fields according to actual leaf contents')
> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> Cc: Ian Jackson <iwj@xenproject.org>
> ---
> Regarding release risks:
> 
> This is a partial revert of a commit.  The main issues could be that a
> partial revert could break the build or leave the remaining code in a
> non-working condition.
> 
> Breaking the build will be easily discovered by our automated testing,
> while leaving the remaining code in a broken state is unlikely, as the
> chunks reverted are isolated from the rest of the change in
> 540d911c28.
> ---
>  xen/arch/x86/traps.c                | 6 ++----
>  xen/include/public/arch-x86/cpuid.h | 6 +-----
>  2 files changed, 3 insertions(+), 9 deletions(-)
> 
> diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
> index a1c2adb7ad..79fd276a41 100644
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -1086,15 +1086,13 @@ void cpuid_hypervisor_leaves(const struct vcpu *v, uint32_t leaf,
>      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;
> diff --git a/xen/include/public/arch-x86/cpuid.h b/xen/include/public/arch-x86/cpuid.h
> index 00926b1fef..ce46305bee 100644
> --- 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__ */
> 

On Tue, Nov 02, 2021 at 01:00:07PM +0100, Jan Beulich wrote:
> On 27.10.2021 16:00, Roger Pau Monne wrote:
> > In order to be compatible with previous Xen versions, and not change
> > max hypervisor leaf as a result of a migration, keep the clamping of
> > the maximum leaf value provided to XEN_CPUID_MAX_NUM_LEAVES, instead
> > of doing it based on the domain type. Also set the default maximum
> > leaf without taking the domain type into account. The maximum
> > hypervisor leaf is not migrated, so we need the default to not regress
> > beyond what might already be reported to a guest by existing Xen
> > versions.
> 
> While this is the missing description to the patch I had submitted
> back in May upon Andrew's request, I have to admit that I don't
> consider this a satisfactory explanation. Shouldn't hypervisor
> leaves undergo similar leveling as other leaves? I.e. upon
> migration leaves or individual bits should neither disappear nor
> appear?

Indeed, but hypervisor max leaves is not properly migrated, as
hv{,2}_limit is not set unless explicitly passed by the toolstack.

> I continue to consider it at least suspicious that HVM guests get
> 5 leaves reported when only 4 are really meaningful to them.

That's indeed fine to fix, but we would need to start saving/restoring
hypervisor max leaf unconditionally as part of the cpuid policy (ie:
set hv{,2}_limit at domain create to the default limit), and populate
it with 5 for backwards compatibility on restore if not set.

> I
> see this has gone in, so I'm likely to trip up on this again in
> the future. Might result in the same patch again then if I don't
> end up doing archeology at that point ...

Maybe there's some comment we could add in the code to make this
clearer? I didn't realize either when reviewing as didn't pay close
attention to how hv{,2}_limit is used.

I hope the above comments help clarify why the current behavior was
an issue.

Regards, Roger.