| 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: <2920d0d6-30a2-4e07-87c5-7b403dce269b@amazon.co.uk>
Date: Fri, 22 Mar 2024 17:02:56 +0000
From: "Landge, Sudan" <sudanl@...zon.co.uk>
To: Rob Herring <robh+dt@...nel.org>, 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 22/03/2024 13:33, Rob Herring wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
>
>
>
> 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
>>
>>
Thank you for the feedback and sorry for my lack of experience regarding
the APIs. I will use the alternatives APIs suggested for devicetree and
revert after thorough testing.
Powered by blists - more mailing lists