[PATCH] /sys/hypervisor/uuid

[PATCH] /sys/hypervisor/uuid

[Markus Armbruster]

New /sys/hypervisor/uuid, containing this domain's UUID.

Stripping off /vm/ from the value of vm to get the UUID isn't exactly
nice.  The alternative is to add a XENVER_get_uuid code to
HYPERVISOR_xen_version(), but I'm not sure that's worth it.

Signed-off-by: Markus Armbruster <armbru@redhat.com>


diff -r ddba92a5cba9 drivers/xen/core/xen_sysfs.c
--- a/drivers/xen/core/xen_sysfs.c	Tue May 09 12:41:38 2006 +0200
+++ b/drivers/xen/core/xen_sysfs.c	Thu May 18 18:06:35 2006 +0200
@@ -8,12 +8,14 @@
  */
 
 #include <linux/config.h>
+#include <linux/err.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/init.h>
 #include <asm/hypervisor.h>
 #include <xen/features.h>
 #include <xen/hypervisor_sysfs.h>
+#include <xen/xenbus.h>
 
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Mike D. Day <ncmike@us.ibm.com>");
@@ -92,6 +94,37 @@ static void xen_sysfs_version_destroy(vo
 static void xen_sysfs_version_destroy(void)
 {
 	sysfs_remove_group(&hypervisor_subsys.kset.kobj, &version_group);
+}
+
+/* UUID */
+
+static ssize_t uuid_show(struct hyp_sysfs_attr *attr, char *buffer)
+{
+	char *val;
+	int ret;
+
+	val = xenbus_read(XBT_NULL, "vm", "", NULL);
+	if (IS_ERR(val))
+		ret = PTR_ERR(val);
+	else if (strncmp(val, "/vm/", 4))
+		ret = -EIO;
+	else
+		ret = sprintf(buffer, "%s\n", val + 4);
+	kfree(val);
+
+	return ret;
+}
+
+HYPERVISOR_ATTR_RO(uuid);
+
+static int __init xen_sysfs_uuid_init(void)
+{
+	return sysfs_create_file(&hypervisor_subsys.kset.kobj, &uuid_attr.attr);
+}
+
+static void xen_sysfs_uuid_destroy(void)
+{
+	sysfs_remove_file(&hypervisor_subsys.kset.kobj, &uuid_attr.attr);
 }
 
 /* xen compilation attributes */
@@ -285,10 +318,15 @@ static int __init hyper_sysfs_init(void)
 	ret = xen_compilation_init();
 	if (ret)
 		goto comp_out;
+	ret = xen_sysfs_uuid_init();
+	if (ret)
+		goto uuid_out;
 	ret = xen_properties_init();
 	if (!ret)
 		goto out;
 
+	xen_sysfs_uuid_destroy();
+uuid_out:
 	xen_compilation_destroy();
 comp_out:
 	xen_sysfs_version_destroy();
@@ -302,6 +340,7 @@ static void hyper_sysfs_exit(void)
 {
 	xen_properties_destroy();
 	xen_compilation_destroy();
+	xen_sysfs_uuid_destroy();
 	xen_sysfs_version_destroy();
 	xen_sysfs_type_destroy();

Re: [PATCH] /sys/hypervisor/uuid

[Keir Fraser]

On 18 May 2006, at 18:20, Markus Armbruster wrote:

> New /sys/hypervisor/uuid, containing this domain's UUID.
>
> Stripping off /vm/ from the value of vm to get the UUID isn't exactly
> nice.  The alternative is to add a XENVER_get_uuid code to
> HYPERVISOR_xen_version(), but I'm not sure that's worth it.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Christian has a fair point that, if you're just reading it out of 
xenstore, you could do that directly from user space. I suppose there 
is an argument of necessary privileges to do so however, since you need 
to be root to open the xenstore device file.

  -- Keir

Re: [PATCH] /sys/hypervisor/uuid

[Anthony Liguori]

Markus Armbruster wrote:
> New /sys/hypervisor/uuid, containing this domain's UUID.
>
> Stripping off /vm/ from the value of vm to get the UUID isn't exactly
> nice.  The alternative is to add a XENVER_get_uuid code to
> HYPERVISOR_xen_version(), but I'm not sure that's worth it.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>   

I really don't think we should be mirroring things in sysfs that are 
available in userspace.  What benefit is there of having two interfaces 
to the same information?

If the argument is that we don't want to rely on libxenstore in a domU, 
then we should be exposing all of XenStore in sysfs.

The uuid parameter is a construction of Xend.  Xend is *not* a part of 
the supported guest interface.  By exposing the uuid in the kernel 
interface, you're tying an unsupported interface to what really should 
be a supported interface.

Plus, there's already a patch floating on LKML for a /sys/hypervisor.  
We shouldn't start adding attributes to this namespace as it could 
potentially conflict with the existing patch.

In the very least, we ought to stick this stuff in /sys/hypervisor/xen.

Regards,

Anthony Liguori

> diff -r ddba92a5cba9 drivers/xen/core/xen_sysfs.c
> --- a/drivers/xen/core/xen_sysfs.c	Tue May 09 12:41:38 2006 +0200
> +++ b/drivers/xen/core/xen_sysfs.c	Thu May 18 18:06:35 2006 +0200
> @@ -8,12 +8,14 @@
>   */
>  
>  #include <linux/config.h>
> +#include <linux/err.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/init.h>
>  #include <asm/hypervisor.h>
>  #include <xen/features.h>
>  #include <xen/hypervisor_sysfs.h>
> +#include <xen/xenbus.h>
>  
>  MODULE_LICENSE("GPL");
>  MODULE_AUTHOR("Mike D. Day <ncmike@us.ibm.com>");
> @@ -92,6 +94,37 @@ static void xen_sysfs_version_destroy(vo
>  static void xen_sysfs_version_destroy(void)
>  {
>  	sysfs_remove_group(&hypervisor_subsys.kset.kobj, &version_group);
> +}
> +
> +/* UUID */
> +
> +static ssize_t uuid_show(struct hyp_sysfs_attr *attr, char *buffer)
> +{
> +	char *val;
> +	int ret;
> +
> +	val = xenbus_read(XBT_NULL, "vm", "", NULL);
> +	if (IS_ERR(val))
> +		ret = PTR_ERR(val);
> +	else if (strncmp(val, "/vm/", 4))
> +		ret = -EIO;
> +	else
> +		ret = sprintf(buffer, "%s\n", val + 4);
> +	kfree(val);
> +
> +	return ret;
> +}
> +
> +HYPERVISOR_ATTR_RO(uuid);
> +
> +static int __init xen_sysfs_uuid_init(void)
> +{
> +	return sysfs_create_file(&hypervisor_subsys.kset.kobj, &uuid_attr.attr);
> +}
> +
> +static void xen_sysfs_uuid_destroy(void)
> +{
> +	sysfs_remove_file(&hypervisor_subsys.kset.kobj, &uuid_attr.attr);
>  }
>  
>  /* xen compilation attributes */
> @@ -285,10 +318,15 @@ static int __init hyper_sysfs_init(void)
>  	ret = xen_compilation_init();
>  	if (ret)
>  		goto comp_out;
> +	ret = xen_sysfs_uuid_init();
> +	if (ret)
> +		goto uuid_out;
>  	ret = xen_properties_init();
>  	if (!ret)
>  		goto out;
>  
> +	xen_sysfs_uuid_destroy();
> +uuid_out:
>  	xen_compilation_destroy();
>  comp_out:
>  	xen_sysfs_version_destroy();
> @@ -302,6 +340,7 @@ static void hyper_sysfs_exit(void)
>  {
>  	xen_properties_destroy();
>  	xen_compilation_destroy();
> +	xen_sysfs_uuid_destroy();
>  	xen_sysfs_version_destroy();
>  	xen_sysfs_type_destroy();
>  
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
>   

Re: [PATCH] /sys/hypervisor/uuid

[Chris Wright]

* Keir Fraser (Keir.Fraser@cl.cam.ac.uk) wrote:
> Christian has a fair point that, if you're just reading it out of 
> xenstore, you could do that directly from user space. I suppose there 
> is an argument of necessary privileges to do so however, since you need 
> to be root to open the xenstore device file.

Privileges part is a bit annoying.  But if the envisioned user
is unprivilged, some init script could always stash uuid in a
world readable file.

thanks,
-chris

Re: [PATCH] /sys/hypervisor/uuid

[Christian Limpach]

On 5/19/06, Chris Wright <chrisw@sous-sol.org> wrote:
> * Keir Fraser (Keir.Fraser@cl.cam.ac.uk) wrote:
> > Christian has a fair point that, if you're just reading it out of
> > xenstore, you could do that directly from user space. I suppose there
> > is an argument of necessary privileges to do so however, since you need
> > to be root to open the xenstore device file.
>
> Privileges part is a bit annoying.  But if the envisioned user
> is unprivilged, some init script could always stash uuid in a
> world readable file.

This solution, as any solution which exposes the uuid to the guest,
will break if/when we support VM forking.  Nevertheless, at this point
I'd almost prefer adding a version sub hypercall since that gives you
at least a chance at getting an up to date value.  Alternatively, you
could add some code to the xenstore dev driver to only allow read-only
access for non-root users.

Also, the guest domain xenstore interface was never really intended to
be used in anger, it was only added to support driver domains getting
information about the devices to configure.  The immediate issue would
be that access to xenstore is through a file in /proc/xen and I'd
expect that to change at some point.  Might be good to already add the
new path to all tools as a fallback option now.  Any suggestions for
where this would end up in /sys I presume?

    christian

Re: [PATCH] /sys/hypervisor/uuid

[Jeremy Katz]

On Fri, 2006-05-19 at 01:57 -0500, Anthony Liguori wrote:
> Markus Armbruster wrote:
> > New /sys/hypervisor/uuid, containing this domain's UUID.
> >
> > Stripping off /vm/ from the value of vm to get the UUID isn't exactly
> > nice.  The alternative is to add a XENVER_get_uuid code to
> > HYPERVISOR_xen_version(), but I'm not sure that's worth it.
> >
> > Signed-off-by: Markus Armbruster <armbru@redhat.com>
>
> I really don't think we should be mirroring things in sysfs that are 
> available in userspace.  What benefit is there of having two interfaces 
> to the same information?

1) Ability to be used by non-privileged user
2) Relying on libxenstore being in domU is a less than ideal state
3) The kernel already has to change if the layout in xenstore changes,
userspace shouldn't have to know or care
4) The exporting of a node in procfs for accessing xenstore is crack
that will never make it upstream :)

