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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

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

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

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

Jan


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