| Message ID | 1438860267-3401-1-git-send-email-leif.lindholm@linaro.org |
|---|---|
| State | New |
| Headers | show |
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
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
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
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,
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
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
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,
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
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
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); }
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(-)