2018-10-09 09:57:02

by Filippo Sironi

[permalink] [raw]
Subject: [PATCH] KVM: Start populating /sys/hypervisor with KVM entries

Start populating /sys/hypervisor with KVM entries when we're running on
KVM. This is to replicate functionality that's available when we're
running on Xen.

Let's start with /sys/hypervisor/uuid, which users prefer over
/sys/devices/virtual/dmi/id/product_uuid as a way to recognize a virtual
machine, since it's also available when running on Xen HVM and on Xen PV
and, on top of that doesn't require root privileges by default.

Signed-off-by: Filippo Sironi <[email protected]>
---
drivers/Kconfig | 2 ++
drivers/Makefile | 2 ++
drivers/kvm/Kconfig | 14 ++++++++++++++
drivers/kvm/Makefile | 1 +
drivers/kvm/sys-hypervisor.c | 26 ++++++++++++++++++++++++++
5 files changed, 45 insertions(+)
create mode 100644 drivers/kvm/Kconfig
create mode 100644 drivers/kvm/Makefile
create mode 100644 drivers/kvm/sys-hypervisor.c

diff --git a/drivers/Kconfig b/drivers/Kconfig
index afc942c54814..597519c5f7c8 100644
--- a/drivers/Kconfig
+++ b/drivers/Kconfig
@@ -135,6 +135,8 @@ source "drivers/hv/Kconfig"

source "drivers/xen/Kconfig"

+source "drivers/kvm/Kconfig"
+
source "drivers/staging/Kconfig"

source "drivers/platform/Kconfig"
diff --git a/drivers/Makefile b/drivers/Makefile
index 1056f9699192..727205e287fc 100644
--- a/drivers/Makefile
+++ b/drivers/Makefile
@@ -47,6 +47,8 @@ obj-y += soc/
obj-$(CONFIG_VIRTIO) += virtio/
obj-$(CONFIG_XEN) += xen/

+obj-$(CONFIG_KVM_GUEST) += kvm/
+
# regulators early, since some subsystems rely on them to initialize
obj-$(CONFIG_REGULATOR) += regulator/

diff --git a/drivers/kvm/Kconfig b/drivers/kvm/Kconfig
new file mode 100644
index 000000000000..3fc041df7c11
--- /dev/null
+++ b/drivers/kvm/Kconfig
@@ -0,0 +1,14 @@
+menu "KVM driver support"
+ depends on KVM_GUEST
+
+config KVM_SYS_HYPERVISOR
+ bool "Create KVM entries under /sys/hypervisor"
+ depends on SYSFS
+ select SYS_HYPERVISOR
+ default y
+ help
+ Create KVM entries under /sys/hypervisor (e.g., uuid). When running
+ native or on another hypervisor, /sys/hypervisor may still be
+ present, but it will have no KVM entries.
+
+endmenu
diff --git a/drivers/kvm/Makefile b/drivers/kvm/Makefile
new file mode 100644
index 000000000000..73a43fc994b9
--- /dev/null
+++ b/drivers/kvm/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_KVM_SYS_HYPERVISOR) += sys-hypervisor.o
diff --git a/drivers/kvm/sys-hypervisor.c b/drivers/kvm/sys-hypervisor.c
new file mode 100644
index 000000000000..ef04ca65cf1a
--- /dev/null
+++ b/drivers/kvm/sys-hypervisor.c
@@ -0,0 +1,26 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#include <asm/kvm_para.h>
+
+#include <linux/dmi.h>
+#include <linux/kobject.h>
+#include <linux/sysfs.h>
+
+static ssize_t uuid_show(struct kobject *obj,
+ struct kobj_attribute *attr,
+ char *buf)
+{
+ const char *uuid = dmi_get_system_info(DMI_PRODUCT_UUID);
+ return sprintf(buf, "%s\n", uuid);
+}
+
+static struct kobj_attribute uuid = __ATTR_RO(uuid);
+
+static int __init uuid_init(void)
+{
+ if (!kvm_para_available())
+ return 0;
+ return sysfs_create_file(hypervisor_kobj, &uuid.attr);
+}
+
+device_initcall(uuid_init);
--
2.7.4



2018-10-09 10:42:58

by Christian Borntraeger

[permalink] [raw]
Subject: Re: [PATCH] KVM: Start populating /sys/hypervisor with KVM entries



On 10/09/2018 11:54 AM, Filippo Sironi wrote:
> Start populating /sys/hypervisor with KVM entries when we're running on
> KVM. This is to replicate functionality that's available when we're
> running on Xen.
>
> Let's start with /sys/hypervisor/uuid, which users prefer over
> /sys/devices/virtual/dmi/id/product_uuid as a way to recognize a virtual
> machine, since it's also available when running on Xen HVM and on Xen PV
> and, on top of that doesn't require root privileges by default.

Can you make this an arch hook? On s390 it is possible to get the uuid with
the stsi instruction.
See
https://git.kernel.org/pub/scm/virt/kvm/kvm.git/tree/arch/s390/kernel/sysinfo.c#n248


We do use uuid_t, but we can certainly return a char*.




>
> Signed-off-by: Filippo Sironi <[email protected]>
> ---
> drivers/Kconfig | 2 ++
> drivers/Makefile | 2 ++
> drivers/kvm/Kconfig | 14 ++++++++++++++
> drivers/kvm/Makefile | 1 +
> drivers/kvm/sys-hypervisor.c | 26 ++++++++++++++++++++++++++
> 5 files changed, 45 insertions(+)
> create mode 100644 drivers/kvm/Kconfig
> create mode 100644 drivers/kvm/Makefile
> create mode 100644 drivers/kvm/sys-hypervisor.c
>
> diff --git a/drivers/Kconfig b/drivers/Kconfig
> index afc942c54814..597519c5f7c8 100644
> --- a/drivers/Kconfig
> +++ b/drivers/Kconfig
> @@ -135,6 +135,8 @@ source "drivers/hv/Kconfig"
>
> source "drivers/xen/Kconfig"
>
> +source "drivers/kvm/Kconfig"
> +
> source "drivers/staging/Kconfig"
>
> source "drivers/platform/Kconfig"
> diff --git a/drivers/Makefile b/drivers/Makefile
> index 1056f9699192..727205e287fc 100644
> --- a/drivers/Makefile
> +++ b/drivers/Makefile
> @@ -47,6 +47,8 @@ obj-y += soc/
> obj-$(CONFIG_VIRTIO) += virtio/
> obj-$(CONFIG_XEN) += xen/
>
> +obj-$(CONFIG_KVM_GUEST) += kvm/
> +
> # regulators early, since some subsystems rely on them to initialize
> obj-$(CONFIG_REGULATOR) += regulator/
>
> diff --git a/drivers/kvm/Kconfig b/drivers/kvm/Kconfig
> new file mode 100644
> index 000000000000..3fc041df7c11
> --- /dev/null
> +++ b/drivers/kvm/Kconfig
> @@ -0,0 +1,14 @@
> +menu "KVM driver support"
> + depends on KVM_GUEST
> +
> +config KVM_SYS_HYPERVISOR
> + bool "Create KVM entries under /sys/hypervisor"
> + depends on SYSFS
> + select SYS_HYPERVISOR
> + default y
> + help
> + Create KVM entries under /sys/hypervisor (e.g., uuid). When running
> + native or on another hypervisor, /sys/hypervisor may still be
> + present, but it will have no KVM entries.
> +
> +endmenu
> diff --git a/drivers/kvm/Makefile b/drivers/kvm/Makefile
> new file mode 100644
> index 000000000000..73a43fc994b9
> --- /dev/null
> +++ b/drivers/kvm/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_KVM_SYS_HYPERVISOR) += sys-hypervisor.o
> diff --git a/drivers/kvm/sys-hypervisor.c b/drivers/kvm/sys-hypervisor.c
> new file mode 100644
> index 000000000000..ef04ca65cf1a
> --- /dev/null
> +++ b/drivers/kvm/sys-hypervisor.c
> @@ -0,0 +1,26 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#include <asm/kvm_para.h>
> +
> +#include <linux/dmi.h>
> +#include <linux/kobject.h>
> +#include <linux/sysfs.h>
> +
> +static ssize_t uuid_show(struct kobject *obj,
> + struct kobj_attribute *attr,
> + char *buf)
> +{
> + const char *uuid = dmi_get_system_info(DMI_PRODUCT_UUID);
> + return sprintf(buf, "%s\n", uuid);
> +}
> +
> +static struct kobj_attribute uuid = __ATTR_RO(uuid);
> +
> +static int __init uuid_init(void)
> +{
> + if (!kvm_para_available())
> + return 0;
> + return sysfs_create_file(hypervisor_kobj, &uuid.attr);
> +}
> +
> +device_initcall(uuid_init);
>


2018-10-09 15:01:59

by Konrad Rzeszutek Wilk

[permalink] [raw]
Subject: Re: [PATCH] KVM: Start populating /sys/hypervisor with KVM entries

On Tue, Oct 09, 2018 at 11:54:39AM +0200, Filippo Sironi wrote:
> Start populating /sys/hypervisor with KVM entries when we're running on
> KVM. This is to replicate functionality that's available when we're
> running on Xen.
>
> Let's start with /sys/hypervisor/uuid, which users prefer over
> /sys/devices/virtual/dmi/id/product_uuid as a way to recognize a virtual
> machine, since it's also available when running on Xen HVM and on Xen PV
> and, on top of that doesn't require root privileges by default.

Yeey! Thank you for doing this. It has been on my TODO but just never
got to it.

