[5/6] tools/libx[cl]: Don't use HVM_PARAM_PAE_ENABLED as a function parameter
diff mbox series

Message ID 20200205165056.11734-6-andrew.cooper3@citrix.com
State New
Headers show
Series
  • tools: Rationalise legacy CPUID handling
Related show

Commit Message

Andrew Cooper Feb. 5, 2020, 4:50 p.m. UTC
The sole use of HVM_PARAM_PAE_ENABLED is as a non-standard calling convention
for xc_cpuid_apply_policy().  Pass PAE as a regular parameter instead.

Leave a rather better explaination of why only HVM guests have a choice in PAE
setting.

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Ian Jackson <Ian.Jackson@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Anthony PERARD <anthony.perard@citrix.com>
---
 tools/libxc/include/xenctrl.h |  2 +-
 tools/libxc/xc_cpuid_x86.c    | 15 +++++----------
 tools/libxl/libxl_cpuid.c     | 14 +++++++++++++-
 3 files changed, 19 insertions(+), 12 deletions(-)

Comments

Ian Jackson Feb. 11, 2020, 5:47 p.m. UTC | #1
Andrew Cooper writes ("[PATCH 5/6] tools/libx[cl]: Don't use HVM_PARAM_PAE_ENABLED as a function parameter"):
> The sole use of HVM_PARAM_PAE_ENABLED is as a non-standard calling convention
> for xc_cpuid_apply_policy().  Pass PAE as a regular parameter instead.
> 
> Leave a rather better explaination of why only HVM guests have a choice in PAE
> setting.

