From: Andrew Cooper <andrew.cooper3@citrix.com>
To: Roger Pau Monne <roger.pau@citrix.com>, <xen-devel@lists.xenproject.org>
Cc: Ian Jackson <iwj@xenproject.org>, Wei Liu <wl@xen.org>
Subject: Re: [PATCH 13/21] libs/guest: switch users of xc_set_domain_cpu_policy
Date: Thu, 1 Apr 2021 16:04:29 +0100 [thread overview]
Message-ID: <1cfae8d7-3708-79e2-715d-dea7f7ff5b2e@citrix.com> (raw)
In-Reply-To: <20210323095849.37858-14-roger.pau@citrix.com>
On 23/03/2021 09:58, Roger Pau Monne wrote:
> To use the new cpu policy interface xc_cpu_policy_set_domain. Note
> that xc_set_domain_cpu_policy is removed from the interface and the
> function is made static to xg_cpuid_x86.c for it's internal callers.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> tools/include/xenctrl.h | 5 -----
> tools/libs/guest/xg_cpuid_x86.c | 22 +++++++++++-----------
> tools/libs/guest/xg_sr_common_x86.c | 28 +++++++++++++++++++++-------
> 3 files changed, 32 insertions(+), 23 deletions(-)
>
> diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
> index 46f5026081c..164a287b367 100644
> --- a/tools/include/xenctrl.h
> +++ b/tools/include/xenctrl.h
> @@ -2625,11 +2625,6 @@ int xc_get_cpu_featureset(xc_interface *xch, uint32_t index,
>
> int xc_cpu_policy_get_size(xc_interface *xch, uint32_t *nr_leaves,
> uint32_t *nr_msrs);
> -int xc_set_domain_cpu_policy(xc_interface *xch, uint32_t domid,
> - uint32_t nr_leaves, xen_cpuid_leaf_t *leaves,
> - uint32_t nr_msrs, xen_msr_entry_t *msrs,
> - uint32_t *err_leaf_p, uint32_t *err_subleaf_p,
> - uint32_t *err_msr_p);
>
> uint32_t xc_get_cpu_featureset_size(void);
>
> diff --git a/tools/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c
> index 07756743e76..f7b662f31aa 100644
> --- a/tools/libs/guest/xg_cpuid_x86.c
> +++ b/tools/libs/guest/xg_cpuid_x86.c
> @@ -204,11 +204,11 @@ static int get_domain_cpu_policy(xc_interface *xch, uint32_t domid,
> return ret;
> }
>
> -int xc_set_domain_cpu_policy(xc_interface *xch, uint32_t domid,
> - uint32_t nr_leaves, xen_cpuid_leaf_t *leaves,
> - uint32_t nr_msrs, xen_msr_entry_t *msrs,
> - uint32_t *err_leaf_p, uint32_t *err_subleaf_p,
> - uint32_t *err_msr_p)
> +static int set_domain_cpu_policy(xc_interface *xch, uint32_t domid,
> + uint32_t nr_leaves, xen_cpuid_leaf_t *leaves,
> + uint32_t nr_msrs, xen_msr_entry_t *msrs,
> + uint32_t *err_leaf_p, uint32_t *err_subleaf_p,
> + uint32_t *err_msr_p)
> {
> DECLARE_DOMCTL;
> DECLARE_HYPERCALL_BOUNCE(leaves,
> @@ -405,8 +405,8 @@ static int xc_cpuid_xend_policy(
> }
>
> /* Feed the transformed currrent policy back up to Xen. */
> - rc = xc_set_domain_cpu_policy(xch, domid, nr_cur, cur, 0, NULL,
> - &err_leaf, &err_subleaf, &err_msr);
> + rc = set_domain_cpu_policy(xch, domid, nr_cur, cur, 0, NULL,
> + &err_leaf, &err_subleaf, &err_msr);
> if ( rc )
> {
> PERROR("Failed to set d%d's policy (err leaf %#x, subleaf %#x, msr %#x)",
> @@ -638,8 +638,8 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
> goto out;
> }
>
> - rc = xc_set_domain_cpu_policy(xch, domid, nr_leaves, leaves, 0, NULL,
> - &err_leaf, &err_subleaf, &err_msr);
> + rc = set_domain_cpu_policy(xch, domid, nr_leaves, leaves, 0, NULL,
> + &err_leaf, &err_subleaf, &err_msr);
> if ( rc )
> {
> PERROR("Failed to set d%d's policy (err leaf %#x, subleaf %#x, msr %#x)",
> @@ -833,8 +833,8 @@ int xc_cpu_policy_set_domain(xc_interface *xch, uint32_t domid,
> if ( rc )
> goto out;
>
> - rc = xc_set_domain_cpu_policy(xch, domid, nr_leaves, leaves, nr_msrs, msrs,
> - &err_leaf, &err_subleaf, &err_msr);
> + rc = set_domain_cpu_policy(xch, domid, nr_leaves, leaves, nr_msrs, msrs,
> + &err_leaf, &err_subleaf, &err_msr);
> if ( rc )
> {
> ERROR("Failed to set domain %u policy (%d = %s)", domid, -rc,
> diff --git a/tools/libs/guest/xg_sr_common_x86.c b/tools/libs/guest/xg_sr_common_x86.c
> index 15265e7a331..02f0c3ae9ed 100644
> --- a/tools/libs/guest/xg_sr_common_x86.c
> +++ b/tools/libs/guest/xg_sr_common_x86.c
> @@ -151,7 +151,10 @@ int x86_static_data_complete(struct xc_sr_context *ctx, unsigned int *missing)
> {
> xc_interface *xch = ctx->xch;
> uint32_t nr_leaves = 0, nr_msrs = 0;
> - uint32_t err_l = ~0, err_s = ~0, err_m = ~0;
> + xc_cpu_policy_t policy = xc_cpu_policy_init();
> +
> + if ( !policy )
> + return -1;
>
> if ( ctx->x86.restore.cpuid.ptr )
> nr_leaves = ctx->x86.restore.cpuid.size / sizeof(xen_cpuid_leaf_t);
> @@ -163,14 +166,25 @@ int x86_static_data_complete(struct xc_sr_context *ctx, unsigned int *missing)
> else
> *missing |= XGR_SDD_MISSING_MSR;
>
> + if ( nr_leaves &&
> + xc_cpu_policy_update_cpuid(xch, policy,
> + ctx->x86.restore.cpuid.ptr, nr_leaves) )
> + {
> + PERROR("Failed to update CPUID policy");
> + return -1;
> + }
> + if ( nr_msrs &&
> + xc_cpu_policy_update_msrs(xch, policy,
> + ctx->x86.restore.msr.ptr, nr_msrs) )
> + {
> + PERROR("Failed to update MSR policy");
> + return -1;
> + }
> +
> if ( (nr_leaves || nr_msrs) &&
> - xc_set_domain_cpu_policy(xch, ctx->domid,
> - nr_leaves, ctx->x86.restore.cpuid.ptr,
> - nr_msrs, ctx->x86.restore.msr.ptr,
> - &err_l, &err_s, &err_m) )
> + xc_cpu_policy_set_domain(xch, ctx->domid, policy) )
> {
> - PERROR("Failed to set CPUID policy: leaf %08x, subleaf %08x, msr %08x",
> - err_l, err_s, err_m);
> + PERROR("Failed to set CPUID policy");
> return -1;
> }
I don't think this is a good move.
All it does is waste time shuffling data in userspace during the VM
downtime window. The format of CPUID/MSR data in the migration stream
is (deliberately) already in the form to hand it straight back to Xen,
and there will never be additional policy changes here (because that
would break the VM).
I'd just drop the patch entirely. I'm not even certain if we want to
remove the thin hypercall wrappers - I'm pretty sure some of my low
level unit testing plans will want the raw accessors without being
forced through the xc_cpuid_policy_t interface.
~Andrew
next prev parent reply other threads:[~2021-04-01 15:05 UTC|newest]
Thread overview: 52+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-03-23 9:58 [PATCH 00/21] libs/guest: new CPUID/MSR interface Roger Pau Monne
2021-03-23 9:58 ` [PATCH 01/21] libxl: don't ignore the return value from xc_cpuid_apply_policy Roger Pau Monne
2021-03-30 15:21 ` Jan Beulich
2021-03-30 15:38 ` Andrew Cooper
2021-03-31 18:12 ` Andrew Cooper
2021-03-23 9:58 ` [PATCH 02/21] libs/guest: rename xc_get_cpu_policy_size to xc_cpu_policy_get_size Roger Pau Monne
2021-03-31 19:03 ` Andrew Cooper
2021-03-23 9:58 ` [PATCH 03/21] libs/guest: introduce xc_cpu_policy_t Roger Pau Monne
2021-03-31 20:10 ` Andrew Cooper
2021-04-01 8:48 ` Roger Pau Monné
2021-03-23 9:58 ` [PATCH 04/21] libs/guest: introduce helper to fetch a system cpu policy Roger Pau Monne
2021-03-30 15:35 ` Jan Beulich
2021-04-01 13:56 ` Andrew Cooper
2021-03-23 9:58 ` [PATCH 05/21] libs/guest: introduce helper to fetch a domain " Roger Pau Monne
2021-03-30 15:37 ` Jan Beulich
2021-03-31 11:06 ` Roger Pau Monné
2021-04-01 13:32 ` Andrew Cooper
2021-03-23 9:58 ` [PATCH 06/21] libs/guest: introduce helper to serialize a " Roger Pau Monne
2021-03-23 9:58 ` [PATCH 07/21] tools: switch existing users of xc_get_{system,domain}_cpu_policy Roger Pau Monne
2021-03-23 9:58 ` [PATCH 08/21] libs/guest: introduce a helper to apply a cpu policy to a domain Roger Pau Monne
2021-03-23 9:58 ` [PATCH 09/21] libs/guest: allow fetching a specific CPUID leaf from a cpu policy Roger Pau Monne
2021-04-01 14:47 ` Andrew Cooper
2021-04-08 15:53 ` Roger Pau Monné
2021-03-23 9:58 ` [PATCH 10/21] libs/guest: allow fetching a specific MSR entry " Roger Pau Monne
2021-03-23 9:58 ` [PATCH 11/21] libs/guest: allow updating a cpu policy CPUID data Roger Pau Monne
2021-03-30 15:56 ` Jan Beulich
2021-03-31 12:47 ` Roger Pau Monné
2021-04-01 14:55 ` Andrew Cooper
2021-03-23 9:58 ` [PATCH 12/21] libs/guest: allow updating a cpu policy MSR data Roger Pau Monne
2021-03-23 9:58 ` [PATCH 13/21] libs/guest: switch users of xc_set_domain_cpu_policy Roger Pau Monne
2021-04-01 15:04 ` Andrew Cooper [this message]
2021-03-23 9:58 ` [PATCH 14/21] libs/guest: introduce helper to check cpu policy compatibility Roger Pau Monne
2021-03-30 16:02 ` Jan Beulich
2021-03-31 12:40 ` Roger Pau Monné
2021-03-31 14:57 ` Jan Beulich
2021-04-01 16:14 ` Andrew Cooper
2021-03-23 9:58 ` [PATCH 15/21] libs/guest: obtain a compatible cpu policy from two input ones Roger Pau Monne
2021-03-31 15:24 ` Jan Beulich
2021-04-01 16:26 ` Andrew Cooper
2021-04-09 10:56 ` Roger Pau Monné
2021-03-23 9:58 ` [PATCH 16/21] libs/guest: make a cpu policy compatible with older Xen versions Roger Pau Monne
2021-03-31 15:29 ` Jan Beulich
2021-04-01 16:31 ` Andrew Cooper
2021-03-23 9:58 ` [PATCH 17/21] libs/guest: introduce helper set cpu topology in cpu policy Roger Pau Monne
2021-04-01 17:22 ` Andrew Cooper
2021-03-23 9:58 ` [PATCH 18/21] libs/guest: rework xc_cpuid_xend_policy Roger Pau Monne
2021-03-23 9:58 ` [PATCH 19/21] libs/guest: apply a featureset into a cpu policy Roger Pau Monne
2021-03-23 9:58 ` [PATCH 20/21] libs/{light,guest}: implement xc_cpuid_apply_policy in libxl Roger Pau Monne
2021-04-01 17:44 ` Andrew Cooper
2021-04-09 14:57 ` Roger Pau Monné
2021-03-23 9:58 ` [PATCH 21/21] libs/guest: (re)move xc_cpu_policy_apply_cpuid Roger Pau Monne
2021-04-01 17:53 ` Andrew Cooper
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1cfae8d7-3708-79e2-715d-dea7f7ff5b2e@citrix.com \
--to=andrew.cooper3@citrix.com \
--cc=iwj@xenproject.org \
--cc=roger.pau@citrix.com \
--cc=wl@xen.org \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).