I think you also need to update the Documentation/../sysfs file.
>
> Signed-off-by: Filippo Sironi <[email protected]>
> ---
> drivers/Kconfig | 2 ++
> drivers/Makefile | 2 ++
> drivers/kvm/Kconfig | 14 ++++++++++++++
> drivers/kvm/Makefile | 1 +
> drivers/kvm/sys-hypervisor.c | 26 ++++++++++++++++++++++++++
> 5 files changed, 45 insertions(+)
> create mode 100644 drivers/kvm/Kconfig
> create mode 100644 drivers/kvm/Makefile
> create mode 100644 drivers/kvm/sys-hypervisor.c
>
> diff --git a/drivers/Kconfig b/drivers/Kconfig
> index afc942c54814..597519c5f7c8 100644
> --- a/drivers/Kconfig
> +++ b/drivers/Kconfig
> @@ -135,6 +135,8 @@ source "drivers/hv/Kconfig"
>
> source "drivers/xen/Kconfig"
>
> +source "drivers/kvm/Kconfig"
> +
> source "drivers/staging/Kconfig"
>
> source "drivers/platform/Kconfig"
> diff --git a/drivers/Makefile b/drivers/Makefile
> index 1056f9699192..727205e287fc 100644
> --- a/drivers/Makefile
> +++ b/drivers/Makefile
> @@ -47,6 +47,8 @@ obj-y += soc/
> obj-$(CONFIG_VIRTIO) += virtio/
> obj-$(CONFIG_XEN) += xen/
>
> +obj-$(CONFIG_KVM_GUEST) += kvm/
> +
> # regulators early, since some subsystems rely on them to initialize
> obj-$(CONFIG_REGULATOR) += regulator/
>
> diff --git a/drivers/kvm/Kconfig b/drivers/kvm/Kconfig
> new file mode 100644
> index 000000000000..3fc041df7c11
> --- /dev/null
> +++ b/drivers/kvm/Kconfig
> @@ -0,0 +1,14 @@
> +menu "KVM driver support"
> + depends on KVM_GUEST
> +
> +config KVM_SYS_HYPERVISOR
> + bool "Create KVM entries under /sys/hypervisor"
> + depends on SYSFS
> + select SYS_HYPERVISOR
> + default y
> + help
> + Create KVM entries under /sys/hypervisor (e.g., uuid). When running
> + native or on another hypervisor, /sys/hypervisor may still be
> + present, but it will have no KVM entries.
> +
> +endmenu
> diff --git a/drivers/kvm/Makefile b/drivers/kvm/Makefile
> new file mode 100644
> index 000000000000..73a43fc994b9
> --- /dev/null
> +++ b/drivers/kvm/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_KVM_SYS_HYPERVISOR) += sys-hypervisor.o
> diff --git a/drivers/kvm/sys-hypervisor.c b/drivers/kvm/sys-hypervisor.c
> new file mode 100644
> index 000000000000..ef04ca65cf1a
> --- /dev/null
> +++ b/drivers/kvm/sys-hypervisor.c
> @@ -0,0 +1,26 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#include <asm/kvm_para.h>
> +
> +#include <linux/dmi.h>
> +#include <linux/kobject.h>
> +#include <linux/sysfs.h>
> +
> +static ssize_t uuid_show(struct kobject *obj,
> + struct kobj_attribute *attr,
> + char *buf)
> +{
> + const char *uuid = dmi_get_system_info(DMI_PRODUCT_UUID);
> + return sprintf(buf, "%s\n", uuid);
> +}
> +
> +static struct kobj_attribute uuid = __ATTR_RO(uuid);
> +
> +static int __init uuid_init(void)
> +{
> + if (!kvm_para_available())
> + return 0;
> + return sysfs_create_file(hypervisor_kobj, &uuid.attr);
> +}
> +
> +device_initcall(uuid_init);
> --
> 2.7.4
>

2018-10-09 16:23:02

by Boris Ostrovsky

[permalink] [raw]
Subject: Re: [PATCH] KVM: Start populating /sys/hypervisor with KVM entries

On 10/9/18 6:41 AM, Christian Borntraeger wrote:
>
>
> On 10/09/2018 11:54 AM, Filippo Sironi wrote:
>> Start populating /sys/hypervisor with KVM entries when we're running on
>> KVM. This is to replicate functionality that's available when we're
>> running on Xen.
>>
>> Let's start with /sys/hypervisor/uuid, which users prefer over
>> /sys/devices/virtual/dmi/id/product_uuid as a way to recognize a virtual
>> machine, since it's also available when running on Xen HVM and on Xen PV
>> and, on top of that doesn't require root privileges by default.
>
> Can you make this an arch hook? On s390 it is possible to get the uuid with
> the stsi instruction.
> See
> https://git.kernel.org/pub/scm/virt/kvm/kvm.git/tree/arch/s390/kernel/sysinfo.c#n248
>
>
> We do use uuid_t, but we can certainly return a char*.


I would suggest having a top-level sys-hypervisor.c that will create
common files that all hypervisors should have (uuid, type, version, etc)
and then have hypervisor- and/or arch-specific hooks.


-boris



2018-10-09 17:51:44

by Cornelia Huck

[permalink] [raw]
Subject: Re: [PATCH] KVM: Start populating /sys/hypervisor with KVM entries

On Tue, 9 Oct 2018 12:21:46 -0400
Boris Ostrovsky <[email protected]> wrote:

> On 10/9/18 6:41 AM, Christian Borntraeger wrote:
> >
> >
> > On 10/09/2018 11:54 AM, Filippo Sironi wrote:
> >> Start populating /sys/hypervisor with KVM entries when we're running on
> >> KVM. This is to replicate functionality that's available when we're
> >> running on Xen.
> >>
> >> Let's start with /sys/hypervisor/uuid, which users prefer over
> >> /sys/devices/virtual/dmi/id/product_uuid as a way to recognize a virtual
> >> machine, since it's also available when running on Xen HVM and on Xen PV
> >> and, on top of that doesn't require root privileges by default.
> >
> > Can you make this an arch hook? On s390 it is possible to get the uuid with
> > the stsi instruction.
> > See
> > https://git.kernel.org/pub/scm/virt/kvm/kvm.git/tree/arch/s390/kernel/sysinfo.c#n248
> >
> >
> > We do use uuid_t, but we can certainly return a char*.
>
>
> I would suggest having a top-level sys-hypervisor.c that will create
> common files that all hypervisors should have (uuid, type, version, etc)
> and then have hypervisor- and/or arch-specific hooks.

I think we really need *both* hypervisor- and arch-specific hooks.

2018-10-10 05:21:58

by kernel test robot

[permalink] [raw]
Subject: Re: [PATCH] KVM: Start populating /sys/hypervisor with KVM entries

Hi Filippo,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linux-sof-driver/master]
[also build test ERROR on v4.19-rc7 next-20181009]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url: https://github.com/0day-ci/linux/commits/Filippo-Sironi/KVM-Start-populating-sys-hypervisor-with-KVM-entries/20181010-064236
base: https://github.com/thesofproject/linux master
config: powerpc64-allmodconfig (attached as .config)
compiler: powerpc64-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.2.0 make.cross ARCH=powerpc64

All error/warnings (new ones prefixed by >>):

In file included from arch/powerpc/include/uapi/asm/kvm_para.h:82:0,
from arch/powerpc/include/asm/kvm_para.h:22,
from drivers//kvm/sys-hypervisor.c:3:
>> arch/powerpc/include/asm/epapr_hcalls.h:109:12: error: expected '=', ',', ';', 'asm' or '__attribute__' before 'epapr_paravirt_early_init'
int __init epapr_paravirt_early_init(void);
^~~~~~~~~~~~~~~~~~~~~~~~~
In file included from arch/powerpc/include/asm/epapr_hcalls.h:53:0,
from arch/powerpc/include/uapi/asm/kvm_para.h:82,
from arch/powerpc/include/asm/kvm_para.h:22,
from drivers//kvm/sys-hypervisor.c:3:
arch/powerpc/include/asm/kvm_para.h: In function 'kvm_arch_para_features':
>> arch/powerpc/include/asm/kvm_para.h:58:40: error: 'KVM_HC_FEATURES' undeclared (first use in this function); did you mean 'KVM_HCALL_TOKEN'?
if(epapr_hypercall0_1(KVM_HCALL_TOKEN(KVM_HC_FEATURES), &r))
^
arch/powerpc/include/uapi/asm/epapr_hcalls.h:75:51: note: in definition of macro '_EV_HCALL_TOKEN'
#define _EV_HCALL_TOKEN(id, num) (((id) << 16) | (num))
^~~
>> arch/powerpc/include/asm/kvm_para.h:58:24: note: in expansion of macro 'KVM_HCALL_TOKEN'
if(epapr_hypercall0_1(KVM_HCALL_TOKEN(KVM_HC_FEATURES), &r))
^~~~~~~~~~~~~~~
arch/powerpc/include/asm/kvm_para.h:58:40: note: each undeclared identifier is reported only once for each function it appears in
if(epapr_hypercall0_1(KVM_HCALL_TOKEN(KVM_HC_FEATURES), &r))
^
arch/powerpc/include/uapi/asm/epapr_hcalls.h:75:51: note: in definition of macro '_EV_HCALL_TOKEN'
#define _EV_HCALL_TOKEN(id, num) (((id) << 16) | (num))
^~~
>> arch/powerpc/include/asm/kvm_para.h:58:24: note: in expansion of macro 'KVM_HCALL_TOKEN'
if(epapr_hypercall0_1(KVM_HCALL_TOKEN(KVM_HC_FEATURES), &r))
^~~~~~~~~~~~~~~

vim +58 arch/powerpc/include/asm/kvm_para.h

bbf45ba5 include/asm-powerpc/kvm_para.h Hollis Blanchard 2008-04-16 21
c3617f72 arch/powerpc/include/asm/kvm_para.h David Howells 2012-10-09 @22 #include <uapi/asm/kvm_para.h>
bbf45ba5 include/asm-powerpc/kvm_para.h Hollis Blanchard 2008-04-16 23
2a342ed5 arch/powerpc/include/asm/kvm_para.h Alexander Graf 2010-07-29 24 #ifdef CONFIG_KVM_GUEST
2a342ed5 arch/powerpc/include/asm/kvm_para.h Alexander Graf 2010-07-29 25
26e673c3 arch/powerpc/include/asm/kvm_para.h Alexander Graf 2010-09-03 26 #include <linux/of.h>
26e673c3 arch/powerpc/include/asm/kvm_para.h Alexander Graf 2010-09-03 27
2a342ed5 arch/powerpc/include/asm/kvm_para.h Alexander Graf 2010-07-29 28 static inline int kvm_para_available(void)
2a342ed5 arch/powerpc/include/asm/kvm_para.h Alexander Graf 2010-07-29 29 {
2a342ed5 arch/powerpc/include/asm/kvm_para.h Alexander Graf 2010-07-29 30 struct device_node *hyper_node;
2a342ed5 arch/powerpc/include/asm/kvm_para.h Alexander Graf 2010-07-29 31
2a342ed5 arch/powerpc/include/asm/kvm_para.h Alexander Graf 2010-07-29 32 hyper_node = of_find_node_by_path("/hypervisor");
2a342ed5 arch/powerpc/include/asm/kvm_para.h Alexander Graf 2010-07-29 33 if (!hyper_node)
2a342ed5 arch/powerpc/include/asm/kvm_para.h Alexander Graf 2010-07-29 34 return 0;
2a342ed5 arch/powerpc/include/asm/kvm_para.h Alexander Graf 2010-07-29 35
2a342ed5 arch/powerpc/include/asm/kvm_para.h Alexander Graf 2010-07-29 36 if (!of_device_is_compatible(hyper_node, "linux,kvm"))
2a342ed5 arch/powerpc/include/asm/kvm_para.h Alexander Graf 2010-07-29 37 return 0;
2a342ed5 arch/powerpc/include/asm/kvm_para.h Alexander Graf 2010-07-29 38
2a342ed5 arch/powerpc/include/asm/kvm_para.h Alexander Graf 2010-07-29 39 return 1;
2a342ed5 arch/powerpc/include/asm/kvm_para.h Alexander Graf 2010-07-29 40 }
2a342ed5 arch/powerpc/include/asm/kvm_para.h Alexander Graf 2010-07-29 41
2a342ed5 arch/powerpc/include/asm/kvm_para.h Alexander Graf 2010-07-29 42 #else
2a342ed5 arch/powerpc/include/asm/kvm_para.h Alexander Graf 2010-07-29 43
bbf45ba5 include/asm-powerpc/kvm_para.h Hollis Blanchard 2008-04-16 44 static inline int kvm_para_available(void)
bbf45ba5 include/asm-powerpc/kvm_para.h Hollis Blanchard 2008-04-16 45 {
bbf45ba5 include/asm-powerpc/kvm_para.h Hollis Blanchard 2008-04-16 46 return 0;
bbf45ba5 include/asm-powerpc/kvm_para.h Hollis Blanchard 2008-04-16 47 }
bbf45ba5 include/asm-powerpc/kvm_para.h Hollis Blanchard 2008-04-16 48
2a342ed5 arch/powerpc/include/asm/kvm_para.h Alexander Graf 2010-07-29 49 #endif
2a342ed5 arch/powerpc/include/asm/kvm_para.h Alexander Graf 2010-07-29 50
bbf45ba5 include/asm-powerpc/kvm_para.h Hollis Blanchard 2008-04-16 51 static inline unsigned int kvm_arch_para_features(void)
bbf45ba5 include/asm-powerpc/kvm_para.h Hollis Blanchard 2008-04-16 52 {
2a342ed5 arch/powerpc/include/asm/kvm_para.h Alexander Graf 2010-07-29 53 unsigned long r;
2a342ed5 arch/powerpc/include/asm/kvm_para.h Alexander Graf 2010-07-29 54
2a342ed5 arch/powerpc/include/asm/kvm_para.h Alexander Graf 2010-07-29 55 if (!kvm_para_available())
bbf45ba5 include/asm-powerpc/kvm_para.h Hollis Blanchard 2008-04-16 56 return 0;
2a342ed5 arch/powerpc/include/asm/kvm_para.h Alexander Graf 2010-07-29 57
b1f0d94c arch/powerpc/include/asm/kvm_para.h Bharat Bhushan 2013-10-08 @58 if(epapr_hypercall0_1(KVM_HCALL_TOKEN(KVM_HC_FEATURES), &r))
2a342ed5 arch/powerpc/include/asm/kvm_para.h Alexander Graf 2010-07-29 59 return 0;
2a342ed5 arch/powerpc/include/asm/kvm_para.h Alexander Graf 2010-07-29 60
2a342ed5 arch/powerpc/include/asm/kvm_para.h Alexander Graf 2010-07-29 61 return r;
bbf45ba5 include/asm-powerpc/kvm_para.h Hollis Blanchard 2008-04-16 62 }
bbf45ba5 include/asm-powerpc/kvm_para.h Hollis Blanchard 2008-04-16 63

