[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH V3 2/3] hw/apci: handle 64-bit MMIO regions corr
|
From: |
Igor Mammedov |
|
Subject: |
Re: [Qemu-devel] [PATCH V3 2/3] hw/apci: handle 64-bit MMIO regions correctly |
|
Date: |
Wed, 29 Jun 2016 11:24:55 +0200 |
On Tue, 28 Jun 2016 20:08:42 +0300
Marcel Apfelbaum <address@hidden> wrote:
> 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.
Reviewed-by: Igor Mammedov <address@hidden>
>
>
> 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) {
> >
>