Re: [Xen-devel] [PATCH v4 04/15] x86: implement data structure and CPU init flow for MBA

[Yi Sun <yi.y.sun@xxxxxxxxxxxxxxx>]

On 17-10-05 02:49:59, Jan Beulich wrote:
> >>> On 05.10.17 at 06:42, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
> > On 17-10-03 23:52:09, Jan Beulich wrote:
> >> >>> Yi Sun <yi.y.sun@xxxxxxxxxxxxxxx> 09/29/17 3:55 AM >>>
> >> >On 17-09-28 05:00:09, Jan Beulich wrote:
> >> >> >>> On 23.09.17 at 11:48, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
> >> >> > @@ -1410,6 +1496,7 @@ static void psr_cpu_init(void)
> >> >> >      unsigned int socket, cpu = smp_processor_id();
> >> >> >      struct feat_node *feat;
> >> >> >      struct cpuid_leaf regs;
> >> >> > +    uint32_t ebx;
> >> >> 
> >> >> Is this local variable really a big help? To me it looks like it only
> >> >> makes the patch larger without actually improving anything,
> >> >> and without being related to the subject of the patch.
> >> >> 
> >> >IMHO, it can avoid the 'cpuid_count_leaf()' being repeatedly called. 
> >> >Without it,
> >> >we have to call 'cpuid_count_leaf()' for 2 more times.
> >> 
> >> Hmm, didn't you simply replace regs.b uses with ebx? Or did I overlook a 
> >> place
> >> where regs is being overwritten before the last of these regs.b uses (in 
> >> which case
> >> I think your change is fine)?
> >> 
> > The regs is overwritten when a feature presents. The old codes are below
> > 
> >     cpuid_count_leaf(PSR_CPUID_LEVEL_CAT, 0, &regs);
> >     if ( regs.b & PSR_RESOURCE_TYPE_L3 )
> >     {
> >         cpuid_count_leaf(PSR_CPUID_LEVEL_CAT, 1, &regs); //It is 
> > overwritten here.
> > ......
> >     }
> > 
> >     cpuid_count_leaf(PSR_CPUID_LEVEL_CAT, 0, &regs);  //So, we have to call 
> > cpuid to get regs again.
> >     if ( regs.b & PSR_RESOURCE_TYPE_L2 )
> >     {
> >         cpuid_count_leaf(PSR_CPUID_LEVEL_CAT, 2, &regs);
> > ......
> > 
> > Because above reason, I defined this ebx local variable to avoid calling 
> > cpuid
> > again for next feature.
> 
> I see. But then please give the variable a better name, reflecting
> the data it holds.
> 
Then, how about 'feat_mask'?

> Jan

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