:::::: The code at line 58 was first introduced by commit
:::::: b1f0d94c26b64e814243b736f47e7ef40d96432c kvm/powerpc: move kvm_hypercall0() and friends to epapr_hypercall0()

:::::: TO: Bharat Bhushan <[email protected]>
:::::: CC: Alexander Graf <[email protected]>

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation


Attachments:
(No filename) (7.35 kB)
.config.gz (54.75 kB)
Download all attachments

2019-05-14 15:18:29

by Filippo Sironi

[permalink] [raw]
Subject: KVM: Start populating /sys/hypervisor with KVM entries

Long-time Xen HVM and Xen PV users are missing /sys/hypervisor entries when
moving to KVM. One report is about getting the VM UUID. The VM UUID can
already be retrieved using /sys/devices/virtual/dmi/id/product_uuid. This has
two downsides: (1) it requires root privileges and (2) it is only available on
KVM and Xen HVM.

By exposing /sys/hypervisor/uuid when running on KVM as well, we provide an
interface that's functional for KVM, Xen HVM, and Xen PV. Let's do so by
providing arch-specific hooks so that different architectures can implement the
hooks in different ways.

Further work can be done by consolidating the creation of the basic
/sys/hypervisor across hypervisors.

Filippo Sironi (2):
KVM: Start populating /sys/hypervisor with KVM entries
KVM: x86: Implement the arch-specific hook to report the VM UUID

2019-05-14 15:20:05

by Filippo Sironi

[permalink] [raw]
Subject: [PATCH v2 2/2] KVM: x86: Implement the arch-specific hook to report the VM UUID

On x86, we report the UUID in DMI System Information (i.e., DMI Type 1)
as VM UUID.

Signed-off-by: Filippo Sironi <[email protected]>
---
arch/x86/kernel/kvm.c | 7 +++++++
1 file changed, 7 insertions(+)

diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
index 5c93a65ee1e5..441cab08a09d 100644
--- a/arch/x86/kernel/kvm.c
+++ b/arch/x86/kernel/kvm.c
@@ -25,6 +25,7 @@
#include <linux/kernel.h>
#include <linux/kvm_para.h>
#include <linux/cpu.h>
+#include <linux/dmi.h>
#include <linux/mm.h>
#include <linux/highmem.h>
#include <linux/hardirq.h>
@@ -694,6 +695,12 @@ bool kvm_para_available(void)
}
EXPORT_SYMBOL_GPL(kvm_para_available);

+const char *kvm_para_get_uuid(void)
+{
+ return dmi_get_system_info(DMI_PRODUCT_UUID);
+}
+EXPORT_SYMBOL_GPL(kvm_para_get_uuid);
+
unsigned int kvm_arch_para_features(void)
{
return cpuid_eax(kvm_cpuid_base() | KVM_CPUID_FEATURES);
--
2.7.4

2019-05-14 15:21:07

by Filippo Sironi

[permalink] [raw]
Subject: [PATCH v2 1/2] KVM: Start populating /sys/hypervisor with KVM entries

Start populating /sys/hypervisor with KVM entries when we're running on
KVM. This is to replicate functionality that's available when we're
running on Xen.

Start with /sys/hypervisor/uuid, which users prefer over
/sys/devices/virtual/dmi/id/product_uuid as a way to recognize a virtual
machine, since it's also available when running on Xen HVM and on Xen PV
and, on top of that doesn't require root privileges by default.
Let's create arch-specific hooks so that different architectures can
provide different implementations.

Signed-off-by: Filippo Sironi <[email protected]>
---
v2:
* move the retrieval of the VM UUID out of uuid_show and into
kvm_para_get_uuid, which is a weak function that can be overwritten

drivers/Kconfig | 2 ++
drivers/Makefile | 2 ++
drivers/kvm/Kconfig | 14 ++++++++++++++
drivers/kvm/Makefile | 1 +
drivers/kvm/sys-hypervisor.c | 30 ++++++++++++++++++++++++++++++
5 files changed, 49 insertions(+)
create mode 100644 drivers/kvm/Kconfig
create mode 100644 drivers/kvm/Makefile
create mode 100644 drivers/kvm/sys-hypervisor.c

diff --git a/drivers/Kconfig b/drivers/Kconfig
index 45f9decb9848..90eb835fe951 100644
--- a/drivers/Kconfig
+++ b/drivers/Kconfig
@@ -146,6 +146,8 @@ source "drivers/hv/Kconfig"

source "drivers/xen/Kconfig"

+source "drivers/kvm/Kconfig"
+
source "drivers/staging/Kconfig"

source "drivers/platform/Kconfig"
diff --git a/drivers/Makefile b/drivers/Makefile
index c61cde554340..79cc92a3f6bf 100644
--- a/drivers/Makefile
+++ b/drivers/Makefile
@@ -44,6 +44,8 @@ obj-y += soc/
obj-$(CONFIG_VIRTIO) += virtio/
obj-$(CONFIG_XEN) += xen/

+obj-$(CONFIG_KVM_GUEST) += kvm/
+
# regulators early, since some subsystems rely on them to initialize
obj-$(CONFIG_REGULATOR) += regulator/

diff --git a/drivers/kvm/Kconfig b/drivers/kvm/Kconfig
new file mode 100644
index 000000000000..3fc041df7c11
--- /dev/null
+++ b/drivers/kvm/Kconfig
@@ -0,0 +1,14 @@
+menu "KVM driver support"
+ depends on KVM_GUEST
+
+config KVM_SYS_HYPERVISOR
+ bool "Create KVM entries under /sys/hypervisor"
+ depends on SYSFS
+ select SYS_HYPERVISOR
+ default y
+ help
+ Create KVM entries under /sys/hypervisor (e.g., uuid). When running
+ native or on another hypervisor, /sys/hypervisor may still be
+ present, but it will have no KVM entries.
+
+endmenu
diff --git a/drivers/kvm/Makefile b/drivers/kvm/Makefile
new file mode 100644
index 000000000000..73a43fc994b9
--- /dev/null
+++ b/drivers/kvm/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_KVM_SYS_HYPERVISOR) += sys-hypervisor.o
diff --git a/drivers/kvm/sys-hypervisor.c b/drivers/kvm/sys-hypervisor.c
new file mode 100644
index 000000000000..43b1d1a09807
--- /dev/null
+++ b/drivers/kvm/sys-hypervisor.c
@@ -0,0 +1,30 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#include <asm/kvm_para.h>
+
+#include <linux/kobject.h>
+#include <linux/sysfs.h>
+
+__weak const char *kvm_para_get_uuid(void)
+{
+ return NULL;
+}
+
+static ssize_t uuid_show(struct kobject *obj,
+ struct kobj_attribute *attr,
+ char *buf)
+{
+ const char *uuid = kvm_para_get_uuid();
+ return sprintf(buf, "%s\n", uuid);
+}
+
+static struct kobj_attribute uuid = __ATTR_RO(uuid);
+
+static int __init uuid_init(void)
+{
+ if (!kvm_para_available())
+ return 0;
+ return sysfs_create_file(hypervisor_kobj, &uuid.attr);
+}
+
+device_initcall(uuid_init);
--
2.7.4

2019-05-14 15:29:14

by Christian Borntraeger

[permalink] [raw]
Subject: Re: [PATCH v2 1/2] KVM: Start populating /sys/hypervisor with KVM entries



