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

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

On Thu, Mar 17, 2016 at 08:14:52PM +0000, Andrew Cooper wrote:
> 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@xxxxxxxxxx>
> >> ---
> >> CC: Jan Beulich <JBeulich@xxxxxxxx>
> >>
> >> 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.

In retrospective it is obvious - as the commit description says that.

What I was wondering if you could express what you said in the commit
description, like:

"We do take into account an feature which depends on a feature
which depends on a feature, and so. The individual dfs[] is the complete
set of _ALL_ eventual features cleared. "

> 
> 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@xxxxxxxxxxxxx
> http://lists.xen.org/xen-devel

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