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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 12.07.18 at 09:21, <luwei.kang@xxxxxxxxx> wrote:
>> >>> 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;
>         }

For exactly this reason - instead of multiple if()-s evaluating
input[0], switch(input[0]) is preferred.

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

As said - how can I convince myself that no further handling is needed?
How did you convince yourself? For example, update_domain_cpuid_info()
will then allow to set the intermediate fields to arbitrary values, without
recalculate_cpuid_policy() doing anything with them. After all
cpuid_featureset_to_policy() only sets explicit fields, but doesn't clear the
policy up front. Andrew - isn't this still a left-over piece of white-listing?

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

Oh, right, I was mistaken here - the immediately containing entity
is a struct, not a union.

Jan



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