Re: [Xen-devel] [PATCH v2 4/8] x86: dynamically attach/detach CQM service for a guest

["Xu, Dongxiao" <dongxiao.xu@xxxxxxxxx>]

> -----Original Message-----
> From: Andrew Cooper [mailto:andrew.cooper3@xxxxxxxxxx]
> Sent: Thursday, November 21, 2013 8:50 PM
> To: Xu, Dongxiao
> Cc: xen-devel@xxxxxxxxxxxxx
> Subject: Re: [Xen-devel] [PATCH v2 4/8] x86: dynamically attach/detach CQM
> service for a guest
> 
> On 21/11/13 07:20, dongxiao.xu@xxxxxxxxx wrote:
> > From: Dongxiao Xu <dongxiao.xu@xxxxxxxxx>
> >
> > Add hypervisor side support for dynamically attach and detach CQM
> > services for a certain guest.
> >
> > Signed-off-by: Jiongxi Li <jiongxi.li@xxxxxxxxx>
> > Signed-off-by: Dongxiao Xu <dongxiao.xu@xxxxxxxxx>
> > ---
> >  xen/arch/x86/domctl.c       |   40
> ++++++++++++++++++++++++++++++++++++++++
> >  xen/include/public/domctl.h |   14 ++++++++++++++
> >  2 files changed, 54 insertions(+)
> >
> > diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
> > index f7e4586..5ef21f9 100644
> > --- a/xen/arch/x86/domctl.c
> > +++ b/xen/arch/x86/domctl.c
> > @@ -35,6 +35,7 @@
> >  #include <asm/mem_sharing.h>
> >  #include <asm/xstate.h>
> >  #include <asm/debugger.h>
> > +#include <asm/pqos.h>
> >
> >  static int gdbsx_guest_mem_io(
> >      domid_t domid, struct xen_domctl_gdbsx_memio *iop)
> > @@ -1223,6 +1224,45 @@ long arch_do_domctl(
> >      }
> >      break;
> >
> > +    case XEN_DOMCTL_attach_pqos:
> > +    {
> > +        if ( domctl->u.qos_res.flags & XEN_DOMCTL_ADF_pqos_cqm )
> > +        {
> > +            if ( !system_supports_cqm() )
> > +                ret = -ENODEV;
> > +            else if ( d->arch.pqos_cqm_rmid > 0 )
> > +                ret = -EINVAL;
> 
> EEXISTS perhaps?

EEXIST stands for "File exists" while EINVAL stands for "Invalid argument ".
Here I used EINVAL because if the domain is already CQM enabled, then the 
"domid" parameter in "xl pqos-attach cqm domid" should be an invalid argument.
Do you think it is OK?

> 
> > +            else
> > +            {
> > +                ret = alloc_cqm_rmid(d);
> > +                if ( ret < 0 )
> > +                    ret = -EBUSY;
> 
> On reflection, EUSERS is probably more accurate here.

Agree.

> 
> > +            }
> > +        }
> > +        else
> > +            ret = -EINVAL;
> > +    }
> > +    break;
> > +
> > +    case XEN_DOMCTL_detach_pqos:
> > +    {
> > +        if ( domctl->u.qos_res.flags & XEN_DOMCTL_ADF_pqos_cqm )
> > +        {
> > +            if ( !system_supports_cqm() )
> > +                ret = -ENODEV;
> > +            else if ( d->arch.pqos_cqm_rmid > 0 )
> > +            {
> > +                free_cqm_rmid(d);
> > +                ret = 0;
> > +            }
> > +            else
> > +                ret = -EINVAL;
> > +        }
> > +        else
> > +            ret = -EINVAL;
> > +    }
> > +    break;
> > +
> >      default:
> >          ret = iommu_do_domctl(domctl, d, u_domctl);
> >          break;
> > diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
> > index 47a850a..f5d7062 100644
> > --- a/xen/include/public/domctl.h
> > +++ b/xen/include/public/domctl.h
> > @@ -872,6 +872,17 @@ struct xen_domctl_set_max_evtchn {
> >  typedef struct xen_domctl_set_max_evtchn
> xen_domctl_set_max_evtchn_t;
> >  DEFINE_XEN_GUEST_HANDLE(xen_domctl_set_max_evtchn_t);
> >
> > +/* XEN_DOMCTL_attach_pqos */
> > +/* XEN_DOMCTL_detach_pqos */
> > +struct xen_domctl_qos_resource {
> > + /* Attach or detach flag for cqm */
> > +#define _XEN_DOMCTL_ADF_pqos_cqm      0
> > +#define XEN_DOMCTL_ADF_pqos_cqm
> (1U<<_XEN_DOMCTL_ADF_pqos_cqm)
> 
> Does the ADF help here?  My personal opinion is that it makes the
> constants more confusing.

OK, will remove the "ADF" characters.

Thanks,
Dongxiao

> 
> ~Andrew
> 
> > +    uint32_t flags;
> > +};
> > +typedef struct xen_domctl_qos_resource xen_domctl_qos_resource_t;
> > +DEFINE_XEN_GUEST_HANDLE(xen_domctl_qos_resource_t);
> > +
> >  struct xen_domctl {
> >      uint32_t cmd;
> >  #define XEN_DOMCTL_createdomain                   1
> > @@ -941,6 +952,8 @@ struct xen_domctl {
> >  #define XEN_DOMCTL_setnodeaffinity               68
> >  #define XEN_DOMCTL_getnodeaffinity               69
> >  #define XEN_DOMCTL_set_max_evtchn                70
> > +#define XEN_DOMCTL_attach_pqos                   71
> > +#define XEN_DOMCTL_detach_pqos                   72
> >  #define XEN_DOMCTL_gdbsx_guestmemio            1000
> >  #define XEN_DOMCTL_gdbsx_pausevcpu             1001
> >  #define XEN_DOMCTL_gdbsx_unpausevcpu           1002
> > @@ -1001,6 +1014,7 @@ struct xen_domctl {
> >          struct xen_domctl_set_broken_page_p2m set_broken_page_p2m;
> >          struct xen_domctl_gdbsx_pauseunp_vcpu gdbsx_pauseunp_vcpu;
> >          struct xen_domctl_gdbsx_domstatus   gdbsx_domstatus;
> > +        struct xen_domctl_qos_resource      qos_res;
> >          uint8_t                             pad[128];
> >      } u;
> >  };


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