5:01 p.m.
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.
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.
There also code which puts in the the vncpasswd in xend_internal.c.
This is the last patch I'll send out for today...
Mark
6:05 p.m.
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:
@@ -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.
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.
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.
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.
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 -=|
6:21 p.m.
On Fri, Jun 15, 2007 at 12:05:33AM +0100, Daniel P. Berrange wrote:
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
No, this is not true, and it's not true in Xen too. This is stuff that
got merged up in my pygrub changes.
Basically the logic is something like:
if there is no kernel specified:
if there is no bootloader specified:
default to pygrub (for Solaris, this will fill in
kernel/ramdisk/extra automatically)
if there is a bootloader specified:
use the specified bootloader
else:
if there is no bootloader specified:
load kernel/ramdisk from dom0 FS
else
use bootloader to load specified files
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.
An example:
(domain
(domid 15)
(on_crash preserve)
(memory 512)
(uuid dd2b0096-0b4f-db69-20da-ecc19202f2e4)
(bootloader_args )
(name sxc16)
(maxmem 512)
(on_reboot restart)
(on_poweroff destroy)
(localtime 0)
(vcpus 1)
(bootloader )
(shadow_memory 0)
(cpu_weight 256)
(cpu_cap 0)
(features )
(on_xend_start ignore)
(on_xend_stop shutdown)
(start_time 1181862774.81)
(cpu_time 49.172084538)
(online_vcpus 1)
(image (linux (kernel ) (args 'root=/dev/dsk/c0d0s0 ')))
(status 2)
(memory_dynamic_min 511)
(memory_dynamic_max 512)
(state -b----)
(store_mfn 918112)
(console_mfn 918111)
(device
(vif
(mac aa:04:03:35:a8:50)
(uuid 1c1010a0-ddfd-7173-7b8f-2d7115524f73)
)
)
(device
(vbd
(uname phy:/dev/zvol/dsk/export/sxc16-root)
(driver paravirtualised)
(mode w)
(dev 0)
(uuid ac1699dd-5154-6432-8dee-73322c80adaa)
)
)
)
7:02 p.m.
On Fri, Jun 15, 2007 at 12:21:09AM +0100, John Levon wrote:
On Fri, Jun 15, 2007 at 12:05:33AM +0100, Daniel P. Berrange wrote:
> 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
No, this is not true, and it's not true in Xen too. This is stuff that
got merged up in my pygrub changes.
Basically the logic is something like:
if there is no kernel specified:
if there is no bootloader specified:
default to pygrub (for Solaris, this will fill in
kernel/ramdisk/extra automatically)
Ah ha - this is the key clause I was missing. I didn't realize that
XenD could now default to pygrub. The change in logic makes perfect
sense now. Though I wonder if we should add in an explicit element
for <bootloader>/usr/bin/pygrub</bootloader> to reflect this default
done by XenD...
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 -=|
8:26 p.m.
On Fri, Jun 15, 2007 at 01:02:16AM +0100, Daniel P. Berrange wrote:
> > 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
>
> No, this is not true, and it's not true in Xen too. This is stuff that
> got merged up in my pygrub changes.
>
> Basically the logic is something like:
>
> if there is no kernel specified:
> if there is no bootloader specified:
> default to pygrub (for Solaris, this will fill in
> kernel/ramdisk/extra automatically)
Ah ha - this is the key clause I was missing. I didn't realize that
XenD could now default to pygrub. The change in logic makes perfect
sense now. Though I wonder if we should add in an explicit element
for <bootloader>/usr/bin/pygrub</bootloader> to reflect this default
done by XenD...
What would be the reason for this?
pygrub doesn't live at /usr/bin/pygrub on Solaris, so if you need this
we'd need OS-specific hacks...
regards
john
9:20 p.m.
On Fri, Jun 15, 2007 at 02:26:36AM +0100, John Levon wrote:
On Fri, Jun 15, 2007 at 01:02:16AM +0100, Daniel P. Berrange wrote:
> > > 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
> >
> > No, this is not true, and it's not true in Xen too. This is stuff that
> > got merged up in my pygrub changes.
> >
> > Basically the logic is something like:
> >
> > if there is no kernel specified:
> > if there is no bootloader specified:
> > default to pygrub (for Solaris, this will fill in
> > kernel/ramdisk/extra automatically)
>
> Ah ha - this is the key clause I was missing. I didn't realize that
> XenD could now default to pygrub. The change in logic makes perfect
> sense now. Though I wonder if we should add in an explicit element
> for <bootloader>/usr/bin/pygrub</bootloader> to reflect this default
> done by XenD...
What would be the reason for this?
Well to give some form of indication as to how the guest is being booted.
Perhaps rather than making up a default path, just an empty <bootloader/>
element would work. The semantics being launch with the default bootloader
for the platform. That would avoid having to include any specific path
info
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 -=|
9:26 p.m.
On Fri, Jun 15, 2007 at 03:20:19AM +0100, Daniel P. Berrange wrote:
Well to give some form of indication as to how the guest is being
booted.
Perhaps rather than making up a default path, just an empty <bootloader/>
element would work. The semantics being launch with the default bootloader
for the platform. That would avoid having to include any specific path
info
Sounds reasonable to me.
regards,
john
4:31 a.m.
On Fri, Jun 15, 2007 at 03:20:19AM +0100, Daniel P. Berrange wrote:
On Fri, Jun 15, 2007 at 02:26:36AM +0100, John Levon wrote:
> On Fri, Jun 15, 2007 at 01:02:16AM +0100, Daniel P. Berrange wrote:
>
> > > > 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
> > >
> > > No, this is not true, and it's not true in Xen too. This is stuff
that
> > > got merged up in my pygrub changes.
> > >
> > > Basically the logic is something like:
> > >
> > > if there is no kernel specified:
> > > if there is no bootloader specified:
> > > default to pygrub (for Solaris, this will fill in
> > > kernel/ramdisk/extra automatically)
> >
> > Ah ha - this is the key clause I was missing. I didn't realize that
> > XenD could now default to pygrub. The change in logic makes perfect
> > sense now. Though I wonder if we should add in an explicit element
> > for <bootloader>/usr/bin/pygrub</bootloader> to reflect this
default
> > done by XenD...
>
> What would be the reason for this?
Well to give some form of indication as to how the guest is being booted.
Perhaps rather than making up a default path, just an empty <bootloader/>
element would work. The semantics being launch with the default bootloader
for the platform. That would avoid having to include any specific path
info
agreed too, and adding an XML comment
<!-- use the default bootloader -->
would IMHO be even more user friendly.
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/
6:39 p.m.
On 6/14/07, Daniel P. Berrange <berrange(a)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 -=|
8:18 p.m.
On 6/14/07, Mark Johnson <johnson.nh(a)gmail.com> wrote:
> 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.
Ah, I see what your saying now.. I've updated the patch to fix change that
part.
Mark.
4:57 a.m.
On Thu, Jun 14, 2007 at 09:18:14PM -0400, Mark Johnson wrote:
On 6/14/07, Mark Johnson <johnson.nh(a)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(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
6:27 a.m.
On Fri, Jun 15, 2007 at 05:57:30AM -0400, Daniel Veillard wrote:
> #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
I recently putback a change to install in xen/sys/* on Linux too. I
think this will need to be a configure test at some point, to support
older Xens. That is:
older Xen on Linux: linux/privcmd.h
newer Xen on Linux: sys/privcmd.h
Xen on Solaris: sys/privcmd.h
makes sense, so you changed the hypervisor to copy or pin the user
data ?
Actually the dom0 kernel does this.
> +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
In terms of cpuid, it works on Linux too, except that Linux has
/dev/cpu/0/cpuid and we have /dev/cpu/self/cpuid. Also I don't think you
can rely on the Linux driver being available.
regards
john
6:46 a.m.
On Fri, Jun 15, 2007 at 12:27:01PM +0100, John Levon wrote:
On Fri, Jun 15, 2007 at 05:57:30AM -0400, Daniel Veillard wrote:
> > #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
I recently putback a change to install in xen/sys/* on Linux too. I
think this will need to be a configure test at some point, to support
older Xens. That is:
older Xen on Linux: linux/privcmd.h
newer Xen on Linux: sys/privcmd.h
Xen on Solaris: sys/privcmd.h
if we can solve this as configure test, that's perfect IMHO.
> makes sense, so you changed the hypervisor to copy or pin the
user data ?
Actually the dom0 kernel does this.
Hum, right that's not a direct call but ioctl :-)
> > +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
In terms of cpuid, it works on Linux too, except that Linux has
/dev/cpu/0/cpuid and we have /dev/cpu/self/cpuid. Also I don't think you
can rely on the Linux driver being available.
Hum, I reacted to the names which to me were register names, and then
I assumed this was arch specific, i.e. would generate problems say on PPC.
It seems your solaris code is x86_64 exclusively. In a linux section we
will need to take care of ppc, ia64 and i386 too. So thing like eax, ebx
ecx and edx looks suspicious to me.
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/
7:17 a.m.
On Fri, Jun 15, 2007 at 07:46:14AM -0400, Daniel Veillard wrote:
> > > +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
>
> In terms of cpuid, it works on Linux too, except that Linux has
> /dev/cpu/0/cpuid and we have /dev/cpu/self/cpuid. Also I don't think you
> can rely on the Linux driver being available.
Hum, I reacted to the names which to me were register names, and then
I assumed this was arch specific, i.e. would generate problems say on PPC.
It seems your solaris code is x86_64 exclusively. In a linux section we
will need to take care of ppc, ia64 and i386 too. So thing like eax, ebx
ecx and edx looks suspicious to me.
Sorry, I misread that as "OS dependent". You're right that it's
specific
to x86 (though should work on x86_64 and i386?)
regards
john
9:34 a.m.
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 -=|
3:32 a.m.
A few general questions (referring to version 2 of this patch):
Solaris doesn't require Xen hypervisor structures to be locked into
memory on hypercalls? I see the same thing happens in libxc too, so
that's right, but I don't understand how the hypervisor swaps pages in
if they aren't present ...
I agree with Dan that if it's possible to do the capabilities stuff
using a hypercall, we should just do that and ditch the old Linux
regexp/parsing code.
I'm curious about this:
+ if (strncmp(s, "Auth" "cAMD" "enti", 12) == 0) {
and this:
+ } else if (strncmp(s, "Genu" "ntel" "ineI", 12) == 0)
{
For the rest I don't have anything to say beyond what Dan has already
said. The patch looks like it's almost there.
Rich.
--
Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/
Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod
Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in
England and Wales under Company Registration No. 03798903
6370
days inactive
6371
days old
15 comments
5 participants
participants (5)
-
Daniel P. Berrange -
Daniel Veillard -
John Levon -
Mark Johnson -
Richard W.M. Jones