Re: [Qemu-devel] [PATCH] hw/i386/acpi-build: place qword descriptors in bridge _CRS's when needed

[Marcel Apfelbaum <marcel@redhat.com>]

On 03/14/2016 03:42 AM, Laszlo Ersek wrote:
> In build_crs(), the calculation & merging of the ranges already happens in
> 64-bit, but the entry boundaries are silently truncated to 32-bit in the
> call to aml_dword_memory(). Use aml_qword_memory() when necessary -- this
> fixes 64-bit BARs behind PXBs.
>

Hi Laszlo,
Thanks for the patch.

Please see below some comments.


> Cc: Marcel Apfelbaum <marcel@redhat.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>   hw/i386/acpi-build.c | 24 ++++++++++++++++++------
>   1 file changed, 18 insertions(+), 6 deletions(-)
>
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index b88800883944..3157cc36db98 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -938,13 +938,25 @@ static Aml *build_crs(PCIHostState *host,
>
>       crs_range_merge(host_mem_ranges);
>       for (i = 0; i < host_mem_ranges->len; i++) {
> +        Aml *mem;
> +        uint64_t length;
> +
>           entry = g_ptr_array_index(host_mem_ranges, i);
> -        aml_append(crs,
> -                   aml_dword_memory(AML_POS_DECODE, AML_MIN_FIXED,
> -                                    AML_MAX_FIXED, AML_NON_CACHEABLE,
> -                                    AML_READ_WRITE,
> -                                    0, entry->base, entry->limit, 0,
> -                                    entry->limit - entry->base + 1));
> +        length = entry->limit - entry->base + 1;
> +        if (entry->limit <= UINT32_MAX && length <= UINT32_MAX) {

Why do we need to check the length if we've already checked the entry->limit ?

> +          mem = aml_dword_memory(AML_POS_DECODE, AML_MIN_FIXED,
> +                                 AML_MAX_FIXED, AML_NON_CACHEABLE,
> +                                 AML_READ_WRITE,
> +                                 0, entry->base, entry->limit, 0,
> +                                 length);
> +        } else {
> +          mem = aml_qword_memory(AML_POS_DECODE, AML_MIN_FIXED,
> +                                 AML_MAX_FIXED, AML_NON_CACHEABLE,
> +                                 AML_READ_WRITE,
> +                                 0, entry->base, entry->limit, 0,
> +                                 length);
> +        }
> +        aml_append(crs, mem);
>           crs_range_insert(mem_ranges, entry->base, entry->limit);
>       }
>       g_ptr_array_free(host_mem_ranges, true);
>

I think it is correct, but this also means the mem_ranges array can have 64-bit ranges =>
the 'crs_replace_with_free_ranges' call for mem_ranges is also incorrect because
it assumes all the ranges are between [pci->w32.begin, pci->w32.end - 1].

And of course this would also interfere with the crs building for pci->w64.
We can't assign all the [pci->w64.begin, pci->w64.end - 1] range to bus 0 anymore,
we need to take out the ranges used by pxbs. (same as we did for pci->w32)

Indeed, this is one of the pxb limitations, supporting only 32bit BARs and your
patch is going in the right direction.

Do you want to continue it? I will not be available for one week, but I can take care of it
after that.

Thanks!
Marcel