WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-ppc-devel

[Xen-devel] Re: [patch] ACM ops interface

To: hollisb@xxxxxxxxxx
Subject: [Xen-devel] Re: [patch] ACM ops interface
From: Reiner Sailer <sailer@xxxxxxxxxx>
Date: Thu, 8 Jun 2006 14:17:56 -0400
Cc: xen-devel@xxxxxxxxxxxxxxxxxxx, xen-ppc-devel <xen-ppc-devel@xxxxxxxxxxxxxxxxxxx>
Delivery-date: Thu, 08 Jun 2006 11:18:31 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxx
In-reply-to: <1149778877.13780.5.camel@xxxxxxxxxxxxxxxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/cgi-bin/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/cgi-bin/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx

hollisb@xxxxxxxxxxxxxxxxxxxxxxx wrote on 06/08/2006 11:01:17 AM:

> [Resend due to xense-devel list borkage.]
>
> Hi, I noticed a few small issues in the ACM code.

Thanks for looking into them!!

> First, it isn't using XEN_GUEST_HANDLEs where it should (i.e. when using
> pointers in the interface). What's a little scary is that because it's
> using void* everywhere, we didn't get any compiler warnings (if you're
> passing buffers, maybe 'unsigned char *' would be a better type?).


Keir responded to this point already

> Second, copy_to/from_user() has been replaced with copy_to/from_guest(),
> since you can't copy to "userspace" on all architectures. (To complicate
> things, there are a couple areas where x86 code actually wants to copy
> to a virtual address, but copy_to/from_user() is only valid in
> arch-specific code.)

Seems a sensible thing to do. This part of the patch makes much sense.

> Third, using 'enum' in the interface makes me very nervous. I'm not a C
> lawyer, but using an explicitly-sized type would make me a lot happier.

We should define constants and a fixed type argument (e.g. 32bit as you suggest in your patch). You are right that this interface benefits from clearly defined in its types and sizes.

> Finally, a request: It would simplify PowerPC code if the ACM ops passed
> around a union that contains every ACM struct, like 'struct dom0_op'.
> It's a little hard to explain, but if you're curious, have a look at
> xenppc_privcmd_dom0_op() in


We just changed away from this for many reasons. It is at any time pretty clear to which structure the void pointer points.
See Keir's e-mail.

Thanks
Reiner