On 14.05.19 17:16, Filippo Sironi wrote:
> Start populating /sys/hypervisor with KVM entries when we're running on
> KVM. This is to replicate functionality that's available when we're
> running on Xen.
>
> Start with /sys/hypervisor/uuid, which users prefer over
> /sys/devices/virtual/dmi/id/product_uuid as a way to recognize a virtual
> machine, since it's also available when running on Xen HVM and on Xen PV
> and, on top of that doesn't require root privileges by default.
> Let's create arch-specific hooks so that different architectures can
> provide different implementations.
>
> Signed-off-by: Filippo Sironi <[email protected]>
> ---
> v2:
> * move the retrieval of the VM UUID out of uuid_show and into
> kvm_para_get_uuid, which is a weak function that can be overwritten
>
> drivers/Kconfig | 2 ++
> drivers/Makefile | 2 ++
> drivers/kvm/Kconfig | 14 ++++++++++++++
> drivers/kvm/Makefile | 1 +
> drivers/kvm/sys-hypervisor.c | 30 ++++++++++++++++++++++++++++++
> 5 files changed, 49 insertions(+)
> create mode 100644 drivers/kvm/Kconfig
> create mode 100644 drivers/kvm/Makefile
> create mode 100644 drivers/kvm/sys-hypervisor.c
>
> diff --git a/drivers/Kconfig b/drivers/Kconfig
> index 45f9decb9848..90eb835fe951 100644
> --- a/drivers/Kconfig
> +++ b/drivers/Kconfig
> @@ -146,6 +146,8 @@ source "drivers/hv/Kconfig"
>
> source "drivers/xen/Kconfig"
>
> +source "drivers/kvm/Kconfig"
> +
> source "drivers/staging/Kconfig"
>
> source "drivers/platform/Kconfig"
> diff --git a/drivers/Makefile b/drivers/Makefile
> index c61cde554340..79cc92a3f6bf 100644
> --- a/drivers/Makefile
> +++ b/drivers/Makefile
> @@ -44,6 +44,8 @@ obj-y += soc/
> obj-$(CONFIG_VIRTIO) += virtio/
> obj-$(CONFIG_XEN) += xen/
>
> +obj-$(CONFIG_KVM_GUEST) += kvm/
> +
> # regulators early, since some subsystems rely on them to initialize
> obj-$(CONFIG_REGULATOR) += regulator/
>
> diff --git a/drivers/kvm/Kconfig b/drivers/kvm/Kconfig
> new file mode 100644
> index 000000000000..3fc041df7c11
> --- /dev/null
> +++ b/drivers/kvm/Kconfig
> @@ -0,0 +1,14 @@
> +menu "KVM driver support"
> + depends on KVM_GUEST
> +
> +config KVM_SYS_HYPERVISOR
> + bool "Create KVM entries under /sys/hypervisor"
> + depends on SYSFS
> + select SYS_HYPERVISOR
> + default y
> + help
> + Create KVM entries under /sys/hypervisor (e.g., uuid). When running
> + native or on another hypervisor, /sys/hypervisor may still be
> + present, but it will have no KVM entries.
> +
> +endmenu
> diff --git a/drivers/kvm/Makefile b/drivers/kvm/Makefile
> new file mode 100644
> index 000000000000..73a43fc994b9
> --- /dev/null
> +++ b/drivers/kvm/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_KVM_SYS_HYPERVISOR) += sys-hypervisor.o
> diff --git a/drivers/kvm/sys-hypervisor.c b/drivers/kvm/sys-hypervisor.c
> new file mode 100644
> index 000000000000..43b1d1a09807
> --- /dev/null
> +++ b/drivers/kvm/sys-hypervisor.c
> @@ -0,0 +1,30 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#include <asm/kvm_para.h>
> +
> +#include <linux/kobject.h>
> +#include <linux/sysfs.h>
> +
> +__weak const char *kvm_para_get_uuid(void)
> +{
> + return NULL;
> +}
> +
> +static ssize_t uuid_show(struct kobject *obj,
> + struct kobj_attribute *attr,
> + char *buf)
> +{
> + const char *uuid = kvm_para_get_uuid();

I would prefer to have kvm_para_get_uuid return a uuid_t
but char * will probably work out as well.


> + return sprintf(buf, "%s\n", uuid);
> +}
> +
> +static struct kobj_attribute uuid = __ATTR_RO(uuid);
> +
> +static int __init uuid_init(void)
> +{
> + if (!kvm_para_available())

Isnt kvm_para_available a function that is defined in the context of the HOST
and not of the guest?

> + return 0;
> + return sysfs_create_file(hypervisor_kobj, &uuid.attr);
> +}
> +
> +device_initcall(uuid_init);
>

2019-05-14 16:10:48

by Filippo Sironi

[permalink] [raw]
Subject: Re: [PATCH v2 1/2] KVM: Start populating /sys/hypervisor with KVM entries



> On 14. May 2019, at 17:26, Christian Borntraeger <[email protected]> wrote:
>
>
>
> On 14.05.19 17:16, Filippo Sironi wrote:
>> Start populating /sys/hypervisor with KVM entries when we're running on
>> KVM. This is to replicate functionality that's available when we're
>> running on Xen.
>>
>> Start with /sys/hypervisor/uuid, which users prefer over
>> /sys/devices/virtual/dmi/id/product_uuid as a way to recognize a virtual
>> machine, since it's also available when running on Xen HVM and on Xen PV
>> and, on top of that doesn't require root privileges by default.
>> Let's create arch-specific hooks so that different architectures can
>> provide different implementations.
>>
>> Signed-off-by: Filippo Sironi <[email protected]>
>> ---
>> v2:
>> * move the retrieval of the VM UUID out of uuid_show and into
>> kvm_para_get_uuid, which is a weak function that can be overwritten
>>
>> drivers/Kconfig | 2 ++
>> drivers/Makefile | 2 ++
>> drivers/kvm/Kconfig | 14 ++++++++++++++
>> drivers/kvm/Makefile | 1 +
>> drivers/kvm/sys-hypervisor.c | 30 ++++++++++++++++++++++++++++++
>> 5 files changed, 49 insertions(+)
>> create mode 100644 drivers/kvm/Kconfig
>> create mode 100644 drivers/kvm/Makefile
>> create mode 100644 drivers/kvm/sys-hypervisor.c
>>
>> diff --git a/drivers/Kconfig b/drivers/Kconfig
>> index 45f9decb9848..90eb835fe951 100644
>> --- a/drivers/Kconfig
>> +++ b/drivers/Kconfig
>> @@ -146,6 +146,8 @@ source "drivers/hv/Kconfig"
>>
>> source "drivers/xen/Kconfig"
>>
>> +source "drivers/kvm/Kconfig"
>> +
>> source "drivers/staging/Kconfig"
>>
>> source "drivers/platform/Kconfig"
>> diff --git a/drivers/Makefile b/drivers/Makefile
>> index c61cde554340..79cc92a3f6bf 100644
>> --- a/drivers/Makefile
>> +++ b/drivers/Makefile
>> @@ -44,6 +44,8 @@ obj-y += soc/
>> obj-$(CONFIG_VIRTIO) += virtio/
>> obj-$(CONFIG_XEN) += xen/
>>
>> +obj-$(CONFIG_KVM_GUEST) += kvm/
>> +
>> # regulators early, since some subsystems rely on them to initialize
>> obj-$(CONFIG_REGULATOR) += regulator/
>>
>> diff --git a/drivers/kvm/Kconfig b/drivers/kvm/Kconfig
>> new file mode 100644
>> index 000000000000..3fc041df7c11
>> --- /dev/null
>> +++ b/drivers/kvm/Kconfig
>> @@ -0,0 +1,14 @@
>> +menu "KVM driver support"
>> + depends on KVM_GUEST
>> +
>> +config KVM_SYS_HYPERVISOR
>> + bool "Create KVM entries under /sys/hypervisor"
>> + depends on SYSFS
>> + select SYS_HYPERVISOR
>> + default y
>> + help
>> + Create KVM entries under /sys/hypervisor (e.g., uuid). When running
>> + native or on another hypervisor, /sys/hypervisor may still be
>> + present, but it will have no KVM entries.
>> +
>> +endmenu
>> diff --git a/drivers/kvm/Makefile b/drivers/kvm/Makefile
>> new file mode 100644
>> index 000000000000..73a43fc994b9
>> --- /dev/null
>> +++ b/drivers/kvm/Makefile
>> @@ -0,0 +1 @@
>> +obj-$(CONFIG_KVM_SYS_HYPERVISOR) += sys-hypervisor.o
>> diff --git a/drivers/kvm/sys-hypervisor.c b/drivers/kvm/sys-hypervisor.c
>> new file mode 100644
>> index 000000000000..43b1d1a09807
>> --- /dev/null
>> +++ b/drivers/kvm/sys-hypervisor.c
>> @@ -0,0 +1,30 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +
>> +#include <asm/kvm_para.h>
>> +
>> +#include <linux/kobject.h>
>> +#include <linux/sysfs.h>
>> +
>> +__weak const char *kvm_para_get_uuid(void)
>> +{
>> + return NULL;
>> +}
>> +
>> +static ssize_t uuid_show(struct kobject *obj,
>> + struct kobj_attribute *attr,
>> + char *buf)
>> +{
>> + const char *uuid = kvm_para_get_uuid();
>
> I would prefer to have kvm_para_get_uuid return a uuid_t
> but char * will probably work out as well.

Let me give this a quick spin.

>> + return sprintf(buf, "%s\n", uuid);
>> +}
>> +
>> +static struct kobj_attribute uuid = __ATTR_RO(uuid);
>> +
>> +static int __init uuid_init(void)
>> +{
>> + if (!kvm_para_available())
>
> Isnt kvm_para_available a function that is defined in the context of the HOST
> and not of the guest?

No, kvm_para_available is defined in the guest context.
On x86, it checks for the presence of the KVM CPUID leafs.

>> + return 0;
>> + return sysfs_create_file(hypervisor_kobj, &uuid.attr);
>> +}
>> +
>> +device_initcall(uuid_init);




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrer: Christian Schlaeger, Ralf Herbrich
Ust-ID: DE 289 237 879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B


2019-05-14 16:33:19

by Christian Borntraeger

[permalink] [raw]
Subject: Re: [PATCH v2 1/2] KVM: Start populating /sys/hypervisor with KVM entries



On 14.05.19 18:09, Sironi, Filippo wrote:

>> Isnt kvm_para_available a function that is defined in the context of the HOST
>> and not of the guest?
>
> No, kvm_para_available is defined in the guest context.
> On x86, it checks for the presence of the KVM CPUID leafs.
>

Right you are, I mixed that up.

2019-05-14 22:11:00

by Filippo Sironi

[permalink] [raw]
Subject: Re: [PATCH v2 1/2] KVM: Start populating /sys/hypervisor with KVM entries



