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

[Alejandro Vallejo <alejandro.vallejo@xxxxxxxxx>]

On 16/05/2024 16:37, Alejandro Vallejo wrote:
> 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

Ugh answered to the old email. pinging to the xenproject one now.

Cheers,
Alejandro