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

[Landge, Sudan]

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@xxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> >   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@xxxxxxxxx>. 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.