[PATCH 0/2] hw/riscv/virt: Fix PCI devices with AIA

[PATCH 0/2] hw/riscv/virt: Fix PCI devices with AIA

[Andrew Jones]

Linux does not properly handle '#msi-cells=<0>' when searching for
MSI controllers for PCI devices which results in the devices being
unable to use MSIs. A patch for Linux has been sent[1] but until it,
or something like it, is merged and in distro kernels we should stop
adding the property. It's harmless to stop adding it since the
absence of the property and a value of zero for the property mean
the same thing according to the DT binding definition.

The first patch stops adding the property by simply reverting the
patch that started adding it and should be applied for 9.1 since
it's a fix. The second patch allows users to bring it back by enabling
a new machine property 'strict-dt' which is meant to be used for cases
like these going forward. There's no rush for the second patch.

[1] https://lore.kernel.org/all/20240816124957.130017-2-ajones@ventanamicro.com/

Thanks,
drew

Andrew Jones (2):
  Revert "hw/riscv/virt.c: imsics DT: add '#msi-cells'"
  hw/riscv/virt: Introduce strict-dt

 docs/system/riscv/virt.rst | 11 ++++++++++
 hw/riscv/virt.c            | 44 +++++++++++++++++++++++++++++---------
 include/hw/riscv/virt.h    |  1 +
 3 files changed, 46 insertions(+), 10 deletions(-)

-- 
2.45.2

[PATCH for-9.1 1/2] Revert "hw/riscv/virt.c: imsics DT: add '#msi-cells'"

[Andrew Jones]

This reverts commit f42cdf2ea5b3a1dc369792d7acbf9cd3e5c90815.

Linux does not properly handle '#msi-cells=<0>' when searching for
MSI controllers for PCI devices which results in the devices being
unable to use MSIs. A patch for Linux has been sent[1] but until it,
or something like it, is merged and in distro kernels we should stop
adding the property. It's harmless to stop adding it since the
absence of the property and a value of zero for the property mean
the same thing according to the DT binding definition.

Link: https://lore.kernel.org/all/20240816124957.130017-2-ajones@ventanamicro.com/ # 1
Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
---
 hw/riscv/virt.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index 9981e0f6c9b9..cef41c150aaf 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -552,7 +552,6 @@ static void create_fdt_one_imsic(RISCVVirtState *s, hwaddr base_addr,
                           FDT_IMSIC_INT_CELLS);
     qemu_fdt_setprop(ms->fdt, imsic_name, "interrupt-controller", NULL, 0);
     qemu_fdt_setprop(ms->fdt, imsic_name, "msi-controller", NULL, 0);
-    qemu_fdt_setprop_cell(ms->fdt, imsic_name, "#msi-cells", 0);
     qemu_fdt_setprop(ms->fdt, imsic_name, "interrupts-extended",
                      imsic_cells, ms->smp.cpus * sizeof(uint32_t) * 2);
     qemu_fdt_setprop(ms->fdt, imsic_name, "reg", imsic_regs,
-- 
2.45.2

Re: [PATCH for-9.1 1/2] Revert "hw/riscv/virt.c: imsics DT: add '#msi-cells'"

[Philippe Mathieu-Daudé]

On 16/8/24 18:07, Andrew Jones wrote:
> This reverts commit f42cdf2ea5b3a1dc369792d7acbf9cd3e5c90815.
> 
> Linux does not properly handle '#msi-cells=<0>' when searching for
> MSI controllers for PCI devices which results in the devices being
> unable to use MSIs. A patch for Linux has been sent[1] but until it,
> or something like it, is merged and in distro kernels we should stop
> adding the property. It's harmless to stop adding it since the
> absence of the property and a value of zero for the property mean
> the same thing according to the DT binding definition.
> 

This reverts commit f42cdf2ea5b3a1dc369792d7acbf9cd3e5c90815.

> Link: https://lore.kernel.org/all/20240816124957.130017-2-ajones@ventanamicro.com/ # 1
> Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
> ---
>   hw/riscv/virt.c | 1 -
>   1 file changed, 1 deletion(-)
> 
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index 9981e0f6c9b9..cef41c150aaf 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -552,7 +552,6 @@ static void create_fdt_one_imsic(RISCVVirtState *s, hwaddr base_addr,
>                             FDT_IMSIC_INT_CELLS);
>       qemu_fdt_setprop(ms->fdt, imsic_name, "interrupt-controller", NULL, 0);
>       qemu_fdt_setprop(ms->fdt, imsic_name, "msi-controller", NULL, 0);
> -    qemu_fdt_setprop_cell(ms->fdt, imsic_name, "#msi-cells", 0);
>       qemu_fdt_setprop(ms->fdt, imsic_name, "interrupts-extended",
>                        imsic_cells, ms->smp.cpus * sizeof(uint32_t) * 2);
>       qemu_fdt_setprop(ms->fdt, imsic_name, "reg", imsic_regs,

Re: [PATCH for-9.1 1/2] Revert "hw/riscv/virt.c: imsics DT: add '#msi-cells'"

[Philippe Mathieu-Daudé]

On 16/8/24 18:27, Philippe Mathieu-Daudé wrote:
> On 16/8/24 18:07, Andrew Jones wrote:
>> This reverts commit f42cdf2ea5b3a1dc369792d7acbf9cd3e5c90815.

Ahah sorry I'm not seeing well after a long day in front of the
monitor =)

>> Linux does not properly handle '#msi-cells=<0>' when searching for
>> MSI controllers for PCI devices which results in the devices being
>> unable to use MSIs. A patch for Linux has been sent[1] but until it,
>> or something like it, is merged and in distro kernels we should stop
>> adding the property. It's harmless to stop adding it since the
>> absence of the property and a value of zero for the property mean
>> the same thing according to the DT binding definition.
>>
> 
> This reverts commit f42cdf2ea5b3a1dc369792d7acbf9cd3e5c90815.
> 
>> Link: 
>> https://lore.kernel.org/all/20240816124957.130017-2-ajones@ventanamicro.com/ # 1
>> Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
>> ---
>>   hw/riscv/virt.c | 1 -
>>   1 file changed, 1 deletion(-)
>>
>> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
>> index 9981e0f6c9b9..cef41c150aaf 100644
>> --- a/hw/riscv/virt.c
>> +++ b/hw/riscv/virt.c
>> @@ -552,7 +552,6 @@ static void create_fdt_one_imsic(RISCVVirtState 
>> *s, hwaddr base_addr,
>>                             FDT_IMSIC_INT_CELLS);
>>       qemu_fdt_setprop(ms->fdt, imsic_name, "interrupt-controller", 
>> NULL, 0);
>>       qemu_fdt_setprop(ms->fdt, imsic_name, "msi-controller", NULL, 0);
>> -    qemu_fdt_setprop_cell(ms->fdt, imsic_name, "#msi-cells", 0);
>>       qemu_fdt_setprop(ms->fdt, imsic_name, "interrupts-extended",
>>                        imsic_cells, ms->smp.cpus * sizeof(uint32_t) * 2);
>>       qemu_fdt_setprop(ms->fdt, imsic_name, "reg", imsic_regs,
> 

Re: [PATCH for-9.1 1/2] Revert "hw/riscv/virt.c: imsics DT: add '#msi-cells'"

[Daniel Henrique Barboza]

On 8/16/24 1:07 PM, Andrew Jones wrote:
> This reverts commit f42cdf2ea5b3a1dc369792d7acbf9cd3e5c90815.
> 
> Linux does not properly handle '#msi-cells=<0>' when searching for
> MSI controllers for PCI devices which results in the devices being
> unable to use MSIs. A patch for Linux has been sent[1] but until it,
> or something like it, is merged and in distro kernels we should stop
> adding the property. It's harmless to stop adding it since the
> absence of the property and a value of zero for the property mean
> the same thing according to the DT binding definition.
> 
> Link: https://lore.kernel.org/all/20240816124957.130017-2-ajones@ventanamicro.com/ # 1
> Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
> ---

Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>


This is indeed a 9.1 fix. Thanks Drew for sending it.

We can discuss whether we should wrap this around the 'strict-dt' flag or not, but
that can wait for 9.2.


Thanks,

Daniel


>   hw/riscv/virt.c | 1 -
>   1 file changed, 1 deletion(-)
> 
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index 9981e0f6c9b9..cef41c150aaf 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -552,7 +552,6 @@ static void create_fdt_one_imsic(RISCVVirtState *s, hwaddr base_addr,
>                             FDT_IMSIC_INT_CELLS);
>       qemu_fdt_setprop(ms->fdt, imsic_name, "interrupt-controller", NULL, 0);
>       qemu_fdt_setprop(ms->fdt, imsic_name, "msi-controller", NULL, 0);
> -    qemu_fdt_setprop_cell(ms->fdt, imsic_name, "#msi-cells", 0);
>       qemu_fdt_setprop(ms->fdt, imsic_name, "interrupts-extended",
>                        imsic_cells, ms->smp.cpus * sizeof(uint32_t) * 2);
>       qemu_fdt_setprop(ms->fdt, imsic_name, "reg", imsic_regs,

Re: [PATCH for-9.1 1/2] Revert "hw/riscv/virt.c: imsics DT: add '#msi-cells'"

[Alistair Francis]

On Sat, Aug 17, 2024 at 2:08 AM Andrew Jones <ajones@ventanamicro.com> wrote:
>
> This reverts commit f42cdf2ea5b3a1dc369792d7acbf9cd3e5c90815.
>
> Linux does not properly handle '#msi-cells=<0>' when searching for
> MSI controllers for PCI devices which results in the devices being
> unable to use MSIs. A patch for Linux has been sent[1] but until it,
> or something like it, is merged and in distro kernels we should stop
> adding the property. It's harmless to stop adding it since the
> absence of the property and a value of zero for the property mean
> the same thing according to the DT binding definition.
>
> Link: https://lore.kernel.org/all/20240816124957.130017-2-ajones@ventanamicro.com/ # 1
> Signed-off-by: Andrew Jones <ajones@ventanamicro.com>

Thanks!

Applied to riscv-to-apply.next

Alistair

> ---
>  hw/riscv/virt.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index 9981e0f6c9b9..cef41c150aaf 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -552,7 +552,6 @@ static void create_fdt_one_imsic(RISCVVirtState *s, hwaddr base_addr,
>                            FDT_IMSIC_INT_CELLS);
>      qemu_fdt_setprop(ms->fdt, imsic_name, "interrupt-controller", NULL, 0);
>      qemu_fdt_setprop(ms->fdt, imsic_name, "msi-controller", NULL, 0);
> -    qemu_fdt_setprop_cell(ms->fdt, imsic_name, "#msi-cells", 0);
>      qemu_fdt_setprop(ms->fdt, imsic_name, "interrupts-extended",
>                       imsic_cells, ms->smp.cpus * sizeof(uint32_t) * 2);
>      qemu_fdt_setprop(ms->fdt, imsic_name, "reg", imsic_regs,
> --
> 2.45.2
>
>

[PATCH 2/2] hw/riscv/virt: Introduce strict-dt

[Andrew Jones]

Older firmwares and OS kernels which use deprecated device tree
properties or are missing support for new properties may not be
tolerant of fully compliant device trees. When divergence to the
bindings specifications is harmless for new firmwares and OS kernels
which are compliant, then it's probably better to also continue
supporting the old firmwares and OS kernels by generating
non-compliant device trees. The '#msi-cells=<0>' property of the
imsic is one such property. Generating that property doesn't provide
anything necessary (no '#msi-cells' property or an '#msi-cells'
property with a value of zero mean the same thing) but it does
cause PCI devices to fail to find the MSI controller on Linux and,
for that reason, riscv virt doesn't currently generate it despite
that putting the DT out of compliance. For users that want a
compliant DT and know their software supports it, introduce a machine
property 'strict-dt' to do so. We also drop the one redundant
property that uses a deprecated name when strict-dt is enabled.

Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
---
 docs/system/riscv/virt.rst | 11 ++++++++++
 hw/riscv/virt.c            | 43 ++++++++++++++++++++++++++++++--------
 include/hw/riscv/virt.h    |  1 +
 3 files changed, 46 insertions(+), 9 deletions(-)

diff --git a/docs/system/riscv/virt.rst b/docs/system/riscv/virt.rst
index 9a06f95a3444..f08d0a053051 100644
--- a/docs/system/riscv/virt.rst
+++ b/docs/system/riscv/virt.rst
@@ -116,6 +116,17 @@ The following machine-specific options are supported:
   having AIA IMSIC (i.e. "aia=aplic-imsic" selected). When not specified,
   the default number of per-HART VS-level AIA IMSIC pages is 0.
 
+- strict-dt=[on|off]
+
+  Older firmwares and OS kernels which use deprecated device tree properties
+  or are missing support for new properties may not be tolerant of fully
+  compliant device trees. When divergence to the bindings specifications is
+  harmless for new firmwares and OS kernels which are compliant, then it's
+  considered better to also continue supporting the old firmwares and OS
+  kernels by generating non-compliant device trees, and doing so is the default
+  behavior. This option may be enabled in order to force QEMU to only generate
+  compliant device trees.
+
 Running Linux kernel
 --------------------
 
diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index cef41c150aaf..6a6e73b96362 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -552,6 +552,9 @@ static void create_fdt_one_imsic(RISCVVirtState *s, hwaddr base_addr,
                           FDT_IMSIC_INT_CELLS);
     qemu_fdt_setprop(ms->fdt, imsic_name, "interrupt-controller", NULL, 0);
     qemu_fdt_setprop(ms->fdt, imsic_name, "msi-controller", NULL, 0);
+    if (s->strict_dt) {
+        qemu_fdt_setprop_cell(ms->fdt, imsic_name, "#msi-cells", 0);
+    }
     qemu_fdt_setprop(ms->fdt, imsic_name, "interrupts-extended",
                      imsic_cells, ms->smp.cpus * sizeof(uint32_t) * 2);
     qemu_fdt_setprop(ms->fdt, imsic_name, "reg", imsic_regs,
@@ -650,15 +653,18 @@ static void create_fdt_one_aplic(RISCVVirtState *s, int socket,
         qemu_fdt_setprop_cells(ms->fdt, aplic_name, "riscv,delegation",
                                aplic_child_phandle, 0x1,
                                VIRT_IRQCHIP_NUM_SOURCES);
-        /*
-         * DEPRECATED_9.1: Compat property kept temporarily
-         * to allow old firmwares to work with AIA. Do *not*
-         * use 'riscv,delegate' in new code: use
-         * 'riscv,delegation' instead.
-         */
-        qemu_fdt_setprop_cells(ms->fdt, aplic_name, "riscv,delegate",
-                               aplic_child_phandle, 0x1,
-                               VIRT_IRQCHIP_NUM_SOURCES);
+
+        if (!s->strict_dt) {
+            /*
+             * DEPRECATED_9.1: Compat property kept temporarily
+             * to allow old firmwares to work with AIA. Do *not*
+             * use 'riscv,delegate' in new code: use
+             * 'riscv,delegation' instead.
+             */
+            qemu_fdt_setprop_cells(ms->fdt, aplic_name, "riscv,delegate",
+                                   aplic_child_phandle, 0x1,
+                                   VIRT_IRQCHIP_NUM_SOURCES);
+        }
     }
 
     riscv_socket_fdt_write_id(ms, aplic_name, socket);
@@ -1732,6 +1738,20 @@ static void virt_set_acpi(Object *obj, Visitor *v, const char *name,
     visit_type_OnOffAuto(v, name, &s->acpi, errp);
 }
 
+static bool virt_get_strict_dt(Object *obj, Error **errp)
+{
+    RISCVVirtState *s = RISCV_VIRT_MACHINE(obj);
+
+    return s->strict_dt;
+}
+
+static void virt_set_strict_dt(Object *obj, bool value, Error **errp)
+{
+    RISCVVirtState *s = RISCV_VIRT_MACHINE(obj);
+
+    s->strict_dt = value;
+}
+
 static HotplugHandler *virt_machine_get_hotplug_handler(MachineState *machine,
                                                         DeviceState *dev)
 {
@@ -1822,6 +1842,11 @@ static void virt_machine_class_init(ObjectClass *oc, void *data)
                               NULL, NULL);
     object_class_property_set_description(oc, "acpi",
                                           "Enable ACPI");
+
+    object_class_property_add_bool(oc, "strict-dt",
+                                   virt_get_strict_dt, virt_set_strict_dt);
+    object_class_property_set_description(oc, "strict-dt",
+        "Set to 'on' to generate a fully-compliant DT without deprecated properties");
 }
 
 static const TypeInfo virt_machine_typeinfo = {
diff --git a/include/hw/riscv/virt.h b/include/hw/riscv/virt.h
index c0dc41ff9a1a..c3b4c000b80a 100644
--- a/include/hw/riscv/virt.h
+++ b/include/hw/riscv/virt.h
@@ -62,6 +62,7 @@ struct RISCVVirtState {
     OnOffAuto acpi;
     const MemMapEntry *memmap;
     struct GPEXHost *gpex_host;
+    bool strict_dt;
 };
 
 enum {
-- 
2.45.2

Re: [PATCH 2/2] hw/riscv/virt: Introduce strict-dt

[Alistair Francis]

On Sat, Aug 17, 2024 at 2:08 AM Andrew Jones <ajones@ventanamicro.com> wrote:
>
> Older firmwares and OS kernels which use deprecated device tree
> properties or are missing support for new properties may not be
> tolerant of fully compliant device trees. When divergence to the
> bindings specifications is harmless for new firmwares and OS kernels
> which are compliant, then it's probably better to also continue
> supporting the old firmwares and OS kernels by generating
> non-compliant device trees. The '#msi-cells=<0>' property of the
> imsic is one such property. Generating that property doesn't provide
> anything necessary (no '#msi-cells' property or an '#msi-cells'
> property with a value of zero mean the same thing) but it does
> cause PCI devices to fail to find the MSI controller on Linux and,
> for that reason, riscv virt doesn't currently generate it despite
> that putting the DT out of compliance. For users that want a
> compliant DT and know their software supports it, introduce a machine
> property 'strict-dt' to do so. We also drop the one redundant
> property that uses a deprecated name when strict-dt is enabled.
>
> Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
> ---
>  docs/system/riscv/virt.rst | 11 ++++++++++
>  hw/riscv/virt.c            | 43 ++++++++++++++++++++++++++++++--------
>  include/hw/riscv/virt.h    |  1 +
>  3 files changed, 46 insertions(+), 9 deletions(-)
>
> diff --git a/docs/system/riscv/virt.rst b/docs/system/riscv/virt.rst
> index 9a06f95a3444..f08d0a053051 100644
> --- a/docs/system/riscv/virt.rst
> +++ b/docs/system/riscv/virt.rst
> @@ -116,6 +116,17 @@ The following machine-specific options are supported:
>    having AIA IMSIC (i.e. "aia=aplic-imsic" selected). When not specified,
>    the default number of per-HART VS-level AIA IMSIC pages is 0.
>
> +- strict-dt=[on|off]

Hmm... I don't love the idea of having yet another command line option.

Does this really buy us a lot? Eventually we should deprecate the
invalid DT bindings anyway

Alistair

Re: [PATCH 2/2] hw/riscv/virt: Introduce strict-dt

[Andrew Jones]

On Mon, Aug 19, 2024 at 11:19:18AM GMT, Alistair Francis wrote:
> On Sat, Aug 17, 2024 at 2:08 AM Andrew Jones <ajones@ventanamicro.com> wrote:
> >
> > Older firmwares and OS kernels which use deprecated device tree
> > properties or are missing support for new properties may not be
> > tolerant of fully compliant device trees. When divergence to the
> > bindings specifications is harmless for new firmwares and OS kernels
> > which are compliant, then it's probably better to also continue
> > supporting the old firmwares and OS kernels by generating
> > non-compliant device trees. The '#msi-cells=<0>' property of the
> > imsic is one such property. Generating that property doesn't provide
> > anything necessary (no '#msi-cells' property or an '#msi-cells'
> > property with a value of zero mean the same thing) but it does
> > cause PCI devices to fail to find the MSI controller on Linux and,
> > for that reason, riscv virt doesn't currently generate it despite
> > that putting the DT out of compliance. For users that want a
> > compliant DT and know their software supports it, introduce a machine
> > property 'strict-dt' to do so. We also drop the one redundant
> > property that uses a deprecated name when strict-dt is enabled.
> >
> > Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
> > ---
> >  docs/system/riscv/virt.rst | 11 ++++++++++
> >  hw/riscv/virt.c            | 43 ++++++++++++++++++++++++++++++--------
> >  include/hw/riscv/virt.h    |  1 +
> >  3 files changed, 46 insertions(+), 9 deletions(-)
> >
> > diff --git a/docs/system/riscv/virt.rst b/docs/system/riscv/virt.rst
> > index 9a06f95a3444..f08d0a053051 100644
> > --- a/docs/system/riscv/virt.rst
> > +++ b/docs/system/riscv/virt.rst
> > @@ -116,6 +116,17 @@ The following machine-specific options are supported:
> >    having AIA IMSIC (i.e. "aia=aplic-imsic" selected). When not specified,
> >    the default number of per-HART VS-level AIA IMSIC pages is 0.
> >
> > +- strict-dt=[on|off]
> 
> Hmm... I don't love the idea of having yet another command line option.
> 
> Does this really buy us a lot? Eventually we should deprecate the
> invalid DT bindings anyway

I agree we should deprecate the invalid DT usage, with the goal of only
generating DTs that make the validator happy. I'm not sure how long that
deprecation period should be, though. It may need to be a while since
we'll need to decide when we've waited long enough to no longer care
about older kernels. In the meantime, we won't be making the validator
happy and may get bug reports due to that. With strct-dt we can just
direct people in that direction. Also, I wouldn't be surprised if
something else like this comes along some day, which is why I tried to
make the option as generic as possible. Finally, the 'if (strict_dt)'
self-documents to some extent. Otherwise we'll need to add comments
around explaining why we're diverging from the specs. Although we should
probably do that anyway, i.e. I should have put a comment on the
'if (strict-dt) then #msi-cells' explaining why it's under strict-dt.
If we want strict-dt, then I'll send a v2 doing that. If we don't want
strict-dt then I'll send a v2 with just a comment explaining why
#msi-cells was left out.

Thanks,
drew

Re: [PATCH 2/2] hw/riscv/virt: Introduce strict-dt

[Richard Henderson]

On 8/19/24 17:50, Andrew Jones wrote:
> I agree we should deprecate the invalid DT usage, with the goal of only
> generating DTs that make the validator happy. I'm not sure how long that
> deprecation period should be, though. It may need to be a while since
> we'll need to decide when we've waited long enough to no longer care
> about older kernels.

This is the kind of thing versioned machine models are good for.

For instance, for the next release define virt-9.1 and virt-9.2.
Set strict-dt in virt-9.2 and reset it in virt-9.1.

C.f. hw/arm/virt.c, where virt_machine_8_2_options invokes virt_machine_9_0_options and 
then adjusts for backward compatibility.


r~

Re: [PATCH 2/2] hw/riscv/virt: Introduce strict-dt

[Alistair Francis]

On Mon, Aug 19, 2024 at 5:50 PM Andrew Jones <ajones@ventanamicro.com> wrote:
>
> On Mon, Aug 19, 2024 at 11:19:18AM GMT, Alistair Francis wrote:
> > On Sat, Aug 17, 2024 at 2:08 AM Andrew Jones <ajones@ventanamicro.com> wrote:
> > >
> > > Older firmwares and OS kernels which use deprecated device tree
> > > properties or are missing support for new properties may not be
> > > tolerant of fully compliant device trees. When divergence to the
> > > bindings specifications is harmless for new firmwares and OS kernels
> > > which are compliant, then it's probably better to also continue
> > > supporting the old firmwares and OS kernels by generating
> > > non-compliant device trees. The '#msi-cells=<0>' property of the
> > > imsic is one such property. Generating that property doesn't provide
> > > anything necessary (no '#msi-cells' property or an '#msi-cells'
> > > property with a value of zero mean the same thing) but it does
> > > cause PCI devices to fail to find the MSI controller on Linux and,
> > > for that reason, riscv virt doesn't currently generate it despite
> > > that putting the DT out of compliance. For users that want a
> > > compliant DT and know their software supports it, introduce a machine
> > > property 'strict-dt' to do so. We also drop the one redundant
> > > property that uses a deprecated name when strict-dt is enabled.
> > >
> > > Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
> > > ---
> > >  docs/system/riscv/virt.rst | 11 ++++++++++
> > >  hw/riscv/virt.c            | 43 ++++++++++++++++++++++++++++++--------
> > >  include/hw/riscv/virt.h    |  1 +
> > >  3 files changed, 46 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/docs/system/riscv/virt.rst b/docs/system/riscv/virt.rst
> > > index 9a06f95a3444..f08d0a053051 100644
> > > --- a/docs/system/riscv/virt.rst
> > > +++ b/docs/system/riscv/virt.rst
> > > @@ -116,6 +116,17 @@ The following machine-specific options are supported:
> > >    having AIA IMSIC (i.e. "aia=aplic-imsic" selected). When not specified,
> > >    the default number of per-HART VS-level AIA IMSIC pages is 0.
> > >
> > > +- strict-dt=[on|off]
> >
> > Hmm... I don't love the idea of having yet another command line option.
> >
> > Does this really buy us a lot? Eventually we should deprecate the
> > invalid DT bindings anyway
>
> I agree we should deprecate the invalid DT usage, with the goal of only
> generating DTs that make the validator happy. I'm not sure how long that
> deprecation period should be, though. It may need to be a while since
> we'll need to decide when we've waited long enough to no longer care
> about older kernels. In the meantime, we won't be making the validator
> happy and may get bug reports due to that. With strct-dt we can just
> direct people in that direction. Also, I wouldn't be surprised if
> something else like this comes along some day, which is why I tried to
> make the option as generic as possible. Finally, the 'if (strict_dt)'
> self-documents to some extent. Otherwise we'll need to add comments
> around explaining why we're diverging from the specs. Although we should
> probably do that anyway, i.e. I should have put a comment on the
> 'if (strict-dt) then #msi-cells' explaining why it's under strict-dt.
> If we want strict-dt, then I'll send a v2 doing that. If we don't want
> strict-dt then I'll send a v2 with just a comment explaining why
> #msi-cells was left out.

I think go without strict-dt and add a comment.

In the future if we decide we really want to keep the validator happy
then we can version the virt machine and use the older machine for
backwards compatible kernels

Alistair

>
> Thanks,
> drew

Re: [PATCH 2/2] hw/riscv/virt: Introduce strict-dt

[Andrew Jones]

On Mon, Sep 09, 2024 at 12:41:24PM GMT, Alistair Francis wrote:
> On Mon, Aug 19, 2024 at 5:50 PM Andrew Jones <ajones@ventanamicro.com> wrote:
> >
> > On Mon, Aug 19, 2024 at 11:19:18AM GMT, Alistair Francis wrote:
> > > On Sat, Aug 17, 2024 at 2:08 AM Andrew Jones <ajones@ventanamicro.com> wrote:
> > > >
> > > > Older firmwares and OS kernels which use deprecated device tree
> > > > properties or are missing support for new properties may not be
> > > > tolerant of fully compliant device trees. When divergence to the
> > > > bindings specifications is harmless for new firmwares and OS kernels
> > > > which are compliant, then it's probably better to also continue
> > > > supporting the old firmwares and OS kernels by generating
> > > > non-compliant device trees. The '#msi-cells=<0>' property of the
> > > > imsic is one such property. Generating that property doesn't provide
> > > > anything necessary (no '#msi-cells' property or an '#msi-cells'
> > > > property with a value of zero mean the same thing) but it does
> > > > cause PCI devices to fail to find the MSI controller on Linux and,
> > > > for that reason, riscv virt doesn't currently generate it despite
> > > > that putting the DT out of compliance. For users that want a
> > > > compliant DT and know their software supports it, introduce a machine
> > > > property 'strict-dt' to do so. We also drop the one redundant
> > > > property that uses a deprecated name when strict-dt is enabled.
> > > >
> > > > Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
> > > > ---
> > > >  docs/system/riscv/virt.rst | 11 ++++++++++
> > > >  hw/riscv/virt.c            | 43 ++++++++++++++++++++++++++++++--------
> > > >  include/hw/riscv/virt.h    |  1 +
> > > >  3 files changed, 46 insertions(+), 9 deletions(-)
> > > >
> > > > diff --git a/docs/system/riscv/virt.rst b/docs/system/riscv/virt.rst
> > > > index 9a06f95a3444..f08d0a053051 100644
> > > > --- a/docs/system/riscv/virt.rst
> > > > +++ b/docs/system/riscv/virt.rst
> > > > @@ -116,6 +116,17 @@ The following machine-specific options are supported:
> > > >    having AIA IMSIC (i.e. "aia=aplic-imsic" selected). When not specified,
> > > >    the default number of per-HART VS-level AIA IMSIC pages is 0.
> > > >
> > > > +- strict-dt=[on|off]
> > >
> > > Hmm... I don't love the idea of having yet another command line option.
> > >
> > > Does this really buy us a lot? Eventually we should deprecate the
> > > invalid DT bindings anyway
> >
> > I agree we should deprecate the invalid DT usage, with the goal of only
> > generating DTs that make the validator happy. I'm not sure how long that
> > deprecation period should be, though. It may need to be a while since
> > we'll need to decide when we've waited long enough to no longer care
> > about older kernels. In the meantime, we won't be making the validator
> > happy and may get bug reports due to that. With strct-dt we can just
> > direct people in that direction. Also, I wouldn't be surprised if
> > something else like this comes along some day, which is why I tried to
> > make the option as generic as possible. Finally, the 'if (strict_dt)'
> > self-documents to some extent. Otherwise we'll need to add comments
> > around explaining why we're diverging from the specs. Although we should
> > probably do that anyway, i.e. I should have put a comment on the
> > 'if (strict-dt) then #msi-cells' explaining why it's under strict-dt.
> > If we want strict-dt, then I'll send a v2 doing that. If we don't want
> > strict-dt then I'll send a v2 with just a comment explaining why
> > #msi-cells was left out.
> 
> I think go without strict-dt and add a comment.
> 
> In the future if we decide we really want to keep the validator happy
> then we can version the virt machine and use the older machine for
> backwards compatible kernels

OK, I'll post a patch with a comment as soon as I have an upstream
Linux commit to reference for the fix. So far the fix is only in
linux-next.

Thanks,
drew