From: Jan Beulich <jbeulich@suse.com>
To: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: "Paul Durrant" <paul@xen.org>,
Xen-devel <xen-devel@lists.xenproject.org>,
"Roger Pau Monné" <roger.pau@citrix.com>, "Wei Liu" <wl@xen.org>,
"Ian Jackson" <Ian.Jackson@citrix.com>
Subject: Re: [PATCH 3/9] tools/libx[cl]: Move processing loop down into xc_cpuid_set()
Date: Tue, 16 Jun 2020 11:16:32 +0200 [thread overview]
Message-ID: <7ac49edf-1a8c-435d-0b5b-96496432e9f6@suse.com> (raw)
In-Reply-To: <20200615141532.1927-4-andrew.cooper3@citrix.com>
On 15.06.2020 16:15, Andrew Cooper wrote:
> Currently, libxl__cpuid_legacy() passes each element of the policy list to
> xc_cpuid_set() individually. This is wasteful both in terms of the number of
> hypercalls made, and the quantity of repeated merging/auditing work performed
> by Xen.
>
> Move the loop processing down into xc_cpuid_set(), which allows us to do one
> set of hypercalls, rather than one per list entry.
>
> In xc_cpuid_set(), obtain the full host, guest max and current policies to
> begin with, and loop over the xend array, processing one leaf at a time.
> Replace the linear search with a binary search, seeing as the serialised
> leaves are sorted.
>
> No change in behaviour from the guests point of view.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
with a few remarks:
> @@ -286,99 +311,101 @@ int xc_cpuid_set(
> }
>
> rc = -ENOMEM;
> - if ( (leaves = calloc(nr_leaves, sizeof(*leaves))) == NULL )
> + if ( (host = calloc(nr_leaves, sizeof(*host))) == NULL ||
> + (max = calloc(nr_leaves, sizeof(*max))) == NULL ||
> + (cur = calloc(nr_leaves, sizeof(*cur))) == NULL )
> {
> ERROR("Unable to allocate memory for %u CPUID leaves", nr_leaves);
> goto fail;
> }
>
> + /* Get the domain's current policy. */
> + nr_msrs = 0;
> + nr_cur = nr_leaves;
> + rc = xc_get_domain_cpu_policy(xch, domid, &nr_cur, cur, &nr_msrs, NULL);
> + if ( rc )
> + {
> + PERROR("Failed to obtain d%d current policy", domid);
> + rc = -errno;
> + goto fail;
> + }
> +
> /* Get the domain's max policy. */
> nr_msrs = 0;
> - policy_leaves = nr_leaves;
> + nr_max = nr_leaves;
> rc = xc_get_system_cpu_policy(xch, di.hvm ? XEN_SYSCTL_cpu_policy_hvm_max
> : XEN_SYSCTL_cpu_policy_pv_max,
> - &policy_leaves, leaves, &nr_msrs, NULL);
> + &nr_max, max, &nr_msrs, NULL);
> if ( rc )
> {
> PERROR("Failed to obtain %s max policy", di.hvm ? "hvm" : "pv");
> rc = -errno;
> goto fail;
> }
> - for ( i = 0; i < policy_leaves; ++i )
> - if ( leaves[i].leaf == xend->leaf &&
> - leaves[i].subleaf == xend->subleaf )
> - {
> - polregs[0] = leaves[i].a;
> - polregs[1] = leaves[i].b;
> - polregs[2] = leaves[i].c;
> - polregs[3] = leaves[i].d;
> - break;
> - }
>
> /* Get the host policy. */
> nr_msrs = 0;
> - policy_leaves = nr_leaves;
> + nr_host = nr_leaves;
> rc = xc_get_system_cpu_policy(xch, XEN_SYSCTL_cpu_policy_host,
> - &policy_leaves, leaves, &nr_msrs, NULL);
> + &nr_host, host, &nr_msrs, NULL);
> if ( rc )
> {
> PERROR("Failed to obtain host policy");
> rc = -errno;
> goto fail;
> }
> - for ( i = 0; i < policy_leaves; ++i )
> - if ( leaves[i].leaf == xend->leaf &&
> - leaves[i].subleaf == xend->subleaf )
> - {
> - regs[0] = leaves[i].a;
> - regs[1] = leaves[i].b;
> - regs[2] = leaves[i].c;
> - regs[3] = leaves[i].d;
> - break;
> - }
>
> - for ( i = 0; i < 4; i++ )
> + rc = -EINVAL;
> + for ( ; xend->leaf != XEN_CPUID_INPUT_UNUSED; ++xend )
> {
> - if ( xend->policy[i] == NULL )
> + xen_cpuid_leaf_t *cur_leaf = find_leaf(cur, nr_cur, xend);
> + const xen_cpuid_leaf_t *max_leaf = find_leaf(max, nr_max, xend);
> + const xen_cpuid_leaf_t *host_leaf = find_leaf(host, nr_host, xend);
> +
> + if ( cur_leaf == NULL || max_leaf == NULL || host_leaf == NULL )
> {
> - regs[i] = polregs[i];
> - continue;
> + ERROR("Missing leaf %#x, subleaf %#x", xend->leaf, xend->subleaf);
> + goto fail;
> }
>
> - /*
> - * Notes for following this algorithm:
> - *
> - * While it will accept any leaf data, it only makes sense to use on
> - * feature leaves. regs[] initially contains the host values. This,
> - * with the fall-through chain, is how the 's' and 'k' options work.
> - */
> - for ( j = 0; j < 32; j++ )
> + for ( int i = 0; i < 4; i++ )
As you move the declaration here, perhaps switch to unsigned int
as well? And express 4 as ARRAY_SIZE()?
> {
> - unsigned char val = !!((regs[i] & (1U << (31 - j))));
> - unsigned char polval = !!((polregs[i] & (1U << (31 - j))));
> -
> - rc = -EINVAL;
> - if ( !strchr("10xks", xend->policy[i][j]) )
> - goto fail;
> -
> - if ( xend->policy[i][j] == '1' )
> - val = 1;
> - else if ( xend->policy[i][j] == '0' )
> - val = 0;
> - else if ( xend->policy[i][j] == 'x' )
> - val = polval;
> -
> - if ( val )
> - set_feature(31 - j, regs[i]);
> - else
> - clear_feature(31 - j, regs[i]);
> + uint32_t *cur_reg = &cur_leaf->a + i;
> + const uint32_t *max_reg = &max_leaf->a + i;
> + const uint32_t *host_reg = &host_leaf->a + i;
> +
> + if ( xend->policy[i] == NULL )
> + continue;
> +
> + for ( int j = 0; j < 32; j++ )
unsigned int again? I don't think there's a suitable array available
to also use ARRAY_SIZE() here.
> + {
> + bool val;
> +
> + if ( xend->policy[i][j] == '1' )
> + val = true;
> + else if ( xend->policy[i][j] == '0' )
> + val = false;
> + else if ( xend->policy[i][j] == 'x' )
> + val = test_bit(31 - j, max_reg);
Still seeing "max" used here is somewhat confusing given the purpose
of the series, and merely judging from the titles I can't yet spot
where later on it'll change. But I assume it will ...
Jan
next prev parent reply other threads:[~2020-06-16 9:16 UTC|newest]
Thread overview: 40+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-06-15 14:15 [PATCH for-4.14 0/9] XSA-320 follow for IvyBridge Andrew Cooper
2020-06-15 14:15 ` [PATCH 1/9] tools/libx[cl]: Introduce struct xc_xend_cpuid for xc_cpuid_set() Andrew Cooper
2020-06-15 14:51 ` Ian Jackson
2020-06-15 14:15 ` [PATCH 2/9] tests/cpu-policy: Confirm that CPUID serialisation is sorted Andrew Cooper
2020-06-15 14:52 ` Ian Jackson
2020-06-15 15:00 ` Andrew Cooper
2020-06-15 15:34 ` Ian Jackson
2020-06-15 16:12 ` Andrew Cooper
2020-06-16 6:51 ` Jan Beulich
2020-06-16 9:01 ` Jan Beulich
2020-06-15 14:15 ` [PATCH 3/9] tools/libx[cl]: Move processing loop down into xc_cpuid_set() Andrew Cooper
2020-06-15 14:54 ` Ian Jackson
2020-06-16 9:16 ` Jan Beulich [this message]
2020-06-16 15:58 ` Andrew Cooper
2020-06-15 14:15 ` [PATCH 4/9] tools/libx[cl]: Merge xc_cpuid_set() into xc_cpuid_apply_policy() Andrew Cooper
2020-06-15 14:55 ` Ian Jackson
2020-06-15 14:15 ` [PATCH 5/9] tools/libx[cl]: Plumb bool restore down " Andrew Cooper
2020-06-15 14:55 ` Ian Jackson
2020-06-15 14:15 ` [PATCH 6/9] x86/gen-cpuid: Distinguish default vs max in feature annotations Andrew Cooper
2020-06-15 14:15 ` [PATCH 7/9] x86/hvm: Disable MPX by default Andrew Cooper
2020-06-16 9:33 ` Jan Beulich
2020-06-16 16:15 ` Andrew Cooper
2020-06-17 10:32 ` Jan Beulich
2020-06-17 11:16 ` Andrew Cooper
2020-06-17 11:24 ` Jan Beulich
2020-06-17 11:28 ` Andrew Cooper
2020-06-17 11:41 ` Jan Beulich
2020-06-17 11:47 ` Andrew Cooper
2020-06-15 14:15 ` [PATCH 8/9] x86/cpuid: Introduce missing feature adjustment in calculate_pv_def_policy() Andrew Cooper
2020-06-16 9:40 ` Jan Beulich
2020-06-16 16:17 ` Andrew Cooper
2020-06-15 14:15 ` [PATCH 9/9] x86/spec-ctrl: Hide RDRAND by default on IvyBridge Andrew Cooper
2020-06-16 10:00 ` Jan Beulich
2020-06-16 16:26 ` Andrew Cooper
2020-06-17 10:39 ` Jan Beulich
2020-06-17 11:21 ` Andrew Cooper
2020-06-15 17:04 ` [PATCH for-4.14 0/9] XSA-320 follow for IvyBridge Paul Durrant
2020-06-17 12:46 ` Paul Durrant
2020-06-18 7:18 ` Jan Beulich
2020-06-18 9:37 ` 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=7ac49edf-1a8c-435d-0b5b-96496432e9f6@suse.com \
--to=jbeulich@suse.com \
--cc=Ian.Jackson@citrix.com \
--cc=andrew.cooper3@citrix.com \
--cc=paul@xen.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).