| Message ID | 20240816160743.220374-6-ajones@ventanamicro.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/riscv/virt: Fix PCI devices with AIA | expand |
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
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
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~
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
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
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 {
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(-)