drivers/misc: vm_gen_counter: initial driver implementation
PATCH v5 1/1 drivers/misc: vm_gen_counter: initial driver implementation
From: Or Idgar <[email protected]>
This patch is a driver which expose the Virtual Machine Generation ID via sysfs. The ID is a UUID value used to differentiate between virtual machines.
The VM-Generation ID is a feature defined by Microsoft (paper:
http://go.microsoft.com/fwlink/?LinkId=260709) and supported by multiple hypervisor vendors.
---
Changes in v5:
- added to VMGENID module dependency on ACPI module.
Documentation/ABI/testing/sysfs-hypervisor | 13 +++ drivers/misc/Kconfig | 7 ++ drivers/misc/Makefile | 1 + drivers/misc/vmgenid.c | 142 +++++++++++++++++++++++++++++ 4 files changed, 163 insertions(+) create mode 100644 Documentation/ABI/testing/sysfs-hypervisor create mode 100644 drivers/misc/vmgenid.c
Documentation/ABI/testing/sysfs-hypervisor new file +13 -0
@@ -0,0 +1,13 @@ +What: /sys/hypervisor/vm_gen_counter +Date: February 2018 +Contact: Or Idgar <[email protected]> + Gal Hammer <[email protected]> +Description: Expose the virtual machine generation ID. The directory + contains two files: "generation_id" and "raw". Both files + represent the same information. + + "generation_id" file is a UUID string + representation. + + "raw" file is a 128-bit integer + representation (binary).
drivers/misc/Kconfig +7 -0
@@ -500,6 +500,13 @@ config MISC_RTSX tristate default MISC_RTSX_PCI || MISC_RTSX_USB +config VMGENID + depends on ACPI + tristate "Virtual Machine Generation ID driver" + help + This is a Virtual Machine Generation ID driver which provides + a virtual machine unique identifier. + source "drivers/misc/c2port/Kconfig" source "drivers/misc/eeprom/Kconfig" source "drivers/misc/cb710/Kconfig"
drivers/misc/Makefile +1 -0
@@ -57,6 +57,7 @@ obj-$(CONFIG_ASPEED_LPC_SNOOP) += aspeed-lpc-snoop.o obj-$(CONFIG_PCI_ENDPOINT_TEST) += pci_endpoint_test.o obj-$(CONFIG_OCXL) += ocxl/ obj-$(CONFIG_MISC_RTSX) += cardreader/ +obj-$(CONFIG_VMGENID) += vmgenid.o lkdtm-$(CONFIG_LKDTM) += lkdtm_core.o lkdtm-$(CONFIG_LKDTM) += lkdtm_bugs.o
drivers/misc/vmgenid.c new file +142 -0
142 lines not on this page · raw message
Signature · 1 line
2.14.3
Thanks for the patch! Yet something to improve (see below):
Quoted 1 line · Or Idgar
From: Or Idgar <[email protected]>
I see addresses at gmail, virtualoco and redhat.com At this point I don't really know which address to use to contact you :) All of them?
Also, I think it's a good idea to CC this more widely. Consider CC-ing qemu contributors to the vm gen id (use git log to get the list), Ben Warren who wrote a prototype driver a while ago, qemu mailing list, maybe more.
Quoted 4 lines
This patch is a driver which expose the Virtual Machine Generation ID via sysfs.
Quoted 8 lines
The ID is a UUID value used to differentiate between virtual machines.
The VM-Generation ID is a feature defined by Microsoft (paper:
http://go.microsoft.com/fwlink/?LinkId=260709) and supported by multiple hypervisor vendors.Signed-off-by Or Idgar <[email protected]>
I think it's a good idea to use sysfs for this. However, there are a couple of missing interfaces here:
1. Userspace needs a way to know when this value changes. I see no change notifications here and that does not seem right.
2. Userspace needs to be able to read these without system calls. Pls add mmap support to the raw format. (Phys address is not guaranteed to be page-aligned so you will probably want an offset attribute for that as well).
While it's possible to add these later in theory, it's easier if userspace can rely on all interfaces being in place just by detecting the directory presence.
Quoted 9 lines
---
Changes in v5:
- added to VMGENID module dependency on ACPI module.Documentation/ABI/testing/sysfs-hypervisor | 13 +++ drivers/misc/Kconfig | 7 ++ drivers/misc/Makefile | 1 + drivers/misc/vmgenid.c | 142 +++++++++++++++++++++++++++++
Do you want to add this under QEMU MACHINE EMULATOR in MAINTAINERS? This way e.g. qemu-devel will be copied.
Documentation/ABI/testing/sysfs-hypervisor line 1
Quoted 11 lines
4 files changed, 163 insertions(+) create mode 100644 Documentation/ABI/testing/sysfs-hypervisor create mode 100644 drivers/misc/vmgenid.cDocumentation/ABI/testing/sysfs-hypervisor new file +1
-0@@ -0,0 +1,13 @@ +What: /sys/hypervisor/vm_gen_counter
It's not a counter, is it?
I'd go with "vm_generation_id" here. Eschew abbreviation.
Documentation/ABI/testing/sysfs-hypervisor line 9
Quoted 8 lines
Documentation/ABI/testing/sysfs-hypervisor new file +8
-0+Date: February 2018 +Contact: Or Idgar <[email protected]> + Gal Hammer <[email protected]> +Description: Expose the virtual machine generation ID. The directory + contains two files: "generation_id" and "raw". Both files + represent the same information. + + "generation_id" file is a UUID string
And this I'd call "uuid" then.
Quoted 67 lines
Documentation/ABI/testing/sysfs-hypervisor new file +4
-0+ representation. + + "raw" file is a 128-bit integer + representation (binary).drivers/misc/Kconfig +7
-0@@ -500,6 +500,13 @@ config MISC_RTSX tristate default MISC_RTSX_PCI || MISC_RTSX_USB +config VMGENID + depends on ACPI + tristate "Virtual Machine Generation ID driver" + help + This is a Virtual Machine Generation ID driver which provides + a virtual machine unique identifier. + source "drivers/misc/c2port/Kconfig" source "drivers/misc/eeprom/Kconfig" source "drivers/misc/cb710/Kconfig"drivers/misc/Makefile +1
-0@@ -57,6 +57,7 @@ obj-$(CONFIG_ASPEED_LPC_SNOOP) += aspeed-lpc-snoop.o obj-$(CONFIG_PCI_ENDPOINT_TEST) += pci_endpoint_test.o obj-$(CONFIG_OCXL) += ocxl/ obj-$(CONFIG_MISC_RTSX) += cardreader/ +obj-$(CONFIG_VMGENID) += vmgenid.o lkdtm-$(CONFIG_LKDTM) += lkdtm_core.o lkdtm-$(CONFIG_LKDTM) += lkdtm_bugs.odrivers/misc/vmgenid.c new file +27
-0@@ -0,0 +1,142 @@ +// SPDX-License-Identifier: GPL-2.0 +/* +* Virtual Machine Generation ID driver +* +* Copyright (C) 2018 Red Hat, Inc. All rights reserved. +* Authors: +* Or Idgar <[email protected]> +* Gal Hammer <[email protected]> +* +*/ +#include <linux/init.h> +#include <linux/module.h> +#include <linux/kernel.h> +#include <linux/kobject.h> +#include <linux/acpi.h> +#include <linux/uuid.h> + +MODULE_LICENSE ("GPL" ); +MODULE_AUTHOR ("Or Idgar <[email protected]>" ); +MODULE_AUTHOR ("Gal Hammer <[email protected]>" ); +MODULE_DESCRIPTION ("Virtual Machine Generation ID" ); +MODULE_VERSION ("0.1" ); + +ACPI_MODULE_NAME ("vmgenid" ); + +static u64 phys_addr; +
drivers/misc/vmgenid.c line 37
Quoted 10 lines
drivers/misc/vmgenid.c new file +10
-0+static ssize_t generation_id_show (struct device *_d, +struct device_attribute *attr,char *buf) +{ +void __iomem *uuid_map; +uuid_t uuid; +ssize_t result; + + uuid_map =acpi_os_map_iomem (phys_addr,sizeof (uuid_t)); +if (!uuid_map) +return -EFAULT ;
Shouldn't this be acpi_os_map_memory? Spec says it's never an IO address.
Quoted 12 lines
drivers/misc/vmgenid.c new file +12
-0+ +memcpy_fromio (&uuid, uuid_map,sizeof (uuid_t)); + result =sprintf (buf,"%pUl\n" , &uuid); +acpi_os_unmap_iomem (uuid_map,sizeof (uuid_t)); +return result; +} +static DEVICE_ATTR_RO (generation_id); + +static ssize_t raw_show (struct device *_d, +struct device_attribute *attr, +char *buf) +{
drivers/misc/vmgenid.c line 59
Quoted 10 lines
drivers/misc/vmgenid.c new file +10
-0+void __iomem *uuid_map; + + uuid_map =acpi_os_map_iomem (phys_addr,sizeof (uuid_t)); +if (!uuid_map) +return -EFAULT ; +memcpy_fromio (buf, uuid_map,sizeof (uuid_t)); +acpi_os_unmap_iomem (uuid_map,sizeof (uuid_t)); +return sizeof (uuid_t); +} +static DEVICE_ATTR_RO (raw);
I think you want BIN_ATTR_RO.
Quoted 12 lines
drivers/misc/vmgenid.c new file +12
-0+ +static struct attribute *vmgenid_attrs[] = { + &dev_attr_generation_id.attr, + &dev_attr_raw.attr, +NULL , +}; +static const struct attribute_group vmgenid_group = { + .name ="vm_gen_counter" , + .attrs = vmgenid_attrs, +}; + +static int get_vmgenid (acpi_handle handle)
drivers/misc/vmgenid.c line 81
Quoted 10 lines
drivers/misc/vmgenid.c new file +10
-0+{ +int i; +struct acpi_buffer buffer = {ACPI_ALLOCATE_BUFFER ,NULL }; +acpi_status status; +union acpi_object *pss; +union acpi_object *element; + + status =acpi_evaluate_object (handle,"ADDR" ,NULL , &buffer); +if (ACPI_FAILURE (status)) { +ACPI_EXCEPTION ((AE_INFO , status,"Evaluating _ADDR" ));
It's ADDR - not _ADDR, right?
drivers/misc/vmgenid.c line 92
Quoted 11 lines
drivers/misc/vmgenid.c new file +11
-0+return -ENODEV ; + } + pss = buffer.pointer; +if (!pss || pss->type !=ACPI_TYPE_PACKAGE || pss->package.count !=2 ) +return -EFAULT ; + + phys_addr =0 ; +for (i =0 ; i < pss->package.count; i++) { + element = &(pss->package.elements[i]); +if (element->type !=ACPI_TYPE_INTEGER ) +return -EFAULT ;
EINVAL here and elsewhere.
drivers/misc/vmgenid.c line 93
Quoted 1 line
drivers/misc/vmgenid.c new file +1
-0+ phys_addr |= element->integer.value << i*32 ;
i * 32
Quoted 16 lines
drivers/misc/vmgenid.c new file +16
-0+ } +return 0 ; +} + +static int acpi_vmgenid_add (struct acpi_device *device) +{ +int retval; + +if (!device) +return -EINVAL ; + retval =get_vmgenid (device->handle); +if (retval <0 ) +return retval; +return sysfs_create_group (hypervisor_kobj, &vmgenid_group); +} +
drivers/misc/vmgenid.c line 119
Quoted 10 lines
drivers/misc/vmgenid.c new file +10
-0+static int acpi_vmgenid_remove (struct acpi_device *device) +{ +sysfs_remove_group (hypervisor_kobj, &vmgenid_group); +return 0 ; +} + +static const struct acpi_device_id vmgenid_ids[] = { + {"QEMUVGID" ,0 }, + {"" ,0 }, +};
That's not right I think. You should go by _CID and maybe _DDN.
Quoted 14 lines
drivers/misc/vmgenid.c new file +13
-0+ +static struct acpi_driver acpi_vmgenid_driver = { + .name ="vm_gen_counter" , + .ids = vmgenid_ids, + .owner =THIS_MODULE , + .ops = { + .add = acpi_vmgenid_add, + .remove = acpi_vmgenid_remove, + } +}; + +static int __init vmgenid_init (void ) +{
drivers/misc/vmgenid.c line 142
Quoted 10 lines
drivers/misc/vmgenid.c new file +10
-0+return acpi_bus_register_driver (&acpi_vmgenid_driver); +} + +static void __exit vmgenid_exit(void) +{ +acpi_bus_unregister_driver (&acpi_vmgenid_driver); +} + +module_init (vmgenid_init); +module_exit (vmgenid_exit);
Thanks for your work, and I hope this helps!
Signature · 1 line
MST
Greg KH
maintainerQuoted 5 lines · Michael S. Tsirkin
I think it's a good idea to use sysfs for this. However, there are a couple of missing interfaces here:
1. Userspace needs a way to know when this value changes. I see no change notifications here and that does not seem right.
How can these change?
Quoted 2 lines
2. Userspace needs to be able to read these without system calls.
Ick, what? Why not?
Quoted 1 line
Pls add mmap support to the raw format.
For a single integer? Why do you need mmap for this? What is so "performant" that needs to touch a sysfs file?
Quoted 2 lines
(Phys address is not guaranteed to be page-aligned so you will
probably want an offset attribute for that as well).
Ick ick ick, that's why it's good to just stick with a sysfs file.
Have you tested just how long this takes to see if the open/read/close is really the bottleneck, or if the io on reading the value is the bottleneck?
thanks,
greg k-h
Quoted 8 lines · Greg KH
Quoted 5 lines · Michael S. Tsirkin
I think it's a good idea to use sysfs for this. However, there are a couple of missing interfaces here:
1. Userspace needs a way to know when this value changes. I see no change notifications here and that does not seem right.
How can these change?
It's a hardware register. It changes when hardware feels like it :) In particular, it changes whenever VM is migrated or snapshotted.
Quoted 14 lines
Quoted 2 lines
2. Userspace needs to be able to read these without system calls.
Ick, what? Why not?
Quoted 1 line
Pls add mmap support to the raw format.
For a single integer? Why do you need mmap for this? What is so "performant" that needs to touch a sysfs file?
Quoted 2 lines
(Phys address is not guaranteed to be page-aligned so you will
probably want an offset attribute for that as well).Ick ick ick, that's why it's good to just stick with a sysfs file.
Quoted 7 lines
Have you tested just how long this takes to see if the open/read/close is really the bottleneck, or if the io on reading the value is the bottleneck?
thanks,
greg k-h
Well an application needs to check this value basically after every database transaction. So I'm pretty sure it's a performance sensitive path. But yes, I didn't profile any apps since they are yet to be written to use this interface.
I'm fine deferring point 2 for now.
Signature · 1 line
MST
Quoted 5 lines · Michael S. Tsirkin
Quoted 4 lines · Greg KH
Quoted 3 lines · Michael S. Tsirkin
I think it's a good idea to use sysfs for this. However, there are a couple of missing interfaces here:
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
1. Userspace needs a way to know when this value changes. I see no change notifications here and that does not seem right.
How can these change?
It's a hardware register. It changes when hardware feels like it :) In particular, it changes whenever VM is migrated or snapshotted.
This value doesn't change when a VM is migrated. It is unlikely that this value will be changed so frequently that a direct access to the memory is required. Even in QEMU, the current implementation was merged without an option to change the generation id on-the-fly. One must run a new instance in order to set a new value, which means that no application will be running during that time.
Quoted 5 lines
Quoted 5 lines
Quoted 2 lines
2. Userspace needs to be able to read these without system calls.
Ick, what? Why not?
Quoted 8 lines
Quoted 8 lines
Quoted 1 line
Pls add mmap support to the raw format.
For a single integer? Why do you need mmap for this? What is so "performant" that needs to touch a sysfs file?
Quoted 2 lines
(Phys address is not guaranteed to be page-aligned so you will
probably want an offset attribute for that as well).Ick ick ick, that's why it's good to just stick with a sysfs file.
I agree with Greg here. The user is able to read the value, and then wait for a notification if she cares about changes.
Quoted 8 lines
Quoted 7 lines
Have you tested just how long this takes to see if the open/read/close is really the bottleneck, or if the io on reading the value is the bottleneck?
thanks,
greg k-h
Quoted 8 lines
Well an application needs to check this value basically after every database transaction. So I'm pretty sure it's a performance sensitive path. But yes, I didn't profile any apps since they are yet to be written to use this interface.
I'm fine deferring point 2 for now.--
MST
Thanks,
Gal.
Quoted 14 lines · Gal Hammer
Quoted 5 lines · Michael S. Tsirkin
Quoted 4 lines · Greg KH
Quoted 3 lines · Michael S. Tsirkin
I think it's a good idea to use sysfs for this. However, there are a couple of missing interfaces here:
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
1. Userspace needs a way to know when this value changes. I see no change notifications here and that does not seem right.
How can these change?
It's a hardware register. It changes when hardware feels like it :) In particular, it changes whenever VM is migrated or snapshotted.
Quoted 6 lines
This value doesn't change when a VM is migrated. It is unlikely that this value will be changed so frequently that a direct access to the memory is required. Even in QEMU, the current implementation was merged without an option to change the generation id on-the-fly. One must run a new instance in order to set a new value, which means that no application will be running during that time.
The point is still that it changes without an application or the kernel doing anything.
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 2 lines
2. Userspace needs to be able to read these without system calls.
Ick, what? Why not?
Quoted 1 line
Pls add mmap support to the raw format.
For a single integer? Why do you need mmap for this? What is so "performant" that needs to touch a sysfs file?
Quoted 7 lines
Quoted 4 lines
Quoted 4 lines
Quoted 2 lines
(Phys address is not guaranteed to be page-aligned so you will
probably want an offset attribute for that as well).Ick ick ick, that's why it's good to just stick with a sysfs file.
I agree with Greg here. The user is able to read the value, and then wait for a notification if she cares about changes.
OK. Patch has to implement notifications for this to work though. That's missing.
Quoted 20 lines
Quoted 8 lines
Quoted 7 lines
Have you tested just how long this takes to see if the open/read/close is really the bottleneck, or if the io on reading the value is the bottleneck?
thanks,
greg k-h
Quoted 8 lines
Well an application needs to check this value basically after every database transaction. So I'm pretty sure it's a performance sensitive path. But yes, I didn't profile any apps since they are yet to be written to use this interface.
I'm fine deferring point 2 for now.--
MSTThanks,
Gal.
Greg KH
maintainerQuoted 11 lines · Michael S. Tsirkin
Quoted 8 lines · Greg KH
Quoted 5 lines · Michael S. Tsirkin
I think it's a good idea to use sysfs for this. However, there are a couple of missing interfaces here:
1. Userspace needs a way to know when this value changes. I see no change notifications here and that does not seem right.
How can these change?
It's a hardware register. It changes when hardware feels like it :)
Does the hardware notify the kernel that it changes? Or does the kernel have to "poll" for it?
Quoted 1 line
In particular, it changes whenever VM is migrated or snapshotted.
So very rarely. And userspace always knows about those events already, right?
Quoted 18 lines
Quoted 18 lines
Quoted 2 lines
2. Userspace needs to be able to read these without system calls.
Ick, what? Why not?
Quoted 1 line
Pls add mmap support to the raw format.
For a single integer? Why do you need mmap for this? What is so "performant" that needs to touch a sysfs file?
Quoted 2 lines
(Phys address is not guaranteed to be page-aligned so you will
probably want an offset attribute for that as well).Ick ick ick, that's why it's good to just stick with a sysfs file.
Have you tested just how long this takes to see if the open/read/close is really the bottleneck, or if the io on reading the value is the bottleneck?
Quoted 6 lines
Quoted 3 lines
thanks,
greg k-h
Well an application needs to check this value basically after every database transaction.
"every"? That's horrid, why would you write a database that has to do an ACPI i/o call for every transaction? That's a sure way to write a very slow database :(
Quoted 1 line
So I'm pretty sure it's a performance sensitive path.
Given that this api is not present today, why is this even needed? Who wants/needs it so badly that it has to be tuned in ways like this?
If it is _really_ performant critical, just make it a new syscall :)
Quoted 3 lines
But yes, I
didn't profile any apps since they
are yet to be written to use this interface.
Then what database are you talking about? What apps need/want this thing?
thanks,
greg k-h
Quoted 9 lines · Greg KH
Quoted 8 lines · Michael S. Tsirkin
Quoted 7 lines · Greg KH
Quoted 5 lines · Michael S. Tsirkin
I think it's a good idea to use sysfs for this. However, there are a couple of missing interfaces here:
1. Userspace needs a way to know when this value changes. I see no change notifications here and that does not seem right.
Quoted 6 lines
Quoted 3 lines
Quoted 1 line
How can these change?
It's a hardware register. It changes when hardware feels like it :)
Does the hardware notify the kernel that it changes? Or does the kernel have to "poll" for it?
Yes - it sends an ACPI interrupt notification.
Quoted 4 lines
Quoted 1 line
In particular, it changes whenever VM is migrated or snapshotted.
So very rarely. And userspace always knows about those events already, right?
Not at all.
Quoted 20 lines
Quoted 20 lines
Quoted 20 lines
Quoted 2 lines
2. Userspace needs to be able to read these without system calls.
Ick, what? Why not?
Quoted 1 line
Pls add mmap support to the raw format.
For a single integer? Why do you need mmap for this? What is so "performant" that needs to touch a sysfs file?
Quoted 2 lines
(Phys address is not guaranteed to be page-aligned so you will
probably want an offset attribute for that as well).Ick ick ick, that's why it's good to just stick with a sysfs file.
Have you tested just how long this takes to see if the open/read/close is really the bottleneck, or if the io on reading the value is the bottleneck?
thanks,
Quoted 8 lines
Quoted 4 lines
Quoted 1 line
greg k-h
Well an application needs to check this value basically after every database transaction.
"every"? That's horrid, why would you write a database that has to do an ACPI i/o call for every transaction? That's a sure way to write a very slow database :(
Absolutely. That's why we might want to do an mmap and then get the ID from memory. An alternative is poll support so userspace can get notified about changes. Opens a bigger window during which you are doing duplicate work, but maybe that's not such a big issue.
Quoted 6 lines
Quoted 1 line
So I'm pretty sure it's a performance sensitive path.
Given that this api is not present today, why is this even needed? Who wants/needs it so badly that it has to be tuned in ways like this?
If it is _really_ performant critical, just make it a new syscall :)
Maybe you are right and it is a premature optimization. Let's put it out there without mmap support and see what happens - but then we definitely need poll support.
Quoted 10 lines
Quoted 3 lines
But yes, I
didn't profile any apps since they
are yet to be written to use this interface.Then what database are you talking about? What apps need/want this thing?
thanks,
greg k-h
Anything that runs within a VM that is snapshoted is at risk of sending duplicate transactions when it's restored and time rolls back to a random point in the past.
If you want the application to have ability to detect these, then VM gen ID offers a way to do it:
id=read_id() do_work() new_id=read_id() if (new_id != id) find_and_handle_duplicate_work()
Signature · 1 line
MST
Quoted 1 line · Michael S. Tsirkin
Thanks for the patch! Yet something to improve (see below):
Thanks for the review.
Quoted 5 lines · Or Idgar
Quoted 1 line
From: Or Idgar <[email protected]>
I see addresses at gmail, virtualoco and redhat.com At this point I don't really know which address to use to contact you :) All of them?
Use his gmail or virtualoco one.
Quoted 16 lines
Also, I think it's a good idea to CC this more widely. Consider CC-ing qemu contributors to the vm gen id (use git log to get the list), Ben Warren who wrote a prototype driver a while ago, qemu mailing list, maybe more.
Quoted 11 lines
This patch is a driver which expose the Virtual Machine Generation ID via sysfs.
The ID is a UUID value used to differentiate between virtual machines.
The VM-Generation ID is a feature defined by Microsoft (paper:
http://go.microsoft.com/fwlink/?LinkId=260709) and supported by multiple hypervisor vendors.
Quoted 7 lines
Quoted 1 line
Signed-off-by Or Idgar <[email protected]>I think it's a good idea to use sysfs for this. However, there are a couple of missing interfaces here:
1. Userspace needs a way to know when this value changes. I see no change notifications here and that does not seem right.
We have the next version which includes notification. The problem is that right now QEMU doesn't include a mean to change the generation id on-the-fly, so it was not published it.
Quoted 4 lines
2. Userspace needs to be able to read these without system calls. Pls add mmap support to the raw format. (Phys address is not guaranteed to be page-aligned so you will probably want an offset attribute for that as well).
I don't agree that this should be a requirement. According to the specs, the value doesn't change frequently. A notification about a change the re-reading the value should be enough for now.
Quoted 9 lines
While it's possible to add these later in theory, it's easier if userspace can rely on all interfaces being in place just by detecting the directory presence.
Quoted 5 lines
---
Changes in v5:
- added to VMGENID module dependency on ACPI module.
Quoted 7 lines
Quoted 4 lines
Documentation/ABI/testing/sysfs-hypervisor | 13 +++ drivers/misc/Kconfig | 7 ++ drivers/misc/Makefile | 1 + drivers/misc/vmgenid.c | 142 +++++++++++++++++++++++++++++Do you want to add this under QEMU MACHINE EMULATOR in
MAINTAINERS? This way e.g. qemu-devel will be copied.
This feature is supported by other hypervisors and this driver should be able to support them in the next versions. I don't see a reason to bound it to QEMU.
Quoted 4 lines
Quoted 4 lines
4 files changed, 163 insertions(+) create mode 100644 Documentation/ABI/testing/sysfs-hypervisor create mode 100644 drivers/misc/vmgenid.c
Quoted 10 lines
Documentation/ABI/testing/sysfs-hypervisor line 1
Quoted 7 lines
Documentation/ABI/testing/sysfs-hypervisor new file +1
-0@@ -0,0 +1,13 @@ +What: /sys/hypervisor/vm_gen_counterIt's not a counter, is it?
I'd go with "vm_generation_id" here. Eschew abbreviation.
The names were chosen so they'll match Microsoft's specification and code examples.
Quoted 10 lines
Documentation/ABI/testing/sysfs-hypervisor line 9
Quoted 8 lines
Documentation/ABI/testing/sysfs-hypervisor new file +8
-0+Date: February 2018 +Contact: Or Idgar <[email protected]> + Gal Hammer <[email protected]> +Description: Expose the virtual machine generation ID. The directory + contains two files: "generation_id" and "raw". Both files + represent the same information. + + "generation_id" file is a UUID stringAnd this I'd call "uuid" then.
Why? The name says what the value is, not its type. This is not common to see values' types in the sysfs directory.
Quoted 170 lines
Quoted 67 lines
Documentation/ABI/testing/sysfs-hypervisor new file +4
-0+ representation. + + "raw" file is a 128-bit integer + representation (binary).drivers/misc/Kconfig +7
-0@@ -500,6 +500,13 @@ config MISC_RTSX tristate default MISC_RTSX_PCI || MISC_RTSX_USB +config VMGENID + depends on ACPI + tristate "Virtual Machine Generation ID driver" + help + This is a Virtual Machine Generation ID driver which provides + a virtual machine unique identifier. + source "drivers/misc/c2port/Kconfig" source "drivers/misc/eeprom/Kconfig" source "drivers/misc/cb710/Kconfig"drivers/misc/Makefile +1
-0@@ -57,6 +57,7 @@ obj-$(CONFIG_ASPEED_LPC_SNOOP) += aspeed-lpc-snoop.o obj-$(CONFIG_PCI_ENDPOINT_TEST) += pci_endpoint_test.o obj-$(CONFIG_OCXL) += ocxl/ obj-$(CONFIG_MISC_RTSX) += cardreader/ +obj-$(CONFIG_VMGENID) += vmgenid.o lkdtm-$(CONFIG_LKDTM) += lkdtm_core.o lkdtm-$(CONFIG_LKDTM) += lkdtm_bugs.odrivers/misc/vmgenid.c new file +27
-0@@ -0,0 +1,142 @@ +// SPDX-License-Identifier: GPL-2.0 +/* +* Virtual Machine Generation ID driver +* +* Copyright (C) 2018 Red Hat, Inc. All rights reserved. +* Authors: +* Or Idgar <[email protected]> +* Gal Hammer <[email protected]> +* +*/ +#include <linux/init.h> +#include <linux/module.h> +#include <linux/kernel.h> +#include <linux/kobject.h> +#include <linux/acpi.h> +#include <linux/uuid.h> + +MODULE_LICENSE ("GPL" ); +MODULE_AUTHOR ("Or Idgar <[email protected]>" ); +MODULE_AUTHOR ("Gal Hammer <[email protected]>" ); +MODULE_DESCRIPTION ("Virtual Machine Generation ID" ); +MODULE_VERSION ("0.1" ); + +ACPI_MODULE_NAME ("vmgenid" ); + +static u64 phys_addr; +drivers/misc/vmgenid.c line 37
Quoted 10 lines
drivers/misc/vmgenid.c new file +10
-0+static ssize_t generation_id_show (struct device *_d, +struct device_attribute *attr,char *buf) +{ +void __iomem *uuid_map; +uuid_t uuid; +ssize_t result; + + uuid_map =acpi_os_map_iomem (phys_addr,sizeof (uuid_t)); +if (!uuid_map) +return -EFAULT ;Shouldn't this be
acpi_os_map_memory? Spec says it's never an IO address.Quoted 12 lines
drivers/misc/vmgenid.c new file +12
-0+ +memcpy_fromio (&uuid, uuid_map,sizeof (uuid_t)); + result =sprintf (buf,"%pUl\n" , &uuid); +acpi_os_unmap_iomem (uuid_map,sizeof (uuid_t)); +return result; +} +static DEVICE_ATTR_RO (generation_id); + +static ssize_t raw_show (struct device *_d, +struct device_attribute *attr, +char *buf) +{drivers/misc/vmgenid.c line 59
Quoted 10 lines
drivers/misc/vmgenid.c new file +10
-0+void __iomem *uuid_map; + + uuid_map =acpi_os_map_iomem (phys_addr,sizeof (uuid_t)); +if (!uuid_map) +return -EFAULT ; +memcpy_fromio (buf, uuid_map,sizeof (uuid_t)); +acpi_os_unmap_iomem (uuid_map,sizeof (uuid_t)); +return sizeof (uuid_t); +} +static DEVICE_ATTR_RO (raw);I think you want
BIN_ATTR_RO.Quoted 12 lines
drivers/misc/vmgenid.c new file +12
-0+ +static struct attribute *vmgenid_attrs[] = { + &dev_attr_generation_id.attr, + &dev_attr_raw.attr, +NULL , +}; +static const struct attribute_group vmgenid_group = { + .name ="vm_gen_counter" , + .attrs = vmgenid_attrs, +}; + +static int get_vmgenid (acpi_handle handle)drivers/misc/vmgenid.c line 81
Quoted 10 lines
drivers/misc/vmgenid.c new file +10
-0+{ +int i; +struct acpi_buffer buffer = {ACPI_ALLOCATE_BUFFER ,NULL }; +acpi_status status; +union acpi_object *pss; +union acpi_object *element; + + status =acpi_evaluate_object (handle,"ADDR" ,NULL , &buffer); +if (ACPI_FAILURE (status)) { +ACPI_EXCEPTION ((AE_INFO , status,"Evaluating _ADDR" ));It's ADDR - not
_ADDR, right?drivers/misc/vmgenid.c line 92
Quoted 11 lines
drivers/misc/vmgenid.c new file +11
-0+return -ENODEV ; + } + pss = buffer.pointer; +if (!pss || pss->type !=ACPI_TYPE_PACKAGE || pss->package.count !=2 ) +return -EFAULT ; + + phys_addr =0 ; +for (i =0 ; i < pss->package.count; i++) { + element = &(pss->package.elements[i]); +if (element->type !=ACPI_TYPE_INTEGER ) +return -EFAULT ;EINVAL here and elsewhere.
drivers/misc/vmgenid.c line 93
Quoted 1 line
drivers/misc/vmgenid.c new file +1
-0+ phys_addr |= element->integer.value << i*32 ;i * 32
Quoted 21 lines
drivers/misc/vmgenid.c new file +21
-0+ } +return 0 ; +} + +static int acpi_vmgenid_add (struct acpi_device *device) +{ +int retval; + +if (!device) +return -EINVAL ; + retval =get_vmgenid (device->handle); +if (retval <0 ) +return retval; +return sysfs_create_group (hypervisor_kobj, &vmgenid_group); +} + +static int acpi_vmgenid_remove (struct acpi_device *device) +{ +sysfs_remove_group (hypervisor_kobj, &vmgenid_group); +return 0 ; +}
Quoted 8 lines
drivers/misc/vmgenid.c line 119
Quoted 5 lines
drivers/misc/vmgenid.c new file +5
-0+ +static const struct acpi_device_id vmgenid_ids[] = { + {"QEMUVGID" ,0 }, + {"" ,0 }, +};That's not right I think. You should go by
_CIDand maybe_DDN.
You are probably right on this. However, we didn't see that Linux supports loading ACPI modules other than using the _HID value.
Quoted 27 lines
Quoted 14 lines
drivers/misc/vmgenid.c new file +13
-0+ +static struct acpi_driver acpi_vmgenid_driver = { + .name ="vm_gen_counter" , + .ids = vmgenid_ids, + .owner =THIS_MODULE , + .ops = { + .add = acpi_vmgenid_add, + .remove = acpi_vmgenid_remove, + } +}; + +static int __init vmgenid_init (void ) +{drivers/misc/vmgenid.c line 142
Quoted 10 lines
drivers/misc/vmgenid.c new file +10
-0+return acpi_bus_register_driver (&acpi_vmgenid_driver); +} + +static void __exit vmgenid_exit(void) +{ +acpi_bus_unregister_driver (&acpi_vmgenid_driver); +} + +module_init (vmgenid_init); +module_exit (vmgenid_exit);Thanks for your work, and I hope this helps!
Quoted 2 lines
--
MST
Thanks,
Gal.
Quoted 34 lines · Gal Hammer
Quoted 1 line · Michael S. Tsirkin
Thanks for the patch! Yet something to improve (see below):
Thanks for the review.
Quoted 5 lines · Or Idgar
Quoted 1 line
From: Or Idgar <[email protected]>
I see addresses at gmail, virtualoco and redhat.com At this point I don't really know which address to use to contact you :) All of them?
Use his gmail or virtualoco one.
Quoted 21 lines
Also, I think it's a good idea to CC this more widely. Consider CC-ing qemu contributors to the vm gen id (use git log to get the list), Ben Warren who wrote a prototype driver a while ago, qemu mailing list, maybe more.
Quoted 4 lines
This patch is a driver which expose the Virtual Machine Generation ID via sysfs.
Quoted 8 lines
The ID is a UUID value used to differentiate between virtual machines.
The VM-Generation ID is a feature defined by Microsoft (paper:
http://go.microsoft.com/fwlink/?LinkId=260709) and supported by multiple hypervisor vendors.Signed-off-by Or Idgar <[email protected]>I think it's a good idea to use sysfs for this. However, there are a couple of missing interfaces here:
Quoted 6 lines
Quoted 2 lines
1. Userspace needs a way to know when this value changes. I see no change notifications here and that does not seem right.
We have the next version which includes notification. The problem is that right now QEMU doesn't include a mean to change the generation id on-the-fly, so it was not published it.
It seems to send this notification on each migration or resume from snapshot.
Quoted 6 lines
Quoted 3 lines
system calls. Pls add mmap support to the raw format. (Phys address is not guaranteed to be page-aligned so you will
probably want an offset attribute for that as well).I don't agree that this should be a requirement. According to the specs, the value doesn't change frequently.
What matters is whether it's read frequently, not whether it changes frequently.
Still, I agree we can defer that
for now until apps actually start using the new interface.
Quoted 2 lines
A notification about a
change the re-reading the value should be enough for now.
Notifications are asynchronous so I am not sure it's robust to rely on them. So I suspect we'll need it down the road but I agree to defer that for now until apps actually start using the new interface.
Quoted 14 lines
Quoted 14 lines
While it's possible to add these later in theory, it's easier if userspace can rely on all interfaces being in place just by detecting the directory presence.
Quoted 9 lines
---
Changes in v5:
- added to VMGENID module dependency on ACPI module.Documentation/ABI/testing/sysfs-hypervisor | 13 +++ drivers/misc/Kconfig | 7 ++ drivers/misc/Makefile | 1 + drivers/misc/vmgenid.c | 142 +++++++++++++++++++++++++++++
Quoted 6 lines
Quoted 2 lines
Do you want to add this under QEMU MACHINE EMULATOR in
MAINTAINERS? This way e.g. qemu-devel will be copied.This feature is supported by other hypervisors and this driver should be able to support them in the next versions. I don't see a reason to bound it to QEMU.
Go ahead and add the microsoft list too, whatever it is. Point is you want people familiar with the spec to look at patches.
Quoted 15 lines
Quoted 4 lines
Quoted 4 lines
4 files changed, 163 insertions(+) create mode 100644 Documentation/ABI/testing/sysfs-hypervisor create mode 100644 drivers/misc/vmgenid.cQuoted 10 lines
Documentation/ABI/testing/sysfs-hypervisor line 1
Quoted 7 lines
Documentation/ABI/testing/sysfs-hypervisor new file +1
-0@@ -0,0 +1,13 @@ +What: /sys/hypervisor/vm_gen_counterIt's not a counter, is it?
I'd go with "vm_generation_id" here. Eschew abbreviation.
Quoted 2 lines
The names were chosen so they'll match Microsoft's specification and code examples.
I don't think this makes sense. We are not going to use '\' in file names to match Microsoft code examples, either.
Maybe they say counter in some docs because someone somewhere uses this as a counter. Or maybe some architect wrote the code examples before the name was changed from gen counter to generation id and did not bother changing the code examples. Who knows - there is still no point in building an API around that.
In particular QEMU certainly does not pass a counter in that field, and spec does not say it should.
Quoted 11 lines
Quoted 10 lines
Documentation/ABI/testing/sysfs-hypervisor line 9
Quoted 8 lines
Documentation/ABI/testing/sysfs-hypervisor new file +8
-0+Date: February 2018 +Contact: Or Idgar <[email protected]> + Gal Hammer <[email protected]> +Description: Expose the virtual machine generation ID. The directory + contains two files: "generation_id" and "raw". Both files + represent the same information. + + "generation_id" file is a UUID stringAnd this I'd call "uuid" then.
Quoted 2 lines
Why? The name says what the value is, not its type. This is not common to see values' types in the sysfs directory.
Look at the hierarchy:
/sys/hypervisor/vm_gen_counter/generation_id/sys/hypervisor/vm_gen_counter/raw
Naming it vm_gen_counter/generation_id makes it seem like there is a gen counter (general counter?) which allows some kind of raw access and which also has a generation id.
But in reality what is going on here is that there is a single value which is a vm generation id, and we present it in two formats: uuid and raw.
Quoted 12 lines
Quoted 12 lines
Quoted 12 lines
Documentation/ABI/testing/sysfs-hypervisor new file +4
-0+ representation. + + "raw" file is a 128-bit integer + representation (binary).drivers/misc/Kconfig +0
-0@@ -500,6 +500,13 @@ config MISC_RTSX tristate default MISC_RTSX_PCI || MISC_RTSX_USB
Quoted 10 lines
Quoted 10 lines
drivers/misc/Kconfig config MISC_RTSX · line 512
Quoted 10 lines
drivers/misc/Kconfig +7
-0+config VMGENID + depends on ACPI + tristate "Virtual Machine Generation ID driver" + help + This is a Virtual Machine Generation ID driver which provides + a virtual machine unique identifier. + source "drivers/misc/c2port/Kconfig" source "drivers/misc/eeprom/Kconfig" source "drivers/misc/cb710/Kconfig"
BTW maybe we want to make this depend on CONFIG_PARAVIRT as well.
Quoted 157 lines
Quoted 148 lines
Quoted 45 lines
drivers/misc/Makefile +1
-0@@ -57,6 +57,7 @@ obj-$(CONFIG_ASPEED_LPC_SNOOP) += aspeed-lpc-snoop.o obj-$(CONFIG_PCI_ENDPOINT_TEST) += pci_endpoint_test.o obj-$(CONFIG_OCXL) += ocxl/ obj-$(CONFIG_MISC_RTSX) += cardreader/ +obj-$(CONFIG_VMGENID) += vmgenid.o lkdtm-$(CONFIG_LKDTM) += lkdtm_core.o lkdtm-$(CONFIG_LKDTM) += lkdtm_bugs.odrivers/misc/vmgenid.c new file +27
-0@@ -0,0 +1,142 @@ +// SPDX-License-Identifier: GPL-2.0 +/* +* Virtual Machine Generation ID driver +* +* Copyright (C) 2018 Red Hat, Inc. All rights reserved. +* Authors: +* Or Idgar <[email protected]> +* Gal Hammer <[email protected]> +* +*/ +#include <linux/init.h> +#include <linux/module.h> +#include <linux/kernel.h> +#include <linux/kobject.h> +#include <linux/acpi.h> +#include <linux/uuid.h> + +MODULE_LICENSE ("GPL" ); +MODULE_AUTHOR ("Or Idgar <[email protected]>" ); +MODULE_AUTHOR ("Gal Hammer <[email protected]>" ); +MODULE_DESCRIPTION ("Virtual Machine Generation ID" ); +MODULE_VERSION ("0.1" ); + +ACPI_MODULE_NAME ("vmgenid" ); + +static u64 phys_addr; +drivers/misc/vmgenid.c line 37
Quoted 10 lines
drivers/misc/vmgenid.c new file +10
-0+static ssize_t generation_id_show (struct device *_d, +struct device_attribute *attr,char *buf) +{ +void __iomem *uuid_map; +uuid_t uuid; +ssize_t result; + + uuid_map =acpi_os_map_iomem (phys_addr,sizeof (uuid_t)); +if (!uuid_map) +return -EFAULT ;Shouldn't this be
acpi_os_map_memory? Spec says it's never an IO address.Quoted 12 lines
drivers/misc/vmgenid.c new file +12
-0+ +memcpy_fromio (&uuid, uuid_map,sizeof (uuid_t)); + result =sprintf (buf,"%pUl\n" , &uuid); +acpi_os_unmap_iomem (uuid_map,sizeof (uuid_t)); +return result; +} +static DEVICE_ATTR_RO (generation_id); + +static ssize_t raw_show (struct device *_d, +struct device_attribute *attr, +char *buf) +{drivers/misc/vmgenid.c line 59
Quoted 10 lines
drivers/misc/vmgenid.c new file +10
-0+void __iomem *uuid_map; + + uuid_map =acpi_os_map_iomem (phys_addr,sizeof (uuid_t)); +if (!uuid_map) +return -EFAULT ; +memcpy_fromio (buf, uuid_map,sizeof (uuid_t)); +acpi_os_unmap_iomem (uuid_map,sizeof (uuid_t)); +return sizeof (uuid_t); +} +static DEVICE_ATTR_RO (raw);I think you want
BIN_ATTR_RO.Quoted 12 lines
drivers/misc/vmgenid.c new file +12
-0+ +static struct attribute *vmgenid_attrs[] = { + &dev_attr_generation_id.attr, + &dev_attr_raw.attr, +NULL , +}; +static const struct attribute_group vmgenid_group = { + .name ="vm_gen_counter" , + .attrs = vmgenid_attrs, +}; + +static int get_vmgenid (acpi_handle handle)drivers/misc/vmgenid.c line 81
Quoted 10 lines
drivers/misc/vmgenid.c new file +10
-0+{ +int i; +struct acpi_buffer buffer = {ACPI_ALLOCATE_BUFFER ,NULL }; +acpi_status status; +union acpi_object *pss; +union acpi_object *element; + + status =acpi_evaluate_object (handle,"ADDR" ,NULL , &buffer); +if (ACPI_FAILURE (status)) { +ACPI_EXCEPTION ((AE_INFO , status,"Evaluating _ADDR" ));It's ADDR - not
_ADDR, right?drivers/misc/vmgenid.c line 92
Quoted 11 lines
drivers/misc/vmgenid.c new file +11
-0+return -ENODEV ; + } + pss = buffer.pointer; +if (!pss || pss->type !=ACPI_TYPE_PACKAGE || pss->package.count !=2 ) +return -EFAULT ; + + phys_addr =0 ; +for (i =0 ; i < pss->package.count; i++) { + element = &(pss->package.elements[i]); +if (element->type !=ACPI_TYPE_INTEGER ) +return -EFAULT ;EINVAL here and elsewhere.
drivers/misc/vmgenid.c line 93
Quoted 1 line
drivers/misc/vmgenid.c new file +1
-0+ phys_addr |= element->integer.value << i*32 ;i * 32
Quoted 21 lines
drivers/misc/vmgenid.c new file +21
-0+ } +return 0 ; +} + +static int acpi_vmgenid_add (struct acpi_device *device) +{ +int retval; + +if (!device) +return -EINVAL ; + retval =get_vmgenid (device->handle); +if (retval <0 ) +return retval; +return sysfs_create_group (hypervisor_kobj, &vmgenid_group); +} + +static int acpi_vmgenid_remove (struct acpi_device *device) +{ +sysfs_remove_group (hypervisor_kobj, &vmgenid_group); +return 0 ; +}Quoted 8 lines
drivers/misc/vmgenid.c line 119
Quoted 5 lines
drivers/misc/vmgenid.c new file +5
-0+ +static const struct acpi_device_id vmgenid_ids[] = { + {"QEMUVGID" ,0 }, + {"" ,0 }, +};That's not right I think. You should go by
_CIDand maybe_DDN.
Quoted 2 lines
You are probably right on this. However, we didn't see that Linux supports loading ACPI modules other than using the
_HIDvalue.
Are you sure about this?
if (info->valid & ACPI_VALID_CID) {
cid_list = &info->compatible_id_list;
for (i = 0; i < cid_list->count; i++)
acpi_add_id(pnp, cid_list->ids[i].string);
}Anyway, spec is pretty clear on this so we have to make it work.
Quoted 33 lines
Quoted 27 lines
Quoted 14 lines
drivers/misc/vmgenid.c new file +13
-0+ +static struct acpi_driver acpi_vmgenid_driver = { + .name ="vm_gen_counter" , + .ids = vmgenid_ids, + .owner =THIS_MODULE , + .ops = { + .add = acpi_vmgenid_add, + .remove = acpi_vmgenid_remove, + } +}; + +static int __init vmgenid_init (void ) +{drivers/misc/vmgenid.c line 142
Quoted 10 lines
drivers/misc/vmgenid.c new file +10
-0+return acpi_bus_register_driver (&acpi_vmgenid_driver); +} + +static void __exit vmgenid_exit(void) +{ +acpi_bus_unregister_driver (&acpi_vmgenid_driver); +} + +module_init (vmgenid_init); +module_exit (vmgenid_exit);Thanks for your work, and I hope this helps!
Quoted 2 lines
--
MSTThanks,
Gal.
drivers/misc/vmgenid.c line 120
Quoted 6 lines · Or Idgar
drivers/misc/vmgenid.c new file +6
-0+ +static const struct acpi_device_id vmgenid_ids[] = { + {"QEMUVGID" ,0 }, + {"" ,0 }, +}; +
This is wrong. You're supposed to match on the _CID "VM_Gen_Counter".