Re: [PATCH RFC 14/31] tools/libxc: Use featureset rather than guesswork

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 05/01/16 15:54, Ian Campbell wrote:
> On Wed, 2015-12-16 at 21:24 +0000, Andrew Cooper wrote:
>> It is conceptually wrong to base a VM's featureset on the features
>> visible to
>> the toolstack which happens to construct it.
>>
>> Instead, the featureset used is either an explicit one passed by the
>> toolstack, or the default which Xen believes it can give to the guest.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: Ian Campbell <Ian.Campbell@citrix.com>
>> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
>> CC: Wei Liu <wei.liu2@citrix.com>
>> ---
>>  tools/libxc/xc_cpuid_x86.c | 229 +++++++++++++------------------------
>> --------
>>  1 file changed, 64 insertions(+), 165 deletions(-)
>>
>> diff --git a/tools/libxc/xc_cpuid_x86.c b/tools/libxc/xc_cpuid_x86.c
>> index 8bd3126..3f39306 100644
>> --- a/tools/libxc/xc_cpuid_x86.c
>> +++ b/tools/libxc/xc_cpuid_x86.c
>> @@ -24,6 +24,7 @@
>>  #include "xc_private.h"
>>  #include <xen/arch-x86/featureset.h>
>>  #include <xen/hvm/params.h>
>> +#include <xen/sysctl.h>
>>  
>>  #define bitmaskof(idx)      (1u << ((idx) & 31))
>>  #define clear_bit(idx, dst) ((dst) &= ~bitmaskof(idx))
>> @@ -211,37 +212,19 @@ static void amd_xc_cpuid_policy(xc_interface *xch,
>>              regs[0] = DEF_MAX_AMDEXT;
>>          break;
>>  
>> -    case 0x80000001: {
>> +    case 0x80000001:
>>          if ( !info->pae )
>>              clear_bit(X86_FEATURE_PAE, regs[3]);
> This (and the others which actually change in this patch) are correct
> because the locally defined clear_bit (contrary to what one might usually
> expect) includes masking off the lower bits to get the offset in the given
> word, is that right?
>
> That confused me a lot until I spotted the clear_bit in the context above.
> What do you think about renaming it to clear_feature, or something else
> without the false expectation? Just thought I'd mention it having scratched
> my head for a bit ;-), feel free to say no.

A later patch renames them so I can include the real set/test/clear_bit
from xc_bitops.h

The problem is that, until this patch, there would be a massive delta
(which is destined for /dev/null) if I proactively renamed them.

~Andrew