Re: [Xen-devel] [PATCH v10 05/25] x86: refactor psr: L3 CAT: implement CPU init and free flow.

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

On 17-04-06 02:32:04, Jan Beulich wrote:
> >>> On 06.04.17 at 07:49, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
> > On 17-04-05 09:10:58, Jan Beulich wrote:
> >> >>> On 01.04.17 at 15:53, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
> >> > +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).
> >> 
> > There may be NULL member in features array. Features array contains all
> > features, including L3 CAT, CDP and L2 CAT. But on some machines, they
> > may only support L3 CAT but do not support CDP and L2 CAT. So, the features
> > array only has L3 CAT member in it and all other members are all NULL. That
> > is the reason we must check if the member is NULL or not.
> 
> No, and this has been explained before: xfree() happily accepts
> NULL pointers.
> 
Ok, got it.

[...]
> >> > +/* 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
> > 
> > May remove this comment. Per current codes, the MSRs are written to default
> > values no matter first time or not.
> > 
> >> registers to their intended values not be taken care of by
> >> context switch code, once vCPU-s get scheduled onto the newly
> >> onlined CPU?
> >> 
> > cat_init_feature is only called when the first CPU on a socket is online.
> > The MSRs to set are per socket. So, we only need set it once when socket
> > is online.
> 
> This does not answer my question. Once again - why does this need
> doing here explicitly, rather than relying on the needed values being
> loaded when the first vCPU gets scheduled onto one of the pCPU-s
> of this socket?
> 
I do not know if I understand your question correctly. Let me try to explain
again. As we discussed in v9, the MSRs values may be wrong values when socket
is online. That is the reason we have to restore them. 

The MSRs are per socket. That means only one group of MSRs on one socket. So
the setting on one CPU can make it valid on whole socket. The 'cat_init_feature'
is executed when the first CPU on a socket is online so we restore them here.

[...]

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