On 6/14/07, Daniel P. Berrange <berrange@redhat.com> wrote:

On Thu, Jun 14, 2007 at 06:01:56PM -0400, Mark Johnson wrote:

...

This is a little bigger of a patch. It has a couple of things in it.

First, in xen_internal.c and xs_internal.c have general dom0 support for Solaris. Again, using #ifdef/ifndef __linux__ to separate the logic.

I'm getting a little confused about the xen_internal.c changes for the hypercalls.

This is chunk:

Yeah, another funcky diff.. I'll send a different diff for this file later tonight. I'm managing the changes out of a hg mq gate.

...

@@ -38,6 +42,8 @@ #include "xml.h"

/* #define DEBUG */ + +#ifdef __linux__ /* * so far there is 2 versions of the structures usable for doing * hypervisor calls. @@ -60,6 +66,14 @@ typedef struct v1_hypercall_struct _IOC(_IOC_NONE, 'P', 0, sizeof(v1_hypercall_t))

typedef v1_hypercall_t hypercall_t; + +#else +typedef struct v0_hypercall_struct { + unsigned long op; + unsigned long arg[5]; +} v0_hypercall_t; +typedef privcmd_hypercall_t hypercall_t; +#endif

Seems to be redefining v0_hypercall_t to be the same struct in both halves of the #ifdef. But then later on, all references to v0_hypercall_t in the code are #ifdef'd out for Solaris, and hypercall_t is defined in terms of 'privcmd_hypercall_t' from the tools/include/SunOS/privcmd.h which is identical to the v0_hypercall_t we've already got.

That's just the diff. I don't actually change this.

I think this could probably be simplified a little, but not quite sure how just yet. Will look at how the patch applies.

The mlock -> lock_pages() stuff all makes sense - was expecting that from the changes you've sent to libxc previously.

The capabilities stuff is interesting - replacing the code which reads from /sys/hypervisor/properties/capabilities with a direct hypercall to fetch it from Xen. I'll have to check if that hypercall is actually available on Linux too - if it is, then I'd probably get rid of the reading of /sys/hypervisor/properties/capabilities completely and make both platforms do the same hypercall.

Yes. This is the actual hypercall which is in /sys/hypercall/../capabilities :-)

...

In xml.c and xend_internal.c, the code which requires kernel to be defined if ifdef __linux__'d. For our setup, If bootloader, kernel, and ramdisk aren't defined, pygrub will got looking for the right bits.

I'm curious as to what the changes for bootloader / kernel are for ? Surely you always have either a bootloader, or a kenrel present in the SEXPR ? So I'm not sure why its neccessary to disable the check for validating presence of one of the two. I'd be interested to see the output of 'xm list --long' SEXPR on a Solaris system with a few guests running.

I saw John answered this..

...

There also code which puts in the the vncpasswd in xend_internal.c.

Yes that's been needed for a while, although I think we want to make inclusion of the password conditional based on a VIR_SECURE_XML flag passed into the virDomainDumpXML method. Its quite common for apps to log XML in various places so rather not have password included by default all the time unless an app explicitly needs it.

OK. Do you want me to take a stab at that or is it an easy change? Thanks, MRJ

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 -=|

On Thu, Jun 14, 2007 at 09:18:14PM -0400, Mark Johnson wrote:

On 6/14/07, Mark Johnson <johnson.nh@gmail.com> wrote:

...

...

Seems to be redefining v0_hypercall_t to be the same struct in both halves of the #ifdef. But then later on, all references to v0_hypercall_t in the code are #ifdef'd out for Solaris, and hypercall_t is defined in terms of 'privcmd_hypercall_t' from the tools/include/SunOS/privcmd.h which is identical to the v0_hypercall_t we've already got.

Ah, I see what your saying now.. I've updated the patch to fix change that part.

I had the same reaction as Dan when reading it. That looks quite nicer. Comments inline:

#include <xen/xen.h> +#ifdef __linux__ #include <xen/linux/privcmd.h> +#else +#include <xen/sys/privcmd.h> +#endif

In general I would prefer Solaris sections to not be defined as !linux but as a positive test on your platform macro, but in that case since it's a Xen path it's fine

/* required for shutdown flags */ #include <xen/sched.h> @@ -38,6 +42,7 @@ #include "xml.h"

