[PATCH] acpi: Add addr_trans in build_crs

Jiahui Cen posted 1 patch 3 years, 4 months ago
Test checkpatch passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20201217132747.4744-1-cenjiahui@huawei.com
Maintainers: "Michael S. Tsirkin" <mst@redhat.com>, Eduardo Habkost <ehabkost@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>, Richard Henderson <richard.henderson@linaro.org>, Igor Mammedov <imammedo@redhat.com>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
hw/acpi/aml-build.c         | 15 ++++++++-------
hw/i386/acpi-build.c        |  3 ++-
hw/pci-host/gpex-acpi.c     |  3 ++-
include/hw/acpi/aml-build.h |  4 +++-
4 files changed, 15 insertions(+), 10 deletions(-)
[PATCH] acpi: Add addr_trans in build_crs
Posted by Jiahui Cen 3 years, 4 months ago
AML needs Address Translation offset to describe how a bridge translates
addresses accross the bridge when using an address descriptor, and
especially on ARM, the translation offset of pio resource is usually
non zero.

Therefore, it's necessary to pass addr_trans for pio, mmio32 and mmio64
into build_crs.

Signed-off-by: Jiahui Cen <cenjiahui@huawei.com>
---
 hw/acpi/aml-build.c         | 15 ++++++++-------
 hw/i386/acpi-build.c        |  3 ++-
 hw/pci-host/gpex-acpi.c     |  3 ++-
 include/hw/acpi/aml-build.h |  4 +++-
 4 files changed, 15 insertions(+), 10 deletions(-)

diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index f976aa667b..be077b3ab6 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -2076,7 +2076,8 @@ void build_tpm2(GArray *table_data, BIOSLinker *linker, GArray *tcpalog)
                  tpm2_ptr, "TPM2", table_data->len - tpm2_start, 4, NULL, NULL);
 }
 
-Aml *build_crs(PCIHostState *host, CrsRangeSet *range_set)
+Aml *build_crs(PCIHostState *host, CrsRangeSet *range_set,
+               hwaddr io_trans, hwaddr mmio32_trans, hwaddr mmio64_trans)
 {
     Aml *crs = aml_resource_template();
     CrsRangeSet temp_range_set;
@@ -2189,10 +2190,10 @@ Aml *build_crs(PCIHostState *host, CrsRangeSet *range_set)
     for (i = 0; i < temp_range_set.io_ranges->len; i++) {
         entry = g_ptr_array_index(temp_range_set.io_ranges, i);
         aml_append(crs,
-                   aml_word_io(AML_MIN_FIXED, AML_MAX_FIXED,
-                               AML_POS_DECODE, AML_ENTIRE_RANGE,
-                               0, entry->base, entry->limit, 0,
-                               entry->limit - entry->base + 1));
+                   aml_dword_io(AML_MIN_FIXED, AML_MAX_FIXED,
+                                AML_POS_DECODE, AML_ENTIRE_RANGE,
+                                0, entry->base, entry->limit, io_trans,
+                                entry->limit - entry->base + 1));
         crs_range_insert(range_set->io_ranges, entry->base, entry->limit);
     }
 
@@ -2205,7 +2206,7 @@ Aml *build_crs(PCIHostState *host, CrsRangeSet *range_set)
                    aml_dword_memory(AML_POS_DECODE, AML_MIN_FIXED,
                                     AML_MAX_FIXED, AML_NON_CACHEABLE,
                                     AML_READ_WRITE,
-                                    0, entry->base, entry->limit, 0,
+                                    0, entry->base, entry->limit, mmio32_trans,
                                     entry->limit - entry->base + 1));
         crs_range_insert(range_set->mem_ranges, entry->base, entry->limit);
     }
@@ -2217,7 +2218,7 @@ Aml *build_crs(PCIHostState *host, CrsRangeSet *range_set)
                    aml_qword_memory(AML_POS_DECODE, AML_MIN_FIXED,
                                     AML_MAX_FIXED, AML_NON_CACHEABLE,
                                     AML_READ_WRITE,
-                                    0, entry->base, entry->limit, 0,
+                                    0, entry->base, entry->limit, mmio64_trans,
                                     entry->limit - entry->base + 1));
         crs_range_insert(range_set->mem_64bit_ranges,
                          entry->base, entry->limit);
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index f18b71dea9..7461ccad2c 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1360,7 +1360,8 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
             }
 
             aml_append(dev, build_prt(false));
