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

[Xen-devel] [patch] ACM ops interface

To: xen-devel@xxxxxxxxxxxxxxxxxxx
Subject: [Xen-devel] [patch] ACM ops interface
From: Hollis Blanchard <hollisb@xxxxxxxxxx>
Date: Thu, 08 Jun 2006 10:01:17 -0500
Cc: Reiner Sailer <sailer@xxxxxxxxxx>, xen-ppc-devel <xen-ppc-devel@xxxxxxxxxxxxxxxxxxx>
Delivery-date: Thu, 08 Jun 2006 09:02:08 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxx
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>
Organization: IBM Linux Technology Center
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
[Resend due to xense-devel list borkage.]

Hi, I noticed a few small issues in the ACM code.

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

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

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.

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