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

[Marcel Apfelbaum]

On 06/28/2016 05:39 PM, Igor Mammedov wrote:
> On Tue, 28 Jun 2016 12:59:27 +0300
> Marcel Apfelbaum <address@hidden> 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 <address@hidden>
> patch indeed fixes issue with truncating in aml_dword_memory()
> as per hunk
>
> @@ -3306,12 +3306,12 @@ DefinitionBlock ("", "DSDT", 1, "BOCHS ", "BXPCDSDT", 
> 0x00000001)
>                       0x00000000,         // Translation Offset
>                       0x00200000,         // Length
>                       ,, , AddressRangeMemory, TypeStatic)
> -                DWordMemory (ResourceProducer, PosDecode, MinFixed, MaxFixed, 
> NonCacheable, ReadWrite,
> -                    0x00000000,         // Granularity
> -                    0x00000000,         // Range Minimum
> -                    0xFFFFFFFF,         // Range Maximum
> -                    0x00000000,         // Translation Offset
> -                    0x00000000,         // Length
> +                QWordMemory (ResourceProducer, PosDecode, MinFixed, MaxFixed, 
> NonCacheable, ReadWrite,
> +                    0x0000000000000000, // Granularity
> +                    0x0000000100000000, // Range Minimum
> +                    0x00000001FFFFFFFF, // Range Maximum
> +                    0x0000000000000000, // Translation Offset
> +                    0x0000000100000000, // Length
>                       ,, , AddressRangeMemory, TypeStatic)
>                   WordBusNumber (ResourceProducer, MinFixed, MaxFixed, 
> PosDecode,
>                       0x0000,             // Granularity
>
> how a second hunk is present which touches 32bit part of _CRS:
>
> @@ -3372,9 +3372,9 @@ DefinitionBlock ("", "DSDT", 1, "BOCHS ", "BXPCDSDT", 
> 0x00000001)
>               DWordMemory (ResourceProducer, PosDecode, MinFixed, MaxFixed, 
> NonCacheable, ReadWrite,
>                   0x00000000,         // Granularity
>                   0xFEA00000,         // Range Minimum
> -                0xFFFFFFFF,         // Range Maximum
> +                0xFEBFFFFF,         // Range Maximum
>                   0x00000000,         // Translation Offset
> -                0x01600000,         // Length
> +                0x00200000,         // Length
>                   ,, , AddressRangeMemory, TypeStatic)
>           })
>
> was it expected? Why?

Yes, it is expected. It is the same bug. If you try a pc machine
without pxb you will have  0xFEBFFFFF as the 32-bit upper IOMMU limit.

However, when having 32-bit ranges being merged with 64-bit ranges will result
in a wrong upper limit.

So this is a second fix to the same problem.


Thanks,
Marcel

> > ---
> >   hw/i386/acpi-build.c | 53 +++++++++++++++++++++++++++++++++++++++++++---------
> >   1 file changed, 44 insertions(+), 9 deletions(-)
> >
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index f306ae3..3808347 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -746,18 +746,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)
> > @@ -915,8 +919,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 =
> > @@ -929,8 +939,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);
> > +                }
> >               }
> >           }
> >       }
> > @@ -958,6 +974,19 @@ static Aml *build_crs(PCIHostState *host, CrsRangeSet 
> > *range_set)
> >           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,
> > @@ -2079,11 +2108,17 @@ build_dsdt(GArray *table_data, BIOSLinker *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));
> > +        }
> >       }
> >
> >       if (misc->tpm_version != TPM_VERSION_UNSPEC) {