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