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

["Aravindh Puthiyaparambil (aravindp)" <aravindp@xxxxxxxxx>]

>> -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.

Sorry, I will get rid of them.
 
>> +        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().

OK, I will give that a shot. 

>> @@ -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?

I was keeping to maintain backward compatibility as I thought I need to retain 
xc_hvm_[sg]et_mem_access(). But after IanC's asking me to nuke them, there is 
no reason for the struct and enum to hang around.

>> @@ -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).

OK I will make it a uint8_t and move it above domid.

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

I will get rid of the prefix. What about the enum? Should I call it 
mem_access_t and MEM_access_*?

Thanks,
Aravindh


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