[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [Libvir] Problem with hypercall



On Thu, Sep 28, 2006 at 07:20:35PM +0100, Daniel P. Berrange wrote:
> On Tue, Sep 19, 2006 at 12:12:19PM -0400, Daniel Veillard wrote:
> > On Wed, Sep 13, 2006 at 09:13:35PM -0400, Daniel Veillard wrote:
> > > On Wed, Sep 13, 2006 at 12:55:01PM -0600, Jim Fehlig wrote:
> > > > I found that the buffer provided for XEN_V1_OP_GETDOMAININFOLIST 
> > > > hypercall differs slightly from the buffer in xen/dom0_ops.h.
> > > 
> > >   ohh, that's quite possible I made a mistake in rebuilding the code, yes
> > > 
> > > > Attached is a patch against current cvs that works for me, but I'm not 
> > > > familiar with this part of the code so not sure if this is the proper fix.
> > > 
> > >   I will try to double check today or tomorrow as I'm on the road,
> > > thanks a lot for the patch.
> > 
> >   Okay, I'm finally back, confirmed my error (sorry I should have tried a
> > 32bit box too but had none handy with the old code). So applied and commited,
> 
> Urm, but this has now broken things on 32-bit  3.0.3 based Xen HV.
> 
> # virsh dominfo Domain-0 | grep CPU
> CPU(s):         115
> 
> And also
> 
> #  virsh vcpuinfo Domain-0 
> libvir: Xen error : failed Xen syscall  ioctl  3166208
> 
> Looks like we need different versions of this struct depending on which
> Xen we're working against.

This is really quite a nasty problem, because the struct is passed into from
numerous locations in the xen_internal.h code & I didn't want to cover the
entire source with conditionals.

So what I've done is declared a new xen_v2_domaininfo struct which is
the same as xen_v0_domaininfo, but with Jim's patch reverted again.
Then provide two unions

  union xen_getdomaininfo {
    struct xen_v0_getdomaininfo v0;
    struct xen_v2_getdomaininfo v2;
  };
  typedef union xen_getdomaininfo xen_getdomaininfo;

  union xen_getdomaininfolist {
    struct xen_v0_getdomaininfo *v0;
    struct xen_v2_getdomaininfo *v2;
  };
  typedef union xen_getdomaininfolist xen_getdomaininfolist;

The caller must populate & read either v0, or v2 as apropriate - to avoid
ugly  if (hypervisor_version < 2) ...v0... else ...v2...  I define a bunhc
of macros for accessing fields in these two unions. eg

  #define XEN_GETDOMAININFOLIST_DOMAIN(domlist, n) \
    (hypervisor_version < 2 ? \
     domlist.v0[n].domain : \
     domlist.v2[n].domain)

Or

  #define XEN_GETDOMAININFOLIST_CLEAR(domlist, size) \
    (hypervisor_version < 2 ? \
     memset(domlist.v0, 0, sizeof(xen_v0_getdomaininfo) * size) : \
     memset(domlist.v2, 0, sizeof(xen_v2_getdomaininfo) * size))


Anyway, I'm attaching a patch which I've tested against 32-bit HV on both
Xen 3.0.2 and 3.0.3, and also a 64-bit HV on 3.0.2 and 3.0.3  and all the
operations now work correctly again...

Regards,
Dan.
-- 
|=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
|=-           Perl modules: http://search.cpan.org/~danberr/              -=|
|=-               Projects: http://freshmeat.net/~danielpb/               -=|
|=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -=| 
Index: src/xen_internal.c
===================================================================
RCS file: /data/cvs/libvirt/src/xen_internal.c,v
retrieving revision 1.41
diff -u -b -r1.41 xen_internal.c
--- src/xen_internal.c	21 Sep 2006 15:24:37 -0000	1.41
+++ src/xen_internal.c	28 Sep 2006 21:23:02 -0000
@@ -99,13 +99,109 @@
 };
 typedef struct xen_v0_getdomaininfo xen_v0_getdomaininfo;
 
