Re: [PATCH v8 08/24] x86: refactor psr: set value: implement framework.

[Yi Sun <yi.y.sun@linux.intel.com>]

On 17-02-28 13:58:55, Roger Pau Monn� wrote:
> > +static int find_cos(const uint64_t *val, uint32_t array_len,
> > +                    enum psr_feat_type feat_type,
> > +                    const struct psr_socket_info *info)
> > +{
>     ASSERT(spin_is_locked(info->ref_lock));
> > +    return -ENOENT;
> > +}
> > +
> > +static int pick_avail_cos(const struct psr_socket_info *info,
> > +                          const uint64_t *val, uint32_t array_len,
> > +                          unsigned int old_cos,
> > +                          enum psr_feat_type feat_type)
> > +{
>     ASSERT(spin_is_locked(info->ref_lock));
> > +    return -ENOENT;
> > +}
> > +
> > +static int write_psr_msr(unsigned int socket, unsigned int cos,
> > +                         const uint64_t *val)
> > +{
>     ASSERT(spin_is_locked(info->ref_lock));
> > +    return -ENOENT;
> > +}
> > +
Thank you!

> > +int psr_set_val(struct domain *d, unsigned int socket,
> > +                uint64_t val, enum cbm_type type)
> > +{

[...]

> > +
> > +    /*
> > +     * Step 5:
> > +     * Update ref according to COS ID.
> > +     */
> > +    ref[cos]++;
> > +    ASSERT(ref[cos] || cos == 0);
> > +    ref[old_cos]--;
> 
> If cos == 0 you can overflow ref[0].
> 
COS[0] stores the default value only. The value in it cannot be changed. So we
do not check its ref.

> > +    spin_unlock(&info->ref_lock);
> > +
> > +    /*
> > +     * Step 6:
> > +     * Save the COS ID into current domain's psr_cos_ids[] so that we can know
> > +     * which COS the domain is using on the socket. One domain can only use
> > +     * one COS ID at same time on each socket.
> > +     */
> > +    d->arch.psr_cos_ids[socket] = cos;
> > +    xfree(val_array);
> > +
> > +    return 0;
> > +}
> > +
> >  /* Called with domain lock held, no extra lock needed for 'psr_cos_ids' */
> >  static void psr_free_cos(struct domain *d)
> >  {
> > -    if( !d->arch.psr_cos_ids )
> > +    unsigned int socket, cos;
> 
> An ASSERT that the domain is locked would be better than a comment.
> 
psr_free_cos is called by psr_domain_free which is finally called by
put_domain().

    #define put_domain(_d) \
        if ( atomic_dec_and_test(&(_d)->refcnt) ) domain_destroy(_d)

So, it is protected by refcnt.

Do you think it is necessary to check refcnt here? If codes outside have
something error to breake this protection, we cannot assure if below codes
go wrong even by ASSERT check here.

> > +    if ( !d->arch.psr_cos_ids )
> >          return;
> >  
> > +    /* Domain is free so its cos_ref should be decreased. */
> > +    for ( socket = 0; socket < nr_sockets; socket++ )
> > +    {
> > +        struct psr_socket_info *info;
> > +
> > +        /* cos 0 is default one which does not need be handled. */
> > +        if ( (cos = d->arch.psr_cos_ids[socket]) == 0 )
> > +            continue;
> > +
> > +        /*
> > +         * If domain uses other cos ids, all corresponding refs must have been
> > +         * increased 1 for this domain. So, we need decrease them.
> > +         */
> > +        info = socket_info + socket;
> > +        ASSERT(info->cos_ref[cos] || cos == 0);
> > +        spin_lock(&info->ref_lock);
> > +        info->cos_ref[cos]--;
> 
> If cos == 0, it is possible to reach this with info->cos_ref[cos] == 0, in
> which case you are overflowing it?
> 
As above explanation, we do not care the ref count of COS[0].

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

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