[PATCH] tools/cpuid: Plumb nested_virt down into xc_cpuid_apply_policy()

[PATCH] tools/cpuid: Plumb nested_virt down into xc_cpuid_apply_policy()

[Andrew Cooper]

Nested Virt is the final special case in legacy CPUID handling.  Pass the
(poorly named) nested_hvm setting down into xc_cpuid_apply_policy() to break
the semantic dependency on HVM_PARAM_NESTEDHVM.

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Ian Jackson <iwj@xenproject.org>
CC: Wei Liu <wl@xen.org>
CC: Anthony PERARD <anthony.perard@citrix.com>
---
 tools/libs/ctrl/include/xenctrl.h |  4 ++--
 tools/libs/guest/xg_cpuid_x86.c   | 14 +++++---------
 tools/libxl/libxl_cpuid.c         |  3 ++-
 3 files changed, 9 insertions(+), 12 deletions(-)

diff --git a/tools/libs/ctrl/include/xenctrl.h b/tools/libs/ctrl/include/xenctrl.h
index 73e9535fc8..ba70bec9c4 100644
--- a/tools/libs/ctrl/include/xenctrl.h
+++ b/tools/libs/ctrl/include/xenctrl.h
@@ -1826,7 +1826,7 @@ struct xc_xend_cpuid {
  * cases, and the generated policy must be compatible with a 4.13.
  *
  * Either pass a full new @featureset (and @nr_features), or adjust individual
- * features (@pae, @itsc).
+ * features (@pae, @itsc, @nested_virt).
  *
  * Then (optionally) apply legacy XEND overrides (@xend) to the result.
  */
@@ -1834,7 +1834,7 @@ int xc_cpuid_apply_policy(xc_interface *xch,
                           uint32_t domid, bool restore,
                           const uint32_t *featureset,
                           unsigned int nr_features, bool pae, bool itsc,
-                          const struct xc_xend_cpuid *xend);
+                          bool nested_virt, const struct xc_xend_cpuid *xend);
 int xc_mca_op(xc_interface *xch, struct xen_mc *mc);
 int xc_mca_op_inject_v2(xc_interface *xch, unsigned int flags,
                         xc_cpumap_t cpumap, unsigned int nr_cpus);
diff --git a/tools/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c
index dc50106975..aae6931a11 100644
--- a/tools/libs/guest/xg_cpuid_x86.c
+++ b/tools/libs/guest/xg_cpuid_x86.c
@@ -427,7 +427,7 @@ static int xc_cpuid_xend_policy(
 
 int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
                           const uint32_t *featureset, unsigned int nr_features,
-                          bool pae, bool itsc,
+                          bool pae, bool itsc, bool nested_virt,
                           const struct xc_xend_cpuid *xend)
 {
     int rc;
@@ -559,7 +559,11 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
         p->extd.itsc = itsc;
 
         if ( di.hvm )
+        {
             p->basic.pae = pae;
+            p->basic.vmx = nested_virt;
+            p->extd.svm = nested_virt;
+        }
     }
 
     if ( !di.hvm )
@@ -625,14 +629,6 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
             }
             break;
         }
-
-        /*
-         * These settings are necessary to cause earlier HVM_PARAM_NESTEDHVM
-         * to be reflected correctly in CPUID.  Xen will discard these bits if
-         * configuration hasn't been set for the domain.
-         */
-        p->basic.vmx = true;
-        p->extd.svm = true;
     }
 
     rc = x86_cpuid_copy_to_buffer(p, leaves, &nr_leaves);
