Re: [PATCH 3/9] tools/libx[cl]: Move processing loop down into xc_cpuid_set()

[Jan Beulich <jbeulich@xxxxxxxx>]

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@xxxxxxxxxx>

Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
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