| Message ID | 20200615141532.1927-4-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | XSA-320 follow for IvyBridge | expand |
Andrew Cooper writes ("[PATCH 3/9] tools/libx[cl]: Move processing loop down into xc_cpuid_set()"):
> 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.
This is not my area of expertise. Ideally at this stage of the
release I would like an ack from a 2nd hypervisor maintainer.
The processing code in libxc looks OK to me.
Ian.
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
On 16/06/2020 10:16, Jan Beulich wrote: > 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. All fixed. > >> + { >> + 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 ... No - it won't change. The legacy xend adjustments have always been based on the max featureset, and changing it will break VM migration. The soon-to-exist (4.15 at this point) brand new "what do I do for a fresh boot" path will do things differently even for the legacy Xend leaves, but the logic here must not semantically change. ~Andrew
diff --git a/tools/libxc/xc_cpuid_x86.c b/tools/libxc/xc_cpuid_x86.c index edc2ad9b47..e827c48fd1 100644 --- a/tools/libxc/xc_cpuid_x86.c +++ b/tools/libxc/xc_cpuid_x86.c @@ -38,8 +38,6 @@ enum { #define bitmaskof(idx) (1u << ((idx) & 31)) #define featureword_of(idx) ((idx) >> 5) -#define clear_feature(idx, dst) ((dst) &= ~bitmaskof(idx)) -#define set_feature(idx, dst) ((dst) |= bitmaskof(idx)) int xc_get_cpu_levelling_caps(xc_interface *xch, uint32_t *caps) { @@ -259,15 +257,42 @@ int xc_set_domain_cpu_policy(xc_interface *xch, uint32_t domid, return ret; } +static int compare_leaves(const void *l, const void *r) +{ + const xen_cpuid_leaf_t *lhs = l; + const xen_cpuid_leaf_t *rhs = r; + + if ( lhs->leaf != rhs->leaf ) + return lhs->leaf < rhs->leaf ? -1 : 1; + + if ( lhs->subleaf != rhs->subleaf ) + return lhs->subleaf < rhs->subleaf ? -1 : 1; + + return 0; +} + +static xen_cpuid_leaf_t *find_leaf( + xen_cpuid_leaf_t *leaves, unsigned int nr_leaves, + const struct xc_xend_cpuid *xend) +{ + const xen_cpuid_leaf_t key = { xend->leaf, xend->subleaf }; + + return bsearch(&key, leaves, nr_leaves, sizeof(*leaves), compare_leaves); +} + int xc_cpuid_set( xc_interface *xch, uint32_t domid, const struct xc_xend_cpuid *xend) { int rc; - unsigned int i, j, regs[4] = {}, polregs[4] = {}; xc_dominfo_t di; - xen_cpuid_leaf_t *leaves = NULL; - unsigned int nr_leaves, policy_leaves, nr_msrs; + unsigned int nr_leaves, nr_msrs; uint32_t err_leaf = -1, err_subleaf = -1, err_msr = -1; + /* + * Three full policies. The host, domain max, and domain current for the + * domain type. + */ + xen_cpuid_leaf_t *host = NULL, *max = NULL, *cur = NULL; + unsigned int nr_host, nr_max, nr_cur; if ( xc_domain_getinfo(xch, domid, 1, &di) != 1 || di.domid != domid ) @@ -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++ ) { - 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++ ) + { + 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); + else if ( xend->policy[i][j] == 'k' || + xend->policy[i][j] == 's' ) + val = test_bit(31 - j, host_reg); + else + { + ERROR("Bad character '%c' in policy[%d] string '%s'", + xend->policy[i][j], i, xend->policy[i]); + goto fail; + } + + clear_bit(31 - j, cur_reg); + if ( val ) + set_bit(31 - j, cur_reg); + } } } - /* Feed the transformed leaf back up to Xen. */ - leaves[0] = (xen_cpuid_leaf_t){ xend->leaf, xend->subleaf, - regs[0], regs[1], regs[2], regs[3] }; - rc = xc_set_domain_cpu_policy(xch, domid, 1, leaves, 0, NULL, + /* 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); if ( rc ) { @@ -391,7 +418,9 @@ int xc_cpuid_set( /* Success! */ fail: - free(leaves); + free(cur); + free(max); + free(host); return rc; } diff --git a/tools/libxl/libxl_cpuid.c b/tools/libxl/libxl_cpuid.c index e001cbcd4f..6f7cf2054e 100644 --- a/tools/libxl/libxl_cpuid.c +++ b/tools/libxl/libxl_cpuid.c @@ -420,7 +420,6 @@ void libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid, libxl_domain_build_info *info) { libxl_cpuid_policy_list cpuid = info->cpuid; - int i; bool pae = true; /* @@ -441,8 +440,7 @@ void libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid, if (!cpuid) return; - for (i = 0; cpuid[i].input[0] != XEN_CPUID_INPUT_UNUSED; i++) - xc_cpuid_set(ctx->xch, domid, &cpuid[i]); + xc_cpuid_set(ctx->xch, domid, info->cpuid); } static const char *input_names[2] = { "leaf", "subleaf" };
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> --- CC: Ian Jackson <Ian.Jackson@citrix.com> CC: Wei Liu <wl@xen.org> CC: Jan Beulich <JBeulich@suse.com> CC: Wei Liu <wl@xen.org> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Paul Durrant <paul@xen.org> --- tools/libxc/xc_cpuid_x86.c | 159 +++++++++++++++++++++++++++------------------ tools/libxl/libxl_cpuid.c | 4 +- 2 files changed, 95 insertions(+), 68 deletions(-)