> On 14. May 2019, at 18:09, Sironi, Filippo <[email protected]> wrote:
>
>> On 14. May 2019, at 17:26, Christian Borntraeger <[email protected]> wrote:
>>
>>> On 14.05.19 17:16, Filippo Sironi wrote:
>>> Start populating /sys/hypervisor with KVM entries when we're running on
>>> KVM. This is to replicate functionality that's available when we're
>>> running on Xen.
>>>
>>> Start with /sys/hypervisor/uuid, which users prefer over
>>> /sys/devices/virtual/dmi/id/product_uuid as a way to recognize a virtual
>>> machine, since it's also available when running on Xen HVM and on Xen PV
>>> and, on top of that doesn't require root privileges by default.
>>> Let's create arch-specific hooks so that different architectures can
>>> provide different implementations.
>>>
>>> Signed-off-by: Filippo Sironi <[email protected]>
>>> ---
>>> v2:
>>> * move the retrieval of the VM UUID out of uuid_show and into
>>> kvm_para_get_uuid, which is a weak function that can be overwritten
>>>
>>> drivers/Kconfig | 2 ++
>>> drivers/Makefile | 2 ++
>>> drivers/kvm/Kconfig | 14 ++++++++++++++
>>> drivers/kvm/Makefile | 1 +
>>> drivers/kvm/sys-hypervisor.c | 30 ++++++++++++++++++++++++++++++
>>> 5 files changed, 49 insertions(+)
>>> create mode 100644 drivers/kvm/Kconfig
>>> create mode 100644 drivers/kvm/Makefile
>>> create mode 100644 drivers/kvm/sys-hypervisor.c
>>>
>>> diff --git a/drivers/Kconfig b/drivers/Kconfig
>>> index 45f9decb9848..90eb835fe951 100644
>>> --- a/drivers/Kconfig
>>> +++ b/drivers/Kconfig
>>> @@ -146,6 +146,8 @@ source "drivers/hv/Kconfig"
>>>
>>> source "drivers/xen/Kconfig"
>>>
>>> +source "drivers/kvm/Kconfig"
>>> +
>>> source "drivers/staging/Kconfig"
>>>
>>> source "drivers/platform/Kconfig"
>>> diff --git a/drivers/Makefile b/drivers/Makefile
>>> index c61cde554340..79cc92a3f6bf 100644
>>> --- a/drivers/Makefile
>>> +++ b/drivers/Makefile
>>> @@ -44,6 +44,8 @@ obj-y += soc/
>>> obj-$(CONFIG_VIRTIO) += virtio/
>>> obj-$(CONFIG_XEN) += xen/
>>>
>>> +obj-$(CONFIG_KVM_GUEST) += kvm/
>>> +
>>> # regulators early, since some subsystems rely on them to initialize
>>> obj-$(CONFIG_REGULATOR) += regulator/
>>>
>>> diff --git a/drivers/kvm/Kconfig b/drivers/kvm/Kconfig
>>> new file mode 100644
>>> index 000000000000..3fc041df7c11
>>> --- /dev/null
>>> +++ b/drivers/kvm/Kconfig
>>> @@ -0,0 +1,14 @@
>>> +menu "KVM driver support"
>>> + depends on KVM_GUEST
>>> +
>>> +config KVM_SYS_HYPERVISOR
>>> + bool "Create KVM entries under /sys/hypervisor"
>>> + depends on SYSFS
>>> + select SYS_HYPERVISOR
>>> + default y
>>> + help
>>> + Create KVM entries under /sys/hypervisor (e.g., uuid). When running
>>> + native or on another hypervisor, /sys/hypervisor may still be
>>> + present, but it will have no KVM entries.
>>> +
>>> +endmenu
>>> diff --git a/drivers/kvm/Makefile b/drivers/kvm/Makefile
>>> new file mode 100644
>>> index 000000000000..73a43fc994b9
>>> --- /dev/null
>>> +++ b/drivers/kvm/Makefile
>>> @@ -0,0 +1 @@
>>> +obj-$(CONFIG_KVM_SYS_HYPERVISOR) += sys-hypervisor.o
>>> diff --git a/drivers/kvm/sys-hypervisor.c b/drivers/kvm/sys-hypervisor.c
>>> new file mode 100644
>>> index 000000000000..43b1d1a09807
>>> --- /dev/null
>>> +++ b/drivers/kvm/sys-hypervisor.c
>>> @@ -0,0 +1,30 @@
>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>> +
>>> +#include <asm/kvm_para.h>
>>> +
>>> +#include <linux/kobject.h>
>>> +#include <linux/sysfs.h>
>>> +
>>> +__weak const char *kvm_para_get_uuid(void)
>>> +{
>>> + return NULL;
>>> +}
>>> +
>>> +static ssize_t uuid_show(struct kobject *obj,
>>> + struct kobj_attribute *attr,
>>> + char *buf)
>>> +{
>>> + const char *uuid = kvm_para_get_uuid();
>>
>> I would prefer to have kvm_para_get_uuid return a uuid_t
>> but char * will probably work out as well.
>
> Let me give this a quick spin.

I looked into getting a uuid_t.

At least for architectures where we retrieve that bit of
information from DMI tables, this is undesirable since
the interpretation of the UUID changed with DMI 2.6
(the first 3 fields are now encoded in little-endian).
This means that we wouldn't know how to print it in this
generic code.

I think that it's best if the architecture specific code
turns the UUID into the string representation.

>>> + return sprintf(buf, "%s\n", uuid);
>>> +}
>>> +
>>> +static struct kobj_attribute uuid = __ATTR_RO(uuid);
>>> +
>>> +static int __init uuid_init(void)
>>> +{
>>> + if (!kvm_para_available())
>>
>> Isnt kvm_para_available a function that is defined in the context of the HOST
>> and not of the guest?
>
> No, kvm_para_available is defined in the guest context.
> On x86, it checks for the presence of the KVM CPUID leafs.
>
>>> + return 0;
>>> + return sysfs_create_file(hypervisor_kobj, &uuid.attr);
>>> +}
>>> +
>>> +device_initcall(uuid_init);




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrer: Christian Schlaeger, Ralf Herbrich
Ust-ID: DE 289 237 879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B


2019-05-16 13:53:30

by Alexander Graf

[permalink] [raw]
Subject: Re: [PATCH v2 1/2] KVM: Start populating /sys/hypervisor with KVM entries

On 14.05.19 08:16, Filippo Sironi wrote:
> Start populating /sys/hypervisor with KVM entries when we're running on
> KVM. This is to replicate functionality that's available when we're
> running on Xen.
>
> Start with /sys/hypervisor/uuid, which users prefer over
> /sys/devices/virtual/dmi/id/product_uuid as a way to recognize a virtual
> machine, since it's also available when running on Xen HVM and on Xen PV
> and, on top of that doesn't require root privileges by default.
> Let's create arch-specific hooks so that different architectures can
> provide different implementations.
>
> Signed-off-by: Filippo Sironi <[email protected]>

I think this needs something akin to

https://www.kernel.org/doc/Documentation/ABI/stable/sysfs-hypervisor-xen

to document which files are available.

> ---
> v2:
> * move the retrieval of the VM UUID out of uuid_show and into
> kvm_para_get_uuid, which is a weak function that can be overwritten
>
> drivers/Kconfig | 2 ++
> drivers/Makefile | 2 ++
> drivers/kvm/Kconfig | 14 ++++++++++++++
> drivers/kvm/Makefile | 1 +
> drivers/kvm/sys-hypervisor.c | 30 ++++++++++++++++++++++++++++++
> 5 files changed, 49 insertions(+)
> create mode 100644 drivers/kvm/Kconfig
> create mode 100644 drivers/kvm/Makefile
> create mode 100644 drivers/kvm/sys-hypervisor.c
>

[...]

> +
> +__weak const char *kvm_para_get_uuid(void)
> +{
> + return NULL;
> +}
> +
> +static ssize_t uuid_show(struct kobject *obj,
> + struct kobj_attribute *attr,
> + char *buf)
> +{
> + const char *uuid = kvm_para_get_uuid();
> + return sprintf(buf, "%s\n", uuid);

The usual return value for the Xen /sys/hypervisor interface is
"<denied>". Wouldn't it make sense to follow that pattern for the KVM
one too? Currently, if we can not determine the UUID this will just
return (null).

Otherwise, looks good to me. Are you aware of any other files we should
provide? Also, is there any reason not to implement ARM as well while at it?

Alex

> +}
> +
> +static struct kobj_attribute uuid = __ATTR_RO(uuid);
> +
> +static int __init uuid_init(void)
> +{
> + if (!kvm_para_available())
> + return 0;
> + return sysfs_create_file(hypervisor_kobj, &uuid.attr);
> +}
> +
> +device_initcall(uuid_init);
>

2019-05-16 13:58:18

by Alexander Graf

[permalink] [raw]
Subject: Re: [PATCH v2 2/2] KVM: x86: Implement the arch-specific hook to report the VM UUID

On 14.05.19 08:16, Filippo Sironi wrote:
> On x86, we report the UUID in DMI System Information (i.e., DMI Type 1)
> as VM UUID.
>
> Signed-off-by: Filippo Sironi <[email protected]>
> ---
> arch/x86/kernel/kvm.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
> index 5c93a65ee1e5..441cab08a09d 100644
> --- a/arch/x86/kernel/kvm.c
> +++ b/arch/x86/kernel/kvm.c
> @@ -25,6 +25,7 @@
> #include <linux/kernel.h>
> #include <linux/kvm_para.h>
> #include <linux/cpu.h>
> +#include <linux/dmi.h>
> #include <linux/mm.h>
> #include <linux/highmem.h>
> #include <linux/hardirq.h>
> @@ -694,6 +695,12 @@ bool kvm_para_available(void)
> }
> EXPORT_SYMBOL_GPL(kvm_para_available);
>
> +const char *kvm_para_get_uuid(void)
> +{
> + return dmi_get_system_info(DMI_PRODUCT_UUID);

This adds a new dependency on CONFIG_DMI. Probably best to guard it with
an #if IS_ENABLED(CONFIG_DMI).

The concept seems sound though.


Alex

> +}
> +EXPORT_SYMBOL_GPL(kvm_para_get_uuid);
> +
> unsigned int kvm_arch_para_features(void)
> {
> return cpuid_eax(kvm_cpuid_base() | KVM_CPUID_FEATURES);
>

2019-05-16 14:11:36

by Andrew Cooper

[permalink] [raw]
Subject: Re: [Xen-devel] [PATCH v2 1/2] KVM: Start populating /sys/hypervisor with KVM entries

On 16/05/2019 14:50, Alexander Graf wrote:
> On 14.05.19 08:16, Filippo Sironi wrote:
>> Start populating /sys/hypervisor with KVM entries when we're running on
>> KVM. This is to replicate functionality that's available when we're
>> running on Xen.
>>
>> Start with /sys/hypervisor/uuid, which users prefer over
>> /sys/devices/virtual/dmi/id/product_uuid as a way to recognize a virtual
>> machine, since it's also available when running on Xen HVM and on Xen PV
>> and, on top of that doesn't require root privileges by default.
>> Let's create arch-specific hooks so that different architectures can
>> provide different implementations.
>>
>> Signed-off-by: Filippo Sironi <[email protected]>
> I think this needs something akin to
>
> https://www.kernel.org/doc/Documentation/ABI/stable/sysfs-hypervisor-xen
>
> to document which files are available.
>
>> ---
>> v2:
>> * move the retrieval of the VM UUID out of uuid_show and into
>> kvm_para_get_uuid, which is a weak function that can be overwritten
>>
>> drivers/Kconfig | 2 ++
>> drivers/Makefile | 2 ++
>> drivers/kvm/Kconfig | 14 ++++++++++++++
>> drivers/kvm/Makefile | 1 +
>> drivers/kvm/sys-hypervisor.c | 30 ++++++++++++++++++++++++++++++
>> 5 files changed, 49 insertions(+)
>> create mode 100644 drivers/kvm/Kconfig
>> create mode 100644 drivers/kvm/Makefile
>> create mode 100644 drivers/kvm/sys-hypervisor.c
>>
> [...]
>
>> +
>> +__weak const char *kvm_para_get_uuid(void)
>> +{
>> + return NULL;
>> +}
>> +
>> +static ssize_t uuid_show(struct kobject *obj,
>> + struct kobj_attribute *attr,
>> + char *buf)
>> +{
>> + const char *uuid = kvm_para_get_uuid();
>> + return sprintf(buf, "%s\n", uuid);
> The usual return value for the Xen /sys/hypervisor interface is
> "<denied>".

This string comes straight from Xen.

It was an effort to reduce the quantity of interesting fingerprintable
data accessable by default to unprivileged guests.

See
https://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=a2fc8d514df2b38c310d4f4432fe06520b0769ed

~Andrew

2019-05-16 17:03:27

by Alexander Graf

[permalink] [raw]
Subject: Re: [PATCH v2 2/2] KVM: x86: Implement the arch-specific hook to report the VM UUID


On 16.05.19 08:25, Sironi, Filippo wrote:
>> On 16. May 2019, at 15:56, Graf, Alexander <[email protected]> wrote:
>>
>> On 14.05.19 08:16, Filippo Sironi wrote:
>>> On x86, we report the UUID in DMI System Information (i.e., DMI Type 1)
>>> as VM UUID.
>>>
>>> Signed-off-by: Filippo Sironi <[email protected]>
>>> ---
>>> arch/x86/kernel/kvm.c | 7 +++++++
>>> 1 file changed, 7 insertions(+)
>>>
>>> diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
>>> index 5c93a65ee1e5..441cab08a09d 100644
>>> --- a/arch/x86/kernel/kvm.c
>>> +++ b/arch/x86/kernel/kvm.c
>>> @@ -25,6 +25,7 @@
>>> #include <linux/kernel.h>
>>> #include <linux/kvm_para.h>
>>> #include <linux/cpu.h>
>>> +#include <linux/dmi.h>
>>> #include <linux/mm.h>
>>> #include <linux/highmem.h>
>>> #include <linux/hardirq.h>
>>> @@ -694,6 +695,12 @@ bool kvm_para_available(void)
>>> }
>>> EXPORT_SYMBOL_GPL(kvm_para_available);
>>>
>>> +const char *kvm_para_get_uuid(void)
>>> +{
>>> + return dmi_get_system_info(DMI_PRODUCT_UUID);
>> This adds a new dependency on CONFIG_DMI. Probably best to guard it with
>> an #if IS_ENABLED(CONFIG_DMI).
>>
>> The concept seems sound though.
>>
>> Alex
> include/linux/dmi.h contains a dummy implementation of
> dmi_get_system_info that returns NULL if CONFIG_DMI isn't defined.


Oh, I missed that bit. Awesome! Less work :).


> This is enough unless we decide to return "<denied>" like in Xen.
> If then, we can have the check in the generic code to turn NULL
> into "<denied>".


Yes. Waiting for someone from Xen to answer this :)