-struct xen_v0_getdomaininfolist {
+struct xen_v2_getdomaininfo {
+    domid_t  domain;	/* the domain number */
+    uint32_t flags;	/* falgs, see before */
+    uint64_t tot_pages;	/* total number of pages used */
+    uint64_t max_pages;	/* maximum number of pages allowed */
+    uint64_t shared_info_frame; /* MFN of shared_info struct */
+    uint64_t cpu_time;  /* CPU time used */
+    uint32_t nr_online_vcpus;  /* Number of VCPUs currently online. */
+    uint32_t max_vcpu_id; /* Maximum VCPUID in use by this domain. */
+    uint32_t ssidref;
+    xen_domain_handle_t handle;
+};
+typedef struct xen_v2_getdomaininfo xen_v2_getdomaininfo;
+
+union xen_getdomaininfo {
+    struct xen_v0_getdomaininfo v0;
+    struct xen_v2_getdomaininfo v2;
+};
+typedef union xen_getdomaininfo xen_getdomaininfo;
+
+union xen_getdomaininfolist {
+    struct xen_v0_getdomaininfo *v0;
+    struct xen_v2_getdomaininfo *v2;
+};
+typedef union xen_getdomaininfolist xen_getdomaininfolist;
+
+#define XEN_GETDOMAININFOLIST_ALLOC(domlist, size) \
+    (hypervisor_version < 2 ? \
+     ((domlist.v0 = malloc(sizeof(xen_v0_getdomaininfo)*(size))) != NULL) : \
+     ((domlist.v2 = malloc(sizeof(xen_v2_getdomaininfo)*(size))) != NULL))
+
+#define XEN_GETDOMAININFOLIST_FREE(domlist) \
+    (hypervisor_version < 2 ? \
+     free(domlist.v0) : \
+     free(domlist.v2))
+
+#define XEN_GETDOMAININFOLIST_CLEAR(domlist, size) \
+    (hypervisor_version < 2 ? \
+     memset(domlist.v0, 0, sizeof(xen_v0_getdomaininfo) * size) : \
+     memset(domlist.v2, 0, sizeof(xen_v2_getdomaininfo) * size))
+
+#define XEN_GETDOMAININFOLIST_DOMAIN(domlist, n) \
+    (hypervisor_version < 2 ? \
+     domlist.v0[n].domain : \
+     domlist.v2[n].domain)
+
+
+
+#define XEN_GETDOMAININFO_CLEAR(dominfo) \
+    (hypervisor_version < 2 ? \
+     memset(&(dominfo.v0), 0, sizeof(xen_v0_getdomaininfo)) : \
+     memset(&(dominfo.v2), 0, sizeof(xen_v2_getdomaininfo)))
+
+#define XEN_GETDOMAININFO_DOMAIN(dominfo) \
+    (hypervisor_version < 2 ? \
+     dominfo.v0.domain : \
+     dominfo.v2.domain)
+
+#define XEN_GETDOMAININFO_CPUTIME(dominfo) \
+    (hypervisor_version < 2 ? \
+     dominfo.v0.cpu_time : \
+     dominfo.v2.cpu_time)
+
+#define XEN_GETDOMAININFO_CPUCOUNT(dominfo) \
+    (hypervisor_version < 2 ? \
+     dominfo.v0.nr_online_vcpus : \
+     dominfo.v2.nr_online_vcpus)
+
+#define XEN_GETDOMAININFO_FLAGS(dominfo) \
+    (hypervisor_version < 2 ? \
+     dominfo.v0.flags : \
+     dominfo.v2.flags)
+
+#define XEN_GETDOMAININFO_TOT_PAGES(dominfo) \
+    (hypervisor_version < 2 ? \
+     dominfo.v0.tot_pages : \
+     dominfo.v2.tot_pages)
+
+#define XEN_GETDOMAININFO_MAX_PAGES(dominfo) \
+    (hypervisor_version < 2 ? \
+     dominfo.v0.max_pages : \
+     dominfo.v2.max_pages)
+
+
+
+struct xen_v0_getdomaininfolistop {
     domid_t   first_domain;
     uint32_t  max_domains;
     struct xen_v0_getdomaininfo *buffer;
     uint32_t  num_domains;
 };
-typedef struct xen_v0_getdomaininfolist xen_v0_getdomaininfolist;
+typedef struct xen_v0_getdomaininfolistop xen_v0_getdomaininfolistop;
+
+
+struct xen_v2_getdomaininfolistop {
+    domid_t   first_domain;
+    uint32_t  max_domains;
+    struct xen_v2_getdomaininfo *buffer;
+    uint32_t  num_domains;
+};
+typedef struct xen_v2_getdomaininfolistop xen_v2_getdomaininfolistop;
+
+
 
 struct xen_v0_domainop {
     domid_t   domain;
@@ -244,7 +340,7 @@
     uint32_t cmd;
     uint32_t interface_version;
     union {
-        xen_v0_getdomaininfolist getdomaininfolist;
+        xen_v0_getdomaininfolistop getdomaininfolist;
 	xen_v0_domainop          domain;
 	xen_v0_setmaxmem         setmaxmem;
 	xen_v0_setmaxvcpu        setmaxvcpu;
@@ -261,7 +357,7 @@
     uint32_t cmd;
     uint32_t interface_version;
     union {
-        xen_v0_getdomaininfolist getdomaininfolist;
+        xen_v2_getdomaininfolistop getdomaininfolist;
 	uint8_t padding[128];
     } u;
 };
@@ -545,7 +641,7 @@
  */
 static int
 virXen_getdomaininfolist(int handle, int first_domain, int maxids,
-                         xen_v0_getdomaininfo *dominfos)
+                         xen_getdomaininfolist *dominfos)
 {
     int ret = -1;
 
@@ -561,7 +657,7 @@
 	op.cmd = XEN_V2_OP_GETDOMAININFOLIST;
 	op.u.getdomaininfolist.first_domain = (domid_t) first_domain;
 	op.u.getdomaininfolist.max_domains = maxids;
-	op.u.getdomaininfolist.buffer = dominfos;
+        op.u.getdomaininfolist.buffer = dominfos->v2;
 	op.u.getdomaininfolist.num_domains = maxids;
 	ret = xenHypervisorDoV2Sys(handle, &op);
 	if (ret == 0)
@@ -573,7 +669,7 @@
 	op.cmd = XEN_V1_OP_GETDOMAININFOLIST;
 	op.u.getdomaininfolist.first_domain = (domid_t) first_domain;
 	op.u.getdomaininfolist.max_domains = maxids;
-	op.u.getdomaininfolist.buffer = dominfos;
+        op.u.getdomaininfolist.buffer = dominfos->v0;
 	op.u.getdomaininfolist.num_domains = maxids;
 	ret = xenHypervisorDoV1Op(handle, &op);
 	if (ret == 0)
@@ -585,7 +681,7 @@
 	op.cmd = XEN_V0_OP_GETDOMAININFOLIST;
 	op.u.getdomaininfolist.first_domain = (domid_t) first_domain;
 	op.u.getdomaininfolist.max_domains = maxids;
-	op.u.getdomaininfolist.buffer = dominfos;
+        op.u.getdomaininfolist.buffer = dominfos->v0;
 	op.u.getdomaininfolist.num_domains = maxids;
 	ret = xenHypervisorDoV0Op(handle, &op);
 	if (ret == 0)
@@ -599,6 +695,21 @@
     return(ret);
 }
 
+static int
+virXen_getdomaininfo(int handle, int first_domain,
+                     xen_getdomaininfo *dominfo) {
+    xen_getdomaininfolist dominfos;
+
+    if (hypervisor_version < 2) {
+        dominfos.v0 = &(dominfo->v0);
+    } else {
+        dominfos.v2 = &(dominfo->v2);
+    }
+
+    return virXen_getdomaininfolist(handle, first_domain, 1, &dominfos);
+}
+
+
 #ifndef PROXY
 /**
  * virXen_pausedomain:
@@ -998,7 +1109,7 @@
     int fd, ret, cmd;
     hypercall_t hc;
     v0_hypercall_t v0_hc;
-    xen_v0_getdomaininfo info;
+    xen_getdomaininfo info;
 
     if (initialized) {
         if (hypervisor_version == -1)
@@ -1073,7 +1184,7 @@
     /* TODO: one probably will need to autodetect thse subversions too */
     sys_interface_version = 2; /* XEN_SYSCTL_INTERFACE_VERSION */
     dom_interface_version = 3; /* XEN_DOMCTL_INTERFACE_VERSION */
-    if (virXen_getdomaininfolist(fd, 0, 1, &info) == 1) {
+    if (virXen_getdomaininfo(fd, 0, &info) == 1) {
 #ifdef DEBUG
         fprintf(stderr, "Using hypervisor call v2, sys version 2\n");
 #endif
@@ -1081,7 +1192,7 @@
     }
     hypervisor_version = 1;
     sys_interface_version = -1;
-    if (virXen_getdomaininfolist(fd, 0, 1, &info) == 1) {
+    if (virXen_getdomaininfo(fd, 0, &info) == 1) {
 #ifdef DEBUG
         fprintf(stderr, "Using hypervisor call v1\n");
 #endif
@@ -1227,7 +1338,7 @@
 int
 xenHypervisorNumOfDomains(virConnectPtr conn)
 {
-    xen_v0_getdomaininfo *dominfos;
+    xen_getdomaininfolist dominfos;
     int ret, nbids;
     static int last_maxids = 2;
     int maxids = last_maxids;
@@ -1236,18 +1347,17 @@
         return (-1);
 
 retry:
-    dominfos = malloc(maxids * sizeof(xen_v0_getdomaininfo));
-    if (dominfos == NULL) {
+    if (!(XEN_GETDOMAININFOLIST_ALLOC(dominfos, maxids))) {
         virXenError(VIR_ERR_NO_MEMORY, _("allocating %d domain info"),
 	            maxids);
 	return(-1);
     }
     
-    memset(dominfos, 0, sizeof(xen_v0_getdomaininfo) * maxids);
+    XEN_GETDOMAININFOLIST_CLEAR(dominfos, maxids);
 
-    ret = virXen_getdomaininfolist(conn->handle, 0, maxids, dominfos);
+    ret = virXen_getdomaininfolist(conn->handle, 0, maxids, &dominfos);
 
-    free(dominfos);
+    XEN_GETDOMAININFOLIST_FREE(dominfos);
 
     if (ret < 0)
         return (-1);
@@ -1276,41 +1386,40 @@
 int
 xenHypervisorListDomains(virConnectPtr conn, int *ids, int maxids)
 {
-    xen_v0_getdomaininfo *dominfos;
+    xen_getdomaininfolist dominfos;
     int ret, nbids, i;
 
     if ((conn == NULL) || (conn->handle < 0) ||
         (ids == NULL) || (maxids < 1))
         return (-1);
 
-    dominfos = malloc(maxids * sizeof(xen_v0_getdomaininfo));
-    if (dominfos == NULL) {
+    if (!(XEN_GETDOMAININFOLIST_ALLOC(dominfos, maxids))) {
         virXenError(VIR_ERR_NO_MEMORY, "allocating %d domain info",
 	            maxids);
 	return(-1);
     }
     
-    memset(dominfos, 0, sizeof(xen_v0_getdomaininfo) * maxids);
+    XEN_GETDOMAININFOLIST_CLEAR(dominfos, maxids);
     memset(ids, 0, maxids * sizeof(int));
 
-    ret = virXen_getdomaininfolist(conn->handle, 0, maxids, dominfos);
+    ret = virXen_getdomaininfolist(conn->handle, 0, maxids, &dominfos);
 
     if (ret < 0) {
-	free(dominfos);
+        XEN_GETDOMAININFOLIST_FREE(dominfos);
         return (-1);
     }
 
     nbids = ret;
     if ((nbids < 0) || (nbids > maxids)) {
-	free(dominfos);
+        XEN_GETDOMAININFOLIST_FREE(dominfos);
         return(-1);
     }
 
     for (i = 0;i < nbids;i++) {
-        ids[i] = dominfos[i].domain;
+        ids[i] = XEN_GETDOMAININFOLIST_DOMAIN(dominfos, i);
     }
 
-    free(dominfos);
+    XEN_GETDOMAININFOLIST_FREE(dominfos);
     return (nbids);
 }
 
@@ -1327,21 +1436,20 @@
 unsigned long
 xenHypervisorGetDomMaxMemory(virConnectPtr conn, int id)
 {
-    xen_v0_getdomaininfo dominfo;
+    xen_getdomaininfo dominfo;
     int ret;
 
     if ((conn == NULL) || (conn->handle < 0))
         return (0);
 
-    memset(&dominfo, 0, sizeof(xen_v0_getdomaininfo));
+    XEN_GETDOMAININFO_CLEAR(dominfo);
 
-    dominfo.domain = id;
-    ret = virXen_getdomaininfolist(conn->handle, id, 1, &dominfo);
+    ret = virXen_getdomaininfo(conn->handle, id, &dominfo);
 
-    if ((ret < 0) || (dominfo.domain != id))
+    if ((ret < 0) || (XEN_GETDOMAININFO_DOMAIN(dominfo) != id))
         return (0);
 
-    return((unsigned long) dominfo.max_pages * 4);
+    return((unsigned long) XEN_GETDOMAININFO_MAX_PAGES(dominfo) * 4);
 }
 
 #ifndef PROXY
@@ -1379,21 +1487,21 @@
 int
 xenHypervisorGetDomInfo(virConnectPtr conn, int id, virDomainInfoPtr info)
 {
-    xen_v0_getdomaininfo dominfo;
+    xen_getdomaininfo dominfo;
     int ret;
 
     if ((conn == NULL) || (conn->handle < 0) || (info == NULL))
         return (-1);
 
     memset(info, 0, sizeof(virDomainInfo));
-    memset(&dominfo, 0, sizeof(xen_v0_getdomaininfo));
+    XEN_GETDOMAININFO_CLEAR(dominfo);
 
-    ret = virXen_getdomaininfolist(conn->handle, id, 1, &dominfo);
+    ret = virXen_getdomaininfo(conn->handle, id, &dominfo);
 
-    if ((ret < 0) || (dominfo.domain != id))
+    if ((ret < 0) || (XEN_GETDOMAININFO_DOMAIN(dominfo) != id))
         return (-1);
 
-    switch (dominfo.flags & 0xFF) {
+    switch (XEN_GETDOMAININFO_FLAGS(dominfo) & 0xFF) {
 	case DOMFLAGS_DYING:
 	    info->state = VIR_DOMAIN_SHUTDOWN;
 	    break;
@@ -1418,10 +1526,10 @@
      * convert to microseconds, same thing convert to
      * kilobytes from page counts
      */
-    info->cpuTime = dominfo.cpu_time;
-    info->memory = dominfo.tot_pages * 4;
-    info->maxMem = dominfo.max_pages * 4;
-    info->nrVirtCpu = dominfo.nr_online_vcpus;
+    info->cpuTime = XEN_GETDOMAININFO_CPUTIME(dominfo);
+    info->memory = XEN_GETDOMAININFO_TOT_PAGES(dominfo) * 4;
+    info->maxMem = XEN_GETDOMAININFO_MAX_PAGES(dominfo) * 4;
+    info->nrVirtCpu = XEN_GETDOMAININFO_CPUCOUNT(dominfo);
     return (0);
 }
 
@@ -1620,7 +1728,7 @@
 		      unsigned char *cpumaps, int maplen)
 {
 #ifndef PROXY
-    xen_v0_getdomaininfo dominfo;
+    xen_getdomaininfo dominfo;
     int ret;
 
     virVcpuInfoPtr ipt;
@@ -1634,13 +1742,13 @@
 	return -1;
 
     /* first get the number of virtual CPUs in this domain */
-    memset(&dominfo, 0, sizeof(xen_v0_getdomaininfo));
-    ret = virXen_getdomaininfolist(domain->conn->handle, domain->handle,
-                                   1, &dominfo);
+    XEN_GETDOMAININFO_CLEAR(dominfo);
+    ret = virXen_getdomaininfo(domain->conn->handle, domain->handle,
+                               &dominfo);
 
-    if ((ret < 0) || (dominfo.domain != domain->handle))
+    if ((ret < 0) || (XEN_GETDOMAININFO_DOMAIN(dominfo) != domain->handle))
         return (-1);
-    nbinfo = dominfo.max_vcpu_id + 1;
+    nbinfo = XEN_GETDOMAININFO_CPUCOUNT(dominfo) + 1;
     if (nbinfo > maxinfo) nbinfo = maxinfo;
 
     if (cpumaps != NULL)
@@ -1666,3 +1774,13 @@
     return(-1);
 #endif
 }
+
+
+/*
+ * Local variables:
+ *  indent-tabs-mode: nil
+ *  c-indent-level: 4
+ *  c-basic-offset: 4
+ *  tab-width: 4
+ * End:
+ */

[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]