-            crs = build_crs(PCI_HOST_BRIDGE(BUS(bus)->parent), &crs_range_set);
+            crs = build_crs(PCI_HOST_BRIDGE(BUS(bus)->parent), &crs_range_set,
+                            0, 0, 0);
             aml_append(dev, aml_name_decl("_CRS", crs));
             aml_append(scope, dev);
             aml_append(dsdt, scope);
diff --git a/hw/pci-host/gpex-acpi.c b/hw/pci-host/gpex-acpi.c
index 7f20ee1c98..071aa11b5c 100644
--- a/hw/pci-host/gpex-acpi.c
+++ b/hw/pci-host/gpex-acpi.c
@@ -168,7 +168,8 @@ void acpi_dsdt_add_gpex(Aml *scope, struct GPEXConfig *cfg)
              * 1. The resources the pci-brige/pcie-root-port need.
              * 2. The resources the devices behind pxb need.
              */
-            crs = build_crs(PCI_HOST_BRIDGE(BUS(bus)->parent), &crs_range_set);
+            crs = build_crs(PCI_HOST_BRIDGE(BUS(bus)->parent), &crs_range_set,
+                            cfg->pio.base, 0, 0);
             aml_append(dev, aml_name_decl("_CRS", crs));
 
             acpi_dsdt_add_pci_osc(dev);
diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
index e727bea1bc..ff3dcb703d 100644
--- a/include/hw/acpi/aml-build.h
+++ b/include/hw/acpi/aml-build.h
@@ -1,6 +1,7 @@
 #ifndef HW_ACPI_AML_BUILD_H
 #define HW_ACPI_AML_BUILD_H
 
+#include "exec/hwaddr.h"
 #include "hw/acpi/acpi-defs.h"
 #include "hw/acpi/bios-linker-loader.h"
 
@@ -452,7 +453,8 @@ void crs_replace_with_free_ranges(GPtrArray *ranges,
 void crs_range_set_init(CrsRangeSet *range_set);
 void crs_range_set_free(CrsRangeSet *range_set);
 
-Aml *build_crs(PCIHostState *host, CrsRangeSet *range_set);
+Aml *build_crs(PCIHostState *host, CrsRangeSet *range_set,
+               hwaddr io_trans, hwaddr mmio32_trans, hwaddr mmio64_trans);
 
 void build_srat_memory(AcpiSratMemoryAffinity *numamem, uint64_t base,
                        uint64_t len, int node, MemoryAffinityFlags flags);
-- 
2.28.0


Re: [PATCH] acpi: Add addr_trans in build_crs
Posted by Michael S. Tsirkin 3 years, 4 months ago
On Thu, Dec 17, 2020 at 09:27:47PM +0800, Jiahui Cen wrote:
> AML needs Address Translation offset to describe how a bridge translates
> addresses accross the bridge when using an address descriptor, and
> especially on ARM, the translation offset of pio resource is usually
> non zero.
> 
> Therefore, it's necessary to pass addr_trans for pio, mmio32 and mmio64
> into build_crs.
> 
> Signed-off-by: Jiahui Cen <cenjiahui@huawei.com>
> ---
>  hw/acpi/aml-build.c         | 15 ++++++++-------
>  hw/i386/acpi-build.c        |  3 ++-
>  hw/pci-host/gpex-acpi.c     |  3 ++-
>  include/hw/acpi/aml-build.h |  4 +++-
>  4 files changed, 15 insertions(+), 10 deletions(-)


Doesn't this result in any changes to expected files for tests?

> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> index f976aa667b..be077b3ab6 100644
> --- a/hw/acpi/aml-build.c
> +++ b/hw/acpi/aml-build.c
> @@ -2076,7 +2076,8 @@ void build_tpm2(GArray *table_data, BIOSLinker *linker, GArray *tcpalog)
>                   tpm2_ptr, "TPM2", table_data->len - tpm2_start, 4, NULL, NULL);
>  }
>  
> -Aml *build_crs(PCIHostState *host, CrsRangeSet *range_set)
> +Aml *build_crs(PCIHostState *host, CrsRangeSet *range_set,
> +               hwaddr io_trans, hwaddr mmio32_trans, hwaddr mmio64_trans)