/* #define DEBUG */ + /* * so far there is 2 versions of the structures usable for doing * hypervisor calls. @@ -47,6 +52,7 @@ typedef struct v0_hypercall_struct { unsigned long op; unsigned long arg[5]; } v0_hypercall_t; +#ifdef __linux__ #define XEN_V0_IOCTL_HYPERCALL_CMD \ _IOC(_IOC_NONE, 'P', 0, sizeof(v0_hypercall_t))

@@ -60,6 +66,10 @@ typedef struct v1_hypercall_struct _IOC(_IOC_NONE, 'P', 0, sizeof(v1_hypercall_t))

typedef v1_hypercall_t hypercall_t; + +#else +typedef privcmd_hypercall_t hypercall_t; +#endif

Just a question. So you have blocked your hypervisor API to be the one defined in the old Xen ?

-#define XEN_HYPERVISOR_SOCKET "/proc/xen/privcmd" +#ifdef __linux__ +#define XEN_HYPERVISOR_SOCKET "/proc/xen/privcmd" +#define HYPERVISOR_CAPABILITIES "/sys/hypervisor/properties/capabilities" +#define CPUINFO "/proc/cpuinfo" +#else +#define XEN_HYPERVISOR_SOCKET "/dev/xen/privcmd" +#define HYPERVISOR_CAPABILITIES "" +#define CPUINFO "/dev/cpu/self/cpuid" +#endif

Good, but the paths are either linux specific, or solaris specific I would prefer something like #ifdef __linux__ #else #ifdef __sun__ /* or whatever macro you see suit */ #else #error "unsupported platform" #endif #endif I use #error in libxml2 and so far never got a protability problem with it

+static int lock_pages(void *addr, size_t len); +static int unlock_pages(void *addr, size_t len);

[...]

