Re: [Xen-devel] [PATCH v4 04/24] x86: refactor psr: implement CPU init and free flow.

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

On 17-01-11 06:48:27, Jan Beulich wrote:
> >>> On 11.01.17 at 04:14, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
> > On 17-01-10 04:45:05, Jan Beulich wrote:
> >> >>> On 14.12.16 at 05:07, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
> >> > +/* L3 CAT callback functions implementation. */
> >> > +static void l3_cat_init_feature(unsigned int eax, unsigned int ebx,
> >> > +                                unsigned int ecx, unsigned int edx,
> >> 
> >> This is rather unfortunate naming: How does the reader of this code
> >> know what these values represent, without having to first go look in
> >> the caller?
> >> 
> > Do you mean the 'eax'-'edx'?
> 
> Yes.
> 
> > How about 'eax_register'?
> 
> How would that be any better? Perhaps the best way of making the
> naming obvious would be to use the new cpuid_leaf structure here.
> 
Ok, will consider to assemble them into a structure.

> >> > +    if ( !cpu_has(c, X86_FEATURE_PQE) || c->cpuid_level < 
> >> > PSR_CPUID_LEVEL_CAT )
> >> > +        return;
> >> 
> >> Instead of such a double check, please consider clearing the PQE
> >> feature bit when the maximum CPUID level is too low (which
> >> shouldn't happen anyway).
> >> 
> > Is this the responsibility of psr.c? X86_FEATURE_PQE bit is set by HW. Even 
> > the
> > bit is set but CPUID level is low, I think SW would be better to keep it but
> > not clear it. Because it indicates the HW capability. How do you think? 
> 
> What use if keeping the flag if we can't use the feature? And to
> answer your first question - whether that's being done in psr.c,
> cpu/common.c, or cpu/intel.c I don't really care all that much; it
> would certainly feel most natural to go here.
> 
Ok, will consider it.

> >> > +    socket = cpu_to_socket(cpu);
> >> > +    info = socket_info + socket;
> >> > +    if ( info->feat_mask )
> >> > +        return;
> >> > +
> >> > +    spin_lock_init(&info->ref_lock);
> >> > +
> >> > +    cpuid_count(PSR_CPUID_LEVEL_CAT, 0, &eax, &ebx, &ecx, &edx);
> >> > +    if ( ebx & PSR_RESOURCE_TYPE_L3 )
> >> > +    {
> >> > +        cpuid_count(PSR_CPUID_LEVEL_CAT, 1, &eax, &ebx, &ecx, &edx);
> >> > +
> >> > +        feat_tmp = feat_l3_cat;
> >> > +        feat_l3_cat = NULL;
> >> > +        feat_tmp->ops = l3_cat_ops;
> >> > +
> >> > +        feat_tmp->ops.init_feature(eax, ebx, ecx, edx, feat_tmp, info);
> >> 
> >> What's the point of the indirect call here, when you know the
> >> function is l3_cat_init_feature()?
> >> 
> > Hmm, just want to keep the callback function calling style.
> 
> Please don't use indirect calls when you don't need them.
> 
Ok, thanks!

> >> > +static int psr_cpu_prepare(unsigned int cpu)
> >> > +{
> >> > +    return cpu_prepare_work(cpu);
> >> > +}
> >> 
> >> What is this wrapper good for?
> >> 
> > Just keep the old codes.
> 
> Well, you're overhauling the old code anyway (and you're actively
> adding this function here), so - please don't introduce pointless
> wrappers like this. They only complicate anyone following call flow,
> even if just slightly.
> 
Sure, thanks!

> Jan

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