Re: [Qemu-devel] [PATCH 4/4] hw/apci: handle 64-bit MMIO regions correctly

[Laszlo Ersek <lersek@redhat.com>]

On 05/02/16 17:37, Marcel Apfelbaum wrote:
> In build_crs(), the calculation and 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(). Fix it by handling the 64-bit MMIO ranges separately.
> This fixes 64-bit BARs behind PXBs.
> 
> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
> ---
>  hw/i386/acpi-build.c | 61 +++++++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 48 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index be002d7..af8b0d3 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -739,18 +739,22 @@ static void crs_range_free(gpointer data)
>  typedef struct CrsRangeSet {
>      GPtrArray *io_ranges;
>      GPtrArray *mem_ranges;
> +    GPtrArray *mem_64bit_ranges;
>   } CrsRangeSet;
>  
>  static void crs_range_set_init(CrsRangeSet *range_set)
>  {
>      range_set->io_ranges = g_ptr_array_new_with_free_func(crs_range_free);
>      range_set->mem_ranges = g_ptr_array_new_with_free_func(crs_range_free);
> +    range_set->mem_64bit_ranges =
> +            g_ptr_array_new_with_free_func(crs_range_free);
>  }
>  
>  static void crs_range_set_free(CrsRangeSet *range_set)
>  {
>      g_ptr_array_free(range_set->io_ranges, true);
>      g_ptr_array_free(range_set->mem_ranges, true);
> +    g_ptr_array_free(range_set->mem_64bit_ranges, true);
>  }
>  
>  static gint crs_range_compare(gconstpointer a, gconstpointer b)
> @@ -908,8 +912,14 @@ static Aml *build_crs(PCIHostState *host, CrsRangeSet *range_set)
>               * that do not support multiple root buses
>               */
>              if (range_base && range_base <= range_limit) {
> -                crs_range_insert(temp_range_set.mem_ranges,
> -                                 range_base, range_limit);
> +                uint64_t length = range_limit - range_base + 1;
> +                if (range_limit <= UINT32_MAX && length <= UINT32_MAX) {
> +                    crs_range_insert(temp_range_set.mem_ranges,
> +                                     range_base, range_limit);
> +                } else {
> +                    crs_range_insert(temp_range_set.mem_64bit_ranges,
> +                                     range_base, range_limit);
> +                }
>              }
>  
>              range_base =
> @@ -922,8 +932,14 @@ static Aml *build_crs(PCIHostState *host, CrsRangeSet *range_set)
>               * that do not support multiple root buses
>               */
>              if (range_base && range_base <= range_limit) {
> -                crs_range_insert(temp_range_set.mem_ranges,
> -                                 range_base, range_limit);
> +                uint64_t length = range_limit - range_base + 1;
> +                if (range_limit <= UINT32_MAX && length <= UINT32_MAX) {
> +                    crs_range_insert(temp_range_set.mem_ranges,
> +                                     range_base, range_limit);
> +                } else {
> +                    crs_range_insert(temp_range_set.mem_64bit_ranges,
> +                                     range_base, range_limit);
> +                }
>              }
>          }
>      }
> @@ -944,13 +960,26 @@ static Aml *build_crs(PCIHostState *host, CrsRangeSet *range_set)
>          entry = g_ptr_array_index(temp_range_set.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));
> +                   AML_MAX_FIXED, AML_NON_CACHEABLE,
> +                   AML_READ_WRITE,
> +                   0, entry->base, entry->limit, 0,
> +                   entry->limit - entry->base + 1));

(Thanks for the opportunity for a little word play here:)

Unintented unindent?

Laszlo

>          crs_range_insert(range_set->mem_ranges, entry->base, entry->limit);
>      }
>  
> +    crs_range_merge(temp_range_set.mem_64bit_ranges);
> +    for (i = 0; i < temp_range_set.mem_64bit_ranges->len; i++) {
> +        entry = g_ptr_array_index(temp_range_set.mem_64bit_ranges, i);
> +        aml_append(crs,
> +                   aml_qword_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));
> +        crs_range_insert(range_set->mem_64bit_ranges,
> +                         entry->base, entry->limit);
> +    }
> +
>      crs_range_set_free(&temp_range_set);
>  
>      aml_append(crs,
> @@ -2182,11 +2211,17 @@ build_dsdt(GArray *table_data, GArray *linker,
>      }
>  
>      if (pci->w64.begin) {
> -        aml_append(crs,
> -            aml_qword_memory(AML_POS_DECODE, AML_MIN_FIXED, AML_MAX_FIXED,
> -                             AML_CACHEABLE, AML_READ_WRITE,
> -                             0, pci->w64.begin, pci->w64.end - 1, 0,
> -                             pci->w64.end - pci->w64.begin));
> +        crs_replace_with_free_ranges(crs_range_set.mem_64bit_ranges,
> +                                     pci->w64.begin, pci->w64.end - 1);
> +        for (i = 0; i < crs_range_set. mem_64bit_ranges->len; i++) {
> +            entry = g_ptr_array_index(crs_range_set.mem_64bit_ranges, i);
> +            aml_append(crs,
> +                       aml_qword_memory(AML_POS_DECODE, AML_MIN_FIXED,
> +                                        AML_MAX_FIXED,
> +                                        AML_CACHEABLE, AML_READ_WRITE,
> +                                        0, entry->base, entry->limit,
> +                                        0, entry->limit - entry->base + 1));
> +        }
>      }
>      aml_append(scope, aml_name_decl("_CRS", crs));
>  
>