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(-)
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;
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-develAndrew 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.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
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.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
HVM_PARAM_PAE_ENABLED is set and consumed by the toolstack only. It is in
practice a complicated and non-standard way of passing a boolean parameter
into xc_cpuid_apply_policy().
This is silly. Pass PAE as a regular parameter instead.
In libxl__cpuid_legacy(), leave a rather better explaination of why only HVM
guests have a choice in PAE setting.
No change in how a guest is constructed.
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>
v2:
* Rewrite commit message and comments.
---
tools/libxc/include/xenctrl.h | 2 +-
tools/libxc/xc_cpuid_x86.c | 15 +++------------
tools/libxl/libxl_cpuid.c | 16 +++++++++++++++-
3 files changed, 19 insertions(+), 14 deletions(-)
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..21b15b86ec 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.
@@ -634,15 +633,7 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid,
break;
}
- /*
- * HVM_PARAM_PAE_ENABLED is a parameter to this function, stashed in
- * Xen. Nothing else has ever taken notice of the value.
- */
- 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..062750102e 100644
--- a/tools/libxl/libxl_cpuid.c
+++ b/tools/libxl/libxl_cpuid.c
@@ -416,8 +416,22 @@ 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;
+
+ /*
+ * For PV guests, PAE is Xen-controlled (it is the 'p' that differentiates
+ * the xen-3.0-x86_32 and xen-3.0-x86_32p ABIs). It is mandatory as Xen
+ * is 64bit only these days.
+ *
+ * For PVH guests, there is no top-level PAE control in the domain config,
+ * so is treated as always available.
+ *
+ * HVM guests get a top-level choice of whether PAE is 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;
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-develAndrew Cooper writes ("[PATCH v2 5/6] tools/libx[cl]: Don't use HVM_PARAM_PAE_ENABLED as a function parameter"):
> HVM_PARAM_PAE_ENABLED is set and consumed by the toolstack only. It is in
> practice a complicated and non-standard way of passing a boolean parameter
> into xc_cpuid_apply_policy().
>
> This is silly. Pass PAE as a regular parameter instead.
>
> In libxl__cpuid_legacy(), leave a rather better explaination of why only HVM
> guests have a choice in PAE setting.
>
> No change in how a guest is constructed.
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
© 2016 - 2026 Red Hat, Inc.