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