> If the argument is that we don't want to rely on libxenstore in a domU, 
> then we should be exposing all of XenStore in sysfs.

Arguably, we should[1].  But even then, I think it would be nice to have
the UUID exported in a fashion that's guaranteed to be consistent over
time.

> The uuid parameter is a construction of Xend.  Xend is *not* a part of 
> the supported guest interface.  By exposing the uuid in the kernel 
> interface, you're tying an unsupported interface to what really should 
> be a supported interface.

The method of where the UUID is and how it's constructed is in Xend, but
a UUID is going to always have to exist.  Even physical systems have
them.  And with good reason.

> Plus, there's already a patch floating on LKML for a /sys/hypervisor.  
> We shouldn't start adding attributes to this namespace as it could 
> potentially conflict with the existing patch.
> 
> In the very least, we ought to stick this stuff in /sys/hypervisor/xen.

That, or propose it upstream in a fashion such that each hypervisor can
have it populated in its own way.  But, yeah, /sys/hypervisor/xen is
likely to be the easier way to go there.

Jeremy

[1] Well, or another magic debugfs -- xenstoredebugfs anyone? :)

Re: [PATCH] /sys/hypervisor/uuid

[B Thomas]

[-- Attachment #1.1: Type: text/plain, Size: 3099 bytes --]

Hi,

This started with a desire to get the uuid. There are a number of approaches
to this issue, most are workable. Of these, I believe that full exposure of
xenstore via proc or sys is the least desirable.  I feel that this
approaches and perhaps crosses the "too much information" boundary.

If UUID is a desirable domU accessible item, perhaps consideration should go
towards placing it into the domU SMBIOS tables that are being proposed
currently on this list.  The DMI system structure has a 16 byte UUID field.
It feels like a natural place to put this type of information.

fwiw,
-b


On 5/19/06, Jeremy Katz <katzj@redhat.com> wrote:
>
> On Fri, 2006-05-19 at 01:57 -0500, Anthony Liguori wrote:
> > Markus Armbruster wrote:
> > > New /sys/hypervisor/uuid, containing this domain's UUID.
> > >
> > > Stripping off /vm/ from the value of vm to get the UUID isn't exactly
> > > nice.  The alternative is to add a XENVER_get_uuid code to
> > > HYPERVISOR_xen_version(), but I'm not sure that's worth it.
> > >
> > > Signed-off-by: Markus Armbruster <armbru@redhat.com>
> >
> > I really don't think we should be mirroring things in sysfs that are
> > available in userspace.  What benefit is there of having two interfaces
> > to the same information?
>
> 1) Ability to be used by non-privileged user
> 2) Relying on libxenstore being in domU is a less than ideal state
> 3) The kernel already has to change if the layout in xenstore changes,
> userspace shouldn't have to know or care
> 4) The exporting of a node in procfs for accessing xenstore is crack
> that will never make it upstream :)
>
> > If the argument is that we don't want to rely on libxenstore in a domU,
> > then we should be exposing all of XenStore in sysfs.
>
> Arguably, we should[1].  But even then, I think it would be nice to have
> the UUID exported in a fashion that's guaranteed to be consistent over
> time.
>
> > The uuid parameter is a construction of Xend.  Xend is *not* a part of
> > the supported guest interface.  By exposing the uuid in the kernel
> > interface, you're tying an unsupported interface to what really should
> > be a supported interface.
>
> The method of where the UUID is and how it's constructed is in Xend, but
> a UUID is going to always have to exist.  Even physical systems have
> them.  And with good reason.
>
> > Plus, there's already a patch floating on LKML for a /sys/hypervisor.
> > We shouldn't start adding attributes to this namespace as it could
> > potentially conflict with the existing patch.
> >
> > In the very least, we ought to stick this stuff in /sys/hypervisor/xen.
>
> That, or propose it upstream in a fashion such that each hypervisor can
> have it populated in its own way.  But, yeah, /sys/hypervisor/xen is
> likely to be the easier way to go there.
>
> Jeremy
>
> [1] Well, or another magic debugfs -- xenstoredebugfs anyone? :)
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
>