More of io_offset etc I think.

>  {
>      Aml *crs = aml_resource_template();
>      CrsRangeSet temp_range_set;
> @@ -2189,10 +2190,10 @@ Aml *build_crs(PCIHostState *host, CrsRangeSet *range_set)
>      for (i = 0; i < temp_range_set.io_ranges->len; i++) {
>          entry = g_ptr_array_index(temp_range_set.io_ranges, i);
>          aml_append(crs,
> -                   aml_word_io(AML_MIN_FIXED, AML_MAX_FIXED,
> -                               AML_POS_DECODE, AML_ENTIRE_RANGE,
> -                               0, entry->base, entry->limit, 0,
> -                               entry->limit - entry->base + 1));
> +                   aml_dword_io(AML_MIN_FIXED, AML_MAX_FIXED,
> +                                AML_POS_DECODE, AML_ENTIRE_RANGE,
> +                                0, entry->base, entry->limit, io_trans,
> +                                entry->limit - entry->base + 1));
>          crs_range_insert(range_set->io_ranges, entry->base, entry->limit);
>      }
>  
> @@ -2205,7 +2206,7 @@ Aml *build_crs(PCIHostState *host, CrsRangeSet *range_set)
>                     aml_dword_memory(AML_POS_DECODE, AML_MIN_FIXED,
>                                      AML_MAX_FIXED, AML_NON_CACHEABLE,
>                                      AML_READ_WRITE,
> -                                    0, entry->base, entry->limit, 0,
> +                                    0, entry->base, entry->limit, mmio32_trans,
>                                      entry->limit - entry->base + 1));
>          crs_range_insert(range_set->mem_ranges, entry->base, entry->limit);
>      }
> @@ -2217,7 +2218,7 @@ Aml *build_crs(PCIHostState *host, CrsRangeSet *range_set)
>                     aml_qword_memory(AML_POS_DECODE, AML_MIN_FIXED,
>                                      AML_MAX_FIXED, AML_NON_CACHEABLE,
>                                      AML_READ_WRITE,
> -                                    0, entry->base, entry->limit, 0,
> +                                    0, entry->base, entry->limit, mmio64_trans,
>                                      entry->limit - entry->base + 1));
>          crs_range_insert(range_set->mem_64bit_ranges,
>                           entry->base, entry->limit);
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index f18b71dea9..7461ccad2c 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -1360,7 +1360,8 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
>              }
>  
>              aml_append(dev, build_prt(false));
> -            crs = build_crs(PCI_HOST_BRIDGE(BUS(bus)->parent), &crs_range_set);
> +            crs = build_crs(PCI_HOST_BRIDGE(BUS(bus)->parent), &crs_range_set,
> +                            0, 0, 0);
>              aml_append(dev, aml_name_decl("_CRS", crs));
>              aml_append(scope, dev);
>              aml_append(dsdt, scope);
> diff --git a/hw/pci-host/gpex-acpi.c b/hw/pci-host/gpex-acpi.c
> index 7f20ee1c98..071aa11b5c 100644
> --- a/hw/pci-host/gpex-acpi.c
> +++ b/hw/pci-host/gpex-acpi.c
> @@ -168,7 +168,8 @@ void acpi_dsdt_add_gpex(Aml *scope, struct GPEXConfig *cfg)
>               * 1. The resources the pci-brige/pcie-root-port need.
>               * 2. The resources the devices behind pxb need.
>               */
> -            crs = build_crs(PCI_HOST_BRIDGE(BUS(bus)->parent), &crs_range_set);
> +            crs = build_crs(PCI_HOST_BRIDGE(BUS(bus)->parent), &crs_range_set,
> +                            cfg->pio.base, 0, 0);
>              aml_append(dev, aml_name_decl("_CRS", crs));
>  
>              acpi_dsdt_add_pci_osc(dev);
> diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
> index e727bea1bc..ff3dcb703d 100644
> --- a/include/hw/acpi/aml-build.h
> +++ b/include/hw/acpi/aml-build.h
> @@ -1,6 +1,7 @@
>  #ifndef HW_ACPI_AML_BUILD_H
>  #define HW_ACPI_AML_BUILD_H
>  
> +#include "exec/hwaddr.h"
>  #include "hw/acpi/acpi-defs.h"
>  #include "hw/acpi/bios-linker-loader.h"
>  
> @@ -452,7 +453,8 @@ void crs_replace_with_free_ranges(GPtrArray *ranges,
>  void crs_range_set_init(CrsRangeSet *range_set);
>  void crs_range_set_free(CrsRangeSet *range_set);
>  
> -Aml *build_crs(PCIHostState *host, CrsRangeSet *range_set);
> +Aml *build_crs(PCIHostState *host, CrsRangeSet *range_set,
> +               hwaddr io_trans, hwaddr mmio32_trans, hwaddr mmio64_trans);
>  
>  void build_srat_memory(AcpiSratMemoryAffinity *numamem, uint64_t base,
>                         uint64_t len, int node, MemoryAffinityFlags flags);
> -- 
> 2.28.0


Re: [PATCH] acpi: Add addr_trans in build_crs
Posted by Jiahui Cen 3 years, 4 months ago
Hi Michael,

On 2020/12/18 2:32, Michael S. Tsirkin wrote:
> On Thu, Dec 17, 2020 at 09:27:47PM +0800, Jiahui Cen wrote:
>> AML needs Address Translation offset to describe how a bridge translates
>> addresses accross the bridge when using an address descriptor, and
>> especially on ARM, the translation offset of pio resource is usually
>> non zero.
>>
>> Therefore, it's necessary to pass addr_trans for pio, mmio32 and mmio64
>> into build_crs.
>>
>> Signed-off-by: Jiahui Cen <cenjiahui@huawei.com>
>> ---
>>  hw/acpi/aml-build.c         | 15 ++++++++-------
>>  hw/i386/acpi-build.c        |  3 ++-
>>  hw/pci-host/gpex-acpi.c     |  3 ++-
>>  include/hw/acpi/aml-build.h |  4 +++-
>>  4 files changed, 15 insertions(+), 10 deletions(-)
> 
> 
> Doesn't this result in any changes to expected files for tests?

Right, I'll update the expected file for test.

BTW, another patch that introduces _DSM #5 also seems to need
changes to expected files. Should I put them in one patch series
and update expected files once?

> 
>> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
>> index f976aa667b..be077b3ab6 100644
>> --- a/hw/acpi/aml-build.c
>> +++ b/hw/acpi/aml-build.c
>> @@ -2076,7 +2076,8 @@ void build_tpm2(GArray *table_data, BIOSLinker *linker, GArray *tcpalog)
>>                   tpm2_ptr, "TPM2", table_data->len - tpm2_start, 4, NULL, NULL);
>>  }
>>  
>> -Aml *build_crs(PCIHostState *host, CrsRangeSet *range_set)
>> +Aml *build_crs(PCIHostState *host, CrsRangeSet *range_set,
>> +               hwaddr io_trans, hwaddr mmio32_trans, hwaddr mmio64_trans)
> 
> 
> More of io_offset etc I think.