diff --git a/tools/libxl/libxl_cpuid.c b/tools/libxl/libxl_cpuid.c
index f54eb83a90..08e85dcffb 100644
--- a/tools/libxl/libxl_cpuid.c
+++ b/tools/libxl/libxl_cpuid.c
@@ -422,6 +422,7 @@ void libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid, bool restore,
 {
     bool pae = true;
     bool itsc;
+    bool nested_virt = libxl_defbool_val(info->nested_hvm);
 
     /*
      * For PV guests, PAE is Xen-controlled (it is the 'p' that differentiates
@@ -452,7 +453,7 @@ void libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid, bool restore,
             info->tsc_mode == LIBXL_TSC_MODE_ALWAYS_EMULATE);
 
     xc_cpuid_apply_policy(ctx->xch, domid, restore, NULL, 0,
-                          pae, itsc, info->cpuid);
+                          pae, itsc, nested_virt, info->cpuid);
 }
 
 static const char *input_names[2] = { "leaf", "subleaf" };
-- 
2.11.0

Re: [PATCH] tools/cpuid: Plumb nested_virt down into xc_cpuid_apply_policy()

[Jan Beulich]

On 29.09.2020 15:48, Andrew Cooper wrote:
> --- a/tools/libs/guest/xg_cpuid_x86.c
> +++ b/tools/libs/guest/xg_cpuid_x86.c
> @@ -427,7 +427,7 @@ static int xc_cpuid_xend_policy(
>  
>  int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
>                            const uint32_t *featureset, unsigned int nr_features,
> -                          bool pae, bool itsc,
> +                          bool pae, bool itsc, bool nested_virt,

This increasing number of bools next to each other bears an
increasing risk of callers getting the order wrong. Luckily
there's just one within the tree, so perhaps not an immediate
problem.

> @@ -559,7 +559,11 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
>          p->extd.itsc = itsc;
>  
>          if ( di.hvm )
> +        {
>              p->basic.pae = pae;
> +            p->basic.vmx = nested_virt;
> +            p->extd.svm = nested_virt;
> +        }
>      }
>  
>      if ( !di.hvm )
> @@ -625,14 +629,6 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
>              }
>              break;
>          }
> -
> -        /*
> -         * These settings are necessary to cause earlier HVM_PARAM_NESTEDHVM
> -         * to be reflected correctly in CPUID.  Xen will discard these bits if
> -         * configuration hasn't been set for the domain.
> -         */
> -        p->basic.vmx = true;
> -        p->extd.svm = true;
>      }

While I can see how the first sentence in the comment has become
irrelevant now, what about the second? It's still odd to see both
svm and vmx getting set to identical values. Therefore perhaps
worth to retain a shorter comment there?

Jan

Re: [PATCH] tools/cpuid: Plumb nested_virt down into xc_cpuid_apply_policy()

[Andrew Cooper]

On 29/09/2020 15:05, Jan Beulich wrote:
> On 29.09.2020 15:48, Andrew Cooper wrote:
>> --- a/tools/libs/guest/xg_cpuid_x86.c
>> +++ b/tools/libs/guest/xg_cpuid_x86.c
>> @@ -427,7 +427,7 @@ static int xc_cpuid_xend_policy(
>>  
>>  int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
>>                            const uint32_t *featureset, unsigned int nr_features,
>> -                          bool pae, bool itsc,
>> +                          bool pae, bool itsc, bool nested_virt,
> This increasing number of bools next to each other bears an
> increasing risk of callers getting the order wrong. Luckily
> there's just one within the tree, so perhaps not an immediate
> problem.

As the commit message said, this is the final special case.

This prototype won't grow any further, although it needs to remain until
Xen 4.13 is thoroughly obsolete, and we're not liable to find a pre-4.14
VM which lacks CPUID data in its migration stream.

>> @@ -559,7 +559,11 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
>>          p->extd.itsc = itsc;
>>  
>>          if ( di.hvm )
>> +        {
>>              p->basic.pae = pae;
>> +            p->basic.vmx = nested_virt;
>> +            p->extd.svm = nested_virt;
>> +        }
>>      }
>>  
>>      if ( !di.hvm )
>> @@ -625,14 +629,6 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
>>              }
>>              break;
>>          }
>> -
>> -        /*
>> -         * These settings are necessary to cause earlier HVM_PARAM_NESTEDHVM
>> -         * to be reflected correctly in CPUID.  Xen will discard these bits if
>> -         * configuration hasn't been set for the domain.
>> -         */
>> -        p->basic.vmx = true;
>> -        p->extd.svm = true;
>>      }
> While I can see how the first sentence in the comment has become
> irrelevant now, what about the second?

Well - I'm writing the series to actually make it irrelevant.

> It's still odd to see both
> svm and vmx getting set to identical values. Therefore perhaps
> worth to retain a shorter comment there?

The comment is true for every feature bit, and is not special to
vmx/svm.  With the absence of the first part, the second part isn't
relevant.

~Andrew

Re: [PATCH] tools/cpuid: Plumb nested_virt down into xc_cpuid_apply_policy()

[Wei Liu]

On Tue, Sep 29, 2020 at 02:48:52PM +0100, Andrew Cooper wrote:
> Nested Virt is the final special case in legacy CPUID handling.  Pass the
> (poorly named) nested_hvm setting down into xc_cpuid_apply_policy() to break
> the semantic dependency on HVM_PARAM_NESTEDHVM.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Wei Liu <wl@xen.org>