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

[Andrew Jones <ajones@ventanamicro.com>]

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