Re: [PATCH] tools/cpuid: Plumb nested_virt down into xc_cpuid_apply_policy()

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 29/09/2020 15:05, Jan Beulich wrote:
> On 29.09.2020 15:48, Andrew Cooper wrote:
>> --- a/tools/libs/guest/xg_cpuid_x86.c
>> +++ b/tools/libs/guest/xg_cpuid_x86.c
>> @@ -427,7 +427,7 @@ static int xc_cpuid_xend_policy(
>>  
>>  int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
>>                            const uint32_t *featureset, unsigned int 
>> nr_features,
>> -                          bool pae, bool itsc,
>> +                          bool pae, bool itsc, bool nested_virt,
> This increasing number of bools next to each other bears an
> increasing risk of callers getting the order wrong. Luckily
> there's just one within the tree, so perhaps not an immediate
> problem.

As the commit message said, this is the final special case.

This prototype won't grow any further, although it needs to remain until
Xen 4.13 is thoroughly obsolete, and we're not liable to find a pre-4.14
VM which lacks CPUID data in its migration stream.

>> @@ -559,7 +559,11 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t 
>> domid, bool restore,
>>          p->extd.itsc = itsc;
>>  
>>          if ( di.hvm )
>> +        {
>>              p->basic.pae = pae;
>> +            p->basic.vmx = nested_virt;
>> +            p->extd.svm = nested_virt;
>> +        }
>>      }
>>  
>>      if ( !di.hvm )
>> @@ -625,14 +629,6 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t 
>> domid, bool restore,
>>              }
>>              break;
>>          }
>> -
>> -        /*
>> -         * These settings are necessary to cause earlier HVM_PARAM_NESTEDHVM
>> -         * to be reflected correctly in CPUID.  Xen will discard these bits 
>> if
>> -         * configuration hasn't been set for the domain.
>> -         */
>> -        p->basic.vmx = true;
>> -        p->extd.svm = true;
>>      }
> While I can see how the first sentence in the comment has become
> irrelevant now, what about the second?

Well - I'm writing the series to actually make it irrelevant.

> It's still odd to see both
> svm and vmx getting set to identical values. Therefore perhaps
> worth to retain a shorter comment there?

The comment is true for every feature bit, and is not special to
vmx/svm.  With the absence of the first part, the second part isn't
relevant.

~Andrew