On Thu, Jun 14, 2007 at 09:18:14PM -0400, Mark Johnson wrote:
> On 6/14/07, Mark Johnson <johnson.nh@xxxxxxxxx> 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@xxxxxxxxxx | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/