[Libvir] PATCH: support Xen 3.0.5

Thu Apr 12 16:33:14 UTC 2007

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