I am inclined believe you that this is right (since you are evidently
familiar with this whole area and I'm not), but the explanations leave
me confused.

>  int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid,
> -                          const uint32_t *featureset, unsigned int nr_features)
> +                          const uint32_t *featureset, unsigned int nr_features,
> +                          bool pae)
>  {
>      int rc;
>      xc_dominfo_t di;
> @@ -579,8 +580,6 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid,
>      }
>      else
>      {
> -        uint64_t val;
> -
>          /*
>           * Topology for HVM guests is entirely controlled by Xen.  For now, we
>           * hardcode APIC_ID = vcpu_id * 2 to give the illusion of no SMT.
> @@ -635,14 +634,10 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid,
>          }
>  
>          /*
> -         * HVM_PARAM_PAE_ENABLED is a parameter to this function, stashed in
> -         * Xen.  Nothing else has ever taken notice of the value.
> +         * PAE used to be a parameter passed to this function by
> +         * HVM_PARAM_PAE_ENABLED.  It is now passed normally.

In particular, I don't understand what these comments mean by
"HVM_PARAM_PAE_ENABLED is a parameter to this function" and "PAE used
to be a parameter passed to this function by HVM_PARAM_PAE_ENABLED".

Maybe this is some loose use of the term "parameter" ?

If you could explain more clearly (ideally, explain the meaning of the
old comment in the commit message, and make the new comment
unambiguous) then that would be great.

Thanks,
Ian.
Andrew Cooper Feb. 11, 2020, 5:55 p.m. UTC | #2
On 11/02/2020 17:47, Ian Jackson wrote:
> Andrew Cooper writes ("[PATCH 5/6] tools/libx[cl]: Don't use HVM_PARAM_PAE_ENABLED as a function parameter"):
>> The sole use of HVM_PARAM_PAE_ENABLED is as a non-standard calling convention
>> for xc_cpuid_apply_policy().  Pass PAE as a regular parameter instead.
>>
>> Leave a rather better explaination of why only HVM guests have a choice in PAE
>> setting.
> I am inclined believe you that this is right (since you are evidently
> familiar with this whole area and I'm not), but the explanations leave
> me confused.
>
>>  int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid,
>> -                          const uint32_t *featureset, unsigned int nr_features)
>> +                          const uint32_t *featureset, unsigned int nr_features,
>> +                          bool pae)
>>  {
>>      int rc;
>>      xc_dominfo_t di;
>> @@ -579,8 +580,6 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid,
>>      }
>>      else
>>      {
>> -        uint64_t val;
>> -
>>          /*
>>           * Topology for HVM guests is entirely controlled by Xen.  For now, we
>>           * hardcode APIC_ID = vcpu_id * 2 to give the illusion of no SMT.
>> @@ -635,14 +634,10 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid,
>>          }
>>  
>>          /*
>> -         * HVM_PARAM_PAE_ENABLED is a parameter to this function, stashed in
>> -         * Xen.  Nothing else has ever taken notice of the value.
>> +         * PAE used to be a parameter passed to this function by
>> +         * HVM_PARAM_PAE_ENABLED.  It is now passed normally.
> In particular, I don't understand what these comments mean by
> "HVM_PARAM_PAE_ENABLED is a parameter to this function" and "PAE used
> to be a parameter passed to this function by HVM_PARAM_PAE_ENABLED".
>
> Maybe this is some loose use of the term "parameter" ?
>
> If you could explain more clearly (ideally, explain the meaning of the
> old comment in the commit message, and make the new comment
> unambiguous) then that would be great.

HVM_PARAM_PAE_ENABLED encapsulates a boolean meaning "should I advertise
the PAE feature to the guest?".

It has only ever been used in a way which should have been "bool pae"
passed into xc_cpuid_apply_policy().  This patch tries to do just that.


I think there might be confusion as to which comment the commit message
referred to.

In xc_cpuid_apply_policy(), I want a comment explaining why we have this
weird pae parameter.  It will disappear from the new way of doing CPUID
at boot, but will have to remain for the pre-4.14 compatibility.

The comment I was referring to in the commit message was actually the
libxl comment, explaining why PV and PVH guests don't get a choice to
hide the PAE feature.

~Andrew
Ian Jackson Feb. 17, 2020, 3:40 p.m. UTC | #3
Andrew Cooper writes ("[PATCH 5/6] tools/libx[cl]: Don't use HVM_PARAM_PAE_ENABLED as a function parameter"):
> The sole use of HVM_PARAM_PAE_ENABLED is as a non-standard calling
> convention for xc_cpuid_apply_policy().  Pass PAE as a regular
> parameter instead.

Following our conversation on irc, I have tried to write an
explanation in my own words of what this commit is doing.

  The value of HVM_PARAM_PAE_ENABLED is set by the toolstack.  And the
  only place that reads it is also in the toolstack, in
  xc_cpuid_apply_policy.  Effectively, this hypervisor domain
  parameter is used solely as a way to pass this boolean parameter
  from one part of the toolstack to another.

  This is not sensible.

  Replace its use in xc_cpuid_apply_policy with a plain boolean
  parameter, passed directly by the one (in-tree) caller.
  The now-redundant code to set the value in the hypervisor will be
  deleted in the next patch.

> Leave a rather better explaination of why only HVM guests have a
> choice in PAE setting.

I approve of this part of the commit message.

> No functional change.

I would write

   No overall functional change.  The new code fior calculating the
   `pae' value (in libxl__cpuid_legacy) is isomorphic to the
   obselescent code (in libxl_x86.c).

I had a look to see whether this was true and it seemed to be.

>          /*
> -         * HVM_PARAM_PAE_ENABLED is a parameter to this function, stashed in
> -         * Xen.  Nothing else has ever taken notice of the value.
> +         * PAE used to be a parameter passed to this function by
> +         * HVM_PARAM_PAE_ENABLED.  It is now passed normally.
>           */

I find this phrasing confusing, particularly this very loose use of
the word `parameter'.  I would drop this comment entirely and let the
commit message stand as the historical documentation.

>      char *cpuid_res[4];
> +    bool pae = true;
> +
> +    /*
> +     * PAE is a Xen-controlled for PV guests (it is the 'p' that causes the
> +     * difference between the xen-3.0-x86_32 and xen-3.0-x86_32p ABIs).  It\
 is
> +     * mandatory as Xen is running in 64bit mode.
> +     *
> +     * PVH guests don't have a top-level PAE control, and is treated as
> +     * available.
> +     */

I approve of putting a new comment here with an explanation.  However,
it should be wrapped rather more tightly (eg, in my MUA it is now
suffering from wrap damage as I demonstrate above) and it seems to
have some problems with the grammar ?  And I think the 2nd sentence
"It is mandatory" could usefully be re-qualified "for PV guests".  Or
you could write something like this.

   PV guests: PAE is Xen-controlled (it is the 'p' that causes the
   difference between the xen-3.0-x86_32 and xen-3.0-x86_32p ABIs);
   Xen is in 64-bit mode so PAE is mandatory.

   PVH guests: there is no top-level PAE control in the libx domain
   config; we always make it available.

   So only this test only applies to HVM guests:

Thanks,
Ian.

Patch
diff mbox series

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 311df1ef0f..4eb4f4c2c6 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -1807,7 +1807,7 @@  int xc_cpuid_set(xc_interface *xch,
 int xc_cpuid_apply_policy(xc_interface *xch,
                           uint32_t domid,
                           const uint32_t *featureset,
-                          unsigned int nr_features);
+                          unsigned int nr_features, bool pae);
 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/libxc/xc_cpuid_x86.c b/tools/libxc/xc_cpuid_x86.c
index 2540aa1e1c..4e74a7ed3b 100644
--- a/tools/libxc/xc_cpuid_x86.c
+++ b/tools/libxc/xc_cpuid_x86.c
@@ -455,7 +455,8 @@  int xc_cpuid_set(
 }
 
 int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid,
-                          const uint32_t *featureset, unsigned int nr_features)
+                          const uint32_t *featureset, unsigned int nr_features,
+                          bool pae)
 {
     int rc;
     xc_dominfo_t di;
@@ -579,8 +580,6 @@  int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid,
     }
     else
     {
-        uint64_t val;
-
         /*
          * Topology for HVM guests is entirely controlled by Xen.  For now, we
          * hardcode APIC_ID = vcpu_id * 2 to give the illusion of no SMT.
@@ -635,14 +634,10 @@  int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid,
         }
 
         /*
-         * HVM_PARAM_PAE_ENABLED is a parameter to this function, stashed in
-         * Xen.  Nothing else has ever taken notice of the value.
+         * PAE used to be a parameter passed to this function by
+         * HVM_PARAM_PAE_ENABLED.  It is now passed normally.
          */
-        rc = xc_hvm_param_get(xch, domid, HVM_PARAM_PAE_ENABLED, &val);
-        if ( rc )
-            goto out;
-
-        p->basic.pae = val;
+        p->basic.pae = pae;
 
         /*
          * These settings are necessary to cause earlier HVM_PARAM_NESTEDHVM /
diff --git a/tools/libxl/libxl_cpuid.c b/tools/libxl/libxl_cpuid.c
index 49d3ca5b26..8c49e34125 100644
--- a/tools/libxl/libxl_cpuid.c
+++ b/tools/libxl/libxl_cpuid.c
@@ -416,8 +416,20 @@  void libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid,
     libxl_cpuid_policy_list cpuid = info->cpuid;
     int i;
     char *cpuid_res[4];
+    bool pae = true;
+
+    /*
+     * PAE is a Xen-controlled for PV guests (it is the 'p' that causes the
+     * difference between the xen-3.0-x86_32 and xen-3.0-x86_32p ABIs).  It is
+     * mandatory as Xen is running in 64bit mode.
+     *
+     * PVH guests don't have a top-level PAE control, and is treated as
+     * available.
+     */
+    if (info->type == LIBXL_DOMAIN_TYPE_HVM)
+        pae = libxl_defbool_val(info->u.hvm.pae);
 
-    xc_cpuid_apply_policy(ctx->xch, domid, NULL, 0);
+    xc_cpuid_apply_policy(ctx->xch, domid, NULL, 0, pae);
 
     if (!cpuid)
         return;