IIUC, io_trans is just the io_offset. Would it be better to rename
io_trans to io_offset? And seems I forgot to add bus number trans.

Thanks,
Jiahui
> 
>>  {
>>      Aml *crs = aml_resource_template();
>>      CrsRangeSet temp_range_set;
>> @@ -2189,10 +2190,10 @@ Aml *build_crs(PCIHostState *host, CrsRangeSet *range_set)
>>      for (i = 0; i < temp_range_set.io_ranges->len; i++) {
>>          entry = g_ptr_array_index(temp_range_set.io_ranges, i);
>>          aml_append(crs,
>> -                   aml_word_io(AML_MIN_FIXED, AML_MAX_FIXED,
>> -                               AML_POS_DECODE, AML_ENTIRE_RANGE,
>> -                               0, entry->base, entry->limit, 0,
>> -                               entry->limit - entry->base + 1));
>> +                   aml_dword_io(AML_MIN_FIXED, AML_MAX_FIXED,
>> +                                AML_POS_DECODE, AML_ENTIRE_RANGE,
>> +                                0, entry->base, entry->limit, io_trans,
>> +                                entry->limit - entry->base + 1));
>>          crs_range_insert(range_set->io_ranges, entry->base, entry->limit);
>>      }
>>  
>> @@ -2205,7 +2206,7 @@ Aml *build_crs(PCIHostState *host, CrsRangeSet *range_set)
>>                     aml_dword_memory(AML_POS_DECODE, AML_MIN_FIXED,
>>                                      AML_MAX_FIXED, AML_NON_CACHEABLE,
>>                                      AML_READ_WRITE,
>> -                                    0, entry->base, entry->limit, 0,
>> +                                    0, entry->base, entry->limit, mmio32_trans,
>>                                      entry->limit - entry->base + 1));
>>          crs_range_insert(range_set->mem_ranges, entry->base, entry->limit);
>>      }
>> @@ -2217,7 +2218,7 @@ Aml *build_crs(PCIHostState *host, CrsRangeSet *range_set)
>>                     aml_qword_memory(AML_POS_DECODE, AML_MIN_FIXED,
>>                                      AML_MAX_FIXED, AML_NON_CACHEABLE,
>>                                      AML_READ_WRITE,
>> -                                    0, entry->base, entry->limit, 0,
>> +                                    0, entry->base, entry->limit, mmio64_trans,
>>                                      entry->limit - entry->base + 1));
>>          crs_range_insert(range_set->mem_64bit_ranges,
>>                           entry->base, entry->limit);
>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>> index f18b71dea9..7461ccad2c 100644
>> --- a/hw/i386/acpi-build.c
>> +++ b/hw/i386/acpi-build.c
>> @@ -1360,7 +1360,8 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
>>              }
>>  
>>              aml_append(dev, build_prt(false));
>> -            crs = build_crs(PCI_HOST_BRIDGE(BUS(bus)->parent), &crs_range_set);
>> +            crs = build_crs(PCI_HOST_BRIDGE(BUS(bus)->parent), &crs_range_set,
>> +                            0, 0, 0);
>>              aml_append(dev, aml_name_decl("_CRS", crs));
>>              aml_append(scope, dev);
>>              aml_append(dsdt, scope);
>> diff --git a/hw/pci-host/gpex-acpi.c b/hw/pci-host/gpex-acpi.c
>> index 7f20ee1c98..071aa11b5c 100644
>> --- a/hw/pci-host/gpex-acpi.c
>> +++ b/hw/pci-host/gpex-acpi.c
>> @@ -168,7 +168,8 @@ void acpi_dsdt_add_gpex(Aml *scope, struct GPEXConfig *cfg)
>>               * 1. The resources the pci-brige/pcie-root-port need.
>>               * 2. The resources the devices behind pxb need.
>>               */
>> -            crs = build_crs(PCI_HOST_BRIDGE(BUS(bus)->parent), &crs_range_set);
>> +            crs = build_crs(PCI_HOST_BRIDGE(BUS(bus)->parent), &crs_range_set,
>> +                            cfg->pio.base, 0, 0);
>>              aml_append(dev, aml_name_decl("_CRS", crs));
>>  
>>              acpi_dsdt_add_pci_osc(dev);
>> diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
>> index e727bea1bc..ff3dcb703d 100644
>> --- a/include/hw/acpi/aml-build.h
>> +++ b/include/hw/acpi/aml-build.h
>> @@ -1,6 +1,7 @@
>>  #ifndef HW_ACPI_AML_BUILD_H
>>  #define HW_ACPI_AML_BUILD_H
>>  
>> +#include "exec/hwaddr.h"
>>  #include "hw/acpi/acpi-defs.h"
>>  #include "hw/acpi/bios-linker-loader.h"
>>  
>> @@ -452,7 +453,8 @@ void crs_replace_with_free_ranges(GPtrArray *ranges,
>>  void crs_range_set_init(CrsRangeSet *range_set);
>>  void crs_range_set_free(CrsRangeSet *range_set);
>>  
>> -Aml *build_crs(PCIHostState *host, CrsRangeSet *range_set);
>> +Aml *build_crs(PCIHostState *host, CrsRangeSet *range_set,
>> +               hwaddr io_trans, hwaddr mmio32_trans, hwaddr mmio64_trans);
>>  
>>  void build_srat_memory(AcpiSratMemoryAffinity *numamem, uint64_t base,
>>                         uint64_t len, int node, MemoryAffinityFlags flags);
>> -- 
>> 2.28.0
> 
> .
>