+static int +lock_pages(void *addr, size_t len) +{ +#ifdef __linux__ + return (mlock(addr, len)); +#else + return (0); +#endif +} + +static int +unlock_pages(void *addr, size_t len) +{ +#ifdef __linux__ + return (munlock(addr, len)); +#else + return (0); +#endif +} + + makes sense, so you changed the hypervisor to copy or pin the user data ? Could we actually move the function just after the macro definitions instead of down at the bottom of the module for readability ?

+#ifdef __linux__ +void +loadCapabilities(FILE *cpuinfo, FILE *capabilities, char *hvm_type, + int *host_pae, char *line, int LINE_SIZE) +{ + regmatch_t subs[4]; + + /* /proc/cpuinfo: flags: Intel calls HVM "vmx", AMD calls it "svm". + * It's not clear if this will work on IA64, let alone other + * architectures and non-Linux. (XXX) + */ + if (cpuinfo) { + while (fgets (line, LINE_SIZE, cpuinfo)) { + if (regexec (&flags_hvm_rec, line, + sizeof(subs)/sizeof(regmatch_t), subs, 0) == 0 + && subs[0].rm_so != -1) { + strncpy (hvm_type, + &line[subs[1].rm_so], subs[1].rm_eo-subs[1].rm_so+1); + hvm_type[subs[1].rm_eo-subs[1].rm_so] = '\0'; + } else if (regexec (&flags_pae_rec, line, 0, NULL, 0) == 0) + *host_pae = 1; + } + } + + /* Expecting one line in this file - ignore any more. */ + (void) fgets(line, LINE_SIZE, capabilities); +} + +#else

I wasn't too fond of the regexp call there, if we could find something cleaner like the hypercall below that would be nice. My main concern is that I'm not sure the hypercall exists for old Xen versions we still try to support on Linux. But that's not your problem, we should apply this and sort it out later.

+void +loadCapabilities(FILE *cpuinfo, FILE *capabilities, char *hvm_type, + int *host_pae, char *line, int LINE_SIZE) +{ + struct { + uint32_t r_eax, r_ebx, r_ecx, r_edx; + } _r, *rp = &_r; + int d, cmd; + char *s; + hypercall_t hc;

So that's arch dependant right ? Could you #else #ifdef __sun__ just before this function then ?

+ + if ((d = open(CPUINFO, O_RDONLY)) == -1) { + goto cpuinfo_open_fail; + } + + if (pread(d, rp, sizeof (*rp), 0) != sizeof (*rp)) { + goto cpuinfo_pread_fail; + } + + s = (char *)&rp->r_ebx; + if (strncmp(s, "Auth" "cAMD" "enti", 12) == 0) { + if (pread(d, rp, sizeof (*rp), 0x80000001) == sizeof (*rp)) { + /* Read secure virtual machine bit (bit 2 of ECX feature ID) */ + if ((rp->r_ecx >> 2) & 1) { + strcpy(hvm_type, "svm"); + } + if ((rp->r_edx >> 6) & 1) { + *host_pae = 1; + } + } + } else if (strncmp(s, "Genu" "ntel" "ineI", 12) == 0) { + if (pread(d, rp, sizeof (*rp), 0x00000001) == sizeof (*rp)) { + /* Read VMXE feature bit (bit 5 of ECX feature ID) */ + if ((rp->r_ecx >> 5) & 1) { + strcpy(hvm_type, "vmx"); + } + if ((rp->r_edx >> 6) & 1) { + *host_pae = 1; + } + } + } +cpuinfo_pread_fail: + (void) close(d); +cpuinfo_open_fail: + + d = open(XEN_HYPERVISOR_SOCKET, O_RDWR); + hc.op = __HYPERVISOR_xen_version; + hc.arg[0] = (unsigned long) XENVER_capabilities; + hc.arg[1] = (unsigned long) line; + cmd = IOCTL_PRIVCMD_HYPERCALL; + (void) ioctl(d, cmd, (unsigned long) &hc); + close(d); +} + +#endif + [...] @@ -1660,10 +1664,13 @@ xend_parse_sexp_desc(virConnectPtr conn, } else if (tmp && !strcmp(tmp, "vnc")) { int port = xenStoreDomainGetVNCPort(conn, domid); const char *listenAddr = sexpr_node(node, "device/vfb/vnclisten"); + const char *vncPasswd = sexpr_node(node, "device/vfb/vncpasswd"); const char *keymap = sexpr_node(node, "device/vfb/keymap"); virBufferVSprintf(&buf, " <graphics type='vnc' port='%d'", port); if (listenAddr) virBufferVSprintf(&buf, " listen='%s'", listenAddr); + if (vncPasswd) + virBufferVSprintf(&buf, " passwd='%s'", vncPasswd); if (keymap) virBufferVSprintf(&buf, " keymap='%s'", keymap); virBufferAdd(&buf, "/>\n", 3);

Hum, didn't we decided on the list that we didn't like having password in clear in the config file. I'm a bit worried by that actually

@@ -1709,6 +1716,7 @@ xend_parse_sexp_desc(virConnectPtr conn, if (tmp[0] == '1') { int port = xenStoreDomainGetVNCPort(conn, domid); const char *listenAddr = sexpr_fmt_node(root, "domain/image/%s/vnclisten", hvm ? "hvm" : "linux"); + const char *vncPasswd = sexpr_fmt_node(root, "domain/image/%s/vncpasswd", hvm ? "hvm" : "linux"); const char *keymap = sexpr_fmt_node(root, "domain/image/%s/keymap", hvm ? "hvm" : "linux"); /* For Xen >= 3.0.3, don't generate a fixed port mapping * because it will almost certainly be wrong ! Just leave @@ -1721,6 +1729,8 @@ xend_parse_sexp_desc(virConnectPtr conn, virBufferVSprintf(&buf, " <graphics type='vnc' port='%d'", port); if (listenAddr) virBufferVSprintf(&buf, " listen='%s'", listenAddr); + if (vncPasswd) + virBufferVSprintf(&buf, " passwd='%s'", vncPasswd); if (keymap) virBufferVSprintf(&buf, " keymap='%s'", keymap); virBufferAdd(&buf, "/>\n", 3); diff --git a/src/xml.c b/src/xml.c --- a/src/xml.c +++ b/src/xml.c @@ -812,12 +812,18 @@ virDomainParseXMLOSDescPV(virConnectPtr return (-1); } virBufferAdd(buf, "(image (linux ", 14); +#ifdef __linux__ if (kernel == NULL) { virXMLError(conn, VIR_ERR_NO_KERNEL, NULL, 0); return (-1); } else { virBufferVSprintf(buf, "(kernel '%s')", (const char *) kernel); } +#else + if (kernel != NULL) { + virBufferVSprintf(buf, "(kernel '%s')", (const char *) kernel); + } +#endif

hum is that really minimal we can probably avoid the duplicate code no ?

if (initrd != NULL) virBufferVSprintf(buf, "(ramdisk '%s')", (const char *) initrd); if (root != NULL) diff --git a/src/xs_internal.c b/src/xs_internal.c --- a/src/xs_internal.c +++ b/src/xs_internal.c @@ -31,7 +31,11 @@ #include "xs_internal.h" #include "xen_internal.h" /* for xenHypervisorCheckID */

+#ifdef __linux__ #define XEN_HYPERVISOR_SOCKET "/proc/xen/privcmd" +#else

#ifdef __sun__

+#define XEN_HYPERVISOR_SOCKET "/dev/xen/privcmd" #else #error "unsupported architecture" #endif +#endif

#ifndef PROXY static char *xenStoreDomainGetOSType(virDomainPtr domain);

Thanks a lot, this all looks pretty good, just a few minor things :-) 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/

On Fri, Jun 15, 2007 at 05:57:30AM -0400, Daniel Veillard wrote:

On Thu, Jun 14, 2007 at 09:18:14PM -0400, Mark Johnson wrote:

...

diff --git a/src/xml.c b/src/xml.c --- a/src/xml.c +++ b/src/xml.c @@ -812,12 +812,18 @@ virDomainParseXMLOSDescPV(virConnectPtr return (-1); } virBufferAdd(buf, "(image (linux ", 14); +#ifdef __linux__ if (kernel == NULL) { virXMLError(conn, VIR_ERR_NO_KERNEL, NULL, 0); return (-1); } else { virBufferVSprintf(buf, "(kernel '%s')", (const char *) kernel); } +#else + if (kernel != NULL) { + virBufferVSprintf(buf, "(kernel '%s')", (const char *) kernel); + } +#endif

hum is that really minimal we can probably avoid the duplicate code no ?

Since John's change to upstream XenD for bootloaders impacts both Solaris & Linux we need this change unconditionally on both platforms. 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 -=|