Alex


2019-05-16 17:16:48

by Filippo Sironi

[permalink] [raw]
Subject: Re: [PATCH v2 2/2] KVM: x86: Implement the arch-specific hook to report the VM UUID


> On 16. May 2019, at 15:56, Graf, Alexander <[email protected]> wrote:
>
> On 14.05.19 08:16, Filippo Sironi wrote:
>> On x86, we report the UUID in DMI System Information (i.e., DMI Type 1)
>> as VM UUID.
>>
>> Signed-off-by: Filippo Sironi <[email protected]>
>> ---
>> arch/x86/kernel/kvm.c | 7 +++++++
>> 1 file changed, 7 insertions(+)
>>
>> diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
>> index 5c93a65ee1e5..441cab08a09d 100644
>> --- a/arch/x86/kernel/kvm.c
>> +++ b/arch/x86/kernel/kvm.c
>> @@ -25,6 +25,7 @@
>> #include <linux/kernel.h>
>> #include <linux/kvm_para.h>
>> #include <linux/cpu.h>
>> +#include <linux/dmi.h>
>> #include <linux/mm.h>
>> #include <linux/highmem.h>
>> #include <linux/hardirq.h>
>> @@ -694,6 +695,12 @@ bool kvm_para_available(void)
>> }
>> EXPORT_SYMBOL_GPL(kvm_para_available);
>>
>> +const char *kvm_para_get_uuid(void)
>> +{
>> + return dmi_get_system_info(DMI_PRODUCT_UUID);
>
> This adds a new dependency on CONFIG_DMI. Probably best to guard it with
> an #if IS_ENABLED(CONFIG_DMI).
>
> The concept seems sound though.
>
> Alex

include/linux/dmi.h contains a dummy implementation of
dmi_get_system_info that returns NULL if CONFIG_DMI isn't defined.
This is enough unless we decide to return "<denied>" like in Xen.
If then, we can have the check in the generic code to turn NULL
into "<denied>".

Filippo


>> +}
>> +EXPORT_SYMBOL_GPL(kvm_para_get_uuid);
>> +
>> unsigned int kvm_arch_para_features(void)
>> {
>> return cpuid_eax(kvm_cpuid_base() | KVM_CPUID_FEATURES);




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrer: Christian Schlaeger, Ralf Herbrich
Ust-ID: DE 289 237 879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B


2019-05-16 18:17:15

by Boris Ostrovsky

[permalink] [raw]
Subject: Re: [PATCH v2 2/2] KVM: x86: Implement the arch-specific hook to report the VM UUID

On 5/16/19 11:33 AM, Alexander Graf wrote:
> On 16.05.19 08:25, Sironi, Filippo wrote:
>>> On 16. May 2019, at 15:56, Graf, Alexander <[email protected]> wrote:
>>>
>>> On 14.05.19 08:16, Filippo Sironi wrote:
>>>> On x86, we report the UUID in DMI System Information (i.e., DMI Type 1)
>>>> as VM UUID.
>>>>
>>>> Signed-off-by: Filippo Sironi <[email protected]>
>>>> ---
>>>> arch/x86/kernel/kvm.c | 7 +++++++
>>>> 1 file changed, 7 insertions(+)
>>>>
>>>> diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
>>>> index 5c93a65ee1e5..441cab08a09d 100644
>>>> --- a/arch/x86/kernel/kvm.c
>>>> +++ b/arch/x86/kernel/kvm.c
>>>> @@ -25,6 +25,7 @@
>>>> #include <linux/kernel.h>
>>>> #include <linux/kvm_para.h>
>>>> #include <linux/cpu.h>
>>>> +#include <linux/dmi.h>
>>>> #include <linux/mm.h>
>>>> #include <linux/highmem.h>
>>>> #include <linux/hardirq.h>
>>>> @@ -694,6 +695,12 @@ bool kvm_para_available(void)
>>>> }
>>>> EXPORT_SYMBOL_GPL(kvm_para_available);
>>>>
>>>> +const char *kvm_para_get_uuid(void)
>>>> +{
>>>> + return dmi_get_system_info(DMI_PRODUCT_UUID);
>>> This adds a new dependency on CONFIG_DMI. Probably best to guard it with
>>> an #if IS_ENABLED(CONFIG_DMI).
>>>
>>> The concept seems sound though.
>>>
>>> Alex
>> include/linux/dmi.h contains a dummy implementation of
>> dmi_get_system_info that returns NULL if CONFIG_DMI isn't defined.
>
> Oh, I missed that bit. Awesome! Less work :).
>
>
>> This is enough unless we decide to return "<denied>" like in Xen.
>> If then, we can have the check in the generic code to turn NULL
>> into "<denied>".
>
> Yes. Waiting for someone from Xen to answer this :)

Not sure I am answering your question but on Xen we return UUID value
zero if access permissions are not sufficient. Not <denied>.

http://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=xen/common/kernel.c;h=612575430f1ce7faf5bd66e7a99f1758c63fb3cb;hb=HEAD#l506

-boris

2019-05-16 18:44:52

by Filippo Sironi

[permalink] [raw]
Subject: Re: [PATCH v2 2/2] KVM: x86: Implement the arch-specific hook to report the VM UUID


> On 16. May 2019, at 18:40, Boris Ostrovsky <[email protected]> wrote:
>
> On 5/16/19 11:33 AM, Alexander Graf wrote:
>> On 16.05.19 08:25, Sironi, Filippo wrote:
>>>> On 16. May 2019, at 15:56, Graf, Alexander <[email protected]> wrote:
>>>>
>>>> On 14.05.19 08:16, Filippo Sironi wrote:
>>>>> On x86, we report the UUID in DMI System Information (i.e., DMI Type 1)
>>>>> as VM UUID.
>>>>>
>>>>> Signed-off-by: Filippo Sironi <[email protected]>
>>>>> ---
>>>>> arch/x86/kernel/kvm.c | 7 +++++++
>>>>> 1 file changed, 7 insertions(+)
>>>>>
>>>>> diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
>>>>> index 5c93a65ee1e5..441cab08a09d 100644
>>>>> --- a/arch/x86/kernel/kvm.c
>>>>> +++ b/arch/x86/kernel/kvm.c
>>>>> @@ -25,6 +25,7 @@
>>>>> #include <linux/kernel.h>
>>>>> #include <linux/kvm_para.h>
>>>>> #include <linux/cpu.h>
>>>>> +#include <linux/dmi.h>
>>>>> #include <linux/mm.h>
>>>>> #include <linux/highmem.h>
>>>>> #include <linux/hardirq.h>
>>>>> @@ -694,6 +695,12 @@ bool kvm_para_available(void)
>>>>> }
>>>>> EXPORT_SYMBOL_GPL(kvm_para_available);
>>>>>
>>>>> +const char *kvm_para_get_uuid(void)
>>>>> +{
>>>>> + return dmi_get_system_info(DMI_PRODUCT_UUID);
>>>> This adds a new dependency on CONFIG_DMI. Probably best to guard it with
>>>> an #if IS_ENABLED(CONFIG_DMI).
>>>>
>>>> The concept seems sound though.
>>>>
>>>> Alex
>>> include/linux/dmi.h contains a dummy implementation of
>>> dmi_get_system_info that returns NULL if CONFIG_DMI isn't defined.
>>
>> Oh, I missed that bit. Awesome! Less work :).
>>
>>
>>> This is enough unless we decide to return "<denied>" like in Xen.
>>> If then, we can have the check in the generic code to turn NULL
>>> into "<denied>".
>>
>> Yes. Waiting for someone from Xen to answer this :)
>
> Not sure I am answering your question but on Xen we return UUID value
> zero if access permissions are not sufficient. Not <denied>.
>
> http://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=xen/common/kernel.c;h=612575430f1ce7faf5bd66e7a99f1758c63fb3cb;hb=HEAD#l506
>
> -boris

Then, I believe that returning 00000000-0000-0000-0000-000000000000
instead of NULL in the weak implementation of 1/2 and translating
NULL into 00000000-0000-0000-0000-000000000000 is the better approach.

I'll repost.

Filippo





Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrer: Christian Schlaeger, Ralf Herbrich
Ust-ID: DE 289 237 879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B


2019-05-16 20:58:02

by Alexander Graf

[permalink] [raw]
Subject: Re: [PATCH v2 2/2] KVM: x86: Implement the arch-specific hook to report the VM UUID


On 16.05.19 10:41, Sironi, Filippo wrote:
>> On 16. May 2019, at 18:40, Boris Ostrovsky <[email protected]> wrote:
>>
>> On 5/16/19 11:33 AM, Alexander Graf wrote:
>>> On 16.05.19 08:25, Sironi, Filippo wrote:
>>>>> On 16. May 2019, at 15:56, Graf, Alexander <[email protected]> wrote:
>>>>>
>>>>> On 14.05.19 08:16, Filippo Sironi wrote:
>>>>>> On x86, we report the UUID in DMI System Information (i.e., DMI Type 1)
>>>>>> as VM UUID.
>>>>>>
>>>>>> Signed-off-by: Filippo Sironi <[email protected]>
>>>>>> ---
>>>>>> arch/x86/kernel/kvm.c | 7 +++++++
>>>>>> 1 file changed, 7 insertions(+)
>>>>>>
>>>>>> diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
>>>>>> index 5c93a65ee1e5..441cab08a09d 100644
>>>>>> --- a/arch/x86/kernel/kvm.c
>>>>>> +++ b/arch/x86/kernel/kvm.c
>>>>>> @@ -25,6 +25,7 @@
>>>>>> #include <linux/kernel.h>
>>>>>> #include <linux/kvm_para.h>
>>>>>> #include <linux/cpu.h>
>>>>>> +#include <linux/dmi.h>
>>>>>> #include <linux/mm.h>
>>>>>> #include <linux/highmem.h>
>>>>>> #include <linux/hardirq.h>
>>>>>> @@ -694,6 +695,12 @@ bool kvm_para_available(void)
>>>>>> }
>>>>>> EXPORT_SYMBOL_GPL(kvm_para_available);
>>>>>>
>>>>>> +const char *kvm_para_get_uuid(void)
>>>>>> +{
>>>>>> + return dmi_get_system_info(DMI_PRODUCT_UUID);
>>>>> This adds a new dependency on CONFIG_DMI. Probably best to guard it with
>>>>> an #if IS_ENABLED(CONFIG_DMI).
>>>>>
>>>>> The concept seems sound though.
>>>>>
>>>>> Alex
>>>> include/linux/dmi.h contains a dummy implementation of
>>>> dmi_get_system_info that returns NULL if CONFIG_DMI isn't defined.
>>> Oh, I missed that bit. Awesome! Less work :).
>>>
>>>
>>>> This is enough unless we decide to return "<denied>" like in Xen.
>>>> If then, we can have the check in the generic code to turn NULL
>>>> into "<denied>".
>>> Yes. Waiting for someone from Xen to answer this :)
>> Not sure I am answering your question but on Xen we return UUID value
>> zero if access permissions are not sufficient. Not <denied>.
>>
>> http://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=xen/common/kernel.c;h=612575430f1ce7faf5bd66e7a99f1758c63fb3cb;hb=HEAD#l506
>>
>> -boris
> Then, I believe that returning 00000000-0000-0000-0000-000000000000
> instead of NULL in the weak implementation of 1/2 and translating
> NULL into 00000000-0000-0000-0000-000000000000 is the better approach.


