Re: [Xen-devel] [PATCH v2 03/10] x86: Add Intel Processor Trace support for cpuid

["Kang, Luwei" <luwei.kang@xxxxxxxxx>]

> >>> On 30.05.18 at 15:27, <luwei.kang@xxxxxxxxx> wrote:
> > @@ -759,12 +760,19 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t 
> > domid,
> >                  continue;
> >          }
> >
> > +        if ( input[0] == 0x14 )
> > +        {
> > +            input[1]++;
> > +            if ( input[1] == 1 )
> > +                continue;
> > +        }
> 
> Together with what's there and what iirc Andrew's series puts here, this 
> should become a switch() imo.

Why use switch() here? I don't think need change to switch() and I can't find 
any example in this function use switch(). For example leaf 4 is also implement 
like this.

        /* Intel cache descriptor leaves. */
        if ( input[0] == 4 )
        {
            input[1]++;
            /* More to do? Then loop keeping %%eax==0x00000004. */
            if ( (regs[0] & 0x1f) != 0 )
                continue;
        }

> 
> > @@ -583,7 +584,19 @@ void recalculate_cpuid_policy(struct domain *d)
> >              __clear_bit(X86_FEATURE_VMX, max_fs);
> >              __clear_bit(X86_FEATURE_SVM, max_fs);
> >          }
> > +
> > +        /*
> > +         * Hide Intel Processor trace feature when hardware not support
> > +         * PT-VMX or ipt option is off.
> > +         */
> > +        if ( ipt_mode == IPT_MODE_OFF )
> > +        {
> > +            __clear_bit(X86_FEATURE_IPT, max_fs);
> > +            zero_leaves(p->ipt.raw, 0, ARRAY_SIZE(p->ipt.raw) - 1);
> > +        }
> 
> The clearing of bits in max_fs further up is needed here because this varies 
> depending on domain config. You, otoh, put a
> conditional here which is not going to change post boot. This instead belongs 
> into
> calculate_hvm_max_policy() I believe.

ipt_mode is any global parameter for all domain. Move to 
calculate_hvm_max_policy() is good to me.
Will fix in next version.

> 
> > @@ -101,6 +102,10 @@ static int update_domain_cpuid_info(struct domain *d,
> >              p->feat.raw[ctl->input[1]] = leaf;
> >              break;
> >
> > +        case IPT_CPUID:
> > +            p->ipt.raw[ctl->input[1]] = leaf;
> > +            break;
> 
> This lacks a bounds check of ctl->input[1] (in the earlier switch()).

Oh, get it. 

> 
> > --- a/xen/include/asm-x86/cpufeature.h
> > +++ b/xen/include/asm-x86/cpufeature.h
> > @@ -102,6 +102,7 @@
> >  #define cpu_has_mpx             boot_cpu_has(X86_FEATURE_MPX)
> >  #define cpu_has_rdseed          boot_cpu_has(X86_FEATURE_RDSEED)
> >  #define cpu_has_smap            boot_cpu_has(X86_FEATURE_SMAP)
> > +#define cpu_has_ipt             boot_cpu_has(X86_FEATURE_IPT)
> 
> This definition is unused.

Will remove it.

> 
> > --- a/xen/include/asm-x86/cpuid.h
> > +++ b/xen/include/asm-x86/cpuid.h
> > @@ -58,10 +58,11 @@ DECLARE_PER_CPU(struct cpuidmasks, cpuidmasks);
> >  /* Default masking MSR values, calculated at boot. */  extern struct
> > cpuidmasks cpuidmask_defaults;
> >
> > -#define CPUID_GUEST_NR_BASIC      (0xdu + 1)
> > +#define CPUID_GUEST_NR_BASIC      (0x14u + 1)
> 
> Is there anything to convince me that the intermediate leaves don't need any 
> further handling added anywhere? Same question btw
> for the libxc side bumping of DEF_MAX_BASE.

They are all zero and meaningless in these intermediate leaves. So I think we 
don't need do anything. what is your concern ?

> 
> > @@ -166,6 +167,15 @@ struct cpuid_policy
> >          } comp[CPUID_GUEST_NR_XSTATE];
> >      } xstate;
> >
> > +    /* Structured feature leaf: 0x00000014[xx] */
> > +    union {
> > +        struct cpuid_leaf raw[CPUID_GUEST_NR_IPT];
> > +        struct {
> > +            /* Subleaf 0. */
> > +            uint32_t max_subleaf;
> > +        };
> > +    } ipt;
> 
> In particular this looks to be placed earlier than it should be (in other 
> words I'm getting the impression that you failed to insert
> some padding for the skipped leaves).

I think we don't need add some padding for skipped leaves because these are 
accessed by name (e.g. xstate, ipt ...) not offset.

Thanks,
Luwei Kang

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel