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

[Alejandro Vallejo <alejandro.vallejo@xxxxxxxxx>]

On 30/04/2024 15:42, Anthony PERARD wrote:
> On Wed, Feb 07, 2024 at 05:39:57PM +0000, Alejandro Vallejo wrote:
>> diff --git a/tools/libs/guest/xg_cpuid_x86.c 
>> b/tools/libs/guest/xg_cpuid_x86.c
>> index 5699a26b946e..cee0be80ba5b 100644
>> --- a/tools/libs/guest/xg_cpuid_x86.c
>> +++ b/tools/libs/guest/xg_cpuid_x86.c
>> @@ -772,49 +616,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 )
>> +                    cur->policy.extd.apic_id_size++;
>>  
>> -                p->policy.extd.nc = (p->policy.extd.nc << 1) | 1;
>> +                cur->policy.extd.nc = (cur->policy.extd.nc << 1) | 1;
>>              }
>>              break;
>>          }
>>      }
>>  
>> -    nr_leaves = ARRAY_SIZE(p->leaves);
>> -    rc = x86_cpuid_copy_to_buffer(&p->policy, p->leaves, &nr_leaves);
>> -    if ( rc )
>> +    if ( xend || msr )
>>      {
>> -        ERROR("Failed to serialise CPUID (%d = %s)", -rc, strerror(-rc));
>> -        goto out;
>> +        // The overrides are over the serialised form of the policy
> 
> Comments should use /* */

Ugh, yes.

> 
>> +        if ( (rc = xc_cpu_policy_serialise(xch, cur)) )
>> +            goto out;
>> +
>> +        if ( (rc = xc_cpuid_xend_policy(xch, domid, xend, host, def, cur)) )
>> +            goto out;
>> +        if ( (rc = xc_msr_policy(xch, domid, msr, host, def, cur)) )
>> +            goto out;
> 
> What if `xend` is set, but `msr` isn't? Looks like there's going to be a
> segv in xc_msr_policy() because it doesn't check that `msr` is actually
> set.
> 
> 
> Thanks,
> 

OOPS! Yes, msrs was meant to have the same check I added for
xc_cpuid_xend_policy. Will do.

Cheers,
Alejandro