Re: [PATCH v3 10/28] xen/x86: Generate deep dependencies of features

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

On 17/03/16 19:45, Konrad Rzeszutek Wilk wrote:
> On Tue, Mar 15, 2016 at 03:35:06PM +0000, Andrew Cooper wrote:
>> Some features depend on other features.  Working out and maintaining the exact
>> dependency tree is complicated, so it is expressed in the automatic generation
>> script, and flattened for faster runtime use.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>>
>> v3:
>>  * Vastly more reserch and comments
>> v2:
>>  * New
>> ---
>>  xen/arch/x86/cpuid.c        |  54 +++++++++++++++++
>>  xen/include/asm-x86/cpuid.h |   2 +
>>  xen/tools/gen-cpuid.py      | 139 +++++++++++++++++++++++++++++++++++++++++++-
>>  3 files changed, 194 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c
>> index 174cfa0..17487f5 100644
>> --- a/xen/arch/x86/cpuid.c
>> +++ b/xen/arch/x86/cpuid.c
>> @@ -11,6 +11,7 @@ const uint32_t special_features[] = INIT_SPECIAL_FEATURES;
>>  static const uint32_t __initconst pv_featuremask[] = INIT_PV_FEATURES;
>>  static const uint32_t __initconst hvm_shadow_featuremask[] = INIT_HVM_SHADOW_FEATURES;
>>  static const uint32_t __initconst hvm_hap_featuremask[] = INIT_HVM_HAP_FEATURES;
>> +static const uint32_t __initconst deep_features[] = INIT_DEEP_FEATURES;
>>  
>>  uint32_t __read_mostly raw_featureset[FSCAPINTS];
>>  uint32_t __read_mostly pv_featureset[FSCAPINTS];
>> @@ -18,12 +19,34 @@ uint32_t __read_mostly hvm_featureset[FSCAPINTS];
>>  
>>  static void __init sanitise_featureset(uint32_t *fs)
>>  {
>> +    uint32_t disabled_features[FSCAPINTS];
>>      unsigned int i;
>>  
>>      for ( i = 0; i < FSCAPINTS; ++i )
>>      {
>>          /* Clamp to known mask. */
>>          fs[i] &= known_features[i];
>> +
>> +        /*
>> +         * Identify which features with deep dependencies have been
>> +         * disabled.
>> +         */
>> +        disabled_features[i] = ~fs[i] & deep_features[i];
>> +    }
>> +
>> +    for_each_set_bit(i, (void *)disabled_features,
>> +                     sizeof(disabled_features) * 8)
>> +    {
>> +        const uint32_t *dfs = lookup_deep_deps(i);
>> +        unsigned int j;
>> +
>> +        ASSERT(dfs); /* deep_features[] should guarentee this. */
>> +
>> +        for ( j = 0; j < FSCAPINTS; ++j )
>> +        {
>> +            fs[j] &= ~dfs[j];
>> +            disabled_features[j] &= ~dfs[j];
> What if this clears the entries you have already skipped over?

That is fine.

>
> Say you are at word 2 (i==2)and disabled_features[j] cleared word 1.
> The loop (for_each_set_bit) won't notice this.
>
> And furthermore lets assume that the clearing of a bit in
> word 1 should have done another dependency check which would clear
> something at word 7. You would miss that?

Each individual dfs[] is the complete set of all eventual features
cleared because of i, which means there is no need to recurse along i's
dependency chain.

The entire point of the dependency flattening logic below is to make
this logic here easier.

>
>> +        # instructions.  MMX is documented to alias the %MM registers over the
>> +        # x87 %ST registers in hardware.
>> +        FPU: [MMX],
>> +
>> +        # The PSE36 feature indicates that reserved bits in a PSE superpage
>> +        # may be used as extra physical address bits.
>> +        PSE: [PSE36],
>> +
>> +        # Entering Long Mode requires that %CR4.PAE is set.  The NX pagetable
>> +        # bit is only representable in the 64bit PTE format offered by PAE.
>> +        PAE: [LM, NX],
>> +
>> +        # APIC is special, but X2APIC does depend on APIC being available in
>> +        # the first place.
>> +        APIC: [X2APIC],
>> +
>> +        # AMD built MMXExtentions and 3DNow as extentions to MMX.
>> +        MMX: [MMXEXT, _3DNOW]
> EWhy the underscore?

Because omitting it would be a syntax error.  One slight hitch with
mutating the global namespace is that is perfectly easy to put items in
it which can be referred back to.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel