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

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

[Leif Lindholm]

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(-)

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);
 }
 
-- 
2.1.4

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

[Andrew Jones]

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

While the kernel change makes sense, I'm not sure we want this QEMU
patch, as there *are* kernels already using ADR. 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. Actually, why was ADR used
the first time? It would be good to know the story there in case there
as a valid reason.

Thanks,
drew

> 
>  hw/arm/virt-acpi-build.c | 6 ------
>  1 file changed, 6 deletions(-)
> 
> 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);
>  }
>  
> -- 
> 2.1.4
> 

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

[Leif Lindholm]

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

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

[Andrew Jones]

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

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

[Peter Maydell]

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

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

[Peter Maydell]

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

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

[Shannon Zhao]

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,
-- 
Shannon

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

[Leif Lindholm]

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

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

[G Gregory]

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

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

[Shannon Zhao]

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,
-- 
Shannon

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

[Leif Lindholm]

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

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

[Andrew Jones]

On Thu, Aug 20, 2015 at 12:21:31PM +0100, Leif Lindholm wrote:
> 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.

Well, it's a shame that the kernel patch that used ADR was committed to
Red Hat's and Linaro's trees before it had been thought through
completely, but it was. 

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

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?

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'll also reiterate the obvious fact that the kernel can switch to CRS
whenever it likes. That'll work just fine with or without QEMU taking
this change.

Thanks,
drew

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

[Peter Maydell]

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

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

[Shannon Zhao]

On 2015/8/6 20:28, 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 athttps://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...
>
> While the kernel change makes sense, I'm not sure we want this QEMU
> patch, as there*are*  kernels already using ADR. 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.

I agree. It would be better after the kernel patch get into upstream kernel.

> Actually, why was ADR used
> the first time? It would be good to know the story there in case there
> as a valid reason.

-- 
Shannon

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

[Peter Maydell]

On 6 August 2015 at 14:19, Shannon Zhao <shannon.zhao@linaro.org> wrote:
> On 2015/8/6 20:28, Andrew Jones wrote:
>> On Thu, Aug 06, 2015 at 12:24:27PM +0100, Leif Lindholm wrote:
>>> >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
>>> > athttps://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...
>>
>> While the kernel change makes sense, I'm not sure we want this QEMU
>> patch, as there*are*  kernels already using ADR. 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.
>
>
> I agree. It would be better after the kernel patch get into upstream kernel.

This kind of thing is why I was sceptical about accepting the ACPI
support patches before the ACPI code for everything went into the
upstream kernel :-(

-- PMM