Re: [PATCH v5] drivers/misc: vm_gen_counter: initial driver implementation

Gal Hammer
2018-03-15T08:00:30Z

On Tue, Mar 13, 2018 at 7:40 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> Thanks for the patch! Yet something to improve (see below):

Thanks for the review.

> On Thu, Mar 01, 2018 at 04:22:15PM +0200, Or Idgar wrote:
>> From: Or Idgar <oridgar@gmail.com>
>
> 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.

> 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.
>
>>
>> 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.
>>
>> Signed-off-by: Or Idgar <oridgar@gmail.com>
>
> 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.

> 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.

> 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.
>
>> ---
>>
>> 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.

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.

>> 4 files changed, 163 insertions(+)
>> create mode 100644 Documentation/ABI/testing/sysfs-hypervisor
>> create mode 100644 drivers/misc/vmgenid.c
>>
>> diff --git a/Documentation/ABI/testing/sysfs-hypervisor b/Documentation/ABI/testing/sysfs-hypervisor
>> new file mode 100644
>> index 000000000000..2f9a7b8eab70
>> --- /dev/null
>> +++ b/Documentation/ABI/testing/sysfs-hypervisor
>> @@ -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.

The names were chosen so they'll match Microsoft's specification and
code examples.

>> +Date: February 2018
>> +Contact: Or Idgar <idgar@virtualoco.com>
>> + Gal Hammer <ghammer@redhat.com>
>> +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.

Why? The name says what the value is, not its type. This is not common
to see values' types in the sysfs directory.

>> + representation.
>> +
>> + "raw" file is a 128-bit integer
>> + representation (binary).
>> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
>> index 03605f8fc0dc..a39feff6a867 100644
>> --- a/drivers/misc/Kconfig
>> +++ b/drivers/misc/Kconfig
>> @@ -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"
>> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
>> index c3c8624f4d95..067aa666bb6a 100644
>> --- a/drivers/misc/Makefile
>> +++ b/drivers/misc/Makefile
>> @@ -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
>> diff --git a/drivers/misc/vmgenid.c b/drivers/misc/vmgenid.c
>> new file mode 100644
>> index 000000000000..6c8d8fe75335
>> --- /dev/null
>> +++ b/drivers/misc/vmgenid.c
>> @@ -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 <oridgar@gmail.com>
>> + * Gal Hammer <ghammer@redhat.com>
>> + *
>> + */
>> +#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 <oridgar@gmail.com>");
>> +MODULE_AUTHOR("Gal Hammer <ghammer@redhat.com>");
>> +MODULE_DESCRIPTION("Virtual Machine Generation ID");
>> +MODULE_VERSION("0.1");
>> +
>> +ACPI_MODULE_NAME("vmgenid");
>> +
>> +static u64 phys_addr;
>> +
>> +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.
>
>> +
>> + 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)
>> +{
>> + 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.
>
>
>> +
>> +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)
>> +{
>> + 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?
>
>> + 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.
>
>> + phys_addr |= element->integer.value << i*32;
>
> i * 32
>
>> + }
>> + 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;
>> +}
>> +
>> +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.

You are probably right on this. However, we didn't see that Linux
supports loading ACPI modules other than using the _HID value.

>>
>> +
>> +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)
>> +{
>> + 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!
>
> --
> MST

Thanks,

Gal.