Just keep it at NULL in kvm_para_get_uuid() and convert to the canonical
00000000-0000-0000-0000-000000000000 in uuid_show().

Alex

2019-05-17 15:43:17

by Filippo Sironi

[permalink] [raw]
Subject: Re: [PATCH v2 1/2] KVM: Start populating /sys/hypervisor with KVM entries


> On 16. May 2019, at 15:50, Graf, Alexander <[email protected]> wrote:
>
> On 14.05.19 08:16, Filippo Sironi wrote:
>> Start populating /sys/hypervisor with KVM entries when we're running on
>> KVM. This is to replicate functionality that's available when we're
>> running on Xen.
>>
>> Start with /sys/hypervisor/uuid, which users prefer over
>> /sys/devices/virtual/dmi/id/product_uuid as a way to recognize a virtual
>> machine, since it's also available when running on Xen HVM and on Xen PV
>> and, on top of that doesn't require root privileges by default.
>> Let's create arch-specific hooks so that different architectures can
>> provide different implementations.
>>
>> Signed-off-by: Filippo Sironi <[email protected]>
>
> I think this needs something akin to
>
> https://www.kernel.org/doc/Documentation/ABI/stable/sysfs-hypervisor-xen
>
> to document which files are available.
>
>> ---
>> v2:
>> * move the retrieval of the VM UUID out of uuid_show and into
>> kvm_para_get_uuid, which is a weak function that can be overwritten
>>
>> drivers/Kconfig | 2 ++
>> drivers/Makefile | 2 ++
>> drivers/kvm/Kconfig | 14 ++++++++++++++
>> drivers/kvm/Makefile | 1 +
>> drivers/kvm/sys-hypervisor.c | 30 ++++++++++++++++++++++++++++++
>> 5 files changed, 49 insertions(+)
>> create mode 100644 drivers/kvm/Kconfig
>> create mode 100644 drivers/kvm/Makefile
>> create mode 100644 drivers/kvm/sys-hypervisor.c
>>
>
> [...]
>
>> +
>> +__weak const char *kvm_para_get_uuid(void)
>> +{
>> + return NULL;
>> +}
>> +
>> +static ssize_t uuid_show(struct kobject *obj,
>> + struct kobj_attribute *attr,
>> + char *buf)
>> +{
>> + const char *uuid = kvm_para_get_uuid();
>> + return sprintf(buf, "%s\n", uuid);
>
> The usual return value for the Xen /sys/hypervisor interface is
> "<denied>". Wouldn't it make sense to follow that pattern for the KVM
> one too? Currently, if we can not determine the UUID this will just
> return (null).
>
> Otherwise, looks good to me. Are you aware of any other files we should
> provide? Also, is there any reason not to implement ARM as well while at it?
>
> Alex

This originated from a customer request that was using /sys/hypervisor/uuid.
My guess is that we would want to expose "type" and "version" moving
forward and that's when we hypervisor hooks will be useful on top
of arch hooks.

On a different note, any idea how to check whether the OS is running
virtualized on KVM on ARM and ARM64? kvm_para_available() isn't an
option and the same is true for S390 where kvm_para_available()
always returns true and it would even if a KVM enabled kernel would
be running on bare metal.

I think we will need another arch hook to call a function that says
whether the OS is running virtualized on KVM.

>> +}
>> +
>> +static struct kobj_attribute uuid = __ATTR_RO(uuid);
>> +
>> +static int __init uuid_init(void)
>> +{
>> + if (!kvm_para_available())
>> + return 0;
>> + return sysfs_create_file(hypervisor_kobj, &uuid.attr);
>> +}
>> +
>> +device_initcall(uuid_init);




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrer: Christian Schlaeger, Ralf Herbrich
Ust-ID: DE 289 237 879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B


2019-05-31 09:08:54

by Alexander Graf

[permalink] [raw]
Subject: Re: [PATCH v2 1/2] KVM: Start populating /sys/hypervisor with KVM entries


On 17.05.19 17:41, Sironi, Filippo wrote:
>> On 16. May 2019, at 15:50, Graf, Alexander <[email protected]> wrote:
>>
>> On 14.05.19 08:16, Filippo Sironi wrote:
>>> Start populating /sys/hypervisor with KVM entries when we're running on
>>> KVM. This is to replicate functionality that's available when we're
>>> running on Xen.
>>>
>>> Start with /sys/hypervisor/uuid, which users prefer over
>>> /sys/devices/virtual/dmi/id/product_uuid as a way to recognize a virtual
>>> machine, since it's also available when running on Xen HVM and on Xen PV
>>> and, on top of that doesn't require root privileges by default.
>>> Let's create arch-specific hooks so that different architectures can
>>> provide different implementations.
>>>
>>> Signed-off-by: Filippo Sironi <[email protected]>
>> I think this needs something akin to
>>
>> https://www.kernel.org/doc/Documentation/ABI/stable/sysfs-hypervisor-xen
>>
>> to document which files are available.
>>
>>> ---
>>> v2:
>>> * move the retrieval of the VM UUID out of uuid_show and into
>>> kvm_para_get_uuid, which is a weak function that can be overwritten
>>>
>>> drivers/Kconfig | 2 ++
>>> drivers/Makefile | 2 ++
>>> drivers/kvm/Kconfig | 14 ++++++++++++++
>>> drivers/kvm/Makefile | 1 +
>>> drivers/kvm/sys-hypervisor.c | 30 ++++++++++++++++++++++++++++++
>>> 5 files changed, 49 insertions(+)
>>> create mode 100644 drivers/kvm/Kconfig
>>> create mode 100644 drivers/kvm/Makefile
>>> create mode 100644 drivers/kvm/sys-hypervisor.c
>>>
>> [...]
>>
>>> +
>>> +__weak const char *kvm_para_get_uuid(void)
>>> +{
>>> + return NULL;
>>> +}
>>> +
>>> +static ssize_t uuid_show(struct kobject *obj,
>>> + struct kobj_attribute *attr,
>>> + char *buf)
>>> +{
>>> + const char *uuid = kvm_para_get_uuid();
>>> + return sprintf(buf, "%s\n", uuid);
>> The usual return value for the Xen /sys/hypervisor interface is
>> "<denied>". Wouldn't it make sense to follow that pattern for the KVM
>> one too? Currently, if we can not determine the UUID this will just
>> return (null).
>>
>> Otherwise, looks good to me. Are you aware of any other files we should
>> provide? Also, is there any reason not to implement ARM as well while at it?
>>
>> Alex
> This originated from a customer request that was using /sys/hypervisor/uuid.
> My guess is that we would want to expose "type" and "version" moving
> forward and that's when we hypervisor hooks will be useful on top
> of arch hooks.
>
> On a different note, any idea how to check whether the OS is running
> virtualized on KVM on ARM and ARM64? kvm_para_available() isn't an


Yeah, ARM doesn't have any KVM PV FWIW. I also can't find any explicit
hint passed into guests that we are indeed running in KVM. The closest
thing I can see is the SMBIOS product identifier in QEMU which gets
patched to "KVM Virtual Machine". Maybe we'll have to do with that for
the sake of backwards compatibility ...


> option and the same is true for S390 where kvm_para_available()
> always returns true and it would even if a KVM enabled kernel would
> be running on bare metal.


For s390, you can figure the topology out using the sthyi instruction.
I'm not sure if there is a nice in-kernel API to leverage that though.
In fact, kvm_para_available() probably should check sthyi output to
determine whether we really can use it, no? Christian?


Alex


>
> I think we will need another arch hook to call a function that says
> whether the OS is running virtualized on KVM.
>
>>> +}
>>> +
>>> +static struct kobj_attribute uuid = __ATTR_RO(uuid);
>>> +
>>> +static int __init uuid_init(void)
>>> +{
>>> + if (!kvm_para_available())
>>> + return 0;
>>> + return sysfs_create_file(hypervisor_kobj, &uuid.attr);
>>> +}
>>> +
>>> +device_initcall(uuid_init);

2019-05-31 09:13:53

by KarimAllah Ahmed

[permalink] [raw]
Subject: Re: [PATCH v2 1/2] KVM: Start populating /sys/hypervisor with KVM entries

