Re: [PATCH v2 3/4] tools: add tools support for Intel CDP

[Chao Peng <chao.p.peng@linux.intel.com>]

On Wed, Sep 09, 2015 at 02:37:36AM -0600, Jan Beulich wrote:
> >>> On 09.09.15 at 10:10, <he.chen@linux.intel.com> wrote:
> > On Wed, Sep 09, 2015 at 03:32:11PM +0800, Chao Peng wrote:
> >> On Wed, Sep 09, 2015 at 01:16:47PM +0800, He Chen wrote:
> >> > @@ -304,14 +305,41 @@ int libxl_psr_cat_set_cbm(libxl_ctx *ctx, uint32_t domid,
> >> >          goto out;
> >> >      }
> >> >  
> >> > +    rc = libxl_psr_cat_get_l3_info(ctx, &info, &nr_sockets);
> >> > +    if (rc) {
> >> > +        LOGE(ERROR, "Failed to get cat info");
> >> > +        goto out;
> >> > +    }
> >> > +
> >> > +    if (!info->cdp_enabled &&
> >> > +       (type == LIBXL_PSR_CBM_TYPE_L3_CODE ||
> >> > +        type == LIBXL_PSR_CBM_TYPE_L3_DATA))
> >> > +    {
> >> > +            LOGE(ERROR, "Unable to set Code/Data CBM with CDP disabled");
> >> > +            rc = EINVAL;
> >> > +            free(info);
> >> > +            goto out;
> >> > +    }
> >> > +
> >> >      libxl_for_each_set_bit(socketid, *target_map) {
> >> >          if (socketid >= nr_sockets)
> >> >              break;
> >> > -        if (xc_psr_cat_set_domain_data(ctx->xch, domid, type, socketid, cbm)) {
> >> > +        if (info->cdp_enabled && type == LIBXL_PSR_CBM_TYPE_L3_CBM)
> >> > +        {
> >> > +            if(xc_psr_cat_set_domain_data(ctx->xch, domid,
> >> > +               LIBXL_PSR_CBM_TYPE_L3_CODE, socketid, cbm) ||
> >> > +               xc_psr_cat_set_domain_data(ctx->xch, domid,
> >> > +               LIBXL_PSR_CBM_TYPE_L3_DATA, socketid, cbm))
> >> > +            {
> >> > +                libxl__psr_cat_log_err_msg(gc, errno);
> >> > +                rc = ERROR_FAIL;
> >> > +            }
> >> 
> >> Will you merge the two if's?
> > 
> > Surely.
> 
> Surely not you mean, or else how will this ...
> 
> >> > +        } else if (xc_psr_cat_set_domain_data(ctx->xch, domid, type, socketid, cbm)) {
> 
> ... work?

Indeed. Then just ignore it.

> 
> Looking at this I'm surprised though that consumers is required to do
> two calls (and know whether CDP is enabled) when they want to set
> things up without caring for the code/data distinction. I think this
> should be taken care of in the hypervisor.

Agreed, hypervisor should take care of it for this case.

Chao