> http://xenbits.xensource.com/ext/linux-ppc-2.6.hg?f=aa55dca4028a;
> file=arch/powerpc/platforms/xen/hcall.c . To support the current ACM
> ops, we would have to define our own union anyways:
>    
>    union {
>       setpolicy;
>       getpolicy;
>       ...
>    } op;
>    
>    switch (cmd) {
>       case SETPOLICY:
>          if (copy_from_user(&op, arg, sizeof(struct acm_setpolicy)))
>             return -EFAULT;
>          break;
>       case SETPOLICY:
>          if (copy_from_user(&op, arg, sizeof(struct acm_setpolicy)))
>             return -EFAULT;
>          break;
>       ...
>    }
>    
>    __u32 *version = (__u32 *)&op;
>    if (*version != ACM_VERSION)
>       return -EACCES;
>    
>    switch (cmd) {
>       case SETPOLICY:
>          /* munge acm_setpolicy */
>       case GETPOLICY:
>          /* munge acm_getpolicy */
>       ...
>    }
>
> If the ACM ops were always passed in a union, we could replace the first
> switch with a single copy_from_user(), and also guarantee that
> "interface_version" is always in the right place (it's only there
> coincidentally now). What do you think?
>
> Anyways, this compile-tested patch addresses the first three issues I
> mentioned. Please add your Signed-off-by and submit upstream if you're
> happy with it.
>
> Thanks!
>
> Signed-off-by: Hollis Blanchard <hollisb@xxxxxxxxxx>
>
> diff -r 5c726b5ab92b tools/python/xen/lowlevel/acm/acm.c
> --- a/tools/python/xen/lowlevel/acm/acm.c   Tue Jun 06 15:30:06 2006 -0500
> +++ b/tools/python/xen/lowlevel/acm/acm.c   Wed Jun 07 12:31:55 2006 -0500
> @@ -52,7 +52,7 @@ void * __getssid(int domid, uint32_t *bu
>      }
>      memset(buf, 0, SSID_BUFFER_SIZE);
>      getssid.interface_version = ACM_INTERFACE_VERSION;
> -    getssid.ssidbuf = buf;
> +    set_xen_guest_handle(getssid.ssidbuf, buf);
>      getssid.ssidbuf_size = SSID_BUFFER_SIZE;
>      getssid.get_ssid_by = DOMAINID;
>      getssid.id.domainid = domid;
> diff -r 5c726b5ab92b xen/acm/acm_policy.c
> --- a/xen/acm/acm_policy.c   Tue Jun 06 15:30:06 2006 -0500
> +++ b/xen/acm/acm_policy.c   Wed Jun 07 12:31:55 2006 -0500
> @@ -32,7 +32,7 @@
>  #include <acm/acm_endian.h>
>  
>  int
> -acm_set_policy(void *buf, u32 buf_size, int isuserbuffer)
> +acm_set_policy(XEN_GUEST_HANDLE(void) buf, u32 buf_size, int isuserbuffer)
>  {
>      u8 *policy_buffer = NULL;
>      struct acm_policy_buffer *pol;
> @@ -45,7 +45,7 @@ acm_set_policy(void *buf, u32 buf_size,
>          return -ENOMEM;
>  
>      if (isuserbuffer) {
> -        if (copy_from_user(policy_buffer, buf, buf_size))
> +        if (copy_from_guest(policy_buffer, buf, buf_size))
>          {
>              printk("%s: Error copying!\n",__func__);
>              goto error_free;
> @@ -116,7 +116,7 @@ acm_set_policy(void *buf, u32 buf_size,
>  }
>  
>  int
> -acm_get_policy(void *buf, u32 buf_size)
> +acm_get_policy(XEN_GUEST_HANDLE(void) buf, u32 buf_size)
>  {
>      u8 *policy_buffer;
>      int ret;
> @@ -162,7 +162,7 @@ acm_get_policy(void *buf, u32 buf_size)
>          goto error_free_unlock;
>  
>      bin_pol->len = htonl(ntohl(bin_pol->len) + ret);
> -    if (copy_to_user(buf, policy_buffer, ntohl(bin_pol->len)))
> +    if (copy_to_guest(buf, policy_buffer, ntohl(bin_pol->len)))
>          goto error_free_unlock;
>  
>      read_unlock(&acm_bin_pol_rwlock);
> @@ -177,7 +177,7 @@ acm_get_policy(void *buf, u32 buf_size)
>  }
>  
>  int
> -acm_dump_statistics(void *buf, u16 buf_size)
> +acm_dump_statistics(XEN_GUEST_HANDLE(void) buf, u16 buf_size)
>  {
>      /* send stats to user space */
>      u8 *stats_buffer;
> @@ -208,7 +208,7 @@ acm_dump_statistics(void *buf, u16 buf_s
>  
>      memcpy(stats_buffer, &acm_stats, sizeof(struct acm_stats_buffer));
>  
> -    if (copy_to_user(buf, stats_buffer, sizeof(struct
> acm_stats_buffer) + len1 + len2))
> +    if (copy_to_guest(buf, stats_buffer, sizeof(struct
> acm_stats_buffer) + len1 + len2))
>          goto error_lock_free;
>  
>      read_unlock(&acm_bin_pol_rwlock);
> @@ -223,7 +223,7 @@ acm_dump_statistics(void *buf, u16 buf_s
>  
>
>  int
> -acm_get_ssid(ssidref_t ssidref, u8 *buf, u16 buf_size)
> +acm_get_ssid(ssidref_t ssidref, XEN_GUEST_HANDLE(void) buf, u16 buf_size)
>  {
>      /* send stats to user space */
>      u8 *ssid_buffer;
> @@ -272,7 +272,7 @@ acm_get_ssid(ssidref_t ssidref, u8 *buf,
>      acm_ssid->len += ret;
>      acm_ssid->secondary_max_types = ret;
>  
> -    if (copy_to_user(buf, ssid_buffer, acm_ssid->len))
> +    if (copy_to_guest(buf, ssid_buffer, acm_ssid->len))
>          goto error_free_unlock;
>  
>      read_unlock(&acm_bin_pol_rwlock);
> diff -r 5c726b5ab92b xen/include/public/acm_ops.h
> --- a/xen/include/public/acm_ops.h   Tue Jun 06 15:30:06 2006 -0500
> +++ b/xen/include/public/acm_ops.h   Wed Jun 07 12:31:55 2006 -0500
> @@ -17,7 +17,7 @@
>   * This makes sure that old versions of acm tools will stop working in a
>   * well-defined way (rather than crashing the machine, for instance).
>   */
> -#define ACM_INTERFACE_VERSION   0xAAAA0007
> +#define ACM_INTERFACE_VERSION   0xAAAA0008
>  
>  /************************************************************************/
>  
> @@ -33,7 +33,7 @@ struct acm_setpolicy {
>  struct acm_setpolicy {
>      /* IN */
>      uint32_t interface_version;
> -    void *pushcache;
> +    XEN_GUEST_HANDLE(void) pushcache;
>      uint32_t pushcache_size;
>  };
>  
> @@ -42,7 +42,7 @@ struct acm_getpolicy {
>  struct acm_getpolicy {
>      /* IN */
>      uint32_t interface_version;
> -    void *pullcache;
> +    XEN_GUEST_HANDLE(void) pullcache;
>      uint32_t pullcache_size;
>  };
>  
> @@ -51,7 +51,7 @@ struct acm_dumpstats {
>  struct acm_dumpstats {
>      /* IN */
>      uint32_t interface_version;
> -    void *pullcache;
> +    XEN_GUEST_HANDLE(void) pullcache;
>      uint32_t pullcache_size;
>  };
>  
> @@ -61,12 +61,12 @@ struct acm_getssid {
>  struct acm_getssid {
>      /* IN */
>      uint32_t interface_version;
> -    enum get_type get_ssid_by;
> +    uint32_t get_ssid_by;
>      union {
>          domaintype_t domainid;
>          ssidref_t    ssidref;
>      } id;
> -    void *ssidbuf;
> +    XEN_GUEST_HANDLE(void) ssidbuf;
>      uint32_t ssidbuf_size;
>  };
>  
> @@ -74,8 +74,8 @@ struct acm_getdecision {
>  struct acm_getdecision {
>      /* IN */
>      uint32_t interface_version;
> -    enum get_type get_decision_by1;
> -    enum get_type get_decision_by2;
> +    uint32_t get_decision_by1;
> +    uint32_t get_decision_by2;
>      union {
>          domaintype_t domainid;
>          ssidref_t    ssidref;
> @@ -84,9 +84,9 @@ struct acm_getdecision {
>          domaintype_t domainid;
>          ssidref_t    ssidref;
>      } id2;
> -    enum acm_hook_type hook;
> +    uint32_t hook;
>      /* OUT */
> -    int acm_decision;
> +    uint32_t acm_decision;
>  };
>  
>  #endif /* __XEN_PUBLIC_ACM_OPS_H__ */
>
>
> --
> Hollis Blanchard
> IBM Linux Technology Center
>
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel