This small series of patches aims to add devicetree bindings support for
the Virtual Machine Generation ID (vmgenid) driver.
Virtual Machine Generation ID driver was introduced in commit af6b54e2b5ba
("virt: vmgenid: notify RNG of VM fork and supply generation ID") as an
ACPI only device.
We would like to extend vmgenid to support devicetree bindings because:
1. A device should not be defined as an ACPI or DT only device.
2. Technically there's no issue with adding devicetree support to vmgenid.
3. This would allow Hypervisors to use vmgenid without the need to
enable ACPI. This is important for hypervisors that want to
keep things minimalistic and enable ACPI only when they have
no other alternative.
While adding the devicetree support we considered re-using existing
structures/code to avoid duplication code and reduce maintenance; so,
we used the same driver to be configured either by ACPI or by DT.
This also meant reimplementing the existing vmgenid ACPI bus driver as a
platform driver and making it discoverable using `driver.of_match_table`
and `driver.acpi_match_table`.
There is no user impact or change in vmgenid functionality when used
with ACPI. We verified ACPI support of these patches on X86 and DT
support on ARM using Firecracker hypervisor
https://github.com/firecracker-microvm/firecracker.
To check schema and syntax errors, the bindings file is verified with:
```
make dt_binding_check \
DT_SCHEMA_FILES=Documentation/devicetree/bindings/vmgenid/vmgenid.yaml
```
and the patches were verified with:
`scripts/checkpatch.pl --strict v1-000*`.
Sudan Landge (4):
virt: vmgenid: rearrange code to make review easier
virt: vmgenid: change implementation to use a platform driver
dt-bindings: Add bindings for vmgenid
virt: vmgenid: add support for devicetree bindings
.../devicetree/bindings/vmgenid/vmgenid.yaml | 57 +++++
MAINTAINERS | 1 +
drivers/virt/Kconfig | 2 +-
drivers/virt/vmgenid.c | 197 ++++++++++++++----
4 files changed, 221 insertions(+), 36 deletions(-)
create mode 100644 Documentation/devicetree/bindings/vmgenid/vmgenid.yaml
--
2.40.1
Rearrage the functions of vmgenid to make the next commit,
which re-implements vmgenid as a platform driver, easier to review.
Signed-off-by: Sudan Landge <[email protected]>
---
drivers/virt/vmgenid.c | 28 ++++++++++++++--------------
1 file changed, 14 insertions(+), 14 deletions(-)
diff --git a/drivers/virt/vmgenid.c b/drivers/virt/vmgenid.c
index b67a28da4702..ea956df02874 100644
--- a/drivers/virt/vmgenid.c
+++ b/drivers/virt/vmgenid.c
@@ -21,6 +21,20 @@ struct vmgenid_state {
u8 this_id[VMGENID_SIZE];
};
+static void vmgenid_notify(struct acpi_device *device, u32 event)
+{
+ struct vmgenid_state *state = acpi_driver_data(device);
+ char *envp[] = { "NEW_VMGENID=1", NULL };
+ u8 old_id[VMGENID_SIZE];
+
+ memcpy(old_id, state->this_id, sizeof(old_id));
+ memcpy(state->this_id, state->next_id, sizeof(state->this_id));
+ if (!memcmp(old_id, state->this_id, sizeof(old_id)))
+ return;
+ add_vmfork_randomness(state->this_id, sizeof(state->this_id));
+ kobject_uevent_env(&device->dev.kobj, KOBJ_CHANGE, envp);
+}
+
static int vmgenid_add(struct acpi_device *device)
{
struct acpi_buffer parsed = { ACPI_ALLOCATE_BUFFER };
@@ -65,20 +79,6 @@ static int vmgenid_add(struct acpi_device *device)
return ret;
}
-static void vmgenid_notify(struct acpi_device *device, u32 event)
-{
- struct vmgenid_state *state = acpi_driver_data(device);
- char *envp[] = { "NEW_VMGENID=1", NULL };
- u8 old_id[VMGENID_SIZE];
-
- memcpy(old_id, state->this_id, sizeof(old_id));
- memcpy(state->this_id, state->next_id, sizeof(state->this_id));
- if (!memcmp(old_id, state->this_id, sizeof(old_id)))
- return;
- add_vmfork_randomness(state->this_id, sizeof(state->this_id));
- kobject_uevent_env(&device->dev.kobj, KOBJ_CHANGE, envp);
-}
-
static const struct acpi_device_id vmgenid_ids[] = {
{ "VMGENCTR", 0 },
{ "VM_GEN_COUNTER", 0 },
--
2.40.1
Virtual Machine Generation ID driver was introduced in commit af6b54e2b5ba
("virt: vmgenid: notify RNG of VM fork and supply generation ID"), as an
ACPI only device.
Add a devicetree binding support for vmgenid so that hypervisors
can support vmgenid without the need to support ACPI.
Signed-off-by: Sudan Landge <[email protected]>
---
.../devicetree/bindings/vmgenid/vmgenid.yaml | 57 +++++++++++++++++++
MAINTAINERS | 1 +
2 files changed, 58 insertions(+)
create mode 100644 Documentation/devicetree/bindings/vmgenid/vmgenid.yaml
diff --git a/Documentation/devicetree/bindings/vmgenid/vmgenid.yaml b/Documentation/devicetree/bindings/vmgenid/vmgenid.yaml
new file mode 100644
index 000000000000..17773aa96f8b
--- /dev/null
+++ b/Documentation/devicetree/bindings/vmgenid/vmgenid.yaml
@@ -0,0 +1,57 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/vmgenid/vmgenid.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Virtual Machine Generation Counter ID device.
+
+maintainers:
+ - Jason A. Donenfeld <[email protected]>
+
+description: |+
+ Firmwares or hypervisors can use this devicetree to describe
+ interrupts and the shared resources to inject a Virtual Machine Generation
+ counter.
+
+properties:
+ compatible:
+ const: linux,vmgenctr
+
+ "#interrupt-cells":
+ const: 3
+ description: |
+ The 1st cell is the interrupt type.
+ The 2nd cell contains the interrupt number for the interrupt type.
+ The 3rd cell is for trigger type and level flags.
+
+ interrupt-controller: true
+
+ reg:
+ description: |
+ specifies the base physical address and
+ size of the regions in memory which holds the VMGenID counter.
+ maxItems: 1
+
+ interrupts:
+ description: |
+ interrupt used to notify that a new VMGenID counter is available.
+ The interrupt should be Edge triggered.
+ maxItems: 1
+
+required:
+ - compatible
+ - reg
+ - interrupts
+
+additionalProperties: false
+
+examples:
+ - |
+ vmgenid@80000000 {
+ compatible = "linux,vmgenctr";
+ reg = <0x80000000 0x1000>;
+ interrupts = <0x00 0x23 0x01>;
+ };
+
+...
diff --git a/MAINTAINERS b/MAINTAINERS
index d818866d6d73..e602a51b9bd7 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -18359,6 +18359,7 @@ M: "Theodore Ts'o" <[email protected]>
M: Jason A. Donenfeld <[email protected]>
S: Maintained
T: git https://git.kernel.org/pub/scm/linux/kernel/git/crng/random.git
+F: Documentation/devicetree/bindings/vmgenid/vmgenid.yaml
F: drivers/char/random.c
F: drivers/virt/vmgenid.c
--
2.40.1
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/vmgenid/vmgenid.yaml
Signed-off-by: Sudan Landge <[email protected]>
---
drivers/virt/Kconfig | 2 +-
drivers/virt/vmgenid.c | 90 +++++++++++++++++++++++++++++++++++++++---
2 files changed, 86 insertions(+), 6 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)
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 e82b344a9d61..96a3ff8ec05a 100644
--- a/drivers/virt/vmgenid.c
+++ b/drivers/virt/vmgenid.c
@@ -2,7 +2,7 @@
/*
* Copyright (C) 2022 Jason A. Donenfeld <[email protected]>. 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.
*/
@@ -12,6 +12,12 @@
#include <linux/acpi.h>
#include <linux/random.h>
#include <acpi/actypes.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/of_address.h>
+#include <linux/of_device.h>
+#include <linux/of_irq.h>
#include <linux/platform_device.h>
ACPI_MODULE_NAME("vmgenid");
@@ -21,6 +27,7 @@ enum { VMGENID_SIZE = 16 };
struct vmgenid_state {
u8 *next_id;
u8 this_id[VMGENID_SIZE];
+ unsigned int irq;
};
static void vmgenid_notify(struct device *device)
@@ -42,6 +49,13 @@ static void vmgenid_acpi_handler(acpi_handle handle, u32 event, void *dev)
vmgenid_notify(dev);
}
+static irqreturn_t vmgenid_of_irq_handler(int irq, void *dev)
+{
+ vmgenid_notify(dev);
+
+ return IRQ_HANDLED;
+}
+
static int setup_vmgenid_state(struct vmgenid_state *state, u8 *next_id)
{
if (IS_ERR(next_id))
@@ -98,6 +112,43 @@ static int vmgenid_add_acpi(struct device *dev, struct vmgenid_state *state)
return ret;
}
+static int vmgenid_add_of(struct device *dev, struct vmgenid_state *state)
+{
+ struct resource res;
+ int ret = 0;
+
+ if (of_address_to_resource(dev->of_node, 0, &res)) {
+ dev_err(dev, "Failed to get resources from device tree");
+ ret = -EINVAL;
+ goto out;
+ }
+
+ if (!__request_mem_region(res.start, resource_size(&res),
+ "vmgenid", IORESOURCE_EXCLUSIVE)) {
+ dev_err(dev, "Failed to request mem region");
+ ret = -EINVAL;
+ goto out;
+ }
+
+ ret = setup_vmgenid_state(state, of_iomap(dev->of_node, 0));
+ if (ret)
+ goto out;
+
+ state->irq = irq_of_parse_and_map(dev->of_node, 0);
+ 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;
+}
+
static int vmgenid_add(struct platform_device *pdev)
{
struct vmgenid_state *state;
@@ -108,7 +159,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,7 +170,30 @@ static int vmgenid_add(struct platform_device *pdev)
return ret;
}
-static const struct acpi_device_id vmgenid_ids[] = {
+static int vmgenid_remove(struct platform_device *pdev)
+{
+ struct vmgenid_state *state = NULL;
+
+ if (!pdev)
+ return -EINVAL;
+
+ state = pdev->dev.driver_data;
+
+ if (state && pdev->dev.of_node)
+ free_irq(state->irq, &pdev->dev);
+
+ if (state)
+ devm_kfree(&pdev->dev, state);
+
+ return 0;
+}
+
+static const struct of_device_id vmgenid_of_ids[] = {
+ { .compatible = "linux,vmgenctr", },
+ {},
+};
+
+static const struct acpi_device_id vmgenid_acpi_ids[] = {
{ "VMGENCTR", 0 },
{ "VM_GEN_COUNTER", 0 },
{ }
@@ -124,9 +201,11 @@ static const struct acpi_device_id vmgenid_ids[] = {
static struct platform_driver vmgenid_plaform_driver = {
.probe = vmgenid_add,
+ .remove = vmgenid_remove,
.driver = {
.name = "vmgenid",
- .acpi_match_table = ACPI_PTR(vmgenid_ids),
+ .acpi_match_table = ACPI_PTR(vmgenid_acpi_ids),
+ .of_match_table = vmgenid_of_ids,
.owner = THIS_MODULE,
},
};
@@ -144,7 +223,8 @@ static void vmgenid_platform_device_exit(void)
module_init(vmgenid_platform_device_init)
module_exit(vmgenid_platform_device_exit)
-MODULE_DEVICE_TABLE(acpi, vmgenid_ids);
+MODULE_DEVICE_TABLE(acpi, vmgenid_acpi_ids);
+MODULE_DEVICE_TABLE(of, vmgenid_of_ids);
MODULE_DESCRIPTION("Virtual Machine Generation ID");
MODULE_LICENSE("GPL v2");
MODULE_AUTHOR("Jason A. Donenfeld <[email protected]>");
--
2.40.1
Re-implement vmgenid as a platform driver in preparation
for adding devicetree bindings support in next commits.
Signed-off-by: Sudan Landge <[email protected]>
---
drivers/virt/vmgenid.c | 99 +++++++++++++++++++++++++++++++-----------
1 file changed, 73 insertions(+), 26 deletions(-)
diff --git a/drivers/virt/vmgenid.c b/drivers/virt/vmgenid.c
index ea956df02874..e82b344a9d61 100644
--- a/drivers/virt/vmgenid.c
+++ b/drivers/virt/vmgenid.c
@@ -11,6 +11,8 @@
#include <linux/module.h>
#include <linux/acpi.h>
#include <linux/random.h>
+#include <acpi/actypes.h>
+#include <linux/platform_device.h>
ACPI_MODULE_NAME("vmgenid");
@@ -21,9 +23,9 @@ struct vmgenid_state {
u8 this_id[VMGENID_SIZE];
};
-static void vmgenid_notify(struct acpi_device *device, u32 event)
+static void vmgenid_notify(struct device *device)
{
- struct vmgenid_state *state = acpi_driver_data(device);
+ struct vmgenid_state *state = device->driver_data;
char *envp[] = { "NEW_VMGENID=1", NULL };
u8 old_id[VMGENID_SIZE];
@@ -32,22 +34,34 @@ static void vmgenid_notify(struct acpi_device *device, u32 event)
if (!memcmp(old_id, state->this_id, sizeof(old_id)))
return;
add_vmfork_randomness(state->this_id, sizeof(state->this_id));
- kobject_uevent_env(&device->dev.kobj, KOBJ_CHANGE, envp);
+ kobject_uevent_env(&device->kobj, KOBJ_CHANGE, envp);
}
-static int vmgenid_add(struct acpi_device *device)
+static void vmgenid_acpi_handler(acpi_handle handle, u32 event, void *dev)
{
+ vmgenid_notify(dev);
+}
+
+static int setup_vmgenid_state(struct vmgenid_state *state, u8 *next_id)
+{
+ if (IS_ERR(next_id))
+ return PTR_ERR(next_id);
+
+ state->next_id = next_id;
+ memcpy(state->this_id, state->next_id, sizeof(state->this_id));
+ add_device_randomness(state->this_id, sizeof(state->this_id));
+ return 0;
+}
+
+static int vmgenid_add_acpi(struct device *dev, struct vmgenid_state *state)
+{
+ struct acpi_device *device = ACPI_COMPANION(dev);
struct acpi_buffer parsed = { ACPI_ALLOCATE_BUFFER };
- struct vmgenid_state *state;
union acpi_object *obj;
phys_addr_t phys_addr;
acpi_status status;
int ret = 0;
- state = devm_kmalloc(&device->dev, sizeof(*state), GFP_KERNEL);
- if (!state)
- return -ENOMEM;
-
status = acpi_evaluate_object(device->handle, "ADDR", NULL, &parsed);
if (ACPI_FAILURE(status)) {
ACPI_EXCEPTION((AE_INFO, status, "Evaluating ADDR"));
@@ -63,19 +77,42 @@ static int vmgenid_add(struct acpi_device *device)
phys_addr = (obj->package.elements[0].integer.value << 0) |
(obj->package.elements[1].integer.value << 32);
- state->next_id = devm_memremap(&device->dev, phys_addr, VMGENID_SIZE, MEMREMAP_WB);
- if (IS_ERR(state->next_id)) {
- ret = PTR_ERR(state->next_id);
+
+ ret = setup_vmgenid_state(state,
+ devm_memremap(&device->dev, phys_addr, VMGENID_SIZE, MEMREMAP_WB)
+ );
+ if (ret)
+ goto out;
+
+ dev->driver_data = state;
+ status = acpi_install_notify_handler(device->handle, ACPI_DEVICE_NOTIFY,
+ vmgenid_acpi_handler, dev);
+ if (ACPI_FAILURE(status)) {
+ dev_err(dev, "Failed to install acpi notify handler");
+ ret = -ENODEV;
+ dev->driver_data = NULL;
goto out;
}
+out:
+ ACPI_FREE(parsed.pointer);
+ return ret;
+}
- memcpy(state->this_id, state->next_id, sizeof(state->this_id));
- add_device_randomness(state->this_id, sizeof(state->this_id));
+static int vmgenid_add(struct platform_device *pdev)
+{
+ struct vmgenid_state *state;
+ struct device *dev = &pdev->dev;
+ int ret = 0;
- device->driver_data = state;
+ state = devm_kmalloc(dev, sizeof(*state), GFP_KERNEL);
+ if (!state)
+ return -ENOMEM;
+
+ ret = vmgenid_add_acpi(dev, state);
+
+ if (ret)
+ devm_kfree(dev, state);
-out:
- ACPI_FREE(parsed.pointer);
return ret;
}
@@ -85,17 +122,27 @@ static const struct acpi_device_id vmgenid_ids[] = {
{ }
};
-static struct acpi_driver vmgenid_driver = {
- .name = "vmgenid",
- .ids = vmgenid_ids,
- .owner = THIS_MODULE,
- .ops = {
- .add = vmgenid_add,
- .notify = vmgenid_notify
- }
+static struct platform_driver vmgenid_plaform_driver = {
+ .probe = vmgenid_add,
+ .driver = {
+ .name = "vmgenid",
+ .acpi_match_table = ACPI_PTR(vmgenid_ids),
+ .owner = THIS_MODULE,
+ },
};
-module_acpi_driver(vmgenid_driver);
+static int vmgenid_platform_device_init(void)
+{
+ return platform_driver_register(&vmgenid_plaform_driver);
+}
+
+static void vmgenid_platform_device_exit(void)
+{
+ platform_driver_unregister(&vmgenid_plaform_driver);
+}
+
+module_init(vmgenid_platform_device_init)
+module_exit(vmgenid_platform_device_exit)
MODULE_DEVICE_TABLE(acpi, vmgenid_ids);
MODULE_DESCRIPTION("Virtual Machine Generation ID");
--
2.40.1
On 19/03/2024 15:32, Sudan Landge wrote:
> This small series of patches aims to add devicetree bindings support for
> the Virtual Machine Generation ID (vmgenid) driver.
>
> Virtual Machine Generation ID driver was introduced in commit af6b54e2b5ba
> ("virt: vmgenid: notify RNG of VM fork and supply generation ID") as an
> ACPI only device.
> We would like to extend vmgenid to support devicetree bindings because:
> 1. A device should not be defined as an ACPI or DT only device.
Virtual stuff is not a device, so your first assumption or rationale is
not correct.
Virtual stuff can be ACPI only, because DT is not for Virtual stuff.
Best regards,
Krzysztof
On 19/03/2024 15:32, Sudan Landge wrote:
> Virtual Machine Generation ID driver was introduced in commit af6b54e2b5ba
> ("virt: vmgenid: notify RNG of VM fork and supply generation ID"), as an
> ACPI only device.
That's not a valid rationale. Second today... we do not add things to
bindings just because someone added some crazy or not crazy idea to Linux.
Bindings represent the hardware.
Please come with real rationale. Even if this is accepted, above reason
is just wrong and will be used as an excuse to promote more crap into
bindings.
A nit, subject: drop second/last, redundant "bindings". The
"dt-bindings" prefix is already stating that these are bindings.
See also:
https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18
Please use subject prefixes matching the subsystem. You can get them for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching.
>
> Add a devicetree binding support for vmgenid so that hypervisors
> can support vmgenid without the need to support ACPI.
Devicetree is not for virtual platforms. Virtual platform can define
whatever interface they want (virtio, ACPI, "VTree" (just invented now)).
>
> Signed-off-by: Sudan Landge <[email protected]>
> ---
> .../devicetree/bindings/vmgenid/vmgenid.yaml | 57 +++++++++++++++++++
No, you do not get your own hardware subsystem. Use existing ones.
> MAINTAINERS | 1 +
> 2 files changed, 58 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/vmgenid/vmgenid.yaml
>
> diff --git a/Documentation/devicetree/bindings/vmgenid/vmgenid.yaml b/Documentation/devicetree/bindings/vmgenid/vmgenid.yaml
> new file mode 100644
> index 000000000000..17773aa96f8b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/vmgenid/vmgenid.yaml
> @@ -0,0 +1,57 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/vmgenid/vmgenid.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Virtual Machine Generation Counter ID device.
Titles are not sentences. Drop full stop.
> +
> +maintainers:
> + - Jason A. Donenfeld <[email protected]>
> +
> +description: |+
Drop |+
> + Firmwares or hypervisors can use this devicetree to describe
> + interrupts and the shared resources to inject a Virtual Machine Generation
> + counter.
> +
> +properties:
> + compatible:
> + const: linux,vmgenctr
> +
> + "#interrupt-cells":
> + const: 3
> + description: |
> + The 1st cell is the interrupt type.
> + The 2nd cell contains the interrupt number for the interrupt type.
> + The 3rd cell is for trigger type and level flags.
> +
> + interrupt-controller: true
> +
> + reg:
> + description: |
> + specifies the base physical address and
> + size of the regions in memory which holds the VMGenID counter.
> + maxItems: 1
> +
> + interrupts:
> + description: |
> + interrupt used to notify that a new VMGenID counter is available.
> + The interrupt should be Edge triggered.
> + maxItems: 1
> +
> +required:
> + - compatible
> + - reg
> + - interrupts
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + vmgenid@80000000 {
Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
No generic name? Kind of, because *it is not a real thing*.
Best regards,
Krzysztof
On 19/03/2024 15:32, Sudan Landge 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/vmgenid/vmgenid.yaml
>
> Signed-off-by: Sudan Landge <[email protected]>
> ---
> drivers/virt/Kconfig | 2 +-
> drivers/virt/vmgenid.c | 90 +++++++++++++++++++++++++++++++++++++++---
> 2 files changed, 86 insertions(+), 6 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)
> 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 e82b344a9d61..96a3ff8ec05a 100644
> --- a/drivers/virt/vmgenid.c
> +++ b/drivers/virt/vmgenid.c
> @@ -2,7 +2,7 @@
> /*
> * Copyright (C) 2022 Jason A. Donenfeld <[email protected]>. 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.
> */
> @@ -12,6 +12,12 @@
> #include <linux/acpi.h>
> #include <linux/random.h>
> #include <acpi/actypes.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/of_address.h>
> +#include <linux/of_device.h>
> +#include <linux/of_irq.h>
> #include <linux/platform_device.h>
>
> ACPI_MODULE_NAME("vmgenid");
> @@ -21,6 +27,7 @@ enum { VMGENID_SIZE = 16 };
> struct vmgenid_state {
> u8 *next_id;
> u8 this_id[VMGENID_SIZE];
> + unsigned int irq;
> };
>
> static void vmgenid_notify(struct device *device)
> @@ -42,6 +49,13 @@ static void vmgenid_acpi_handler(acpi_handle handle, u32 event, void *dev)
> vmgenid_notify(dev);
> }
>
> +static irqreturn_t vmgenid_of_irq_handler(int irq, void *dev)
> +{
> + vmgenid_notify(dev);
> +
> + return IRQ_HANDLED;
> +}
> +
> static int setup_vmgenid_state(struct vmgenid_state *state, u8 *next_id)
> {
> if (IS_ERR(next_id))
> @@ -98,6 +112,43 @@ static int vmgenid_add_acpi(struct device *dev, struct vmgenid_state *state)
> return ret;
> }
>
> +static int vmgenid_add_of(struct device *dev, struct vmgenid_state *state)
> +{
> + struct resource res;
> + int ret = 0;
> +
> + if (of_address_to_resource(dev->of_node, 0, &res)) {
> + dev_err(dev, "Failed to get resources from device tree");
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + if (!__request_mem_region(res.start, resource_size(&res),
> + "vmgenid", IORESOURCE_EXCLUSIVE)) {
> + dev_err(dev, "Failed to request mem region");
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + ret = setup_vmgenid_state(state, of_iomap(dev->of_node, 0));
> + if (ret)
> + goto out;
> +
> + state->irq = irq_of_parse_and_map(dev->of_node, 0);
> + 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;
> +}
> +
> static int vmgenid_add(struct platform_device *pdev)
> {
> struct vmgenid_state *state;
> @@ -108,7 +159,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,7 +170,30 @@ static int vmgenid_add(struct platform_device *pdev)
> return ret;
> }
>
> -static const struct acpi_device_id vmgenid_ids[] = {
> +static int vmgenid_remove(struct platform_device *pdev)
> +{
> + struct vmgenid_state *state = NULL;
> +
How is this related to the patch? Removal is valid (or invalid)
regardless your bind method.
> + if (!pdev)
> + return -EINVAL;
> +
> + state = pdev->dev.driver_data;
> +
> + if (state && pdev->dev.of_node)
> + free_irq(state->irq, &pdev->dev);
> +
> + if (state)
> + devm_kfree(&pdev->dev, state);
Why do you free devm memory? Which driver callback allocated it?
> +
> + return 0;
> +}
> +
> +static const struct of_device_id vmgenid_of_ids[] = {
> + { .compatible = "linux,vmgenctr", },
> + {},
> +};
> +
> +static const struct acpi_device_id vmgenid_acpi_ids[] = {
> { "VMGENCTR", 0 },
> { "VM_GEN_COUNTER", 0 },
> { }
> @@ -124,9 +201,11 @@ static const struct acpi_device_id vmgenid_ids[] = {
>
> static struct platform_driver vmgenid_plaform_driver = {
> .probe = vmgenid_add,
> + .remove = vmgenid_remove,
> .driver = {
> .name = "vmgenid",
> - .acpi_match_table = ACPI_PTR(vmgenid_ids),
> + .acpi_match_table = ACPI_PTR(vmgenid_acpi_ids),
> + .of_match_table = vmgenid_of_ids,
> .owner = THIS_MODULE,
> },
> };
> @@ -144,7 +223,8 @@ static void vmgenid_platform_device_exit(void)
> module_init(vmgenid_platform_device_init)
> module_exit(vmgenid_platform_device_exit)
>
> -MODULE_DEVICE_TABLE(acpi, vmgenid_ids);
> +MODULE_DEVICE_TABLE(acpi, vmgenid_acpi_ids);
> +MODULE_DEVICE_TABLE(of, vmgenid_of_ids);
MODULE_DEVICE_TABLE goes immediately after the table.
Best regards,
Krzysztof
On 19/03/2024 15:32, Sudan Landge wrote:
>
> To check schema and syntax errors, the bindings file is verified with:
> ```
> make dt_binding_check \
> DT_SCHEMA_FILES=Documentation/devicetree/bindings/vmgenid/vmgenid.yaml
> ```
> and the patches were verified with:
> `scripts/checkpatch.pl --strict v1-000*`.
BTW, if you insist on that and claim that this is a real hardware thing,
please upstream your hardware DTS...
Best regards,
Krzysztof
Hi Sudan,
kernel test robot noticed the following build warnings:
[auto build test WARNING on robh/for-next]
[also build test WARNING on linus/master v6.8 next-20240320]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Sudan-Landge/virt-vmgenid-rearrange-code-to-make-review-easier/20240319-223642
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link: https://lore.kernel.org/r/20240319143253.22317-5-sudanl%40amazon.com
patch subject: [PATCH v1 4/4] virt: vmgenid: add support for devicetree bindings
config: alpha-allyesconfig (https://download.01.org/0day-ci/archive/20240320/[email protected]/config)
compiler: alpha-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240320/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All warnings (new ones prefixed by >>):
drivers/virt/vmgenid.c: In function 'vmgenid_add_acpi':
drivers/virt/vmgenid.c:79:45: error: invalid use of undefined type 'struct acpi_device'
79 | status = acpi_evaluate_object(device->handle, "ADDR", NULL, &parsed);
| ^~
drivers/virt/vmgenid.c:96:56: error: invalid use of undefined type 'struct acpi_device'
96 | devm_memremap(&device->dev, phys_addr, VMGENID_SIZE, MEMREMAP_WB)
| ^~
drivers/virt/vmgenid.c:102:52: error: invalid use of undefined type 'struct acpi_device'
102 | status = acpi_install_notify_handler(device->handle, ACPI_DEVICE_NOTIFY,
| ^~
drivers/virt/vmgenid.c: At top level:
>> drivers/virt/vmgenid.c:196:36: warning: 'vmgenid_acpi_ids' defined but not used [-Wunused-const-variable=]
196 | static const struct acpi_device_id vmgenid_acpi_ids[] = {
| ^~~~~~~~~~~~~~~~
vim +/vmgenid_acpi_ids +196 drivers/virt/vmgenid.c
195
> 196 static const struct acpi_device_id vmgenid_acpi_ids[] = {
197 { "VMGENCTR", 0 },
198 { "VM_GEN_COUNTER", 0 },
199 { }
200 };
201
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
On 20/03/2024 11:17, Landge, Sudan wrote:
>
> On 19/03/2024 15:28, Krzysztof Kozlowski 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.
>>
>>
Why did you remove all the people from CC list?
>>
>> On 19/03/2024 15:32, Sudan Landge wrote:
>>> Virtual Machine Generation ID driver was introduced in commit af6b54e2b5ba
>>> ("virt: vmgenid: notify RNG of VM fork and supply generation ID"), as an
>>> ACPI only device.
>> That's not a valid rationale. Second today... we do not add things to
>> bindings just because someone added some crazy or not crazy idea to Linux.
>>
>> Bindings represent the hardware.
>>
>> Please come with real rationale. Even if this is accepted, above reason
>> is just wrong and will be used as an excuse to promote more crap into
>> bindings.
>
> Thank you for the quick review.
>
> I will add more details to the problem we are trying to fix with an
> updated cover letter
>
> but to summarize the problem briefly:
>
> Firecracker is a minimalist feature hypervisor and we do not have ACPI
> support
>
> for ARM yet. The vmgenid devicetree support looked a better option because
>
> supporting ACPI on ARM means supporting UEFI which adds a lot of complexity.
That does not convince me. Amount of work for you is not making virtual
stuff real hardware. Come with some other discoverable protocol - you
have full control of both sides of this thing.
>
>> A nit, subject: drop second/last, redundant "bindings". The
>> "dt-bindings" prefix is already stating that these are bindings.
>> See also:
>> https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18
>>
>> Please use subject prefixes matching the subsystem. You can get them for
>> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
>> your patch is touching.
> Got it, thanks.
>>> Add a devicetree binding support for vmgenid so that hypervisors
>>> can support vmgenid without the need to support ACPI.
>> Devicetree is not for virtual platforms. Virtual platform can define
>> whatever interface they want (virtio, ACPI, "VTree" (just invented now)).
> Sorry for my lack of experience in this area. I took reference of virtio
> devices when I
>
> uploaded the patch. We would still like to support vmgenid via a
> devicetree so I'll
>
> revert with a new approach.
There are other solutions, I think. This was discussed already multiple
times.
>
>>> Signed-off-by: Sudan Landge<[email protected]>
>>> ---
>>> .../devicetree/bindings/vmgenid/vmgenid.yaml | 57 +++++++++++++++++++
>> No, you do not get your own hardware subsystem. Use existing ones.
>
> Got it. The changes are related to the "rng" subsystem so I'll rethink
> if that is the
>
> right place for this and revert.
Your wrapping is odd. Please use some decent email client.
Anyway, I am not discussing topics semi-private. Keep all maintainers in
loop.
Best regards,
Krzysztof
Hi Krzysztof,
The recipient were removed by mistake. I have added them all back and
fixed the email client to send mail in the right format. Sorry about that.
I'll revert after I have done more analysis and better data to explain.
Thank you for the quick feedback again.
Thanks and regards,
Sudan
On 20/03/2024 10:24, Krzysztof Kozlowski 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 20/03/2024 11:17, Landge, Sudan wrote:
>>
>> On 19/03/2024 15:28, Krzysztof Kozlowski 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.
>>>
>>>
>
> Why did you remove all the people from CC list?
> >>>
>>> On 19/03/2024 15:32, Sudan Landge wrote:
>>>> Virtual Machine Generation ID driver was introduced in commit af6b54e2b5ba
>>>> ("virt: vmgenid: notify RNG of VM fork and supply generation ID"), as an
>>>> ACPI only device.
>>> That's not a valid rationale. Second today... we do not add things to
>>> bindings just because someone added some crazy or not crazy idea to Linux.
>>>
>>> Bindings represent the hardware.
>>>
>>> Please come with real rationale. Even if this is accepted, above reason
>>> is just wrong and will be used as an excuse to promote more crap into
>>> bindings.
>>
>> Thank you for the quick review.
>>
>> I will add more details to the problem we are trying to fix with an
>> updated cover letter
>>
>> but to summarize the problem briefly:
>>
>> Firecracker is a minimalist feature hypervisor and we do not have ACPI
>> support
>>
>> for ARM yet. The vmgenid devicetree support looked a better option because
>>
>> supporting ACPI on ARM means supporting UEFI which adds a lot of complexity.
>
> That does not convince me. Amount of work for you is not making virtual
> stuff real hardware. Come with some other discoverable protocol - you
> have full control of both sides of this thing.
>
>>
>>> A nit, subject: drop second/last, redundant "bindings". The
>>> "dt-bindings" prefix is already stating that these are bindings.
>>> See also:
>>> https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18
>>>
>>> Please use subject prefixes matching the subsystem. You can get them for
>>> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
>>> your patch is touching.
>> Got it, thanks.
>>>> Add a devicetree binding support for vmgenid so that hypervisors
>>>> can support vmgenid without the need to support ACPI.
>>> Devicetree is not for virtual platforms. Virtual platform can define
>>> whatever interface they want (virtio, ACPI, "VTree" (just invented now)).
>> Sorry for my lack of experience in this area. I took reference of virtio
>> devices when I
>>
>> uploaded the patch. We would still like to support vmgenid via a
>> devicetree so I'll
>>
>> revert with a new approach.
>
> There are other solutions, I think. This was discussed already multiple
> times.
>
>>
>>>> Signed-off-by: Sudan Landge<[email protected]>
>>>> ---
>>>> .../devicetree/bindings/vmgenid/vmgenid.yaml | 57 +++++++++++++++++++
>>> No, you do not get your own hardware subsystem. Use existing ones.
>>
>> Got it. The changes are related to the "rng" subsystem so I'll rethink
>> if that is the
>>
>> right place for this and revert.
>
> Your wrapping is odd. Please use some decent email client.
>
> Anyway, I am not discussing topics semi-private. Keep all maintainers in
> loop.
>
> Best regards,
> Krzysztof
>
Hi Sudan,
kernel test robot noticed the following build warnings:
[auto build test WARNING on robh/for-next]
[also build test WARNING on linus/master v6.8 next-20240320]
[cannot apply to crng-random/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Sudan-Landge/virt-vmgenid-rearrange-code-to-make-review-easier/20240319-223642
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link: https://lore.kernel.org/r/20240319143253.22317-5-sudanl%40amazon.com
patch subject: [PATCH v1 4/4] virt: vmgenid: add support for devicetree bindings
config: x86_64-randconfig-121-20240320 (https://download.01.org/0day-ci/archive/20240320/[email protected]/config)
compiler: gcc-7 (Ubuntu 7.5.0-6ubuntu2) 7.5.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240320/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
sparse warnings: (new ones prefixed by >>)
>> drivers/virt/vmgenid.c:133:50: sparse: sparse: incorrect type in argument 2 (different address spaces) @@ expected unsigned char [usertype] *next_id @@ got void [noderef] __iomem * @@
drivers/virt/vmgenid.c:133:50: sparse: expected unsigned char [usertype] *next_id
drivers/virt/vmgenid.c:133:50: sparse: got void [noderef] __iomem *
vim +133 drivers/virt/vmgenid.c
114
115 static int vmgenid_add_of(struct device *dev, struct vmgenid_state *state)
116 {
117 struct resource res;
118 int ret = 0;
119
120 if (of_address_to_resource(dev->of_node, 0, &res)) {
121 dev_err(dev, "Failed to get resources from device tree");
122 ret = -EINVAL;
123 goto out;
124 }
125
126 if (!__request_mem_region(res.start, resource_size(&res),
127 "vmgenid", IORESOURCE_EXCLUSIVE)) {
128 dev_err(dev, "Failed to request mem region");
129 ret = -EINVAL;
130 goto out;
131 }
132
> 133 ret = setup_vmgenid_state(state, of_iomap(dev->of_node, 0));
134 if (ret)
135 goto out;
136
137 state->irq = irq_of_parse_and_map(dev->of_node, 0);
138 dev->driver_data = state;
139
140 if (request_irq(state->irq, vmgenid_of_irq_handler,
141 IRQF_SHARED, "vmgenid", dev) < 0) {
142 dev_err(dev, "request_irq failed");
143 dev->driver_data = NULL;
144 ret = -EINVAL;
145 goto out;
146 }
147
148 out:
149 return ret;
150 }
151
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
On Tue, 2024-03-19 at 16:24 +0100, Krzysztof Kozlowski wrote:
> On 19/03/2024 15:32, Sudan Landge wrote:
> > This small series of patches aims to add devicetree bindings support for
> > the Virtual Machine Generation ID (vmgenid) driver.
> >
> > Virtual Machine Generation ID driver was introduced in commit af6b54e2b5ba
> > ("virt: vmgenid: notify RNG of VM fork and supply generation ID") as an
> > ACPI only device.
> > We would like to extend vmgenid to support devicetree bindings because:
> > 1. A device should not be defined as an ACPI or DT only device.
>
> Virtual stuff is not a device, so your first assumption or rationale is
> not correct.
>
> Virtual stuff can be ACPI only, because DT is not for Virtual stuff.
I strongly disagree with this.
Discovering things is what the device-tree is *for*.
We don't want to add extra complexity and overhead on both host and
guest side to make things discoverable in a *less* efficient way.
The device-tree isn't just a last-resort for when we can't possibly do
things differently in a discoverable way. The device-tree is a first-
class citizen and perfectly valid choice as a way to discover things.
We shouldn't be forcing people to turn things into PCI devices just to
avoid adding DT bindings for them.
And we *certainly* shouldn't be directing people towards all the
awfulness of ACPI, and in-kernel bytecode interpreters, and all that
horridness, just because we don't want to use DT to... describe things.
On Wed, Mar 20, 2024 at 01:50:43PM +0000, David Woodhouse wrote:
> On Tue, 2024-03-19 at 16:24 +0100, Krzysztof Kozlowski wrote:
> > On 19/03/2024 15:32, Sudan Landge wrote:
> > > This small series of patches aims to add devicetree bindings support for
> > > the Virtual Machine Generation ID (vmgenid) driver.
> > >
> > > Virtual Machine Generation ID driver was introduced in commit af6b54e2b5ba
> > > ("virt: vmgenid: notify RNG of VM fork and supply generation ID") as an
> > > ACPI only device.
> > > We would like to extend vmgenid to support devicetree bindings because:
> > > 1. A device should not be defined as an ACPI or DT only device.
This (and the binding patch) tells me nothing about what "Virtual
Machine Generation ID driver" is and isn't really justification for
"why".
> >
> > Virtual stuff is not a device, so your first assumption or rationale is
> > not correct.
> >
> > Virtual stuff can be ACPI only, because DT is not for Virtual stuff.
>
> I strongly disagree with this.
>
> Discovering things is what the device-tree is *for*.
DT/ACPI is for discovering what hardware folks failed to make
discoverable. But here, both sides are software. Can't the software
folks do better?
This is just the latest in $hypervisor bindings[1][2][3]. The value add
must be hypervisors because every SoC vendor seems to be creating their
own with their own interfaces.
> We don't want to add extra complexity and overhead on both host and
> guest side to make things discoverable in a *less* efficient way.
>
> The device-tree isn't just a last-resort for when we can't possibly do
> things differently in a discoverable way. The device-tree is a first-
> class citizen and perfectly valid choice as a way to discover things.
>
> We shouldn't be forcing people to turn things into PCI devices just to
> avoid adding DT bindings for them.
>
> And we *certainly* shouldn't be directing people towards all the
> awfulness of ACPI, and in-kernel bytecode interpreters, and all that
> horridness, just because we don't want to use DT to... describe things.
I assume you have other calls into the hypervisor and notifications from
the hypervisor? Are you going to add DT nodes for each one? I'd be more
comfortable with DT describing THE communication channel with the
hypervisor than what sounds like a singular function. Otherwise, what's
the next binding?
Rob
[1] https://lore.kernel.org/all/[email protected]/
[2] https://lore.kernel.org/all/[email protected]/
[3] https://lore.kernel.org/all/[email protected]/
Hi Sudan,
kernel test robot noticed the following build errors:
[auto build test ERROR on robh/for-next]
[also build test ERROR on linus/master v6.8 next-20240320]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Sudan-Landge/virt-vmgenid-rearrange-code-to-make-review-easier/20240319-223642
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link: https://lore.kernel.org/r/20240319143253.22317-5-sudanl%40amazon.com
patch subject: [PATCH v1 4/4] virt: vmgenid: add support for devicetree bindings
config: hexagon-allmodconfig (https://download.01.org/0day-ci/archive/20240321/[email protected]/config)
compiler: clang version 19.0.0git (https://github.com/llvm/llvm-project 8f68022f8e6e54d1aeae4ed301f5a015963089b7)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240321/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All errors (new ones prefixed by >>):
In file included from drivers/virt/vmgenid.c:16:
In file included from include/linux/interrupt.h:11:
In file included from include/linux/hardirq.h:11:
In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/hexagon/include/asm/io.h:328:
include/asm-generic/io.h:547:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
547 | val = __raw_readb(PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:560:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
560 | val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
| ~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
37 | #define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
| ^
In file included from drivers/virt/vmgenid.c:16:
In file included from include/linux/interrupt.h:11:
In file included from include/linux/hardirq.h:11:
In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/hexagon/include/asm/io.h:328:
include/asm-generic/io.h:573:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
573 | val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
| ~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
35 | #define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
| ^
In file included from drivers/virt/vmgenid.c:16:
In file included from include/linux/interrupt.h:11:
In file included from include/linux/hardirq.h:11:
In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/hexagon/include/asm/io.h:328:
include/asm-generic/io.h:584:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
584 | __raw_writeb(value, PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:594:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
594 | __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:604:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
604 | __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
>> drivers/virt/vmgenid.c:79:38: error: incomplete definition of type 'struct acpi_device'
79 | status = acpi_evaluate_object(device->handle, "ADDR", NULL, &parsed);
| ~~~~~~^
include/linux/acpi.h:795:8: note: forward declaration of 'struct acpi_device'
795 | struct acpi_device;
| ^
drivers/virt/vmgenid.c:96:28: error: incomplete definition of type 'struct acpi_device'
96 | devm_memremap(&device->dev, phys_addr, VMGENID_SIZE, MEMREMAP_WB)
| ~~~~~~^
include/linux/acpi.h:795:8: note: forward declaration of 'struct acpi_device'
795 | struct acpi_device;
| ^
drivers/virt/vmgenid.c:102:45: error: incomplete definition of type 'struct acpi_device'
102 | status = acpi_install_notify_handler(device->handle, ACPI_DEVICE_NOTIFY,
| ~~~~~~^
include/linux/acpi.h:795:8: note: forward declaration of 'struct acpi_device'
795 | struct acpi_device;
| ^
6 warnings and 3 errors generated.
vim +79 drivers/virt/vmgenid.c
657fab4d1001e1 Sudan Landge 2024-03-19 69
657fab4d1001e1 Sudan Landge 2024-03-19 70 static int vmgenid_add_acpi(struct device *dev, struct vmgenid_state *state)
657fab4d1001e1 Sudan Landge 2024-03-19 71 {
657fab4d1001e1 Sudan Landge 2024-03-19 72 struct acpi_device *device = ACPI_COMPANION(dev);
af6b54e2b5baa5 Jason A. Donenfeld 2022-02-23 73 struct acpi_buffer parsed = { ACPI_ALLOCATE_BUFFER };
af6b54e2b5baa5 Jason A. Donenfeld 2022-02-23 74 union acpi_object *obj;
af6b54e2b5baa5 Jason A. Donenfeld 2022-02-23 75 phys_addr_t phys_addr;
af6b54e2b5baa5 Jason A. Donenfeld 2022-02-23 76 acpi_status status;
af6b54e2b5baa5 Jason A. Donenfeld 2022-02-23 77 int ret = 0;
af6b54e2b5baa5 Jason A. Donenfeld 2022-02-23 78
af6b54e2b5baa5 Jason A. Donenfeld 2022-02-23 @79 status = acpi_evaluate_object(device->handle, "ADDR", NULL, &parsed);
af6b54e2b5baa5 Jason A. Donenfeld 2022-02-23 80 if (ACPI_FAILURE(status)) {
af6b54e2b5baa5 Jason A. Donenfeld 2022-02-23 81 ACPI_EXCEPTION((AE_INFO, status, "Evaluating ADDR"));
af6b54e2b5baa5 Jason A. Donenfeld 2022-02-23 82 return -ENODEV;
af6b54e2b5baa5 Jason A. Donenfeld 2022-02-23 83 }
af6b54e2b5baa5 Jason A. Donenfeld 2022-02-23 84 obj = parsed.pointer;
af6b54e2b5baa5 Jason A. Donenfeld 2022-02-23 85 if (!obj || obj->type != ACPI_TYPE_PACKAGE || obj->package.count != 2 ||
af6b54e2b5baa5 Jason A. Donenfeld 2022-02-23 86 obj->package.elements[0].type != ACPI_TYPE_INTEGER ||
af6b54e2b5baa5 Jason A. Donenfeld 2022-02-23 87 obj->package.elements[1].type != ACPI_TYPE_INTEGER) {
af6b54e2b5baa5 Jason A. Donenfeld 2022-02-23 88 ret = -EINVAL;
af6b54e2b5baa5 Jason A. Donenfeld 2022-02-23 89 goto out;
af6b54e2b5baa5 Jason A. Donenfeld 2022-02-23 90 }
af6b54e2b5baa5 Jason A. Donenfeld 2022-02-23 91
af6b54e2b5baa5 Jason A. Donenfeld 2022-02-23 92 phys_addr = (obj->package.elements[0].integer.value << 0) |
af6b54e2b5baa5 Jason A. Donenfeld 2022-02-23 93 (obj->package.elements[1].integer.value << 32);
657fab4d1001e1 Sudan Landge 2024-03-19 94
657fab4d1001e1 Sudan Landge 2024-03-19 95 ret = setup_vmgenid_state(state,
657fab4d1001e1 Sudan Landge 2024-03-19 96 devm_memremap(&device->dev, phys_addr, VMGENID_SIZE, MEMREMAP_WB)
657fab4d1001e1 Sudan Landge 2024-03-19 97 );
657fab4d1001e1 Sudan Landge 2024-03-19 98 if (ret)
657fab4d1001e1 Sudan Landge 2024-03-19 99 goto out;
657fab4d1001e1 Sudan Landge 2024-03-19 100
657fab4d1001e1 Sudan Landge 2024-03-19 101 dev->driver_data = state;
657fab4d1001e1 Sudan Landge 2024-03-19 102 status = acpi_install_notify_handler(device->handle, ACPI_DEVICE_NOTIFY,
657fab4d1001e1 Sudan Landge 2024-03-19 103 vmgenid_acpi_handler, dev);
657fab4d1001e1 Sudan Landge 2024-03-19 104 if (ACPI_FAILURE(status)) {
657fab4d1001e1 Sudan Landge 2024-03-19 105 dev_err(dev, "Failed to install acpi notify handler");
657fab4d1001e1 Sudan Landge 2024-03-19 106 ret = -ENODEV;
657fab4d1001e1 Sudan Landge 2024-03-19 107 dev->driver_data = NULL;
af6b54e2b5baa5 Jason A. Donenfeld 2022-02-23 108 goto out;
af6b54e2b5baa5 Jason A. Donenfeld 2022-02-23 109 }
657fab4d1001e1 Sudan Landge 2024-03-19 110 out:
657fab4d1001e1 Sudan Landge 2024-03-19 111 ACPI_FREE(parsed.pointer);
657fab4d1001e1 Sudan Landge 2024-03-19 112 return ret;
657fab4d1001e1 Sudan Landge 2024-03-19 113 }
af6b54e2b5baa5 Jason A. Donenfeld 2022-02-23 114
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
On Wed, 2024-03-20 at 11:15 -0500, Rob Herring wrote:
> On Wed, Mar 20, 2024 at 01:50:43PM +0000, David Woodhouse wrote:
> > On Tue, 2024-03-19 at 16:24 +0100, Krzysztof Kozlowski wrote:
> > > On 19/03/2024 15:32, Sudan Landge wrote:
> > > > This small series of patches aims to add devicetree bindings support for
> > > > the Virtual Machine Generation ID (vmgenid) driver.
> > > >
> > > > Virtual Machine Generation ID driver was introduced in commit af6b54e2b5ba
> > > > ("virt: vmgenid: notify RNG of VM fork and supply generation ID") as an
> > > > ACPI only device.
> > > > We would like to extend vmgenid to support devicetree bindings because:
> > > > 1. A device should not be defined as an ACPI or DT only device.
>
> This (and the binding patch) tells me nothing about what "Virtual
> Machine Generation ID driver" is and isn't really justification for
> "why".
It's a reference to a memory area which the OS can use to tell whether
it's been snapshotted and restored (or 'forked'). A future submission
should have a reference to something like
https://www.qemu.org/docs/master/specs/vmgenid.html or the Microsoft
doc which is linked from there.
> DT/ACPI is for discovering what hardware folks failed to make
> discoverable. But here, both sides are software. Can't the software
> folks do better?
We are. Using device-tree *is* better. :)
> This is just the latest in $hypervisor bindings[1][2][3]. The value add
> must be hypervisors because every SoC vendor seems to be creating their
> own with their own interfaces.
The VMGenId one is cross-platform; we don't *want* to reinvent the
wheel there. We just want to discover that same memory area with
precisely the same semantics, but through the device-tree instead of
being forced to shoe-horn the whole of the ACPI horridness into a
platform which doesn't need it. (Or make it the BAR of a newly-invented
PCI device and have to add PCI to a microVM platform which doesn't
otherwise need it, etc.)
> I assume you have other calls into the hypervisor and notifications from
> the hypervisor? Are you going to add DT nodes for each one? I'd be more
> comfortable with DT describing THE communication channel with the
> hypervisor than what sounds like a singular function.
This isn't hypervisor-specific. There is a memory region with certain
semantics which may exist on all kinds of platforms, and we're just
allowing the guest to discover where it is. I don't see how it fits
into the model you're describing above.
> Otherwise, what's the next binding?
You meant that last as a rhetorical question, but I'll answer it
anyway. The thing I'm actually working on this week is a mechanism to
expose clock synchronisation (since it's kind of pointless for *all* of
the guests running on a host to run NTP/PTP/PPS/whatever to calibrate
the *same* underlying oscillator).
As far as the *discoverability* is concerned, it's fundamentally the
same thing — just a memory region with certain defined semantics, and
probably an interrupt for when the contents change.
There *isn't* an ACPI specification for that one already; I was
thinking of describing it *only* in DT and if someone wants it on a
platform which is afflicted with ACPI, they can just do it in a PRP0001
device.
As with vmgenid, there's really very little benefit to wrapping a whole
bunch of pointless emulated "hardware discoverability" around it, when
it can just be described by ACPI/DT directly. That's what they're
*for*.
Hi Sudan,
kernel test robot noticed the following build errors:
[auto build test ERROR on robh/for-next]
[also build test ERROR on linus/master v6.8 next-20240320]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Sudan-Landge/virt-vmgenid-rearrange-code-to-make-review-easier/20240319-223642
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link: https://lore.kernel.org/r/20240319143253.22317-5-sudanl%40amazon.com
patch subject: [PATCH v1 4/4] virt: vmgenid: add support for devicetree bindings
config: alpha-allyesconfig (https://download.01.org/0day-ci/archive/20240321/[email protected]/config)
compiler: alpha-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240321/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All errors (new ones prefixed by >>):
drivers/virt/vmgenid.c: In function 'vmgenid_add_acpi':
>> drivers/virt/vmgenid.c:79:45: error: invalid use of undefined type 'struct acpi_device'
79 | status = acpi_evaluate_object(device->handle, "ADDR", NULL, &parsed);
| ^~
drivers/virt/vmgenid.c:96:56: error: invalid use of undefined type 'struct acpi_device'
96 | devm_memremap(&device->dev, phys_addr, VMGENID_SIZE, MEMREMAP_WB)
| ^~
drivers/virt/vmgenid.c:102:52: error: invalid use of undefined type 'struct acpi_device'
102 | status = acpi_install_notify_handler(device->handle, ACPI_DEVICE_NOTIFY,
| ^~
drivers/virt/vmgenid.c: At top level:
drivers/virt/vmgenid.c:196:36: warning: 'vmgenid_acpi_ids' defined but not used [-Wunused-const-variable=]
196 | static const struct acpi_device_id vmgenid_acpi_ids[] = {
| ^~~~~~~~~~~~~~~~
vim +79 drivers/virt/vmgenid.c
657fab4d1001e1 Sudan Landge 2024-03-19 69
657fab4d1001e1 Sudan Landge 2024-03-19 70 static int vmgenid_add_acpi(struct device *dev, struct vmgenid_state *state)
657fab4d1001e1 Sudan Landge 2024-03-19 71 {
657fab4d1001e1 Sudan Landge 2024-03-19 72 struct acpi_device *device = ACPI_COMPANION(dev);
af6b54e2b5baa5 Jason A. Donenfeld 2022-02-23 73 struct acpi_buffer parsed = { ACPI_ALLOCATE_BUFFER };
af6b54e2b5baa5 Jason A. Donenfeld 2022-02-23 74 union acpi_object *obj;
af6b54e2b5baa5 Jason A. Donenfeld 2022-02-23 75 phys_addr_t phys_addr;
af6b54e2b5baa5 Jason A. Donenfeld 2022-02-23 76 acpi_status status;
af6b54e2b5baa5 Jason A. Donenfeld 2022-02-23 77 int ret = 0;
af6b54e2b5baa5 Jason A. Donenfeld 2022-02-23 78
af6b54e2b5baa5 Jason A. Donenfeld 2022-02-23 @79 status = acpi_evaluate_object(device->handle, "ADDR", NULL, &parsed);
af6b54e2b5baa5 Jason A. Donenfeld 2022-02-23 80 if (ACPI_FAILURE(status)) {
af6b54e2b5baa5 Jason A. Donenfeld 2022-02-23 81 ACPI_EXCEPTION((AE_INFO, status, "Evaluating ADDR"));
af6b54e2b5baa5 Jason A. Donenfeld 2022-02-23 82 return -ENODEV;
af6b54e2b5baa5 Jason A. Donenfeld 2022-02-23 83 }
af6b54e2b5baa5 Jason A. Donenfeld 2022-02-23 84 obj = parsed.pointer;
af6b54e2b5baa5 Jason A. Donenfeld 2022-02-23 85 if (!obj || obj->type != ACPI_TYPE_PACKAGE || obj->package.count != 2 ||
af6b54e2b5baa5 Jason A. Donenfeld 2022-02-23 86 obj->package.elements[0].type != ACPI_TYPE_INTEGER ||
af6b54e2b5baa5 Jason A. Donenfeld 2022-02-23 87 obj->package.elements[1].type != ACPI_TYPE_INTEGER) {
af6b54e2b5baa5 Jason A. Donenfeld 2022-02-23 88 ret = -EINVAL;
af6b54e2b5baa5 Jason A. Donenfeld 2022-02-23 89 goto out;
af6b54e2b5baa5 Jason A. Donenfeld 2022-02-23 90 }
af6b54e2b5baa5 Jason A. Donenfeld 2022-02-23 91
af6b54e2b5baa5 Jason A. Donenfeld 2022-02-23 92 phys_addr = (obj->package.elements[0].integer.value << 0) |
af6b54e2b5baa5 Jason A. Donenfeld 2022-02-23 93 (obj->package.elements[1].integer.value << 32);
657fab4d1001e1 Sudan Landge 2024-03-19 94
657fab4d1001e1 Sudan Landge 2024-03-19 95 ret = setup_vmgenid_state(state,
657fab4d1001e1 Sudan Landge 2024-03-19 96 devm_memremap(&device->dev, phys_addr, VMGENID_SIZE, MEMREMAP_WB)
657fab4d1001e1 Sudan Landge 2024-03-19 97 );
657fab4d1001e1 Sudan Landge 2024-03-19 98 if (ret)
657fab4d1001e1 Sudan Landge 2024-03-19 99 goto out;
657fab4d1001e1 Sudan Landge 2024-03-19 100
657fab4d1001e1 Sudan Landge 2024-03-19 101 dev->driver_data = state;
657fab4d1001e1 Sudan Landge 2024-03-19 102 status = acpi_install_notify_handler(device->handle, ACPI_DEVICE_NOTIFY,
657fab4d1001e1 Sudan Landge 2024-03-19 103 vmgenid_acpi_handler, dev);
657fab4d1001e1 Sudan Landge 2024-03-19 104 if (ACPI_FAILURE(status)) {
657fab4d1001e1 Sudan Landge 2024-03-19 105 dev_err(dev, "Failed to install acpi notify handler");
657fab4d1001e1 Sudan Landge 2024-03-19 106 ret = -ENODEV;
657fab4d1001e1 Sudan Landge 2024-03-19 107 dev->driver_data = NULL;
af6b54e2b5baa5 Jason A. Donenfeld 2022-02-23 108 goto out;
af6b54e2b5baa5 Jason A. Donenfeld 2022-02-23 109 }
657fab4d1001e1 Sudan Landge 2024-03-19 110 out:
657fab4d1001e1 Sudan Landge 2024-03-19 111 ACPI_FREE(parsed.pointer);
657fab4d1001e1 Sudan Landge 2024-03-19 112 return ret;
657fab4d1001e1 Sudan Landge 2024-03-19 113 }
af6b54e2b5baa5 Jason A. Donenfeld 2022-02-23 114
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
On Wed, Mar 20, 2024 at 04:55:45PM +0000, David Woodhouse wrote:
> On Wed, 2024-03-20 at 11:15 -0500, Rob Herring wrote:
> > On Wed, Mar 20, 2024 at 01:50:43PM +0000, David Woodhouse wrote:
> > > On Tue, 2024-03-19 at 16:24 +0100, Krzysztof Kozlowski wrote:
> > > > On 19/03/2024 15:32, Sudan Landge wrote:
> > > > > This small series of patches aims to add devicetree bindings support for
> > > > > the Virtual Machine Generation ID (vmgenid) driver.
> > > > >
> > > > > Virtual Machine Generation ID driver was introduced in commit af6b54e2b5ba
> > > > > ("virt: vmgenid: notify RNG of VM fork and supply generation ID") as an
> > > > > ACPI only device.
> > > > > We would like to extend vmgenid to support devicetree bindings because:
> > > > > 1. A device should not be defined as an ACPI or DT only device.
> >
> > This (and the binding patch) tells me nothing about what "Virtual
> > Machine Generation ID driver" is and isn't really justification for
> > "why".
>
> It's a reference to a memory area which the OS can use to tell whether
> it's been snapshotted and restored (or 'forked'). A future submission
> should have a reference to something like
> https://www.qemu.org/docs/master/specs/vmgenid.html or the Microsoft
> doc which is linked from there.
That doc mentions fw_cfg for which we already have a binding. Why can't
it be used/extended here?
Rob
[1] https://www.kernel.org/doc/Documentation/devicetree/bindings/firmware/qemu%2Cfw-cfg-mmio.yaml
On 21/03/2024 13:32, 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 04:55:45PM +0000, David Woodhouse wrote:
>> On Wed, 2024-03-20 at 11:15 -0500, Rob Herring wrote:
>>> On Wed, Mar 20, 2024 at 01:50:43PM +0000, David Woodhouse wrote:
>>>> On Tue, 2024-03-19 at 16:24 +0100, Krzysztof Kozlowski wrote:
>>>>> On 19/03/2024 15:32, Sudan Landge wrote:
>>>>>> This small series of patches aims to add devicetree bindings support for
>>>>>> the Virtual Machine Generation ID (vmgenid) driver.
>>>>>>
>>>>>> Virtual Machine Generation ID driver was introduced in commit af6b54e2b5ba
>>>>>> ("virt: vmgenid: notify RNG of VM fork and supply generation ID") as an
>>>>>> ACPI only device.
>>>>>> We would like to extend vmgenid to support devicetree bindings because:
>>>>>> 1. A device should not be defined as an ACPI or DT only device.
>>>
>>> This (and the binding patch) tells me nothing about what "Virtual
>>> Machine Generation ID driver" is and isn't really justification for
>>> "why".
>>
>> It's a reference to a memory area which the OS can use to tell whether
>> it's been snapshotted and restored (or 'forked'). A future submission
>> should have a reference to something like
>> https://www.qemu.org/docs/master/specs/vmgenid.html or the Microsoft
>> doc which is linked from there.
>
> That doc mentions fw_cfg for which we already have a binding. Why can't
> it be used/extended here?
QEMU has support for vmgenid but even they do not pass vmgenid directly
to the guest kernel using fw_cfg. QEMU passes the vmgenid/UUID via
fw_cfg to an intermediate UEFI firmware. This UEFI firmware, running as
a guest in QEMU, reads the UUID from fw_cfg and creates ACPI tables for
it. The UEFI firmware then passes the UUID information to the guest
kernel via ACPI.
This approach requires yet another intermediary which is UEFI firmware
and adds more complexity than ACPI for minimalist hypervisors that do
not have an intermediate bootloader today.
>
> Rob
>
> [1] https://www.kernel.org/doc/Documentation/devicetree/bindings/firmware/qemu%2Cfw-cfg-mmio.yaml
On 21/03/2024 18:39, Landge, Sudan wrote:
>
>
> On 21/03/2024 13:32, 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 04:55:45PM +0000, David Woodhouse wrote:
>>> On Wed, 2024-03-20 at 11:15 -0500, Rob Herring wrote:
>>>> On Wed, Mar 20, 2024 at 01:50:43PM +0000, David Woodhouse wrote:
>>>>> On Tue, 2024-03-19 at 16:24 +0100, Krzysztof Kozlowski wrote:
>>>>>> On 19/03/2024 15:32, Sudan Landge wrote:
>>>>>>> This small series of patches aims to add devicetree bindings support for
>>>>>>> the Virtual Machine Generation ID (vmgenid) driver.
>>>>>>>
>>>>>>> Virtual Machine Generation ID driver was introduced in commit af6b54e2b5ba
>>>>>>> ("virt: vmgenid: notify RNG of VM fork and supply generation ID") as an
>>>>>>> ACPI only device.
>>>>>>> We would like to extend vmgenid to support devicetree bindings because:
>>>>>>> 1. A device should not be defined as an ACPI or DT only device.
>>>>
>>>> This (and the binding patch) tells me nothing about what "Virtual
>>>> Machine Generation ID driver" is and isn't really justification for
>>>> "why".
>>>
>>> It's a reference to a memory area which the OS can use to tell whether
>>> it's been snapshotted and restored (or 'forked'). A future submission
>>> should have a reference to something like
>>> https://www.qemu.org/docs/master/specs/vmgenid.html or the Microsoft
>>> doc which is linked from there.
>>
>> That doc mentions fw_cfg for which we already have a binding. Why can't
>> it be used/extended here?
> QEMU has support for vmgenid but even they do not pass vmgenid directly
> to the guest kernel using fw_cfg. QEMU passes the vmgenid/UUID via
> fw_cfg to an intermediate UEFI firmware. This UEFI firmware, running as
> a guest in QEMU, reads the UUID from fw_cfg and creates ACPI tables for
> it. The UEFI firmware then passes the UUID information to the guest
> kernel via ACPI.
> This approach requires yet another intermediary which is UEFI firmware
> and adds more complexity than ACPI for minimalist hypervisors that do
> not have an intermediate bootloader today.
What stops you from passing fw_cfg not to UEFI FW? BTW, no actual VM
name was used in your posting, but now suddenly it is a talk about QEMU.
Best regards,
Krzysztof
On Fri, 2024-03-22 at 06:40 +0100, Krzysztof Kozlowski wrote:
> On 21/03/2024 18:39, Landge, Sudan wrote:
> >
> >
> > On 21/03/2024 13:32, 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 04:55:45PM +0000, David Woodhouse wrote:
> > > > On Wed, 2024-03-20 at 11:15 -0500, Rob Herring wrote:
> > > > > On Wed, Mar 20, 2024 at 01:50:43PM +0000, David Woodhouse wrote:
> > > > > > On Tue, 2024-03-19 at 16:24 +0100, Krzysztof Kozlowski wrote:
> > > > > > > On 19/03/2024 15:32, Sudan Landge wrote:
> > > > > > > > This small series of patches aims to add devicetree bindings support for
> > > > > > > > the Virtual Machine Generation ID (vmgenid) driver.
> > > > > > > >
> > > > > > > > Virtual Machine Generation ID driver was introduced in commit af6b54e2b5ba
> > > > > > > > ("virt: vmgenid: notify RNG of VM fork and supply generation ID") as an
> > > > > > > > ACPI only device.
> > > > > > > > We would like to extend vmgenid to support devicetree bindings because:
> > > > > > > > 1. A device should not be defined as an ACPI or DT only device.
> > > > >
> > > > > This (and the binding patch) tells me nothing about what "Virtual
> > > > > Machine Generation ID driver" is and isn't really justification for
> > > > > "why".
> > > >
> > > > It's a reference to a memory area which the OS can use to tell whether
> > > > it's been snapshotted and restored (or 'forked'). A future submission
> > > > should have a reference to something like
> > > > https://www.qemu.org/docs/master/specs/vmgenid.html or the Microsoft
> > > > doc which is linked from there.
> > >
> > > That doc mentions fw_cfg for which we already have a binding. Why can't
> > > it be used/extended here?
> > QEMU has support for vmgenid but even they do not pass vmgenid directly
> > to the guest kernel using fw_cfg. QEMU passes the vmgenid/UUID via
> > fw_cfg to an intermediate UEFI firmware. This UEFI firmware, running as
> > a guest in QEMU, reads the UUID from fw_cfg and creates ACPI tables for
> > it. The UEFI firmware then passes the UUID information to the guest
> > kernel via ACPI.
> > This approach requires yet another intermediary which is UEFI firmware
> > and adds more complexity than ACPI for minimalist hypervisors that do
> > not have an intermediate bootloader today.
>
> What stops you from passing fw_cfg not to UEFI FW? BTW, no actual VM
> name was used in your posting, but now suddenly it is a talk about QEMU.
That would be possible. But not ideal. Just as exposing it via PCI
would be possible, but not ideal. Or forcing ACPI onto the guests in
question, and various other less efficient options.
If what we're really looking at here is a hostile takeover of the DT
bindings repository, with a blanket "No, DT is dead. Go use something
else, preferably ACPI", than all those other options are possible. We
*never* have to add a new binding to DT ever again. Let's just set the
existing bindings in stone and move on.
But hopefully that isn't the case. DT is the simplest and most
effective way to provide discovery, especially for embedded and microVM
systems. It isn't just a *workaround* for broken hardware which *can't*
do a slower and more complex form of discovery.
And it's absolutely the right thing to do for exposing the equivalent
of the ACPI vmgenid device in a system which isn't afflicted by ACPI
and doesn't *want* to be.
On Fri, Mar 22, 2024 at 3:21 AM David Woodhouse <[email protected]> wrote:
>
> On Fri, 2024-03-22 at 06:40 +0100, Krzysztof Kozlowski wrote:
> > On 21/03/2024 18:39, Landge, Sudan wrote:
> > >
> > >
> > > On 21/03/2024 13:32, 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 04:55:45PM +0000, David Woodhouse wrote:
> > > > > On Wed, 2024-03-20 at 11:15 -0500, Rob Herring wrote:
> > > > > > On Wed, Mar 20, 2024 at 01:50:43PM +0000, David Woodhouse wrote:
> > > > > > > On Tue, 2024-03-19 at 16:24 +0100, Krzysztof Kozlowski wrote:
> > > > > > > > On 19/03/2024 15:32, Sudan Landge wrote:
> > > > > > > > > This small series of patches aims to add devicetree bindings support for
> > > > > > > > > the Virtual Machine Generation ID (vmgenid) driver.
> > > > > > > > >
> > > > > > > > > Virtual Machine Generation ID driver was introduced in commit af6b54e2b5ba
> > > > > > > > > ("virt: vmgenid: notify RNG of VM fork and supply generation ID") as an
> > > > > > > > > ACPI only device.
> > > > > > > > > We would like to extend vmgenid to support devicetree bindings because:
> > > > > > > > > 1. A device should not be defined as an ACPI or DT only device.
> > > > > >
> > > > > > This (and the binding patch) tells me nothing about what "Virtual
> > > > > > Machine Generation ID driver" is and isn't really justification for
> > > > > > "why".
> > > > >
> > > > > It's a reference to a memory area which the OS can use to tell whether
> > > > > it's been snapshotted and restored (or 'forked'). A future submission
> > > > > should have a reference to something like
> > > > > https://www.qemu.org/docs/master/specs/vmgenid.html or the Microsoft
> > > > > doc which is linked from there.
> > > >
> > > > That doc mentions fw_cfg for which we already have a binding. Why can't
> > > > it be used/extended here?
> > > QEMU has support for vmgenid but even they do not pass vmgenid directly
> > > to the guest kernel using fw_cfg. QEMU passes the vmgenid/UUID via
> > > fw_cfg to an intermediate UEFI firmware. This UEFI firmware, running as
> > > a guest in QEMU, reads the UUID from fw_cfg and creates ACPI tables for
> > > it. The UEFI firmware then passes the UUID information to the guest
> > > kernel via ACPI.
> > > This approach requires yet another intermediary which is UEFI firmware
> > > and adds more complexity than ACPI for minimalist hypervisors that do
> > > not have an intermediate bootloader today.
> >
> > What stops you from passing fw_cfg not to UEFI FW? BTW, no actual VM
> > name was used in your posting, but now suddenly it is a talk about QEMU.
>
> That would be possible. But not ideal.
Why not ideal?
To rephrase the question, why is it fine for UEFI to read the vmgenid
from fw_cfg, but the kernel can't use the same mechanism? The response
that you'd have to use UEFI to use fw_cfg makes no sense to me. The
only reason I can think of is just being lazy and wanting to have
minimal changes to some existing driver. It looks to me like you could
implement this entirely in userspace already with zero kernel or
binding changes. From a quick look, we already have a fw_cfg driver
exposing UUID (that's the same thing as vmgenid AIUI) to userspace,
and you can feed that back into the random pool.
I am concerned that we already have a mechanism and you want to add a
second way. When do we ever think that's a good idea? What happens on
the next piece of fw_cfg data? We add yet another binding?
> Just as exposing it via PCI
> would be possible, but not ideal. Or forcing ACPI onto the guests in
> question, and various other less efficient options.
>
> If what we're really looking at here is a hostile takeover of the DT
> bindings repository, with a blanket "No, DT is dead. Go use something
> else, preferably ACPI", than all those other options are possible. We
> *never* have to add a new binding to DT ever again. Let's just set the
> existing bindings in stone and move on.
I'll refrain from all my snarky replies to this that aren't helpful to
the discussion.
Rob
On Fri, 2024-03-22 at 08:22 -0500, Rob Herring wrote:
>
> > > What stops you from passing fw_cfg not to UEFI FW? BTW, no actual VM
> > > name was used in your posting, but now suddenly it is a talk about QEMU.
(Forgot to address the second part of that last time. No specific VMM
was mentioned in the first place because this isn't VMM-specific)
> > That would be possible. But not ideal.
>
> Why not ideal?
>
> To rephrase the question, why is it fine for UEFI to read the vmgenid
> from fw_cfg, but the kernel can't use the same mechanism?
Because fw_cfg an incestuous way to get data from the VMM into the BIOS
(both SeaBIOS and UEFI). It's the way we pass the ACPI tables and
things like that.
It *isn't* designed as a general-purpose way of doing device discovery
for use by various operating systems.
I'm also not sure Firecracker, which is the VMM Sudan is working on,
even *has* fw_cfg. Especially on ARM. If we're going to be forced to
add some complicated device with MMIO and DMA just to be able to
advertise the existence of a simple memory region, that's just as bad
as being forced to expose it as an emulated PCI device.
This is what DT is *for*.
> The response
> that you'd have to use UEFI to use fw_cfg makes no sense to me. The
> only reason I can think of is just being lazy and wanting to have
> minimal changes to some existing driver. It looks to me like you could
> implement this entirely in userspace already with zero kernel or
> binding changes. From a quick look, we already have a fw_cfg driver
> exposing UUID (that's the same thing as vmgenid AIUI) to userspace,
> and you can feed that back into the random pool.
>
> I am concerned that we already have a mechanism and you want to add a
> second way. When do we ever think that's a good idea? What happens
> on the next piece of fw_cfg data? We add yet another binding?
No, because fw_cfg is a way for the VMM to give configuration
information to the firmware. There's a clue in the name. The firmware
then sets up ACPI tables or DT to pass information in a more coherent
and structured fashion to general-purpose operating systems.
And some VMMs *don't* use fw_cfg at all because for the minimal microvm
case it's overkill.
On 22/03/2024 14:27, David Woodhouse wrote:
> On Fri, 2024-03-22 at 08:22 -0500, Rob Herring wrote:
>>
>>>> What stops you from passing fw_cfg not to UEFI FW? BTW, no actual VM
>>>> name was used in your posting, but now suddenly it is a talk about QEMU.
>
> (Forgot to address the second part of that last time. No specific VMM
> was mentioned in the first place because this isn't VMM-specific)
>
QEMU is referenced to explain `vmgenid` which they are also using and
have more documentation on it. We mentioned the hypervisor we tested the
changes with in the cover letter which is
https://github.com/firecracker-microvm/firecracker but this change isn't
VMM specific.
>>> That would be possible. But not ideal.
>>
>> Why not ideal?
>>
>> To rephrase the question, why is it fine for UEFI to read the vmgenid
>> from fw_cfg, but the kernel can't use the same mechanism?
>
> Because fw_cfg an incestuous way to get data from the VMM into the BIOS
> (both SeaBIOS and UEFI). It's the way we pass the ACPI tables and
> things like that.
>
> It *isn't* designed as a general-purpose way of doing device discovery
> for use by various operating systems.
>
> I'm also not sure Firecracker, which is the VMM Sudan is working on,
> even *has* fw_cfg. Especially on ARM. If we're going to be forced to
> add some complicated device with MMIO and DMA just to be able to
> advertise the existence of a simple memory region, that's just as bad
> as being forced to expose it as an emulated PCI device.
>
> This is what DT is *for*.
>
>
>> The response
>> that you'd have to use UEFI to use fw_cfg makes no sense to me. The
>> only reason I can think of is just being lazy and wanting to have
>> minimal changes to some existing driver. It looks to me like you could
>> implement this entirely in userspace already with zero kernel or
>> binding changes. From a quick look, we already have a fw_cfg driver
>> exposing UUID (that's the same thing as vmgenid AIUI) to userspace,
>> and you can feed that back into the random pool.
>>
>> I am concerned that we already have a mechanism and you want to add a
>> second way. When do we ever think that's a good idea? What happens
>> on the next piece of fw_cfg data? We add yet another binding?
>
> No, because fw_cfg is a way for the VMM to give configuration
> information to the firmware. There's a clue in the name. The firmware
> then sets up ACPI tables or DT to pass information in a more coherent
> and structured fashion to general-purpose operating systems.
>
> And some VMMs *don't* use fw_cfg at all because for the minimal microvm
> case it's overkill.
>
The hypervisor we work on
(https://github.com/firecracker-microvm/firecracker) does not have
fw_cfg, it loads kernel directly without the need for UEFI or any
intermediate firmware. It is, as said, an overkill to enable UEFI and
fw_cfg just to support `vmgenid` specially when there is an alternative
available which could keep things simple for the vmm and for the linux
driver.