| Message ID | 1467107968-10410-3-git-send-email-marcel@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Tue, 28 Jun 2016 12:59:27 +0300 Marcel Apfelbaum <marcel@redhat.com> 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 | 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) { Isn't range_limit <= UINT32_MAX a sufficient, why length check is required? > + 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) {
On 06/28/2016 02:17 PM, Igor Mammedov wrote: > On Tue, 28 Jun 2016 12:59:27 +0300 > Marcel Apfelbaum <marcel@redhat.com> 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 | 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) { > Isn't range_limit <= UINT32_MAX a sufficient, why length check is required? > Hi Igor, Thanks for the review! Please see Laszlo's mathematical completeness theory :) : https://www.mail-archive.com/qemu-devel@nongnu.org/msg359583.html I preferred to keep it. Thanks, Marcel >> + 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) { >
On Tue, 28 Jun 2016 12:59:27 +0300 Marcel Apfelbaum <marcel@redhat.com> 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> 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? > --- > 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) {
On 06/28/2016 05:39 PM, Igor Mammedov wrote: > On Tue, 28 Jun 2016 12:59:27 +0300 > Marcel Apfelbaum <marcel@redhat.com> 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> > 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) { >
On Tue, 28 Jun 2016 20:08:42 +0300 Marcel Apfelbaum <marcel@redhat.com> wrote: > On 06/28/2016 05:39 PM, Igor Mammedov wrote: > > On Tue, 28 Jun 2016 12:59:27 +0300 > > Marcel Apfelbaum <marcel@redhat.com> 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> > > 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 <imammedo@redhat.com> > > > 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) { > > >
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) {
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 | 53 +++++++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 44 insertions(+), 9 deletions(-)