Re: [Qemu-devel] [PATCH] hw/arm/virt-acpi-build: drop _ADR entry from SPCR

[Andrew Jones <drjones@redhat.com>]

On Thu, Aug 06, 2015 at 01:55:14PM +0100, Leif Lindholm wrote:
> On Thu, Aug 06, 2015 at 02:28:03PM +0200, Andrew Jones wrote:
> > On Thu, Aug 06, 2015 at 12:24:27PM +0100, Leif Lindholm wrote:
> > > The _ADR entry in SPCR is optional and redundant. The same information
> > > is already provided in _CRS (which is mandatory).
> > > 
> > > Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
> > > ---
> > > 
> > > So, this _ADR entry is only consumed by a set of not-widely-circulated
> > > patches for the Linux kernel. And while the ARM Server Base Boot
> > > Requirements specification mandates SPCR, it does not mandate this _ADR
> > > entry.
> > > 
> > > In the interest of not propagating non-standard extensions, I would be
> > > really happy if we could consider dropping this from 2.4.
> > > I do realize that this is a completely unreasonable request this late
> > > in the release process, but I only spotted this yesterday, and it is a
> > > very isolated change with very quantifiable effects.
> > > 
> > > The patch at https://git.linaro.org/leg/acpi/leg-kernel.git/commitdiff/46eeec7b7332bdd104941703696d3812afd934c8
> > > converts the non-upstream kernel SPCR handling code to use the _CRS
> > > information instead.
> > 
> > Grr... So when I saw how the kernel (the original non-upstream patch)
> > was using ADR, I presumed that that was a documented behavior. I guess
> > I should have confirmed that...
> 
> Yeah, I found it the other way, by SCPR not working on a platform that
> clearly had it defined.
> 
> > While the kernel change makes sense, I'm not sure we want this QEMU
> > patch, as there *are* kernels already using ADR.
> 
> Well ... yes, there are kernels with out-of-tree patches that have
> never worked with released versions of QEMU.
> 
> Could a variant of my kernel patch, but with an "_ADR" fallback be
> added to those kernels? I could whip that up in a couple of minutes.

The kernel patch should be fine, right? Not taking this QEMU patch
just allows us to support the old ADR kernels too. The new CRS kernels
will just work, as ADR is indeed redundant.

> 
> > In the least I wouldn't want to get burned twice, so I'd prefer to
> > see the SPCR code actually get into Linux first this time. That
> > would also allow us to point at something when we start breaking
> > guests.
> 
> So, if that's the way it has to be, that's the way it has to be.
> I'd just prefer not having different pieces of firmware validating
> different software behaviours for the same thing.

Yeah, now it's messy. I'm actually OK with this QEMU patch, with regard
to the downstream stuff that I'm involved with, but other downstreams
may not be so flexible... We need Peter to chime in with his opinion,
CCed.

> 
> If we can't just rip it out, could we at least stick a warning comment
> in a blink tag next to the _ADR addition?

:-) We definitely need to use blink tags more generously.

> 
> > Actually, why was ADR used the first time? It would be good to know
> > the story there in case there as a valid reason.
> 
> The phrase "it was just a quick hack" was whispered in my ear...
> But since it was never reviewed in public, the reasons were never
> discussed.

Weep,
drew