Re: [PATCH v2 1/2] tools/xg: Streamline cpu policy serialise/deserialise calls

[Alejandro Vallejo <alejandro.vallejo@cloud.com>]

On 20/05/2024 14:47, Roger Pau Monné wrote:
>> @@ -917,17 +922,14 @@ int xc_cpu_policy_set_domain(xc_interface *xch, uint32_t domid,
>>                               xc_cpu_policy_t *policy)
>>  {
>>      uint32_t err_leaf = -1, err_subleaf = -1, err_msr = -1;
>> -    unsigned int nr_leaves = ARRAY_SIZE(policy->leaves);
>> -    unsigned int nr_msrs = ARRAY_SIZE(policy->msrs);
>>      int rc;
>>  
>> -    rc = xc_cpu_policy_serialise(xch, policy, policy->leaves, &nr_leaves,
>> -                                 policy->msrs, &nr_msrs);
>> +    rc = xc_cpu_policy_serialise(xch, policy);
>>      if ( rc )
>>          return rc;
>>  
>> -    rc = xc_set_domain_cpu_policy(xch, domid, nr_leaves, policy->leaves,
>> -                                  nr_msrs, policy->msrs,
>> +    rc = xc_set_domain_cpu_policy(xch, domid, policy->nr_leaves, policy->leaves,
>> +                                  policy->nr_msrs, policy->msrs,
> 
> I would be tempted to just pass the policy to
> xc_set_domain_cpu_policy() and get rid of the separate cpuid and msrs
> serialized arrays, but that hides (or makes it less obvious) that the
> policy needs to be serialized before providing to
> xc_set_domain_cpu_policy().  Just a rant, no need to change it here.

I'm still pondering what to do about that. I'd like to refactor all that
faff away as well, but I'm not sure how to do it cleanly yet. The
biggest danger I see is modifying one side of the policy and then wiping
those changes by mistake reserializing or deserializing at the wrong time.

Not for this series, I reckon.


>> +int xc_cpu_policy_get_msrs(xc_interface *xch,
>> +                           const xc_cpu_policy_t *policy,
>> +                           const xen_msr_entry_t **msrs,
>> +                           uint32_t *nr)
>> +{
>> +    if ( !policy )
>> +    {
>> +        ERROR("Failed to fetch MSRs from policy object");
>> +        errno = -EINVAL;
>> +        return -1;
>> +    }
>> +
>> +    *msrs = policy->msrs;
>> +    *nr = policy->nr_msrs;
>> +
>> +    return 0;
>> +}
> 
> My preference would probably be to return NULL or
> xen_{leaf,msr}_entry_t * from those, as we can then avoid an extra
> leaves/msrs parameter.  Again I'm fine with leaving it like this.
> 

It didn't feel right to have an output parameter as the return value
doubling as status code when another output is in the parameter list. I
can perfectly imagine someone grabbing "nr" and ignoring "msrs" because
"msrs" doesn't happen to be needed for them.

I think there's extra safety in making it harder to ignore the error.

>> -    cpuid.length = nr_leaves * sizeof(xen_cpuid_leaf_t);
>> -    if ( cpuid.length )
>> +    record = (struct xc_sr_record) {
>> +        .type = REC_TYPE_X86_CPUID_POLICY,
>> +        .data = policy->leaves,
>> +        .length = policy->nr_leaves * sizeof(*policy->leaves),
>> +    };
>> +    if ( record.length )
>>      {
>> -        rc = write_record(ctx, &cpuid);
>> +        rc = write_record(ctx, &record);
>>          if ( rc )
>>              goto out;
>>      }
> 
> 
> You could maybe write this as:
> 
> if ( policy->nr_leaves )
> {
>     const struct xc_sr_record r = {
>         .type = REC_TYPE_X86_CPUID_POLICY,
>         .data = policy->leaves,
>         .length = policy->nr_leaves * sizeof(*policy->leaves),
>     };
> 
>     rc = write_record(ctx, &record);
> }
> 
> (same for the msr record)
> 

Ack. Looks nicer that way.

>>  
>> -    msrs.length = nr_msrs * sizeof(xen_msr_entry_t);
>> -    if ( msrs.length )
>> +    record = (struct xc_sr_record) {
>> +        .type = REC_TYPE_X86_MSR_POLICY,
>> +        .data = policy->msrs,
>> +        .length = policy->nr_msrs * sizeof(*policy->msrs),
>> +    };
>> +    if ( record.length )
>>      {
>> -        rc = write_record(ctx, &msrs);
>> +        rc = write_record(ctx, &record);
>>          if ( rc )
>>              goto out;
>>      }
>> @@ -100,8 +84,6 @@ int write_x86_cpu_policy_records(struct xc_sr_context *ctx)
>>      rc = 0;
>>  
>>   out:
>> -    free(cpuid.data);
>> -    free(msrs.data);
>>      xc_cpu_policy_destroy(policy);
>>  
>>      return rc;
>> diff --git a/tools/misc/xen-cpuid.c b/tools/misc/xen-cpuid.c
>> index 8893547bebce..1c9ba6d32060 100644
>> --- a/tools/misc/xen-cpuid.c
>> +++ b/tools/misc/xen-cpuid.c
>> @@ -409,17 +409,21 @@ static void dump_info(xc_interface *xch, bool detail)
>>      free(fs);
>>  }
>>  
>> -static void print_policy(const char *name,
>> -                         xen_cpuid_leaf_t *leaves, uint32_t nr_leaves,
>> -                         xen_msr_entry_t *msrs, uint32_t nr_msrs)
>> +static void print_policy(xc_interface *xch, const char *name, const xc_cpu_policy_t *policy)
> 
> Line length.

Ack

> 
>>  {
>> -    unsigned int l;
>> +    const xen_cpuid_leaf_t *leaves;
>> +    const xen_msr_entry_t *msrs;
>> +    uint32_t nr_leaves, nr_msrs;
>> +
>> +    if ( xc_cpu_policy_get_leaves(xch, policy, &leaves, &nr_leaves) ||
>> +         xc_cpu_policy_get_msrs(xch, policy, &msrs, &nr_msrs) )
>> +        err(1, "print_policy()");
> 
> Shouldn't the error message be "xc_cpu_policy_get_{leaves,msrs}()"
> instead, as one of those is the cause of the error?
> 
> Other err() usages do print the function triggering the error, not the
> function context name.
> 

Sure. I've demultiplexed them into separate conditionals as well now.

>>  
>>      printf("%s policy: %u leaves, %u MSRs\n", name, nr_leaves, nr_msrs);
>>      printf(" CPUID:\n");
>>      printf("  %-8s %-8s -> %-8s %-8s %-8s %-8s\n",
>>             "leaf", "subleaf", "eax", "ebx", "ecx", "edx");
>> -    for ( l = 0; l < nr_leaves; ++l )
>> +    for ( uint32_t l = 0; l < nr_leaves; ++l )
>>      {
>>          /* Skip empty leaves. */
>>          if ( !leaves[l].a && !leaves[l].b && !leaves[l].c && !leaves[l].d )
>> @@ -432,7 +436,7 @@ static void print_policy(const char *name,
>>  
>>      printf(" MSRs:\n");
>>      printf("  %-8s -> %-16s\n", "index", "value");
>> -    for ( l = 0; l < nr_msrs; ++l )
>> +    for ( uint32_t l = 0; l < nr_msrs; ++l )
> 
> I would be tempted to leave `l` as-is, seeing as there's no real need
> to modify it in the patch context, and the patch is already fairly
> long.

done

Cheers,
Alejandro