Re: [PATCH v2 2/2] tools/xg: Clean up xend-style overrides for CPU policies

[Alejandro Vallejo <alejandro.vallejo@xxxxxxxxx>]

On 20/05/2024 16:02, Roger Pau Monné wrote:
>> -
>>  static int xc_msr_policy(xc_interface *xch, domid_t domid,
>> -                         const struct xc_msr *msr)
>> +                         const struct xc_msr *msr,
>> +                         xc_cpu_policy_t *host,
>> +                         xc_cpu_policy_t *def,
> 
> host and def should likely be const?

I tried, but I can't. All policies go through find_msr(), which takes a
non-const policy, and must be non-const because it's also used for the
cur policy.

I did the next best thing (I think) by const-ifying the result of
find_msr inside the loop for host and def. Same thing on the cpuid function.

>> -    if ( rc )
>> -    {
>> -        PERROR("Failed to obtain host policy");
>> -        rc = -errno;
>> -        goto out;
>> -    }
>> +    if ( !msrs )
> 
> Does this build?  Where is 'msrs' defined in this context?  The
> function parameter is 'msr' AFAICT.

Ugh. I fixed that while adjusting it for testing within XenServer and
then neglected to make the change in the actual for-upstream patches.

You're right.

> 
>> +        return 0;
> 
> Should we also check for host, def, cur != NULL also?

It's already done by the caller, but can do out of paranoia; returning
-EINVAL.

>> @@ -583,14 +436,16 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t 
>> domid, bool restore,
>>      int rc;
>>      bool hvm;
>>      xc_domaininfo_t di;
>> -    struct xc_cpu_policy *p = xc_cpu_policy_init();
>> -    unsigned int i, nr_leaves = ARRAY_SIZE(p->leaves), nr_msrs = 0;
>> -    uint32_t err_leaf = -1, err_subleaf = -1, err_msr = -1;
>> -    uint32_t host_featureset[FEATURESET_NR_ENTRIES] = {};
>> -    uint32_t len = ARRAY_SIZE(host_featureset);
>>  
>> -    if ( !p )
>> -        return -ENOMEM;
>> +    struct xc_cpu_policy *host = xc_cpu_policy_init();
>> +    struct xc_cpu_policy *def = xc_cpu_policy_init();
> 
> I would be helpful to have some kind of mechanism to allocate + init a
> policy at the same time, so that the resulting object could be made
> const here.  (Not that you need to do it in this patch).

That would seem sensible, but we'd also need a way to clone it to avoid
repeating hypercalls when they aren't required. I had a patch that did
that, but was quite complicated for other reasons. I might get back to
it at some point now that per-vCPU policies don't seem to be required.

>> @@ -695,24 +542,24 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t 
>> domid, bool restore,
>>                   !(dfs = x86_cpu_policy_lookup_deep_deps(b)) )
>>                  continue;
>>  
>> -            for ( i = 0; i < ARRAY_SIZE(disabled_features); ++i )
>> +            for ( size_t i = 0; i < ARRAY_SIZE(disabled_features); ++i )
> 
> All this loop index type changes could be done as a separate patch,
> you are not even touching the surrounding lines.  It adds a lot of
> churn to this patch for no reason IMO.

I got carried away. Let me revert that. I still want to get rid of all
those overscoped indices, but this is not the patch for it.

>> @@ -772,49 +619,45 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t 
>> domid, bool restore,
>>               * apic_id_size values greater than 7.  Limit the value to
>>               * 7 for now.
>>               */
>> -            if ( p->policy.extd.nc < 0x7f )
>> +            if ( cur->policy.extd.nc < 0x7f )
>>              {
>> -                if ( p->policy.extd.apic_id_size != 0 && 
>> p->policy.extd.apic_id_size < 0x7 )
>> -                    p->policy.extd.apic_id_size++;
>> +                if ( cur->policy.extd.apic_id_size != 0 && 
>> cur->policy.extd.apic_id_size < 0x7 )
> 
> I would split the line while there, it's overly long.

Ack

> 
> Thanks, Roger.

Cheers,
Alejandro