* [PATCH] libs/guest: Don't hide the indirection on xc_cpu_policy_t
@ 2021-05-04 18:53 Andrew Cooper
2021-05-05 6:27 ` Jan Beulich
2021-05-05 9:16 ` Roger Pau Monné
0 siblings, 2 replies; 4+ messages in thread
From: Andrew Cooper @ 2021-05-04 18:53 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Jan Beulich, Roger Pau Monné, Wei Liu
It is bad form in C, perhaps best demonstrated by trying to read
xc_cpu_policy_destroy(), and causes const qualification to have
less-than-obvious behaviour (the hidden pointer becomes const, not the thing
it points at).
xc_cpu_policy_set_domain() needs to drop its (now normal) const qualification,
as the policy object is modified by the serialisation operation.
This also shows up a problem with the x86_cpu_policies_are_compatible(), where
the intermediate pointers are non-const.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
Discovered while trying to start the integration into XenServer. This wants
fixing ASAP, before futher uses get added.
Unsure what to do about x86_cpu_policies_are_compatible(). It would be nice
to have xc_cpu_policy_is_compatible() sensibly const'd, but maybe that means
we need a struct const_cpu_policy and that smells like it is spiralling out of
control.
---
tools/include/xenctrl.h | 22 +++++++++++-----------
tools/libs/guest/xg_cpuid_x86.c | 22 +++++++++++-----------
tools/libs/guest/xg_sr_common_x86.c | 2 +-
tools/misc/xen-cpuid.c | 2 +-
4 files changed, 24 insertions(+), 24 deletions(-)
diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index 0fdb2e8885..58d3377d6a 100644
--- a/tools/include/xenctrl.h
+++ b/tools/include/xenctrl.h
@@ -2590,33 +2590,33 @@ int xc_psr_get_domain_data(xc_interface *xch, uint32_t domid,
int xc_psr_get_hw_info(xc_interface *xch, uint32_t socket,
xc_psr_feat_type type, xc_psr_hw_info *hw_info);
-typedef struct xc_cpu_policy *xc_cpu_policy_t;
+typedef struct xc_cpu_policy xc_cpu_policy_t;
/* Create and free a xc_cpu_policy object. */
-xc_cpu_policy_t xc_cpu_policy_init(void);
-void xc_cpu_policy_destroy(xc_cpu_policy_t policy);
+xc_cpu_policy_t *xc_cpu_policy_init(void);
+void xc_cpu_policy_destroy(xc_cpu_policy_t *policy);
/* Retrieve a system policy, or get/set a domains policy. */
int xc_cpu_policy_get_system(xc_interface *xch, unsigned int policy_idx,
- xc_cpu_policy_t policy);
+ xc_cpu_policy_t *policy);
int xc_cpu_policy_get_domain(xc_interface *xch, uint32_t domid,
- xc_cpu_policy_t policy);
+ xc_cpu_policy_t *policy);
int xc_cpu_policy_set_domain(xc_interface *xch, uint32_t domid,
- const xc_cpu_policy_t policy);
+ xc_cpu_policy_t *policy);
/* Manipulate a policy via architectural representations. */
-int xc_cpu_policy_serialise(xc_interface *xch, const xc_cpu_policy_t policy,
+int xc_cpu_policy_serialise(xc_interface *xch, const xc_cpu_policy_t *policy,
xen_cpuid_leaf_t *leaves, uint32_t *nr_leaves,
xen_msr_entry_t *msrs, uint32_t *nr_msrs);
-int xc_cpu_policy_update_cpuid(xc_interface *xch, xc_cpu_policy_t policy,
+int xc_cpu_policy_update_cpuid(xc_interface *xch, xc_cpu_policy_t *policy,
const xen_cpuid_leaf_t *leaves,
uint32_t nr);
-int xc_cpu_policy_update_msrs(xc_interface *xch, xc_cpu_policy_t policy,
+int xc_cpu_policy_update_msrs(xc_interface *xch, xc_cpu_policy_t *policy,
const xen_msr_entry_t *msrs, uint32_t nr);
/* Compatibility calculations. */
-bool xc_cpu_policy_is_compatible(xc_interface *xch, const xc_cpu_policy_t host,
- const xc_cpu_policy_t guest);
+bool xc_cpu_policy_is_compatible(xc_interface *xch, xc_cpu_policy_t *host,
+ xc_cpu_policy_t *guest);
int xc_get_cpu_levelling_caps(xc_interface *xch, uint32_t *caps);
int xc_get_cpu_featureset(xc_interface *xch, uint32_t index,
diff --git a/tools/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c
index d4e02cecb1..1ebc108213 100644
--- a/tools/libs/guest/xg_cpuid_x86.c
+++ b/tools/libs/guest/xg_cpuid_x86.c
@@ -672,18 +672,18 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
return rc;
}
-xc_cpu_policy_t xc_cpu_policy_init(void)
+xc_cpu_policy_t *xc_cpu_policy_init(void)
{
return calloc(1, sizeof(struct xc_cpu_policy));
}
-void xc_cpu_policy_destroy(xc_cpu_policy_t policy)
+void xc_cpu_policy_destroy(xc_cpu_policy_t *policy)
{
if ( policy )
free(policy);
}
-static int deserialize_policy(xc_interface *xch, xc_cpu_policy_t policy,
+static int deserialize_policy(xc_interface *xch, xc_cpu_policy_t *policy,
unsigned int nr_leaves, unsigned int nr_entries)
{
uint32_t err_leaf = -1, err_subleaf = -1, err_msr = -1;
@@ -713,7 +713,7 @@ static int deserialize_policy(xc_interface *xch, xc_cpu_policy_t policy,
}
int xc_cpu_policy_get_system(xc_interface *xch, unsigned int policy_idx,
- xc_cpu_policy_t policy)
+ xc_cpu_policy_t *policy)
{
unsigned int nr_leaves = ARRAY_SIZE(policy->leaves);
unsigned int nr_entries = ARRAY_SIZE(policy->entries);
@@ -738,7 +738,7 @@ int xc_cpu_policy_get_system(xc_interface *xch, unsigned int policy_idx,
}
int xc_cpu_policy_get_domain(xc_interface *xch, uint32_t domid,
- xc_cpu_policy_t policy)
+ xc_cpu_policy_t *policy)
{
unsigned int nr_leaves = ARRAY_SIZE(policy->leaves);
unsigned int nr_entries = ARRAY_SIZE(policy->entries);
@@ -763,7 +763,7 @@ int xc_cpu_policy_get_domain(xc_interface *xch, uint32_t domid,
}
int xc_cpu_policy_set_domain(xc_interface *xch, uint32_t domid,
- const xc_cpu_policy_t policy)
+ xc_cpu_policy_t *policy)
{
uint32_t err_leaf = -1, err_subleaf = -1, err_msr = -1;
unsigned int nr_leaves = ARRAY_SIZE(policy->leaves);
@@ -791,7 +791,7 @@ int xc_cpu_policy_set_domain(xc_interface *xch, uint32_t domid,
return rc;
}
-int xc_cpu_policy_serialise(xc_interface *xch, const xc_cpu_policy_t p,
+int xc_cpu_policy_serialise(xc_interface *xch, const xc_cpu_policy_t *p,
xen_cpuid_leaf_t *leaves, uint32_t *nr_leaves,
xen_msr_entry_t *msrs, uint32_t *nr_msrs)
{
@@ -823,7 +823,7 @@ int xc_cpu_policy_serialise(xc_interface *xch, const xc_cpu_policy_t p,
return 0;
}
-int xc_cpu_policy_update_cpuid(xc_interface *xch, xc_cpu_policy_t policy,
+int xc_cpu_policy_update_cpuid(xc_interface *xch, xc_cpu_policy_t *policy,
const xen_cpuid_leaf_t *leaves,
uint32_t nr)
{
@@ -843,7 +843,7 @@ int xc_cpu_policy_update_cpuid(xc_interface *xch, xc_cpu_policy_t policy,
return rc;
}
-int xc_cpu_policy_update_msrs(xc_interface *xch, xc_cpu_policy_t policy,
+int xc_cpu_policy_update_msrs(xc_interface *xch, xc_cpu_policy_t *policy,
const xen_msr_entry_t *msrs, uint32_t nr)
{
unsigned int err_msr = -1;
@@ -861,8 +861,8 @@ int xc_cpu_policy_update_msrs(xc_interface *xch, xc_cpu_policy_t policy,
return rc;
}
-bool xc_cpu_policy_is_compatible(xc_interface *xch, const xc_cpu_policy_t host,
- const xc_cpu_policy_t guest)
+bool xc_cpu_policy_is_compatible(xc_interface *xch, xc_cpu_policy_t *host,
+ xc_cpu_policy_t *guest)
{
struct cpu_policy_errors err = INIT_CPU_POLICY_ERRORS;
struct cpu_policy h = { &host->cpuid, &host->msr };
diff --git a/tools/libs/guest/xg_sr_common_x86.c b/tools/libs/guest/xg_sr_common_x86.c
index 15265e7a33..563b4f0168 100644
--- a/tools/libs/guest/xg_sr_common_x86.c
+++ b/tools/libs/guest/xg_sr_common_x86.c
@@ -48,7 +48,7 @@ int write_x86_cpu_policy_records(struct xc_sr_context *ctx)
struct xc_sr_record cpuid = { .type = REC_TYPE_X86_CPUID_POLICY, };
struct xc_sr_record msrs = { .type = REC_TYPE_X86_MSR_POLICY, };
uint32_t nr_leaves = 0, nr_msrs = 0;
- xc_cpu_policy_t policy = NULL;
+ xc_cpu_policy_t *policy = NULL;
int rc;
if ( xc_cpu_policy_get_size(xch, &nr_leaves, &nr_msrs) < 0 )
diff --git a/tools/misc/xen-cpuid.c b/tools/misc/xen-cpuid.c
index b2a36deacc..d4bc83d8c9 100644
--- a/tools/misc/xen-cpuid.c
+++ b/tools/misc/xen-cpuid.c
@@ -468,7 +468,7 @@ int main(int argc, char **argv)
uint32_t i, max_leaves, max_msrs;
xc_interface *xch = xc_interface_open(0, 0, 0);
- xc_cpu_policy_t policy = xc_cpu_policy_init();
+ xc_cpu_policy_t *policy = xc_cpu_policy_init();
if ( !xch )
err(1, "xc_interface_open");
--
2.11.0
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] libs/guest: Don't hide the indirection on xc_cpu_policy_t
2021-05-04 18:53 [PATCH] libs/guest: Don't hide the indirection on xc_cpu_policy_t Andrew Cooper
@ 2021-05-05 6:27 ` Jan Beulich
2021-05-05 9:16 ` Roger Pau Monné
1 sibling, 0 replies; 4+ messages in thread
From: Jan Beulich @ 2021-05-05 6:27 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Roger Pau Monné, Wei Liu, Xen-devel
On 04.05.2021 20:53, Andrew Cooper wrote:
> Unsure what to do about x86_cpu_policies_are_compatible(). It would be nice
> to have xc_cpu_policy_is_compatible() sensibly const'd, but maybe that means
> we need a struct const_cpu_policy and that smells like it is spiralling out of
> control.
Not sure it would be that bad. In fact, looking at the few uses of struct
cpu_policy in the hypervisor, I wonder if the two contained pointers
couldn't be pointer-to-const right there. Once you've constructed a full
struct cpu_policy instance, it shouldn't typically be modified anymore,
should it? Would require the respective struct arch_domain fields to also
become pointer-to-const. And if the tool stack had any need for a
container with pointer-to-non-const, it could locally define a type,
keeping lib/x86/ tidy.
Jan
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] libs/guest: Don't hide the indirection on xc_cpu_policy_t
2021-05-04 18:53 [PATCH] libs/guest: Don't hide the indirection on xc_cpu_policy_t Andrew Cooper
2021-05-05 6:27 ` Jan Beulich
@ 2021-05-05 9:16 ` Roger Pau Monné
2021-05-05 12:48 ` Andrew Cooper
1 sibling, 1 reply; 4+ messages in thread
From: Roger Pau Monné @ 2021-05-05 9:16 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Xen-devel, Jan Beulich, Wei Liu
On Tue, May 04, 2021 at 07:53:22PM +0100, Andrew Cooper wrote:
> It is bad form in C, perhaps best demonstrated by trying to read
> xc_cpu_policy_destroy(), and causes const qualification to have
> less-than-obvious behaviour (the hidden pointer becomes const, not the thing
> it points at).
Would this also affect cpuid_leaf_buffer_t and msr_entry_buffer_t
which hide an array behind a typedef?
> xc_cpu_policy_set_domain() needs to drop its (now normal) const qualification,
> as the policy object is modified by the serialisation operation.
>
> This also shows up a problem with the x86_cpu_policies_are_compatible(), where
> the intermediate pointers are non-const.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Wei Liu <wl@xen.org>
>
> Discovered while trying to start the integration into XenServer. This wants
> fixing ASAP, before futher uses get added.
>
> Unsure what to do about x86_cpu_policies_are_compatible(). It would be nice
> to have xc_cpu_policy_is_compatible() sensibly const'd, but maybe that means
> we need a struct const_cpu_policy and that smells like it is spiralling out of
> control.
Not sure TBH, I cannot think of any alternative right now, but
introducing a const_cpu_policy feels kind of code duplication.
Thanks, Roger.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] libs/guest: Don't hide the indirection on xc_cpu_policy_t
2021-05-05 9:16 ` Roger Pau Monné
@ 2021-05-05 12:48 ` Andrew Cooper
0 siblings, 0 replies; 4+ messages in thread
From: Andrew Cooper @ 2021-05-05 12:48 UTC (permalink / raw)
To: Roger Pau Monné; +Cc: Xen-devel, Jan Beulich, Wei Liu
On 05/05/2021 10:16, Roger Pau Monné wrote:
> On Tue, May 04, 2021 at 07:53:22PM +0100, Andrew Cooper wrote:
>> It is bad form in C, perhaps best demonstrated by trying to read
>> xc_cpu_policy_destroy(), and causes const qualification to have
>> less-than-obvious behaviour (the hidden pointer becomes const, not the thing
>> it points at).
> Would this also affect cpuid_leaf_buffer_t and msr_entry_buffer_t
> which hide an array behind a typedef?
They're a total pain because in userspace, they're plain arrays, and in
Xen, they're GUEST_HANDLE's.
Hiding arrays in a typedef like that (unlike hiding pointers) doesn't
change the interaction with const.
So the code there is correct AFAICT, even if it doesn't appear so.
>> xc_cpu_policy_set_domain() needs to drop its (now normal) const qualification,
>> as the policy object is modified by the serialisation operation.
>>
>> This also shows up a problem with the x86_cpu_policies_are_compatible(), where
>> the intermediate pointers are non-const.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Acked-by: Roger Pau Monné <roger.pau@citrix.com>
Thanks.
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Roger Pau Monné <roger.pau@citrix.com>
>> CC: Wei Liu <wl@xen.org>
>>
>> Discovered while trying to start the integration into XenServer. This wants
>> fixing ASAP, before futher uses get added.
>>
>> Unsure what to do about x86_cpu_policies_are_compatible(). It would be nice
>> to have xc_cpu_policy_is_compatible() sensibly const'd, but maybe that means
>> we need a struct const_cpu_policy and that smells like it is spiralling out of
>> control.
> Not sure TBH, I cannot think of any alternative right now, but
> introducing a const_cpu_policy feels kind of code duplication.
At least this is all internals. We've got time and flexibility to
experiment.
~Andrew
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-05-05 12:48 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-04 18:53 [PATCH] libs/guest: Don't hide the indirection on xc_cpu_policy_t Andrew Cooper
2021-05-05 6:27 ` Jan Beulich
2021-05-05 9:16 ` Roger Pau Monné
2021-05-05 12:48 ` Andrew Cooper
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).