hw/arm/virt-acpi-build: drop _ADR entry from SPCR

Message ID 1438860267-3401-1-git-send-email-leif.lindholm@linaro.org
State New
Headers show

Commit Message

Leif Lindholm Aug. 6, 2015, 11:24 a.m.
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.

 hw/arm/virt-acpi-build.c | 6 ------
 1 file changed, 6 deletions(-)

Comments

Leif Lindholm Aug. 6, 2015, 12:55 p.m. | #1
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.

> 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.

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?

> 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.

/
    Leif
Peter Maydell Aug. 7, 2015, 7:17 a.m. | #2
On 6 August 2015 at 14:25, Andrew Jones <drjones@redhat.com> wrote:
> 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.

You've missed the boat for 2.4. For 2.5, I have no objection
to this patch. People who ship not-yet-upstream kernel patches
must expect to get burned by that from time to time...

-- PMM
Peter Maydell Aug. 20, 2015, 12:24 a.m. | #3
On 6 August 2015 at 14:25, Andrew Jones <drjones@redhat.com> wrote:
> 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:
>> > 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.

Could somebody who understands ACPI and the ramifications
here let me know if I should apply this patch, please?
(since we're now post-2.4)

thanks
-- PMM
Shannon Zhao Aug. 20, 2015, 1:17 a.m. | #4
On 2015/8/20 8:24, Peter Maydell wrote:
> On 6 August 2015 at 14:25, Andrew Jones <drjones@redhat.com> wrote:
>> 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:
>>>> 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.
> 
> Could somebody who understands ACPI and the ramifications
> here let me know if I should apply this patch, please?
> (since we're now post-2.4)
> 

I think we should hold back this patch until the kernel patch goes to
upstream kernel. And without this patch I think it doesn't break anything.

Thanks,
Leif Lindholm Aug. 20, 2015, 10:18 a.m. | #5
On Thu, Aug 20, 2015 at 01:24:39AM +0100, Peter Maydell wrote:
> On 6 August 2015 at 14:25, Andrew Jones <drjones@redhat.com> wrote:
> > 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:
> >> > 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.
> 
> Could somebody who understands ACPI and the ramifications
> here let me know if I should apply this patch, please?
> (since we're now post-2.4)

I presume my opinion is clear, but I'm cc:ing some of the Linaro ACPI
team.

Graeme, Al - the patch in question is:
https://www.mail-archive.com/qemu-devel%40nongnu.org/msg314356.html

/
    Leif
Graeme Gregory Aug. 20, 2015, 10:48 a.m. | #6
On 20 August 2015 at 11:18, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Thu, Aug 20, 2015 at 01:24:39AM +0100, Peter Maydell wrote:
>> On 6 August 2015 at 14:25, Andrew Jones <drjones@redhat.com> wrote:
>> > 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:
>> >> > 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.
>>
>> Could somebody who understands ACPI and the ramifications
>> here let me know if I should apply this patch, please?
>> (since we're now post-2.4)
>
> I presume my opinion is clear, but I'm cc:ing some of the Linaro ACPI
> team.
>
> Graeme, Al - the patch in question is:
> https://www.mail-archive.com/qemu-devel%40nongnu.org/msg314356.html
>
Using _ADR for a non enumerable bus is undefined behaviour in the ACPI
specification.

How it is used in Redhats SPCR patch is IMO wrong becuase there is no
guarantee that _ADR will be defined for any MMIO device in DSDT.

I believe QEMU should not follow this just to make a non upstreamed
Redhat patch work.

Graeme
Shannon Zhao Aug. 20, 2015, 11:09 a.m. | #7
On 2015/8/20 18:48, G Gregory wrote:
> On 20 August 2015 at 11:18, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>> On Thu, Aug 20, 2015 at 01:24:39AM +0100, Peter Maydell wrote:
>>> On 6 August 2015 at 14:25, Andrew Jones <drjones@redhat.com> wrote:
>>>> 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:
>>>>>> 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.
>>>
>>> Could somebody who understands ACPI and the ramifications
>>> here let me know if I should apply this patch, please?
>>> (since we're now post-2.4)
>>
>> I presume my opinion is clear, but I'm cc:ing some of the Linaro ACPI
>> team.
>>
>> Graeme, Al - the patch in question is:
>> https://www.mail-archive.com/qemu-devel%40nongnu.org/msg314356.html
>>
> Using _ADR for a non enumerable bus is undefined behaviour in the ACPI
> specification.
>
> How it is used in Redhats SPCR patch is IMO wrong becuase there is no
> guarantee that _ADR will be defined for any MMIO device in DSDT.
>
> I believe QEMU should not follow this just to make a non upstreamed
> Redhat patch work.
>
Yeah, but when will the right kernel patch be upstreamed? Do you have a 
plan for upstreaming it? Or it's on the list already?

As said before, we can apply this patch after the kernel patch upstreamed.

Thanks,
Leif Lindholm Aug. 20, 2015, 11:21 a.m. | #8
On Thu, Aug 20, 2015 at 07:09:57PM +0800, Shannon Zhao wrote:
> >>>Could somebody who understands ACPI and the ramifications
> >>>here let me know if I should apply this patch, please?
> >>>(since we're now post-2.4)
> >>
> >>I presume my opinion is clear, but I'm cc:ing some of the Linaro ACPI
> >>team.
> >>
> >>Graeme, Al - the patch in question is:
> >>https://www.mail-archive.com/qemu-devel%40nongnu.org/msg314356.html
> >>
> >Using _ADR for a non enumerable bus is undefined behaviour in the ACPI
> >specification.
> >
> >How it is used in Redhats SPCR patch is IMO wrong becuase there is no
> >guarantee that _ADR will be defined for any MMIO device in DSDT.
> >
> >I believe QEMU should not follow this just to make a non upstreamed
> >Redhat patch work.
> >
> Yeah, but when will the right kernel patch be upstreamed? Do you
> have a plan for upstreaming it? Or it's on the list already?

It's on my way too long to-do list, but I'll need to send it out in
whatever state as an RFC this week anyway.

> As said before, we can apply this patch after the kernel patch upstreamed.

Meanwhile, it would be very bad if this becomes a de-facto standard,
using QEMU as a vector to (needlessly) change specifications through
the back door.

/
    Leif
Peter Maydell Sept. 4, 2015, 5:14 p.m. | #9
On 20 August 2015 at 22:54, Andrew Jones <drjones@redhat.com> wrote:
> If I understand correctly, then the concern is that vendors, ones which
> use QEMU code as their specification, will start building ACPI tables
> with ADR unnecessarily populated in the console uart's device table.
> Actually, some vendors must have already been doing that, otherwise the
> out-of-tree patches in RH's and Linaro's trees wouldn't have worked on
> bare-metal. So, what is the problem with them doing it? Just wrong
> because it's pointless?

Yeah, I don't think anybody's using QEMU as their model for
writing firmware...

> If I'm right about the concerns, then I don't see why we should rush
> this QEMU change. Also, it would be much easier to apologize to the guest
> kernels that the change will break, if we can point at an upstream patch
> that they need to backport. I.e. I still vote that we wait for the kernel
> patch to get upstream first.

I think I agree with this. Please ping me and/or just resend the
patch when the kernel fix has got into upstream.

thanks
-- PMM

Patch

diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index f365140..85eb48c 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -84,12 +84,6 @@  static void acpi_dsdt_add_uart(Aml *scope, const MemMapEntry *uart_memmap,
                aml_interrupt(AML_CONSUMER, AML_LEVEL, AML_ACTIVE_HIGH,
                              AML_EXCLUSIVE, uart_irq));
     aml_append(dev, aml_name_decl("_CRS", crs));
-
-    /* The _ADR entry is used to link this device to the UART described
-     * in the SPCR table, i.e. SPCR.base_address.address == _ADR.
-     */
-    aml_append(dev, aml_name_decl("_ADR", aml_int(uart_memmap->base)));
-
     aml_append(scope, dev);
 }