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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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.

> @@ -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.

> @@ -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()).

> --- 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.

> --- 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.

> @@ -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).

Jan



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