| Message ID | 20200205165056.11734-6-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | tools: Rationalise legacy CPUID handling | expand |
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.
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
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.
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;
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(-)