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