[-- Attachment #1.2: Type: text/html, Size: 3775 bytes --]

[-- Attachment #2: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH] /sys/hypervisor/uuid

[Jeremy Katz]

On Fri, 2006-05-19 at 09:43 -0400, B Thomas wrote:
> If UUID is a desirable domU accessible item, perhaps consideration
> should go towards placing it into the domU SMBIOS tables that are
> being proposed currently on this list.  The DMI system structure has a
> 16 byte UUID field.  It feels like a natural place to put this type of
> information.

While this is possible for fully virt guests, it's not doable in the
context of paravirtualized guests as there isn't any BIOS memory space
to put SMBIOS tables into

Jeremy

Re: [PATCH] /sys/hypervisor/uuid

[B Thomas]

[-- Attachment #1.1: Type: text/plain, Size: 755 bytes --]

Sigh - yeah, I'm pretty focused on fully-virtualized domains.  Too bad, it's
an interesting approach. Sorry for the diversion.

-b

On 5/19/06, Jeremy Katz <katzj@redhat.com> wrote:
>
> On Fri, 2006-05-19 at 09:43 -0400, B Thomas wrote:
> > If UUID is a desirable domU accessible item, perhaps consideration
> > should go towards placing it into the domU SMBIOS tables that are
> > being proposed currently on this list.  The DMI system structure has a
> > 16 byte UUID field.  It feels like a natural place to put this type of
> > information.
>
> While this is possible for fully virt guests, it's not doable in the
> context of paravirtualized guests as there isn't any BIOS memory space
> to put SMBIOS tables into
>
> Jeremy
>
>

[-- Attachment #1.2: Type: text/html, Size: 1113 bytes --]

[-- Attachment #2: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH] /sys/hypervisor/uuid

[John Levon]

On Fri, May 19, 2006 at 10:43:18AM +0100, Christian Limpach wrote:

> information about the devices to configure.  The immediate issue would
> be that access to xenstore is through a file in /proc/xen and I'd
> expect that to change at some point.  Might be good to already add the
> new path to all tools as a fallback option now.  Any suggestions for
> where this would end up in /sys I presume?

If somebody gets to this before I get to cleaning it up, please
consolidate all knowledge of such paths into libxc/xc_linux.c, since
these paths are OS-specific...

cheers
john

Re: [PATCH] /sys/hypervisor/uuid

[Jeremy Katz]

On Fri, 2006-05-19 at 10:43 +0100, Christian Limpach wrote:
> Also, the guest domain xenstore interface was never really intended to
> be used in anger, it was only added to support driver domains getting
> information about the devices to configure.  The immediate issue would
> be that access to xenstore is through a file in /proc/xen and I'd
> expect that to change at some point.  Might be good to already add the
> new path to all tools as a fallback option now.  Any suggestions for
> where this would end up in /sys I presume?

Actually, I'd suggest just taking it out and shooting it altogether.

What was the _intended_ use of having this exposed to the guest,
especially given that anything exposed shouldn't be considered a stable
ABI/API?

Jeremy

Re: [PATCH] /sys/hypervisor/uuid

[Christian Limpach]

On 5/19/06, Jeremy Katz <katzj@redhat.com> wrote:
> On Fri, 2006-05-19 at 10:43 +0100, Christian Limpach wrote:
> > Also, the guest domain xenstore interface was never really intended to
> > be used in anger, it was only added to support driver domains getting
> > information about the devices to configure.  The immediate issue would
> > be that access to xenstore is through a file in /proc/xen and I'd
> > expect that to change at some point.  Might be good to already add the
> > new path to all tools as a fallback option now.  Any suggestions for
> > where this would end up in /sys I presume?
>
> Actually, I'd suggest just taking it out and shooting it altogether.

Well, no.

> What was the _intended_ use of having this exposed to the guest,
> especially given that anything exposed shouldn't be considered a stable
> ABI/API?

Let me copy/paste from my previous message you replied to and quoted:
it was only added to support driver domains getting information about
the devices to configure.

     christian

Re: [PATCH] /sys/hypervisor/uuid

[Markus Armbruster]

"Christian Limpach" <christian.limpach@gmail.com> writes:

> On 5/19/06, Chris Wright <chrisw@sous-sol.org> wrote:
>> * Keir Fraser (Keir.Fraser@cl.cam.ac.uk) wrote:
>> > Christian has a fair point that, if you're just reading it out of
>> > xenstore, you could do that directly from user space. I suppose there
>> > is an argument of necessary privileges to do so however, since you need
>> > to be root to open the xenstore device file.
>>
>> Privileges part is a bit annoying.  But if the envisioned user
>> is unprivilged, some init script could always stash uuid in a
>> world readable file.
>
> This solution, as any solution which exposes the uuid to the guest,
> will break if/when we support VM forking.
[...]

Is there any scenario other than VM forking that makes the UUID
change?

Can we agree that machines having a UUID is useful?  Real machines
have one, for a reason.

VM forking has no non-virtual equivalent, hasn't it?  It's just one of
the things that are different in a virtual environment.  If your
virtual machine differs in a noticeable ways from real machines, you
can't expect all software to just work.  Software broken by the
difference needs to be adapted to the environment.  In the case of
forking VMs, you have to teach it to expect and cope with UUID
changes.  I don't see why we should break software that wants a UUID
now, by refusing to disclose the UUID we have, rather than possibly
later, when we fork VMs.

>                                            Nevertheless, at this point
> I'd almost prefer adding a version sub hypercall since that gives you
> at least a chance at getting an up to date value.

What's the technical advantage of a version sub hypercall over
xenbus_read()?

>                                                    Alternatively, you
> could add some code to the xenstore dev driver to only allow read-only
> access for non-root users.

Does the dev driver enforce root?  Isn't that policy in the kernel?

Is it safe to allow unpriveleged read-only access to *all* of xenstore
in domU?

[...]

Re: [PATCH] /sys/hypervisor/uuid

[Jeremy Katz]

On Fri, 2006-05-19 at 18:01 +0100, Christian Limpach wrote:
> On 5/19/06, Jeremy Katz <katzj@redhat.com> wrote:
> > On Fri, 2006-05-19 at 10:43 +0100, Christian Limpach wrote:
> > > Also, the guest domain xenstore interface was never really intended to
> > > be used in anger, it was only added to support driver domains getting
> > > information about the devices to configure.  The immediate issue would
> > > be that access to xenstore is through a file in /proc/xen and I'd
> > > expect that to change at some point.  Might be good to already add the
> > > new path to all tools as a fallback option now.  Any suggestions for
> > > where this would end up in /sys I presume?
> >
> > Actually, I'd suggest just taking it out and shooting it altogether.
> 
> Well, no.
> 
> > What was the _intended_ use of having this exposed to the guest,
> > especially given that anything exposed shouldn't be considered a stable
> > ABI/API?
> 
> Let me copy/paste from my previous message you replied to and quoted:
> it was only added to support driver domains getting information about
> the devices to configure.

... one of these days I'll learn to read :-)

But isn't the device configuration there happening from the kernel?
I'll admit I haven't played with getting any of the device passthrough
stuff going since those patches went in.  I'm just not clear on what
benefit having userspace being able to frob at xenstore from domU really
has.  And in any case, getting it upstream is going to be an uphill
battle at best

Jeremy

Re: [PATCH] /sys/hypervisor/uuid

[Keir Fraser]

On 19 May 2006, at 18:21, Markus Armbruster wrote:

>>                                                    Alternatively, you
>> could add some code to the xenstore dev driver to only allow read-only
>> access for non-root users.
>
> Does the dev driver enforce root?  Isn't that policy in the kernel?

It's enforced only by the device file permissions and owner/group right 
now.

> Is it safe to allow unpriveleged read-only access to *all* of xenstore
> in domU?

Not naively, I'm pretty sure. Not because I think that the 
guest-accessible portions of xenstore contain big secrets, but simply 
because I don't particularly trust the xenstore dev driver (for 
example, a process that starts a transaction and never finishes it will 
prevent save/restore from working). If we allowed a non-root process to 
execute only XS_READ, I think that would be okay.

I'm personally not against the sysfs solution though, if we agree that 
seeing your own uuid is useful at all. At least it is small and 
self-contained and, in the face of VM fork, I can imagine supporting 
poll/select/sigio on that sysfs file or some other to notify processes 
when platform/guest details have changed due to virtualisation-specific 
events. It's maybe possible to support that kind of thing in other 
ways, but it sounds like a pita to me.

  -- Keir

Re: [PATCH] /sys/hypervisor/uuid

[Markus Armbruster]

Keir Fraser <Keir.Fraser@cl.cam.ac.uk> writes:

[...]
> I'm personally not against the sysfs solution though, if we agree that
> seeing your own uuid is useful at all.
[...]

How do we reach agreement (or conclude we don't)?  Do we still need
more or clearer arguments for seeing one's UUID?

Re: [PATCH] /sys/hypervisor/uuid

[Keir Fraser]

On 22 May 2006, at 07:21, Markus Armbruster wrote:

>> I'm personally not against the sysfs solution though, if we agree that
>> seeing your own uuid is useful at all.
> [...]
>
> How do we reach agreement (or conclude we don't)?  Do we still need
> more or clearer arguments for seeing one's UUID?

That's one problem, since nothing in the Xen tree will be making use of 
this new functionality.

The main argument seems to be about how this should be exposed to user 
space. It seems reasonable to me to add this to sysfs given that we 
already have a driver that exports even weirder stuff like compilation 
info for Xen itself! OTOH there is absolutely no guarantee that the 
xen_sysfs driver will get picked up by mainline -- there are plenty of 
people who think that kind of info has no place in the guest kernel, 
and they have a point. So xen_sysfs may be a dead end.

If we allowed unprivileged read access to xenstore, that would require 
only a smaller and more general patch to the kernel. Then you could put 
the uuid extraction logic in user space?

  -- Keir

Re: [PATCH] /sys/hypervisor/uuid

[Christian Limpach]

On 5/19/06, Jeremy Katz <katzj@redhat.com> wrote:
> > Let me copy/paste from my previous message you replied to and quoted:
> > it was only added to support driver domains getting information about
> > the devices to configure.
>
> But isn't the device configuration there happening from the kernel?
> I'll admit I haven't played with getting any of the device passthrough
> stuff going since those patches went in.  I'm just not clear on what
> benefit having userspace being able to frob at xenstore from domU really
> has.  And in any case, getting it upstream is going to be an uphill
> battle at best

For some of the device setup, it is more flexible if it happens in
userspace.  For example, figuring out what device a given name
corresponds too (needs lookup in /dev) or binding a loop device for a
file backed vbd.

Instead of inventing yet another interface for the kernel to
communicate with our hotplug scripts (and back), we decided to use
xenstore.

    christian

Re: [PATCH] /sys/hypervisor/uuid

[Jeremy Katz]

On Mon, 2006-05-22 at 07:33 +0100, Keir Fraser wrote:
> On 22 May 2006, at 07:21, Markus Armbruster wrote:
> >> I'm personally not against the sysfs solution though, if we agree that
> >> seeing your own uuid is useful at all.
> > [...]
> >
> > How do we reach agreement (or conclude we don't)?  Do we still need
> > more or clearer arguments for seeing one's UUID?
> 
> That's one problem, since nothing in the Xen tree will be making use of 
> this new functionality.
> 
> The main argument seems to be about how this should be exposed to user 
> space. It seems reasonable to me to add this to sysfs given that we 
> already have a driver that exports even weirder stuff like compilation 
> info for Xen itself! OTOH there is absolutely no guarantee that the 
> xen_sysfs driver will get picked up by mainline -- there are plenty of 
> people who think that kind of info has no place in the guest kernel, 
> and they have a point. So xen_sysfs may be a dead end.

And accessing xenstore from the guest kernel may also be a dead end.
There are no guarantees until things get upstream which is why that is
so vitally important.  Then again, stability of sysfs/procfs isn't all
that existent in the kernel anyway, so those who use functionality there
are used to having to change it over time ;-)

> If we allowed unprivileged read access to xenstore, that would require 
> only a smaller and more general patch to the kernel. Then you could put 
> the uuid extraction logic in user space?

But if you still require xs transactions, then it remains complicated
and a potential way to introduce fun problems for dom0 from a domU.
Especially as I would expect that then people will start coding their
own stuff instead of bringing a dep on libxs into domUs

Jeremy

Re: [PATCH] /sys/hypervisor/uuid

[Keir Fraser]

On 22 May 2006, at 13:44, Jeremy Katz wrote:

>> If we allowed unprivileged read access to xenstore, that would require
>> only a smaller and more general patch to the kernel. Then you could 
>> put
>> the uuid extraction logic in user space?
>
> But if you still require xs transactions, then it remains complicated
> and a potential way to introduce fun problems for dom0 from a domU.
> Especially as I would expect that then people will start coding their
> own stuff instead of bringing a dep on libxs into domUs

Well, the bottom line is: I will apply the uuid patch for sysfs if you 
want it. But don't be surprised if the whole lot goes in the bin at 
some point when we have the inevitable fight with the kernel 
maintainers. In contrast, libxs presents a stable supported interface.

  -- Keir

Re: [PATCH] /sys/hypervisor/uuid

[Jeremy Katz]

On Mon, 2006-05-22 at 14:24 +0100, Keir Fraser wrote:
> On 22 May 2006, at 13:44, Jeremy Katz wrote:
> >> If we allowed unprivileged read access to xenstore, that would require
> >> only a smaller and more general patch to the kernel. Then you could 
> >> put
> >> the uuid extraction logic in user space?
> >
> > But if you still require xs transactions, then it remains complicated
> > and a potential way to introduce fun problems for dom0 from a domU.
> > Especially as I would expect that then people will start coding their
> > own stuff instead of bringing a dep on libxs into domUs
> 
> Well, the bottom line is: I will apply the uuid patch for sysfs if you 
> want it. But don't be surprised if the whole lot goes in the bin at 
> some point when we have the inevitable fight with the kernel 
> maintainers. In contrast, libxs presents a stable supported interface.

I really think the "do transactions via xenstore in domU" is going to
have as much or more of a fight involved.

Having the UUID easily accessible via sysfs definitely seems like it
would be advantageous to more than just me based on the comments here.  

Jeremy

Re: [PATCH] /sys/hypervisor/uuid

[Christian Limpach]

On 5/22/06, Jeremy Katz <katzj@redhat.com> wrote:
> On Mon, 2006-05-22 at 14:24 +0100, Keir Fraser wrote:
> > Well, the bottom line is: I will apply the uuid patch for sysfs if you
> > want it. But don't be surprised if the whole lot goes in the bin at
> > some point when we have the inevitable fight with the kernel
> > maintainers. In contrast, libxs presents a stable supported interface.
>
> I really think the "do transactions via xenstore in domU" is going to
> have as much or more of a fight involved.

Could you maybe find out?  It's a bit odd to decide to add an
additional interface just based on speculation?  Especially when the
added interface is a lot less generic than the already existing one.

    christian

Re: [PATCH] /sys/hypervisor/uuid

[Jeremy Katz]

On Mon, 2006-05-22 at 16:44 +0100, Christian Limpach wrote:
> On 5/22/06, Jeremy Katz <katzj@redhat.com> wrote:
> > On Mon, 2006-05-22 at 14:24 +0100, Keir Fraser wrote:
> > > Well, the bottom line is: I will apply the uuid patch for sysfs if you
> > > want it. But don't be surprised if the whole lot goes in the bin at
> > > some point when we have the inevitable fight with the kernel
> > > maintainers. In contrast, libxs presents a stable supported interface.
> >
> > I really think the "do transactions via xenstore in domU" is going to
> > have as much or more of a fight involved.
> 
> Could you maybe find out?  It's a bit odd to decide to add an
> additional interface just based on speculation?  Especially when the
> added interface is a lot less generic than the already existing one.

The only way to find out is actually submitting patches and getting them
reviewed and accepted.

Anything else is tea leaves and psychic voodoo :-)

Jeremy

Re: [PATCH] /sys/hypervisor/uuid

[Anthony Liguori]

Jeremy Katz wrote:
> I really think the "do transactions via xenstore in domU" is going to
> have as much or more of a fight involved.
>
> Having the UUID easily accessible via sysfs definitely seems like it
> would be advantageous to more than just me based on the comments here.  
>   

What will you be using the UUID for?

I know we wished to use it to replace the UUID normally available in 
SMBIOS.  If that is the case, then perhaps we should reconsider the 
location of the UUID attribute since it could be available even when a 
hypervisor isn't around.

Regards,

Anthony Liguori

> Jeremy
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
>   

Re: [PATCH] /sys/hypervisor/uuid

[Jeremy Katz]

On Mon, 2006-05-22 at 10:57 -0500, Anthony Liguori wrote:
> Jeremy Katz wrote:
> > I really think the "do transactions via xenstore in domU" is going to
> > have as much or more of a fight involved.
> >
> > Having the UUID easily accessible via sysfs definitely seems like it
> > would be advantageous to more than just me based on the comments here.  
>
> What will you be using the UUID for?
> 
> I know we wished to use it to replace the UUID normally available in 
> SMBIOS.  If that is the case, then perhaps we should reconsider the 
> location of the UUID attribute since it could be available even when a 
> hypervisor isn't around.

Generally speaking, the UUID is used to be able to uniquely identify a
host for management (and other types) of apps.  So yes, this is
basically the extension of the SMBIOS side to paravirt guests.  

And I had entertained the thought of throwing together the patch to make
the UUID more generically available, I just haven't really had time to
try to track gregkh down and talk him into it

Jeremy

Re: [PATCH] /sys/hypervisor/uuid

[Stephen C. Tweedie]

Hi,

On Fri, 2006-05-19 at 10:43 +0100, Christian Limpach wrote:

> This solution, as any solution which exposes the uuid to the guest,
> will break if/when we support VM forking.

Right.  But this isn't unique to Xen --- uuids in ext2/3 filesystem
superblocks have the same problem when you have LVM snapshots, for
example.  That doesn't mean they aren't useful --- it does mean you may
need to take care when creating forks/clones/snapshots.

There are just so many unique IDs out there.  The breakage opportunities
on VM fork are boundless.  You now have two VMs mounting the same on-
disk filesystems, which have the same UUIDs; your management agents' own
internal UUIDs are duplicated; you have duplicated IP connections out
over duplicate MAC addresses, etc.

So I think VM forking is not a major obstacle here, not because UUID
doesn't affect fork, but because fork is *so* hard, and will require so
much cooperation from the domU if you want to get these things right,
that domU-visible UUID is the least of your worries. :-)

--Stephen

Re: [PATCH] /sys/hypervisor/uuid

[Markus Armbruster]

New /sys/hypervisor/uuid, containing this domain's UUID.

Signed-off-by: Markus Armbruster <armbru@redhat.com>

---
Second try, using the newly available /vm/<UUID>/uuid key.

diff -r ddba92a5cba9 drivers/xen/core/xen_sysfs.c
--- a/drivers/xen/core/xen_sysfs.c	Tue May 09 12:41:38 2006 +0200
+++ b/drivers/xen/core/xen_sysfs.c	Wed May 24 14:53:25 2006 +0200
@@ -8,12 +8,14 @@
  */
 
 #include <linux/config.h>
+#include <linux/err.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/init.h>
 #include <asm/hypervisor.h>
 #include <xen/features.h>
 #include <xen/hypervisor_sysfs.h>
+#include <xen/xenbus.h>
 
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Mike D. Day <ncmike@us.ibm.com>");
@@ -92,6 +94,37 @@ static void xen_sysfs_version_destroy(vo
 static void xen_sysfs_version_destroy(void)
 {
 	sysfs_remove_group(&hypervisor_subsys.kset.kobj, &version_group);
+}
+
+/* UUID */
+
+static ssize_t uuid_show(struct hyp_sysfs_attr *attr, char *buffer)
+{
+	char *vm, *val;
+	int ret;
+
+	vm = xenbus_read(XBT_NULL, "vm", "", NULL);
+	if (IS_ERR(vm))
+		return PTR_ERR(vm);
+	val = xenbus_read(XBT_NULL, vm, "uuid", NULL);
+	kfree(vm);
+	if (IS_ERR(val))
+		return PTR_ERR(val);
+	ret = sprintf(buffer, "%s\n", val);
+	kfree(val);
+	return ret;
+}
+
+HYPERVISOR_ATTR_RO(uuid);
+
+static int __init xen_sysfs_uuid_init(void)
+{
+	return sysfs_create_file(&hypervisor_subsys.kset.kobj, &uuid_attr.attr);
+}
+
+static void xen_sysfs_uuid_destroy(void)
+{
+	sysfs_remove_file(&hypervisor_subsys.kset.kobj, &uuid_attr.attr);
 }
 
 /* xen compilation attributes */
@@ -285,10 +318,15 @@ static int __init hyper_sysfs_init(void)
 	ret = xen_compilation_init();
 	if (ret)
 		goto comp_out;
+	ret = xen_sysfs_uuid_init();
+	if (ret)
+		goto uuid_out;
 	ret = xen_properties_init();
 	if (!ret)
 		goto out;
 
+	xen_sysfs_uuid_destroy();
+uuid_out:
 	xen_compilation_destroy();
 comp_out:
 	xen_sysfs_version_destroy();
@@ -302,6 +340,7 @@ static void hyper_sysfs_exit(void)
 {
 	xen_properties_destroy();
 	xen_compilation_destroy();
+	xen_sysfs_uuid_destroy();
 	xen_sysfs_version_destroy();
 	xen_sysfs_type_destroy();

Re: [PATCH] /sys/hypervisor/uuid

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> New /sys/hypervisor/uuid, containing this domain's UUID.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

If there's anything in my patch that makes it unacceptable, please let
me know, so I can fix it.