[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v10 05/25] x86: refactor psr: L3 CAT: implement CPU init and free flow.
>>> On 01.04.17 at 15:53, <yi.y.sun@xxxxxxxxxxxxxxx> wrote: > @@ -76,7 +79,7 @@ struct feat_node { > * > * Feature independent HW info and common values are also defined in it. > */ > - const struct feat_props { > + struct feat_props { As said before, the const here should stay. The init-time writing to the structure can be done without going through this pointer. > +static void free_socket_resources(unsigned int socket) > +{ > + unsigned int i; > + struct psr_socket_info *info = socket_info + socket; > + > + if ( !info ) > + return; > + > + /* > + * Free resources of features. The global feature object, e.g. > feat_l3_cat, > + * may not be freed here if it is not added into array. It is simply > being > + * kept until the next CPU online attempt. > + */ > + for ( i = 0; i < PSR_SOCKET_MAX_FEAT; i++ ) > + { > + if ( !info->features[i] ) > + continue; > + > + xfree(info->features[i]); > + info->features[i] = NULL; There's no need for the if() here. And I'm sure this was pointed out already (perhaps in a different context). > +static bool feat_init_done(const struct psr_socket_info *info) > +{ > + unsigned int i; > + > + for ( i = 0; i < PSR_SOCKET_MAX_FEAT; i++ ) > + { > + if ( !info->features[i] ) > + continue; > + > + return true; > + } > + > + return false; > +} At the first glance this is a very strange function. > +/* CAT common functions implementation. */ > +static void cat_init_feature(const struct cpuid_leaf *regs, > + struct feat_node *feat, > + struct psr_socket_info *info, > + enum psr_feat_type type) > +{ > + unsigned int socket, i; > + > + /* No valid value so do not enable feature. */ > + if ( !regs->a || !regs->d ) > + return; > + > + feat->props->cbm_len = (regs->a & CAT_CBM_LEN_MASK) + 1; > + feat->props->cos_max = min(opt_cos_max, regs->d & CAT_COS_MAX_MASK); > + > + switch ( type ) > + { > + case PSR_SOCKET_L3_CAT: > + /* cos=0 is reserved as default cbm(all bits within cbm_len are 1). > */ > + feat->cos_reg_val[0] = cat_default_val(feat->props->cbm_len); > + > + /* > + * To handle cpu offline and then online case, we need restore MSRs > to > + * default values. > + */ > + for ( i = 1; i <= feat->props->cos_max; i++ ) > + { > + wrmsrl(MSR_IA32_PSR_L3_MASK(i), feat->cos_reg_val[0]); > + feat->cos_reg_val[i] = feat->cos_reg_val[0]; > + } I continue to have difficulty with this: Why is offline-then-online any different from first-time-online? Why wouldn't setting the registers to their intended values not be taken care of by context switch code, once vCPU-s get scheduled onto the newly onlined CPU? > + break; > + > + default: > + return; > + } > + > + /* Add this feature into array. */ > + info->features[type] = feat; > + > + socket = cpu_to_socket(smp_processor_id()); No need for this variable, and definitely no need to do the assignment ahead of ... > + if ( !opt_cpu_info ) > + return; ... this. > static void psr_cpu_init(void) > { > + struct psr_socket_info *info; > + unsigned int socket; > + unsigned int cpu = smp_processor_id(); > + struct feat_node *feat; > + struct cpuid_leaf regs; > + > + if ( !psr_alloc_feat_enabled() || !boot_cpu_has(X86_FEATURE_PQE) ) > + goto assoc_init; > + > + if ( boot_cpu_data.cpuid_level < PSR_CPUID_LEVEL_CAT ) > + { > + setup_clear_cpu_cap(X86_FEATURE_PQE); > + goto assoc_init; > + } > + > + socket = cpu_to_socket(cpu); > + info = socket_info + socket; > + if ( feat_init_done(info) ) > + goto assoc_init; Hmm, so you bail here if any of the features was already set up. But you don't bail if none of the features were available as the reason for the setup not having been done before. I think this can be solved in a better way once we have the static props array: You need to do anything here only if the props slot is not NULL, but the feature slot is NULL. In any event you intentions are likely easier to understand for a reader if this single-use function was inlined here. Jan _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx https://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |