| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAL_JsqJoB5CYajWuntMdQrJZir+ZA-69Q0cwvxcVZAqs-mXC+Q@mail.gmail.com>
Date: Fri, 22 Mar 2024 08:33:55 -0500
From: Rob Herring <robh+dt@...nel.org>
To: Sudan Landge <sudanl@...zon.com>
Cc: tytso@....edu, Jason@...c4.com, krzysztof.kozlowski+dt@...aro.org,
conor+dt@...nel.org, sathyanarayanan.kuppuswamy@...ux.intel.com,
thomas.lendacky@....com, dan.j.williams@...el.com, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, graf@...zon.de, dwmw@...zon.co.uk,
bchalios@...zon.es, xmarcalx@...zon.co.uk
Subject: Re: [PATCH v2 4/4] virt: vmgenid: add support for devicetree bindings
On Wed, Mar 20, 2024 at 9:51 PM Sudan Landge <sudanl@...zon.com> wrote:
>
> - Extend the vmgenid platform driver to support devicetree bindings.
> With this support, hypervisors can send vmgenid notifications to
> the virtual machine without the need to enable ACPI. The bindings
> are located at: Documentation/devicetree/bindings/rng/vmgenid.yaml
>
> - Use proper FLAGS to compile with and without ACPI and/or devicetree.
>
> Signed-off-by: Sudan Landge <sudanl@...zon.com>
> ---
> drivers/virt/Kconfig | 2 +-
> drivers/virt/vmgenid.c | 106 ++++++++++++++++++++++++++++++++++++++---
> 2 files changed, 101 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/virt/Kconfig b/drivers/virt/Kconfig
> index 40129b6f0eca..4f33ee2f0372 100644
> --- a/drivers/virt/Kconfig
> +++ b/drivers/virt/Kconfig
> @@ -16,7 +16,7 @@ if VIRT_DRIVERS
> config VMGENID
> tristate "Virtual Machine Generation ID driver"
> default y
> - depends on ACPI
> + depends on (ACPI || OF)
One of those is pretty much always enabled, so it can probably be dropped.
> help
> Say Y here to use the hypervisor-provided Virtual Machine Generation ID
> to reseed the RNG when the VM is cloned. This is highly recommended if
> diff --git a/drivers/virt/vmgenid.c b/drivers/virt/vmgenid.c
> index d5394c706bd9..ec378c37a2a2 100644
> --- a/drivers/virt/vmgenid.c
> +++ b/drivers/virt/vmgenid.c
> @@ -2,7 +2,7 @@
> /*
> * Copyright (C) 2022 Jason A. Donenfeld <Jason@...c4.com>. All Rights Reserved.
> *
> - * The "Virtual Machine Generation ID" is exposed via ACPI and changes when a
> + * The "Virtual Machine Generation ID" is exposed via ACPI or DT and changes when a
> * virtual machine forks or is cloned. This driver exists for shepherding that
> * information to random.c.
> */
> @@ -13,14 +13,27 @@
> #include <linux/random.h>
> #include <acpi/actypes.h>
> #include <linux/platform_device.h>
> -
> +#ifdef CONFIG_OF
You don't need nor want ifdefs around includes.
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/of_address.h>
> +#include <linux/of_device.h>
Doubtful you need this header.
> +#include <linux/of_irq.h>
> +#endif
> +
> +#ifdef CONFIG_ACPI
Probably don't need this.
> ACPI_MODULE_NAME("vmgenid");
> +#endif
>
> enum { VMGENID_SIZE = 16 };
>
> struct vmgenid_state {
> u8 *next_id;
> u8 this_id[VMGENID_SIZE];
> +#ifdef CONFIG_OF
Really worth saving 1 int on ACPI systems?
> + unsigned int irq;
> +#endif
> };
>
> static void vmgenid_notify(struct device *device)
> @@ -37,10 +50,24 @@ static void vmgenid_notify(struct device *device)
> kobject_uevent_env(&device->kobj, KOBJ_CHANGE, envp);
> }
>
> +#ifdef CONFIG_ACPI
Avoid ifdefs. Use "IS_ENABLED(CONFIG_ACPI)" instead if you must.
> static void vmgenid_acpi_handler(acpi_handle handle, u32 event, void *dev)
> {
> + (void)handle;
> + (void)event;
I don't think these are ever needed.
> vmgenid_notify(dev);
> }
> +#endif
> +
> +#ifdef CONFIG_OF
> +static irqreturn_t vmgenid_of_irq_handler(int irq, void *dev)
> +{
> + (void)irq;
> + vmgenid_notify(dev);
> +
> + return IRQ_HANDLED;
> +}
> +#endif
>
> static int setup_vmgenid_state(struct vmgenid_state *state, u8 *next_id)
> {
> @@ -55,6 +82,7 @@ static int setup_vmgenid_state(struct vmgenid_state *state, u8 *next_id)
>
> static int vmgenid_add_acpi(struct device *dev, struct vmgenid_state *state)
> {
> +#ifdef CONFIG_ACPI
> struct acpi_device *device = ACPI_COMPANION(dev);
> struct acpi_buffer parsed = { ACPI_ALLOCATE_BUFFER };
> union acpi_object *obj;
> @@ -96,6 +124,54 @@ static int vmgenid_add_acpi(struct device *dev, struct vmgenid_state *state)
> out:
> ACPI_FREE(parsed.pointer);
> return ret;
> +#else
> + (void)dev;
> + (void)state;
> + return -EINVAL;
> +#endif
> +}
> +
> +static int vmgenid_add_of(struct device *dev, struct vmgenid_state *state)
> +{
> +#ifdef CONFIG_OF
> + struct resource res;
> + int ret = 0;
> +
> + if (of_address_to_resource(dev->of_node, 0, &res)) {
No, use the platform_ APIs which work for both ACPI and DT.
> + dev_err(dev, "Failed to get resources from device tree");
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + if (!__request_mem_region(res.start, resource_size(&res),
There's a single API to do this and ioremap. Use it.
> + "vmgenid", IORESOURCE_EXCLUSIVE)) {
> + dev_err(dev, "Failed to request mem region");
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + ret = setup_vmgenid_state(state, (u8 *)of_iomap(dev->of_node, 0));
> + if (ret)
> + goto out;
> +
> + state->irq = irq_of_parse_and_map(dev->of_node, 0);
platform_get_irq()
> + dev->driver_data = state;
> +
> + if (request_irq(state->irq, vmgenid_of_irq_handler,
> + IRQF_SHARED, "vmgenid", dev) < 0) {
> + dev_err(dev, "request_irq failed");
> + dev->driver_data = NULL;
> + ret = -EINVAL;
> + goto out;
> + }
> +
> +out:
> + return ret;
> +#else
> + (void)dev;
> + (void)state;
> + return -EINVAL;
> +#endif
> }
>
> static int vmgenid_add(struct platform_device *pdev)
> @@ -108,7 +184,10 @@ static int vmgenid_add(struct platform_device *pdev)
> if (!state)
> return -ENOMEM;
>
> - ret = vmgenid_add_acpi(dev, state);
> + if (dev->of_node)
> + ret = vmgenid_add_of(dev, state);
> + else
> + ret = vmgenid_add_acpi(dev, state);
>
> if (ret)
> devm_kfree(dev, state);
> @@ -116,18 +195,33 @@ static int vmgenid_add(struct platform_device *pdev)
> return ret;
> }
>
> -static const struct acpi_device_id vmgenid_ids[] = {
> +#ifdef CONFIG_OF
> +static const struct of_device_id vmgenid_of_ids[] = {
> + { .compatible = "linux,vmgenctr", },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, vmgenid_of_ids);
> +#endif
> +
> +#ifdef CONFIG_ACPI
> +static const struct acpi_device_id vmgenid_acpi_ids[] = {
> { "VMGENCTR", 0 },
> { "VM_GEN_COUNTER", 0 },
> { }
> };
> -MODULE_DEVICE_TABLE(acpi, vmgenid_ids);
> +MODULE_DEVICE_TABLE(acpi, vmgenid_acpi_ids);
> +#endif
>
> static struct platform_driver vmgenid_plaform_driver = {
> .probe = vmgenid_add,
> .driver = {
> .name = "vmgenid",
> - .acpi_match_table = ACPI_PTR(vmgenid_ids),
> +#ifdef CONFIG_ACPI
> + .acpi_match_table = ACPI_PTR(vmgenid_acpi_ids),
Pretty sure you don't need the ifdef AND ACPI_PTR.
> +#endif
> +#ifdef CONFIG_OF
> + .of_match_table = vmgenid_of_ids,
> +#endif
> .owner = THIS_MODULE,
> },
> };
> --
> 2.40.1
>
>
Powered by blists - more mailing lists