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(a)redhat.com | libxml GNOME XML XSLT toolkit
http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine
http://rpmfind.net/