On Thu, Apr 12, 2007 at 02:46:46AM +0100, Daniel P. Berrange wrote:
I've been doing some testing with current xen-unstable (ie what will very shortly be 3.0.5) and came across a whole bunch of things which needed fixing - some expected, others not expected. The attached patch addresses the following issues:
Okay, thanks a lot for this !
- Many of the hypercalls have their structs changed so that int64_t or 'foo *' members are always 64-bit aligned even on 32-bit platforms. This is part of the work to allow 32-bit Dom0/DomU to work on 64-bit hypervisor.
For the int64_t types I had to annotate with __attribute__((aligned(8))). This did not work for pointer data types, so I for those I had to do a more complex hack with
union { foo *v; int64_t pad __attribute__((aligned(8))) }
This matches what is done in the public (BSD licensed) Xen HV header files.
We already had ways to deal with v0 vs v2 hypercalls structs. This change is still techincally v2, but just a minor revision of the domctl or sysctl interfaces. Thus I have named the extra structs v2d5 or v2s3 to indicated hypercall version 2, domctl version 5 or hypercall version 2, sysctl version 3 respectively.
Though not completely see xen_op_v2_dom remark below
- The 'flags' field in the getdomaininfo hypercall now has an extra flag defined '(1<<1)' which was previously not used, is now used to indicate that the guest is HVM. Thus when fetching domain state, we have to mask out that flag, otherwise we'll never match the correct paused/running/ blocked/etc states.
<grin/>
- In the xenHypervisorNumOfDomains method, under certain scenarios we will re-try the hypercall, allocating a bigger memory buffer. Well due to the ABI alignment changes we hit that scenario everytime, and ended up allocating a multi-GB buffer :-) The fixed structs sort this out, but as a preventative measure for any future HV changes the patch will break out of the loop at the 10,000 guest mark to avoid allocating GB of memory.
That was a bug on our side :-)
- The unified Xen driver broke the GetVCPUs method - it was mistakenly
That too !
- The method to open the XenD connection was calling xenDaemonGetVersion to test if the connection succeeded. But then also calling the xend_detect_config_version which does pretty much same thing. So I removed the former, and now we only do the latter as a 'ping' test when opening. This removes 1 HTTP GET which is worthwhile performance boost given how horrifically slow XenD is.
Good catch, I guess the detection was done only in one of the pre-driver code then it was cleaned up, and the test was added at connection open time, but unfortunately wasn't removed in the latter case, bug++
- The HVM SEXPR for configuring the VNC / SDL graphics is no longere part of the (image) block. it now matches the PVFB graphics config and is an explicit (vfb) block within the (devices) block. So if xend_config_format >= 4 we use the new style config - this is assuming upstream XenD is patched to increment xend_config_format from 3 to 4 - I send a patch & am confident it will be applied very shortly.
you mean the patch will be in before 3.0.5 ?
- The QEMU device model allows a user to specify multiple devices for the boot order, eg 'andc' to indicated 'floppy', 'network' 'cdrom', 'disk'. We assumed it was a single letter only. I now serialize this into multiple <boot dev='XXX'/> elements, ordered according to priority. The XML -> SEXPR conversion allows the same.
I've tested all this on a 32-bit Dom0 running on 32-bit HV, and 64-bit HV, but not tested a 64-bit Dom0 on 64-bit HV. I'm pretty sure it'll work,but if anyone is runnning 64-on-64 please test this patch.
cool thanks, a few comments on the patch below I suggest to commit this, wait for xen-3.0.5 to officially roll out and then make a new libvirt release. The painful thing is regression tests, we don't really have a good answer some of the entry points are tested by virt-manager but for example the CPU affinity stuff is really uncommon, actually it took months before we found an error in the last change of hypercalls. From a patch perspective I feel relatively safe that this won't break with the older hypervisor, but getting to actually testing it doesn't look fun at all. [...]
+ +/* As of Hypervisor Call v2, DomCtl v5 we are now 8-byte aligned + even on 32-bit archs when dealing with uint64_t */ +#define ALIGN_64 __attribute__((aligned(8)))
I'm wondering, should we test for GCC version here and #error if not, so that people who may compile with a different compiler may have a chance to catch potential problem here ?
@@ -415,10 +508,14 @@ struct xen_op_v2_dom { domid_t domain; union { xen_v2_setmaxmem setmaxmem; + xen_v2d5_setmaxmem setmaxmemd5; xen_v2_setmaxvcpu setmaxvcpu; xen_v2_setvcpumap setvcpumap; + xen_v2d5_setvcpumap setvcpumapd5; xen_v2_vcpuinfo getvcpuinfo; + xen_v2d5_vcpuinfo getvcpuinfod5; xen_v2_getvcpumap getvcpumap; + xen_v2d5_getvcpumap getvcpumapd5; uint8_t padding[128]; } u; };
I was a bit surprized by that, somehow I was expecting different struct xen_op_v2_dom and struct xen_op_v2d5_dom but that allows to minimize the change and only the fields in the union are impacted so that's probably better, yes.
@@ -1802,10 +1949,18 @@ xenHypervisorNumOfDomains(virConnectPtr return (-1);
nbids = ret; + /* Can't possibly have more than 10,000 concurrent guests + * so limit how many times we try, to avoid increasing + * without bound & thus allocating all of system memory ! + * XXX I'll regret this comment in a few years time ;-) + */
hehe, now if Xen headers exported a maximum number of domain that would be be clean. I would be surprized if there wasn't a hardcoded limit but I was unable to find one under /usr/include/xen headers ...
if (nbids == maxids) { - last_maxids *= 2; - maxids *= 2; - goto retry; + if (maxids < 10000) { + last_maxids *= 2; + maxids *= 2; + goto retry; + } + nbids = -1; } if ((nbids < 0) || (nbids > maxids)) return(-1);
I tried to look for other places where we may grow/realloc data like that in the code, but apparently that's the only place, maybe except some buffer handling but that looked safe, not as a loop like this !
@@ -1994,7 +2149,8 @@ xenHypervisorGetDomInfo(virConnectPtr co return (-1);
domain_flags = XEN_GETDOMAININFO_FLAGS(dominfo); - domain_state = domain_flags & 0xFF; + domain_flags &= ~DOMFLAGS_HVM; /* Mask out HVM flags */ + domain_state = domain_flags & 0xFF; /* Mask out high bits */ switch (domain_state) { case DOMFLAGS_DYING: info->state = VIR_DOMAIN_SHUTDOWN;
<regrin/> thanks again, please apply, Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/