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 <address@hidden>
> ---
> 
> 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
>