Received-SPF: pass (google.com: domain of linux-kernel+bounces-111548-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Precedence: bulk
MIME-Version: 1.0
References: <20240321025105.53210-1-sudanl@amazon.com> <20240321025105.53210-5-sudanl@amazon.com>
In-Reply-To: <20240321025105.53210-5-sudanl@amazon.com>
From: Rob Herring <robh+dt@kernel.org>
Date: Fri, 22 Mar 2024 08:33:55 -0500
Message-ID: <CAL_JsqJoB5CYajWuntMdQrJZir+ZA-69Q0cwvxcVZAqs-mXC+Q@mail.gmail.com>
Subject: Re: [PATCH v2 4/4] virt: vmgenid: add support for devicetree bindings
To: 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
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Wed, Mar 20, 2024 at 9:51=E2=80=AFPM Sudan Landge <sudanl@amazon.com> wr=
ote:
>
> - 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 Gener=
ation ID
>           to reseed the RNG when the VM is cloned. This is highly recomme=
nded 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 R=
eserved.
>   *
> - * The "Virtual Machine Generation ID" is exposed via ACPI and changes w=
hen a
> + * The "Virtual Machine Generation ID" is exposed via ACPI or DT and cha=
nges when a
>   * virtual machine forks or is cloned. This driver exists for shepherdin=
g 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 =3D 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 *de=
v)
>  {
> +       (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 *st=
ate, u8 *next_id)
>
>  static int vmgenid_add_acpi(struct device *dev, struct vmgenid_state *st=
ate)
>  {
> +#ifdef CONFIG_ACPI
>         struct acpi_device *device =3D ACPI_COMPANION(dev);
>         struct acpi_buffer parsed =3D { ACPI_ALLOCATE_BUFFER };
>         union acpi_object *obj;
> @@ -96,6 +124,54 @@ static int vmgenid_add_acpi(struct device *dev, struc=
t 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 *stat=
e)
> +{
> +#ifdef CONFIG_OF
> +       struct resource res;
> +       int ret =3D 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 =3D -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 =3D -EINVAL;
> +               goto out;
> +       }
> +
> +       ret =3D setup_vmgenid_state(state, (u8 *)of_iomap(dev->of_node, 0=
));
> +       if (ret)
> +               goto out;
> +
> +       state->irq =3D irq_of_parse_and_map(dev->of_node, 0);

platform_get_irq()

> +       dev->driver_data =3D state;
> +
> +       if (request_irq(state->irq, vmgenid_of_irq_handler,
> +                       IRQF_SHARED, "vmgenid", dev) < 0) {
> +               dev_err(dev, "request_irq failed");
> +               dev->driver_data =3D NULL;
> +               ret =3D -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 =3D vmgenid_add_acpi(dev, state);
> +       if (dev->of_node)
> +               ret =3D vmgenid_add_of(dev, state);
> +       else
> +               ret =3D 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[] =3D {
> +#ifdef CONFIG_OF
> +static const struct of_device_id vmgenid_of_ids[] =3D {
> +       { .compatible =3D "linux,vmgenctr", },
> +       {},
> +};
> +MODULE_DEVICE_TABLE(of, vmgenid_of_ids);
> +#endif
> +
> +#ifdef CONFIG_ACPI
> +static const struct acpi_device_id vmgenid_acpi_ids[] =3D {
>         { "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 =3D {
>         .probe      =3D vmgenid_add,
>         .driver     =3D {
>                 .name   =3D "vmgenid",
> -               .acpi_match_table =3D ACPI_PTR(vmgenid_ids),
> +#ifdef CONFIG_ACPI
> +               .acpi_match_table =3D ACPI_PTR(vmgenid_acpi_ids),

Pretty sure you don't need the ifdef AND ACPI_PTR.

> +#endif
> +#ifdef CONFIG_OF
> +               .of_match_table =3D vmgenid_of_ids,
> +#endif
>                 .owner =3D THIS_MODULE,
>         },
>  };
> --
> 2.40.1
>
>