On Fri, 2019-05-31 at 11:06 +0200, Alexander Graf wrote:
> On 17.05.19 17:41, Sironi, Filippo wrote:
> >
> > >
> > > On 16. May 2019, at 15:50, Graf, Alexander <[email protected]> wrote:
> > >
> > > On 14.05.19 08:16, Filippo Sironi wrote:
> > > >
> > > > Start populating /sys/hypervisor with KVM entries when we're running on
> > > > KVM. This is to replicate functionality that's available when we're
> > > > running on Xen.
> > > >
> > > > Start with /sys/hypervisor/uuid, which users prefer over
> > > > /sys/devices/virtual/dmi/id/product_uuid as a way to recognize a virtual
> > > > machine, since it's also available when running on Xen HVM and on Xen PV
> > > > and, on top of that doesn't require root privileges by default.
> > > > Let's create arch-specific hooks so that different architectures can
> > > > provide different implementations.
> > > >
> > > > Signed-off-by: Filippo Sironi <[email protected]>
> > > I think this needs something akin to
> > >
> > > https://www.kernel.org/doc/Documentation/ABI/stable/sysfs-hypervisor-xen
> > >
> > > to document which files are available.
> > >
> > > >
> > > > ---
> > > > v2:
> > > > * move the retrieval of the VM UUID out of uuid_show and into
> > > > kvm_para_get_uuid, which is a weak function that can be overwritten
> > > >
> > > > drivers/Kconfig | 2 ++
> > > > drivers/Makefile | 2 ++
> > > > drivers/kvm/Kconfig | 14 ++++++++++++++
> > > > drivers/kvm/Makefile | 1 +
> > > > drivers/kvm/sys-hypervisor.c | 30 ++++++++++++++++++++++++++++++
> > > > 5 files changed, 49 insertions(+)
> > > > create mode 100644 drivers/kvm/Kconfig
> > > > create mode 100644 drivers/kvm/Makefile
> > > > create mode 100644 drivers/kvm/sys-hypervisor.c
> > > >
> > > [...]
> > >
> > > >
> > > > +
> > > > +__weak const char *kvm_para_get_uuid(void)
> > > > +{
> > > > + return NULL;
> > > > +}
> > > > +
> > > > +static ssize_t uuid_show(struct kobject *obj,
> > > > + struct kobj_attribute *attr,
> > > > + char *buf)
> > > > +{
> > > > + const char *uuid = kvm_para_get_uuid();
> > > > + return sprintf(buf, "%s\n", uuid);
> > > The usual return value for the Xen /sys/hypervisor interface is
> > > "<denied>". Wouldn't it make sense to follow that pattern for the KVM
> > > one too? Currently, if we can not determine the UUID this will just
> > > return (null).
> > >
> > > Otherwise, looks good to me. Are you aware of any other files we should
> > > provide? Also, is there any reason not to implement ARM as well while at it?
> > >
> > > Alex
> > This originated from a customer request that was using /sys/hypervisor/uuid.
> > My guess is that we would want to expose "type" and "version" moving
> > forward and that's when we hypervisor hooks will be useful on top
> > of arch hooks.
> >
> > On a different note, any idea how to check whether the OS is running
> > virtualized on KVM on ARM and ARM64? kvm_para_available() isn't an
>
>
> Yeah, ARM doesn't have any KVM PV FWIW. I also can't find any explicit
> hint passed into guests that we are indeed running in KVM. The closest
> thing I can see is the SMBIOS product identifier in QEMU which gets
> patched to "KVM Virtual Machine". Maybe we'll have to do with that for
> the sake of backwards compatibility ...

How about "psci_ops.conduit" (PSCI_CONDUIT_HVC vs PSCI_CONDUIT_SMC)?

>
>
> >
> > option and the same is true for S390 where kvm_para_available()
> > always returns true and it would even if a KVM enabled kernel would
> > be running on bare metal.
>
>
> For s390, you can figure the topology out using the sthyi instruction.
> I'm not sure if there is a nice in-kernel API to leverage that though.
> In fact, kvm_para_available() probably should check sthyi output to
> determine whether we really can use it, no? Christian?
>
>
> Alex
>
>
> >
> >
> > I think we will need another arch hook to call a function that says
> > whether the OS is running virtualized on KVM.
> >
> > >
> > > >
> > > > +}
> > > > +
> > > > +static struct kobj_attribute uuid = __ATTR_RO(uuid);
> > > > +
> > > > +static int __init uuid_init(void)
> > > > +{
> > > > + if (!kvm_para_available())
> > > > + return 0;
> > > > + return sysfs_create_file(hypervisor_kobj, &uuid.attr);
> > > > +}
> > > > +
> > > > +device_initcall(uuid_init);



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879


2019-05-31 09:28:24

by Alexander Graf

[permalink] [raw]
Subject: Re: [PATCH v2 1/2] KVM: Start populating /sys/hypervisor with KVM entries


On 31.05.19 11:12, Raslan, KarimAllah wrote:
> On Fri, 2019-05-31 at 11:06 +0200, Alexander Graf wrote:
>> On 17.05.19 17:41, Sironi, Filippo wrote:
>>>> On 16. May 2019, at 15:50, Graf, Alexander <[email protected]> wrote:
>>>>
>>>> On 14.05.19 08:16, Filippo Sironi wrote:
>>>>> Start populating /sys/hypervisor with KVM entries when we're running on
>>>>> KVM. This is to replicate functionality that's available when we're
>>>>> running on Xen.
>>>>>
>>>>> Start with /sys/hypervisor/uuid, which users prefer over
>>>>> /sys/devices/virtual/dmi/id/product_uuid as a way to recognize a virtual
>>>>> machine, since it's also available when running on Xen HVM and on Xen PV
>>>>> and, on top of that doesn't require root privileges by default.
>>>>> Let's create arch-specific hooks so that different architectures can
>>>>> provide different implementations.
>>>>>
>>>>> Signed-off-by: Filippo Sironi <[email protected]>
>>>> I think this needs something akin to
>>>>
>>>> https://www.kernel.org/doc/Documentation/ABI/stable/sysfs-hypervisor-xen
>>>>
>>>> to document which files are available.
>>>>
>>>>> ---
>>>>> v2:
>>>>> * move the retrieval of the VM UUID out of uuid_show and into
>>>>> kvm_para_get_uuid, which is a weak function that can be overwritten
>>>>>
>>>>> drivers/Kconfig | 2 ++
>>>>> drivers/Makefile | 2 ++
>>>>> drivers/kvm/Kconfig | 14 ++++++++++++++
>>>>> drivers/kvm/Makefile | 1 +
>>>>> drivers/kvm/sys-hypervisor.c | 30 ++++++++++++++++++++++++++++++
>>>>> 5 files changed, 49 insertions(+)
>>>>> create mode 100644 drivers/kvm/Kconfig
>>>>> create mode 100644 drivers/kvm/Makefile
>>>>> create mode 100644 drivers/kvm/sys-hypervisor.c
>>>>>
>>>> [...]
>>>>
>>>>> +
>>>>> +__weak const char *kvm_para_get_uuid(void)
>>>>> +{
>>>>> + return NULL;
>>>>> +}
>>>>> +
>>>>> +static ssize_t uuid_show(struct kobject *obj,
>>>>> + struct kobj_attribute *attr,
>>>>> + char *buf)
>>>>> +{
>>>>> + const char *uuid = kvm_para_get_uuid();
>>>>> + return sprintf(buf, "%s\n", uuid);
>>>> The usual return value for the Xen /sys/hypervisor interface is
>>>> "<denied>". Wouldn't it make sense to follow that pattern for the KVM
>>>> one too? Currently, if we can not determine the UUID this will just
>>>> return (null).
>>>>
>>>> Otherwise, looks good to me. Are you aware of any other files we should
>>>> provide? Also, is there any reason not to implement ARM as well while at it?
>>>>
>>>> Alex
>>> This originated from a customer request that was using /sys/hypervisor/uuid.
>>> My guess is that we would want to expose "type" and "version" moving
>>> forward and that's when we hypervisor hooks will be useful on top
>>> of arch hooks.
>>>
>>> On a different note, any idea how to check whether the OS is running
>>> virtualized on KVM on ARM and ARM64? kvm_para_available() isn't an
>>
>> Yeah, ARM doesn't have any KVM PV FWIW. I also can't find any explicit
>> hint passed into guests that we are indeed running in KVM. The closest
>> thing I can see is the SMBIOS product identifier in QEMU which gets
>> patched to "KVM Virtual Machine". Maybe we'll have to do with that for
>> the sake of backwards compatibility ...
> How about "psci_ops.conduit" (PSCI_CONDUIT_HVC vs PSCI_CONDUIT_SMC)?


This won't work for 2 reasons:

  a) You don't know it's KVM. You only know you might be running in EL1.
  b) KVM may choose to just use SMC for PSCI going forward and trap on it.


Alex


2019-05-31 09:40:27

by Marc Zyngier

[permalink] [raw]
Subject: Re: [PATCH v2 1/2] KVM: Start populating /sys/hypervisor with KVM entries

On Fri, 31 May 2019 10:12:03 +0100,
"Raslan, KarimAllah" <[email protected]> wrote:
>
> On Fri, 2019-05-31 at 11:06 +0200, Alexander Graf wrote:
> > On 17.05.19 17:41, Sironi, Filippo wrote:
> > >
> > > >
> > > > On 16. May 2019, at 15:50, Graf, Alexander <[email protected]> wrote:
> > > >
> > > > On 14.05.19 08:16, Filippo Sironi wrote:
> > > > >
> > > > > Start populating /sys/hypervisor with KVM entries when we're running on
> > > > > KVM. This is to replicate functionality that's available when we're
> > > > > running on Xen.
> > > > >
> > > > > Start with /sys/hypervisor/uuid, which users prefer over
> > > > > /sys/devices/virtual/dmi/id/product_uuid as a way to recognize a virtual
> > > > > machine, since it's also available when running on Xen HVM and on Xen PV
> > > > > and, on top of that doesn't require root privileges by default.
> > > > > Let's create arch-specific hooks so that different architectures can
> > > > > provide different implementations.
> > > > >
> > > > > Signed-off-by: Filippo Sironi <[email protected]>
> > > > I think this needs something akin to
> > > >
> > > > https://www.kernel.org/doc/Documentation/ABI/stable/sysfs-hypervisor-xen
> > > >
> > > > to document which files are available.
> > > >
> > > > >
> > > > > ---
> > > > > v2:
> > > > > * move the retrieval of the VM UUID out of uuid_show and into
> > > > > kvm_para_get_uuid, which is a weak function that can be overwritten
> > > > >
> > > > > drivers/Kconfig | 2 ++
> > > > > drivers/Makefile | 2 ++
> > > > > drivers/kvm/Kconfig | 14 ++++++++++++++
> > > > > drivers/kvm/Makefile | 1 +
> > > > > drivers/kvm/sys-hypervisor.c | 30 ++++++++++++++++++++++++++++++
> > > > > 5 files changed, 49 insertions(+)
> > > > > create mode 100644 drivers/kvm/Kconfig
> > > > > create mode 100644 drivers/kvm/Makefile
> > > > > create mode 100644 drivers/kvm/sys-hypervisor.c
> > > > >
> > > > [...]
> > > >
> > > > >
> > > > > +
> > > > > +__weak const char *kvm_para_get_uuid(void)
> > > > > +{
> > > > > + return NULL;
> > > > > +}
> > > > > +
> > > > > +static ssize_t uuid_show(struct kobject *obj,
> > > > > + struct kobj_attribute *attr,
> > > > > + char *buf)
> > > > > +{
> > > > > + const char *uuid = kvm_para_get_uuid();
> > > > > + return sprintf(buf, "%s\n", uuid);
> > > > The usual return value for the Xen /sys/hypervisor interface is
> > > > "<denied>". Wouldn't it make sense to follow that pattern for the KVM
> > > > one too? Currently, if we can not determine the UUID this will just
> > > > return (null).
> > > >
> > > > Otherwise, looks good to me. Are you aware of any other files we should
> > > > provide? Also, is there any reason not to implement ARM as well while at it?
> > > >
> > > > Alex
> > > This originated from a customer request that was using /sys/hypervisor/uuid.
> > > My guess is that we would want to expose "type" and "version" moving
> > > forward and that's when we hypervisor hooks will be useful on top
> > > of arch hooks.
> > >
> > > On a different note, any idea how to check whether the OS is running
> > > virtualized on KVM on ARM and ARM64? kvm_para_available() isn't an
> >
> >
> > Yeah, ARM doesn't have any KVM PV FWIW. I also can't find any explicit
> > hint passed into guests that we are indeed running in KVM. The closest
> > thing I can see is the SMBIOS product identifier in QEMU which gets
> > patched to "KVM Virtual Machine". Maybe we'll have to do with that for
> > the sake of backwards compatibility ...
>
> How about "psci_ops.conduit" (PSCI_CONDUIT_HVC vs PSCI_CONDUIT_SMC)?

[changing Christoffer address for one that actually]

That's not enough. HVC only tells you about the fact that you are
running under a hypervisor without telling you which one, and doesn't
cater for nested virt. It doesn't tell you anything about a hypervisor
that doesn't use HVC at all (it could only advertise SMC, for example).

If you want to identify the hypervisor, don't guess. Use the SMCCC
discovery mechanism, and make KVM identify itself as the hypervisor. I
have some code for that stashed at [1] as part of an unrelated series,
which I may post at some point.

Thanks,

M.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/log/?h=kvm-arm64/pvcy

--
Jazz is not dead, it just smell funny.