Right now, they're the same underlying type, containing disjoint information.
Use a single object instead. Also take the opportunity to rename 'entries' to
'msrs' which is more descriptive, and more in line with nr_msrs being the
count of MSR entries in the API.
test-tsx uses xg_private.h to access the internals of xc_cpu_policy, so needs
updating at the same time.
No practical change, but it does reduce the size of struct xc_cpu_policy by
~2k.
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>
---
tools/libs/guest/xg_cpuid_x86.c | 36 ++++++++++----------
tools/libs/guest/xg_private.h | 5 ++-
tools/tests/tsx/test-tsx.c | 58 ++++++++++++++++-----------------
3 files changed, 48 insertions(+), 51 deletions(-)
diff --git a/tools/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c
index 5fae06e77804..5061fe357767 100644
--- a/tools/libs/guest/xg_cpuid_x86.c
+++ b/tools/libs/guest/xg_cpuid_x86.c
@@ -431,7 +431,7 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
xc_dominfo_t di;
unsigned int i, nr_leaves, nr_msrs;
xen_cpuid_leaf_t *leaves = NULL;
- struct cpuid_policy *p = NULL;
+ struct cpu_policy *p = NULL;
uint32_t err_leaf = -1, err_subleaf = -1, err_msr = -1;
uint32_t host_featureset[FEATURESET_NR_ENTRIES] = {};
uint32_t len = ARRAY_SIZE(host_featureset);
@@ -692,7 +692,7 @@ static int deserialize_policy(xc_interface *xch, xc_cpu_policy_t *policy,
uint32_t err_leaf = -1, err_subleaf = -1, err_msr = -1;
int rc;
- rc = x86_cpuid_copy_from_buffer(&policy->cpuid, policy->leaves,
+ rc = x86_cpuid_copy_from_buffer(&policy->policy, policy->leaves,
nr_leaves, &err_leaf, &err_subleaf);
if ( rc )
{
@@ -702,7 +702,7 @@ static int deserialize_policy(xc_interface *xch, xc_cpu_policy_t *policy,
return rc;
}
- rc = x86_msr_copy_from_buffer(&policy->msr, policy->entries,
+ rc = x86_msr_copy_from_buffer(&policy->policy, policy->msrs,
nr_entries, &err_msr);
if ( rc )
{
@@ -719,18 +719,18 @@ int xc_cpu_policy_get_system(xc_interface *xch, unsigned int policy_idx,
xc_cpu_policy_t *policy)
{
unsigned int nr_leaves = ARRAY_SIZE(policy->leaves);
- unsigned int nr_entries = ARRAY_SIZE(policy->entries);
+ unsigned int nr_msrs = ARRAY_SIZE(policy->msrs);
int rc;
rc = get_system_cpu_policy(xch, policy_idx, &nr_leaves, policy->leaves,
- &nr_entries, policy->entries);
+ &nr_msrs, policy->msrs);
if ( rc )
{
PERROR("Failed to obtain %u policy", policy_idx);
return rc;
}
- rc = deserialize_policy(xch, policy, nr_leaves, nr_entries);
+ rc = deserialize_policy(xch, policy, nr_leaves, nr_msrs);
if ( rc )
{
errno = -rc;
@@ -744,18 +744,18 @@ int xc_cpu_policy_get_domain(xc_interface *xch, uint32_t domid,
xc_cpu_policy_t *policy)
{
unsigned int nr_leaves = ARRAY_SIZE(policy->leaves);
- unsigned int nr_entries = ARRAY_SIZE(policy->entries);
+ unsigned int nr_msrs = ARRAY_SIZE(policy->msrs);
int rc;
rc = get_domain_cpu_policy(xch, domid, &nr_leaves, policy->leaves,
- &nr_entries, policy->entries);
+ &nr_msrs, policy->msrs);
if ( rc )
{
PERROR("Failed to obtain domain %u policy", domid);
return rc;
}
- rc = deserialize_policy(xch, policy, nr_leaves, nr_entries);
+ rc = deserialize_policy(xch, policy, nr_leaves, nr_msrs);
if ( rc )
{
errno = -rc;
@@ -770,16 +770,16 @@ int xc_cpu_policy_set_domain(xc_interface *xch, uint32_t domid,
{
uint32_t err_leaf = -1, err_subleaf = -1, err_msr = -1;
unsigned int nr_leaves = ARRAY_SIZE(policy->leaves);
- unsigned int nr_entries = ARRAY_SIZE(policy->entries);
+ unsigned int nr_msrs = ARRAY_SIZE(policy->msrs);
int rc;
rc = xc_cpu_policy_serialise(xch, policy, policy->leaves, &nr_leaves,
- policy->entries, &nr_entries);
+ policy->msrs, &nr_msrs);
if ( rc )
return rc;
rc = xc_set_domain_cpu_policy(xch, domid, nr_leaves, policy->leaves,
- nr_entries, policy->entries,
+ nr_msrs, policy->msrs,
&err_leaf, &err_subleaf, &err_msr);
if ( rc )
{
@@ -802,7 +802,7 @@ int xc_cpu_policy_serialise(xc_interface *xch, const xc_cpu_policy_t *p,
if ( leaves )
{
- rc = x86_cpuid_copy_to_buffer(&p->cpuid, leaves, nr_leaves);
+ rc = x86_cpuid_copy_to_buffer(&p->policy, leaves, nr_leaves);
if ( rc )
{
ERROR("Failed to serialize CPUID policy");
@@ -813,7 +813,7 @@ int xc_cpu_policy_serialise(xc_interface *xch, const xc_cpu_policy_t *p,
if ( msrs )
{
- rc = x86_msr_copy_to_buffer(&p->msr, msrs, nr_msrs);
+ rc = x86_msr_copy_to_buffer(&p->policy, msrs, nr_msrs);
if ( rc )
{
ERROR("Failed to serialize MSR policy");
@@ -831,7 +831,7 @@ int xc_cpu_policy_update_cpuid(xc_interface *xch, xc_cpu_policy_t *policy,
uint32_t nr)
{
unsigned int err_leaf = -1, err_subleaf = -1;
- int rc = x86_cpuid_copy_from_buffer(&policy->cpuid, leaves, nr,
+ int rc = x86_cpuid_copy_from_buffer(&policy->policy, leaves, nr,
&err_leaf, &err_subleaf);
if ( rc )
@@ -850,7 +850,7 @@ 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;
- int rc = x86_msr_copy_from_buffer(&policy->msr, msrs, nr, &err_msr);
+ int rc = x86_msr_copy_from_buffer(&policy->policy, msrs, nr, &err_msr);
if ( rc )
{
@@ -868,8 +868,8 @@ 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 old_cpu_policy h = { &host->cpuid, &host->msr };
- struct old_cpu_policy g = { &guest->cpuid, &guest->msr };
+ struct old_cpu_policy h = { &host->policy, &host->policy };
+ struct old_cpu_policy g = { &guest->policy, &guest->policy };
int rc = x86_cpu_policies_are_compatible(&h, &g, &err);
if ( !rc )
diff --git a/tools/libs/guest/xg_private.h b/tools/libs/guest/xg_private.h
index 09e24f122760..e729a8106c3e 100644
--- a/tools/libs/guest/xg_private.h
+++ b/tools/libs/guest/xg_private.h
@@ -173,10 +173,9 @@ int pin_table(xc_interface *xch, unsigned int type, unsigned long mfn,
#include <xen/lib/x86/cpu-policy.h>
struct xc_cpu_policy {
- struct cpuid_policy cpuid;
- struct msr_policy msr;
+ struct cpu_policy policy;
xen_cpuid_leaf_t leaves[CPUID_MAX_SERIALISED_LEAVES];
- xen_msr_entry_t entries[MSR_MAX_SERIALISED_ENTRIES];
+ xen_msr_entry_t msrs[MSR_MAX_SERIALISED_ENTRIES];
};
#endif /* x86 */
diff --git a/tools/tests/tsx/test-tsx.c b/tools/tests/tsx/test-tsx.c
index d6d98c299bf9..6164cd86c466 100644
--- a/tools/tests/tsx/test-tsx.c
+++ b/tools/tests/tsx/test-tsx.c
@@ -151,15 +151,15 @@ static void test_tsx_msrs(void)
{
printf("Testing MSR_TSX_FORCE_ABORT consistency\n");
test_tsx_msr_consistency(
- MSR_TSX_FORCE_ABORT, host.cpuid.feat.tsx_force_abort);
+ MSR_TSX_FORCE_ABORT, host.policy.feat.tsx_force_abort);
printf("Testing MSR_TSX_CTRL consistency\n");
test_tsx_msr_consistency(
- MSR_TSX_CTRL, host.msr.arch_caps.tsx_ctrl);
+ MSR_TSX_CTRL, host.policy.arch_caps.tsx_ctrl);
printf("Testing MSR_MCU_OPT_CTRL consistency\n");
test_tsx_msr_consistency(
- MSR_MCU_OPT_CTRL, host.cpuid.feat.srbds_ctrl);
+ MSR_MCU_OPT_CTRL, host.policy.feat.srbds_ctrl);
}
/*
@@ -281,7 +281,7 @@ static void test_rtm_behaviour(void)
else
return fail(" Got unexpected behaviour %d\n", rtm_behaviour);
- if ( host.cpuid.feat.rtm )
+ if ( host.policy.feat.rtm )
{
if ( rtm_behaviour == RTM_UD )
fail(" Host reports RTM, but appears unavailable\n");
@@ -297,53 +297,51 @@ static void dump_tsx_details(const struct xc_cpu_policy *p, const char *pref)
{
printf(" %s RTM %u, HLE %u, TSX_FORCE_ABORT %u, RTM_ALWAYS_ABORT %u, TSX_CTRL %u\n",
pref,
- p->cpuid.feat.rtm,
- p->cpuid.feat.hle,
- p->cpuid.feat.tsx_force_abort,
- p->cpuid.feat.rtm_always_abort,
- p->msr.arch_caps.tsx_ctrl);
+ p->policy.feat.rtm,
+ p->policy.feat.hle,
+ p->policy.feat.tsx_force_abort,
+ p->policy.feat.rtm_always_abort,
+ p->policy.arch_caps.tsx_ctrl);
}
/* Sanity test various invariants we expect in the default/max policies. */
static void test_guest_policies(const struct xc_cpu_policy *max,
const struct xc_cpu_policy *def)
{
- const struct cpuid_policy *cm = &max->cpuid;
- const struct cpuid_policy *cd = &def->cpuid;
- const struct msr_policy *mm = &max->msr;
- const struct msr_policy *md = &def->msr;
+ const struct cpu_policy *m = &max->policy;
+ const struct cpu_policy *d = &def->policy;
dump_tsx_details(max, "Max:");
dump_tsx_details(def, "Def:");
- if ( ((cm->feat.raw[0].d | cd->feat.raw[0].d) &
+ if ( ((m->feat.raw[0].d | d->feat.raw[0].d) &
(bitmaskof(X86_FEATURE_TSX_FORCE_ABORT) |
bitmaskof(X86_FEATURE_RTM_ALWAYS_ABORT) |
bitmaskof(X86_FEATURE_SRBDS_CTRL))) ||
- ((mm->arch_caps.raw | md->arch_caps.raw) & ARCH_CAPS_TSX_CTRL) )
+ ((m->arch_caps.raw | d->arch_caps.raw) & ARCH_CAPS_TSX_CTRL) )
fail(" Xen-only TSX controls offered to guest\n");
switch ( rtm_behaviour )
{
case RTM_UD:
- if ( (cm->feat.raw[0].b | cd->feat.raw[0].b) &
+ if ( (m->feat.raw[0].b | d->feat.raw[0].b) &
(bitmaskof(X86_FEATURE_HLE) | bitmaskof(X86_FEATURE_RTM)) )
fail(" HLE/RTM offered to guests despite not being available\n");
break;
case RTM_ABORT:
- if ( cd->feat.raw[0].b &
+ if ( d->feat.raw[0].b &
(bitmaskof(X86_FEATURE_HLE) | bitmaskof(X86_FEATURE_RTM)) )
fail(" HLE/RTM offered to guests by default despite not being usable\n");
break;
case RTM_OK:
- if ( !cm->feat.rtm || !cd->feat.rtm )
+ if ( !m->feat.rtm || !d->feat.rtm )
fail(" RTM not offered to guests despite being available\n");
break;
}
- if ( cd->feat.hle )
+ if ( d->feat.hle )
fail(" Fail: HLE offered in default policy\n");
}
@@ -387,18 +385,18 @@ static void test_guest(struct xen_domctl_createdomain *c)
/*
* Check defaults given to the guest.
*/
- if ( guest_policy.cpuid.feat.rtm != (rtm_behaviour == RTM_OK) )
+ if ( guest_policy.policy.feat.rtm != (rtm_behaviour == RTM_OK) )
fail(" RTM %u in guest, despite rtm behaviour\n",
- guest_policy.cpuid.feat.rtm);
+ guest_policy.policy.feat.rtm);
- if ( guest_policy.cpuid.feat.hle ||
- guest_policy.cpuid.feat.tsx_force_abort ||
- guest_policy.cpuid.feat.rtm_always_abort ||
- guest_policy.cpuid.feat.srbds_ctrl ||
- guest_policy.msr.arch_caps.tsx_ctrl )
+ if ( guest_policy.policy.feat.hle ||
+ guest_policy.policy.feat.tsx_force_abort ||
+ guest_policy.policy.feat.rtm_always_abort ||
+ guest_policy.policy.feat.srbds_ctrl ||
+ guest_policy.policy.arch_caps.tsx_ctrl )
fail(" Unexpected features advertised\n");
- if ( host.cpuid.feat.rtm )
+ if ( host.policy.feat.rtm )
{
unsigned int _7b0;
@@ -406,7 +404,7 @@ static void test_guest(struct xen_domctl_createdomain *c)
* If host RTM is available, all combinations of guest flags should be
* possible. Flip both HLE/RTM to check non-default settings.
*/
- _7b0 = (guest_policy.cpuid.feat.raw[0].b ^=
+ _7b0 = (guest_policy.policy.feat.raw[0].b ^=
(bitmaskof(X86_FEATURE_HLE) | bitmaskof(X86_FEATURE_RTM)));
/* Set the new policy. */
@@ -429,10 +427,10 @@ static void test_guest(struct xen_domctl_createdomain *c)
dump_tsx_details(&guest_policy, "Cur:");
- if ( guest_policy.cpuid.feat.raw[0].b != _7b0 )
+ if ( guest_policy.policy.feat.raw[0].b != _7b0 )
{
fail(" Expected CPUID.7[1].b 0x%08x differs from actual 0x%08x\n",
- _7b0, guest_policy.cpuid.feat.raw[0].b);
+ _7b0, guest_policy.policy.feat.raw[0].b);
goto out;
}
}
--
2.30.2
On 29.03.2023 22:51, Andrew Cooper wrote:
> /* Sanity test various invariants we expect in the default/max policies. */
> static void test_guest_policies(const struct xc_cpu_policy *max,
> const struct xc_cpu_policy *def)
> {
> - const struct cpuid_policy *cm = &max->cpuid;
> - const struct cpuid_policy *cd = &def->cpuid;
> - const struct msr_policy *mm = &max->msr;
> - const struct msr_policy *md = &def->msr;
> + const struct cpu_policy *m = &max->policy;
> + const struct cpu_policy *d = &def->policy;
Looks like you could reduce code churn by keeping "cm" and "cd" as the
names, which would also be consistent with you continuing to use "cp"
in hypervisor code.
Jan
> dump_tsx_details(max, "Max:");
> dump_tsx_details(def, "Def:");
>
> - if ( ((cm->feat.raw[0].d | cd->feat.raw[0].d) &
> + if ( ((m->feat.raw[0].d | d->feat.raw[0].d) &
> (bitmaskof(X86_FEATURE_TSX_FORCE_ABORT) |
> bitmaskof(X86_FEATURE_RTM_ALWAYS_ABORT) |
> bitmaskof(X86_FEATURE_SRBDS_CTRL))) ||
> - ((mm->arch_caps.raw | md->arch_caps.raw) & ARCH_CAPS_TSX_CTRL) )
> + ((m->arch_caps.raw | d->arch_caps.raw) & ARCH_CAPS_TSX_CTRL) )
> fail(" Xen-only TSX controls offered to guest\n");
>
> switch ( rtm_behaviour )
> {
> case RTM_UD:
> - if ( (cm->feat.raw[0].b | cd->feat.raw[0].b) &
> + if ( (m->feat.raw[0].b | d->feat.raw[0].b) &
> (bitmaskof(X86_FEATURE_HLE) | bitmaskof(X86_FEATURE_RTM)) )
> fail(" HLE/RTM offered to guests despite not being available\n");
> break;
>
> case RTM_ABORT:
> - if ( cd->feat.raw[0].b &
> + if ( d->feat.raw[0].b &
> (bitmaskof(X86_FEATURE_HLE) | bitmaskof(X86_FEATURE_RTM)) )
> fail(" HLE/RTM offered to guests by default despite not being usable\n");
> break;
>
> case RTM_OK:
> - if ( !cm->feat.rtm || !cd->feat.rtm )
> + if ( !m->feat.rtm || !d->feat.rtm )
> fail(" RTM not offered to guests despite being available\n");
> break;
> }
>
> - if ( cd->feat.hle )
> + if ( d->feat.hle )
> fail(" Fail: HLE offered in default policy\n");
> }
>
On 30/03/2023 11:04 am, Jan Beulich wrote:
> On 29.03.2023 22:51, Andrew Cooper wrote:
>> /* Sanity test various invariants we expect in the default/max policies. */
>> static void test_guest_policies(const struct xc_cpu_policy *max,
>> const struct xc_cpu_policy *def)
>> {
>> - const struct cpuid_policy *cm = &max->cpuid;
>> - const struct cpuid_policy *cd = &def->cpuid;
>> - const struct msr_policy *mm = &max->msr;
>> - const struct msr_policy *md = &def->msr;
>> + const struct cpu_policy *m = &max->policy;
>> + const struct cpu_policy *d = &def->policy;
> Looks like you could reduce code churn by keeping "cm" and "cd" as the
> names, which would also be consistent with you continuing to use "cp"
> in hypervisor code.
It's a unit test - I'm not worried about churn; I'm more concerned about
the end readability.
I did consider swapping {d,m} with {max,def} so the logic below reads:
if ( ((max->feat.raw[0].d | def->feat.raw[0].d) &
but it occurs to me that because the policies are now merged, I can get
rid of all of the xc_cpu_policy passing and use cpu_policy instead.
(For inspection purposes, we don't care about the serialisation buffers
on the tail of xc_cpu_policy).
It will be a slightly larger change, but the end result will be better IMO.
~Andrew
© 2016 - 2026 Red Hat, Inc.