Re: [PATCH v2 4/4] virt: vmgenid: add support for devicetree bindings

From: "Landge, Sudan" <sudanl@amazon.co.uk>
To: Rob Herring <robh+dt@kernel.org>, Sudan Landge <sudanl@amazon.com>
Cc: <tytso@mit.edu>, <Jason@zx2c4.com>,
	<krzysztof.kozlowski+dt@linaro.org>, <conor+dt@kernel.org>,
	<sathyanarayanan.kuppuswamy@linux.intel.com>,
	<thomas.lendacky@amd.com>, <dan.j.williams@intel.com>,
	<devicetree@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
	<graf@amazon.de>, <dwmw@amazon.co.uk>, <bchalios@amazon.es>,
	<xmarcalx@amazon.co.uk>
Subject: Re: [PATCH v2 4/4] virt: vmgenid: add support for devicetree bindings
Date: Fri, 22 Mar 2024 17:02:56 +0000	[thread overview]
Message-ID: <2920d0d6-30a2-4e07-87c5-7b403dce269b@amazon.co.uk> (raw)
In-Reply-To: <CAL_JsqJoB5CYajWuntMdQrJZir+ZA-69Q0cwvxcVZAqs-mXC+Q@mail.gmail.com>


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@amazon.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@amazon.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@zx2c4.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.