Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Tue, 13 Mar 2018 19:40:51 +0200
From:   "Michael S. Tsirkin" <mst@redhat.com>
To:     Or Idgar <idgar@virtualoco.com>
Cc:     linux-kernel@vger.kernel.org, arnd@arndb.de,
        gregkh@linuxfoundation.org, oidgar@redhat.com, ghammer@redhat.com,
        Or Idgar <oridgar@gmail.com>
Subject: Re: [PATCH v5] drivers/misc: vm_gen_counter: initial driver
 implementation
Message-ID: <20180313190617-mutt-send-email-mst@kernel.org>
References: <20180301142215.11812-1-idgar@virtualoco.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20180301142215.11812-1-idgar@virtualoco.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Thanks for the patch! Yet something to improve (see below):

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?

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.

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.

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

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

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

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

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