Re: [Xen-devel] [PATCH RFC 1/2] x86/mem_access: Make the mem_access ops generic

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 09.04.14 at 07:50, <aravindp@xxxxxxxxx> wrote:
> -int mem_access_memop(struct domain *d, xen_mem_event_op_t *meo)
> +int mem_access_memop(XEN_GUEST_HANDLE_PARAM(void) arg)
>  {
>      int rc;
> +    xen_mem_access_op_t mao;
> +    struct domain *d;
> +
> +    if ( copy_from_guest(&mao, arg, 1) )
> +        return -EFAULT;
> +
> +    rc = rcu_lock_live_remote_domain_by_id(mao.domid, &d);
> +    if ( rc )
> +        return rc;
> +
> +    if ( !is_hvm_domain(d) )
> +        return -EINVAL;
> +
> +    rc = xsm_mem_event_op(XSM_TARGET, d, XENMEM_access_op);
> +    if ( rc )
> +        goto out;
>  
>      if ( unlikely(!d->mem_event->access.ring_page) )
>          return -ENODEV;
>  
> -    switch( meo->op )
> +    switch ( mao.op )
>      {
>      case XENMEM_access_op_resume:
>      {
>          p2m_mem_access_resume(d);
>          rc = 0;
> +        break;
> +    }
> +
> +    case XENMEM_access_op_set_access:
> +    {

Please no pointless braces.

> +        rc = -EINVAL;
> +
> +        if ( (mao.pfn != ~0ull) &&
> +             (((mao.pfn + mao.nr - 1) < mao.pfn) ||
> +              ((mao.pfn + mao.nr - 1) > domain_get_maximum_gpfn(d))) )
> +            break;
> +
> +        rc = p2m_set_mem_access(d, mao.pfn, mao.nr, mao.xenmem_access);
> +        if ( rc > 0 )
> +        {
> +            mao.pfn += mao.nr - rc;
> +            mao.nr = rc;
> +            if ( __copy_to_guest(arg, &mao, 1) )
> +                rc = -EFAULT;
> +            else
> +                rc = hypercall_create_continuation(__HYPERVISOR_memory_op, 
> "lh",
> +                                                   XENMEM_access_op, arg);

As already said - this should be done without playing with the interface
structure. All you need for this is to pass down the upper "cmd" bits
from do_memory_op().

> @@ -162,36 +163,31 @@ DEFINE_XEN_GUEST_HANDLE(xen_hvm_xentrace_t);
>  /* Following tools-only interfaces may change in future. */
>  #if defined(__XEN__) || defined(__XEN_TOOLS__)
>  
> +/* Deprecated by XENMEM_access_op_set_access */
>  #define HVMOP_set_mem_access        12
>  typedef enum {
> -    HVMMEM_access_n,
> -    HVMMEM_access_r,
> -    HVMMEM_access_w,
> -    HVMMEM_access_rw,
> -    HVMMEM_access_x,
> -    HVMMEM_access_rx,
> -    HVMMEM_access_wx,
> -    HVMMEM_access_rwx,
> -    HVMMEM_access_rx2rw,       /* Page starts off as r-x, but automatically
> -                                * change to r-w on a write */
> -    HVMMEM_access_n2rwx,       /* Log access: starts off as n, automatically 
> 
> -                                * goes to rwx, generating an event without
> -                                * pausing the vcpu */
> -    HVMMEM_access_default      /* Take the domain default */
> +    HVMMEM_access_n = XENMEM_access_n,
> +    HVMMEM_access_r = XENMEM_access_r,
> +    HVMMEM_access_w = XENMEM_access_w,
> +    HVMMEM_access_rw = XENMEM_access_rw,
> +    HVMMEM_access_x = XENMEM_access_x,
> +    HVMMEM_access_rx = XENMEM_access_rx,
> +    HVMMEM_access_wx = XENMEM_access_wx,
> +    HVMMEM_access_rwx = XENMEM_access_rwx,
> +    /*
> +     * Page starts off as r-x, but automatically
> +     * change to r-w on a write
> +     */
> +    HVMMEM_access_rx2rw = XENMEM_access_rx2rw,
> +    /*
> +     * Log access: starts off as n, automatically
> +     * goes to rwx, generating an event without
> +     * pausing the vcpu
> +     */
> +    HVMMEM_access_n2rwx = XENMEM_access_n2rwx,
> +    /* Take the domain default */
> +    HVMMEM_access_default = XENMEM_access_default
>  } hvmmem_access_t;

So what is the reason for you to keep this while deleting the interface
structures?

> @@ -379,6 +376,56 @@ struct xen_mem_event_op {
>  typedef struct xen_mem_event_op xen_mem_event_op_t;
>  DEFINE_XEN_GUEST_HANDLE(xen_mem_event_op_t);
>  
> +#define XENMEM_access_op                    21
> +#define XENMEM_access_op_resume             0
> +#define XENMEM_access_op_set_access         1
> +#define XENMEM_access_op_get_access         2
> +
> +typedef enum {
> +    XENMEM_access_n,
> +    XENMEM_access_r,
> +    XENMEM_access_w,
> +    XENMEM_access_rw,
> +    XENMEM_access_x,
> +    XENMEM_access_rx,
> +    XENMEM_access_wx,
> +    XENMEM_access_rwx,
> +    /*
> +     * Page starts off as r-x, but automatically
> +     * change to r-w on a write
> +     */
> +    XENMEM_access_rx2rw,
> +    /*
> +     * Log access: starts off as n, automatically
> +     * goes to rwx, generating an event without
> +     * pausing the vcpu
> +     */
> +    XENMEM_access_n2rwx,
> +    /* Take the domain default */
> +    XENMEM_access_default
> +} xenmem_access_t;
> +
> +struct xen_mem_access_op {
> +    /* XENMEM_access_op_* */
> +    uint8_t     op;
> +    domid_t     domid;
> +    /* xenmem_access_t */
> +    uint16_t xenmem_access;

If you limited this to 8 bits and put it above domid, you'd get away
without any implicit padding (which I would otherwise ask you to make
explicit).

Also I don't see the need for the xenmem_ prefix.

Jan

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