Re: [Qemu-devel] [PATCH v4 38/42] pc: acpi-build: create PCI0._CRS dynamically

[Michael S. Tsirkin]

On Wed, Feb 18, 2015 at 07:14:51PM +0000, Igor Mammedov wrote:
> Replace template patching and runtime
> calculation in _CRS() method with static _CRS
> defined in SSDT.
> 
> It also drops manual hole patching for reserved
> PCI/MEM/CPU hoptlug MMIO resources and utilizes
> the fact that MMIO resources are reserved by
> respective child /i.e. PHPR, MHPD, PRES/ containers.
> 
> Signed-off-by: Igor Mammedov <address@hidden>

This should be split.

Can we have
patch 1 replace ASL with C
patch 2 simplify ranges
?

I seem to remember reserving ranges like that caused
problems in the past, and even if not, it's cleaner
not to mix code rework and functional changes.

> ---
>  hw/i386/acpi-build.c          | 69 ++++++++++++++++----------------
>  hw/i386/acpi-dsdt-pci-crs.dsl | 92 
> -------------------------------------------
>  hw/i386/acpi-dsdt.dsl         | 45 ---------------------
>  hw/i386/q35-acpi-dsdt.dsl     | 18 ---------
>  hw/i386/ssdt-misc.dsl         | 19 ---------
>  5 files changed, 33 insertions(+), 210 deletions(-)
>  delete mode 100644 hw/i386/acpi-dsdt-pci-crs.dsl
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 4d5d7e3..ee254df 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -346,24 +346,6 @@ static void acpi_align_size(GArray *blob, unsigned align)
>      g_array_set_size(blob, ROUND_UP(acpi_data_len(blob), align));
>  }
>  
> -/* Set a value within table in a safe manner */
> -#define ACPI_BUILD_SET_LE(table, size, off, bits, val) \
> -    do { \
> -        uint64_t ACPI_BUILD_SET_LE_val = cpu_to_le64(val); \
> -        memcpy(acpi_data_get_ptr(table, size, off, \
> -                                 (bits) / BITS_PER_BYTE), \
> -               &ACPI_BUILD_SET_LE_val, \
> -               (bits) / BITS_PER_BYTE); \
> -    } while (0)
> -
> -static inline void *acpi_data_get_ptr(uint8_t *table_data, unsigned 
> table_size,
> -                                      unsigned off, unsigned size)
> -{
> -    assert(off + size > off);
> -    assert(off + size <= table_size);
> -    return table_data + off;
> -}
> -
>  static inline void acpi_add_table(GArray *table_offsets, GArray *table_data)
>  {
>      uint32_t offset = cpu_to_le32(table_data->len);
> @@ -860,22 +842,6 @@ static void build_pci_bus_end(PCIBus *bus, void 
> *bus_state)
>      g_free(child);
>  }
>  
> -static void patch_pci_windows(PcPciInfo *pci, uint8_t *start, unsigned size)
> -{
> -    ACPI_BUILD_SET_LE(start, size, acpi_pci32_start[0], 32, pci->w32.begin);
> -
> -    ACPI_BUILD_SET_LE(start, size, acpi_pci32_end[0], 32, pci->w32.end - 1);
> -
> -    if (pci->w64.end || pci->w64.begin) {
> -        ACPI_BUILD_SET_LE(start, size, acpi_pci64_valid[0], 8, 1);
> -        ACPI_BUILD_SET_LE(start, size, acpi_pci64_start[0], 64, 
> pci->w64.begin);
> -        ACPI_BUILD_SET_LE(start, size, acpi_pci64_end[0], 64, pci->w64.end - 
> 1);
> -        ACPI_BUILD_SET_LE(start, size, acpi_pci64_length[0], 64, 
> pci->w64.end - pci->w64.begin);
> -    } else {
> -        ACPI_BUILD_SET_LE(start, size, acpi_pci64_valid[0], 8, 0);
> -    }
> -}
> -
>  static void
>  build_ssdt(GArray *table_data, GArray *linker,
>             AcpiCpuInfo *cpu, AcpiPmInfo *pm, AcpiMiscInfo *misc,
> @@ -898,9 +864,40 @@ build_ssdt(GArray *table_data, GArray *linker,
>      ssdt_ptr = acpi_data_push(ssdt->buf, sizeof(ssdp_misc_aml));
>      memcpy(ssdt_ptr, ssdp_misc_aml, sizeof(ssdp_misc_aml));
>  
> -    patch_pci_windows(pci, ssdt_ptr, sizeof(ssdp_misc_aml));
> -
>      scope = aml_scope("\\_SB.PCI0");
> +    /* build PCI0._CRS */
> +    crs = aml_resource_template();
> +    aml_append(crs,
> +        aml_word_bus_number(aml_min_fixed, aml_max_fixed, aml_pos_decode,
> +                            0x0000, 0x0000, 0x00FF, 0x0000, 0x0100));
> +    aml_append(crs, aml_io(aml_decode16, 0x0CF8, 0x0CF8, 0x01, 0x08));
> +
> +    aml_append(crs,
> +        aml_word_io(aml_min_fixed, aml_max_fixed,
> +                    aml_pos_decode, aml_entire_range,
> +                    0x0000, 0x0000, 0x0CF7, 0x0000, 0x0CF8));
> +    aml_append(crs,
> +        aml_word_io(aml_min_fixed, aml_max_fixed,
> +                    aml_pos_decode, aml_entire_range,
> +                    0x0000, 0x0D00, 0xFFFF, 0x0000, 0xF300));
> +    aml_append(crs,
> +        aml_dword_memory(aml_pos_decode, aml_min_fixed, aml_max_fixed,
> +                         aml_cacheable, aml_ReadWrite,
> +                         0, 0x000A0000, 0x000BFFFF, 0, 0x00020000));
> +    aml_append(crs,
> +        aml_dword_memory(aml_pos_decode, aml_min_fixed, aml_max_fixed,
> +                         aml_non_cacheable, aml_ReadWrite,
> +                         0, pci->w32.begin, pci->w32.end - 1, 0,
> +                         pci->w32.end - pci->w32.begin));
> +    if (pci->w64.begin) {
> +        aml_append(crs,
> +            aml_qword_memory(aml_pos_decode, aml_min_fixed, aml_max_fixed,
> +                             aml_cacheable, aml_ReadWrite,
> +                             0, pci->w64.begin, pci->w64.end - 1, 0,
> +                             pci->w64.end - pci->w64.begin));
> +    }
> +    aml_append(scope, aml_name_decl("_CRS", crs));
> +
>      /* reserve PCIHP resources */
>      if (pm->pcihp_io_len) {
>          dev = aml_device("PHPR");
> diff --git a/hw/i386/acpi-dsdt-pci-crs.dsl b/hw/i386/acpi-dsdt-pci-crs.dsl
> deleted file mode 100644
> index 4648e90..0000000
> --- a/hw/i386/acpi-dsdt-pci-crs.dsl
> +++ /dev/null
> @@ -1,92 +0,0 @@
> -/*
> - * This program is free software; you can redistribute it and/or modify
> - * it under the terms of the GNU General Public License as published by
> - * the Free Software Foundation; either version 2 of the License, or
> - * (at your option) any later version.
> -
> - * This program is distributed in the hope that it will be useful,
> - * but WITHOUT ANY WARRANTY; without even the implied warranty of
> - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> - * GNU General Public License for more details.
> -
> - * You should have received a copy of the GNU General Public License along
> - * with this program; if not, see <http://www.gnu.org/licenses/>.
> - */
> -
> -/* PCI CRS (current resources) definition. */
> -Scope(\_SB.PCI0) {
> -
> -    Name(CRES, ResourceTemplate() {
> -        WordBusNumber(ResourceProducer, MinFixed, MaxFixed, PosDecode,
> -            0x0000,             // Address Space Granularity
> -            0x0000,             // Address Range Minimum
> -            0x00FF,             // Address Range Maximum
> -            0x0000,             // Address Translation Offset
> -            0x0100,             // Address Length
> -            ,, )
> -        IO(Decode16,
> -            0x0CF8,             // Address Range Minimum
> -            0x0CF8,             // Address Range Maximum
> -            0x01,               // Address Alignment
> -            0x08,               // Address Length
> -            )
> -        BOARD_SPECIFIC_PCI_RESOURSES
> -        DWordMemory(ResourceProducer, PosDecode, MinFixed, MaxFixed, 
> Cacheable, ReadWrite,
> -            0x00000000,         // Address Space Granularity
> -            0x000A0000,         // Address Range Minimum
> -            0x000BFFFF,         // Address Range Maximum
> -            0x00000000,         // Address Translation Offset
> -            0x00020000,         // Address Length
> -            ,, , AddressRangeMemory, TypeStatic)
> -        DWordMemory(ResourceProducer, PosDecode, MinFixed, MaxFixed, 
> NonCacheable, ReadWrite,
> -            0x00000000,         // Address Space Granularity
> -            0xE0000000,         // Address Range Minimum
> -            0xFEBFFFFF,         // Address Range Maximum
> -            0x00000000,         // Address Translation Offset
> -            0x1EC00000,         // Address Length
> -            ,, PW32, AddressRangeMemory, TypeStatic)
> -    })
> -
> -    Name(CR64, ResourceTemplate() {
> -        QWordMemory(ResourceProducer, PosDecode, MinFixed, MaxFixed, 
> Cacheable, ReadWrite,
> -            0x00000000,          // Address Space Granularity
> -            0x8000000000,        // Address Range Minimum
> -            0xFFFFFFFFFF,        // Address Range Maximum
> -            0x00000000,          // Address Translation Offset
> -            0x8000000000,        // Address Length
> -            ,, PW64, AddressRangeMemory, TypeStatic)
> -    })
> -
> -    Method(_CRS, 0) {
> -        /* Fields provided by dynamically created ssdt */
> -        External(P0S, IntObj)
> -        External(P0E, IntObj)
> -        External(P1V, IntObj)
> -        External(P1S, BuffObj)
> -        External(P1E, BuffObj)
> -        External(P1L, BuffObj)
> -
> -        /* fixup 32bit pci io window */
> -        CreateDWordField(CRES, \_SB.PCI0.PW32._MIN, PS32)
> -        CreateDWordField(CRES, \_SB.PCI0.PW32._MAX, PE32)
> -        CreateDWordField(CRES, \_SB.PCI0.PW32._LEN, PL32)
> -        Store(P0S, PS32)
> -        Store(P0E, PE32)
> -        Store(Add(Subtract(P0E, P0S), 1), PL32)
> -
> -        If (LEqual(P1V, Zero)) {
> -            Return (CRES)
> -        }
> -
> -        /* fixup 64bit pci io window */
> -        CreateQWordField(CR64, \_SB.PCI0.PW64._MIN, PS64)
> -        CreateQWordField(CR64, \_SB.PCI0.PW64._MAX, PE64)
> -        CreateQWordField(CR64, \_SB.PCI0.PW64._LEN, PL64)
> -        Store(P1S, PS64)
> -        Store(P1E, PE64)
> -        Store(P1L, PL64)
> -        /* add window and return result */
> -        ConcatenateResTemplate(CRES, CR64, Local0)
> -        Return (Local0)
> -    }
> -}
> diff --git a/hw/i386/acpi-dsdt.dsl b/hw/i386/acpi-dsdt.dsl
> index a611e07..09b68f0 100644
> --- a/hw/i386/acpi-dsdt.dsl
> +++ b/hw/i386/acpi-dsdt.dsl
> @@ -31,50 +31,6 @@ DefinitionBlock (
>  
>  #include "acpi-dsdt-dbug.dsl"
>  
> -
> -/****************************************************************
> - * PCI Bus definition
> - ****************************************************************/
> -#define BOARD_SPECIFIC_PCI_RESOURSES \
> -     WordIO(ResourceProducer, MinFixed, MaxFixed, PosDecode, EntireRange, \
> -         0x0000, \
> -         0x0000, \
> -         0x0CF7, \
> -         0x0000, \
> -         0x0CF8, \
> -         ,, , TypeStatic) \
> -     WordIO(ResourceProducer, MinFixed, MaxFixed, PosDecode, EntireRange, \
> -         0x0000, \
> -         0x0D00, \
> -         0xADFF, \
> -         0x0000, \
> -         0xA100, \
> -         ,, , TypeStatic) \
> -     /* 0xae00-0xae0e hole for PCI hotplug, hw/acpi/piix4.c:PCI_HOTPLUG_ADDR 
> */ \
> -     WordIO(ResourceProducer, MinFixed, MaxFixed, PosDecode, EntireRange, \
> -         0x0000, \
> -         0xAE0F, \
> -         0xAEFF, \
> -         0x0000, \
> -         0x00F1, \
> -         ,, , TypeStatic) \
> -     /* 0xaf00-0xaf1f hole for CPU hotplug, hw/acpi/piix4.c:PIIX4_PROC_BASE 
> */ \
> -     WordIO(ResourceProducer, MinFixed, MaxFixed, PosDecode, EntireRange, \
> -         0x0000, \
> -         0xAF20, \
> -         0xAFDF, \
> -         0x0000, \
> -         0x00C0, \
> -         ,, , TypeStatic) \
> -     /* 0xafe0-0xafe3 hole for ACPI.GPE0, hw/acpi/piix4.c:GPE_BASE */ \
> -     WordIO(ResourceProducer, MinFixed, MaxFixed, PosDecode, EntireRange, \
> -         0x0000, \
> -         0xAFE4, \
> -         0xFFFF, \
> -         0x0000, \
> -         0x501C, \
> -         ,, , TypeStatic)
> -
>      Scope(\_SB) {
>          Device(PCI0) {
>              Name(_HID, EisaId("PNP0A03"))
> @@ -85,7 +41,6 @@ DefinitionBlock (
>          }
>      }
>  
> -#include "acpi-dsdt-pci-crs.dsl"
>  #include "acpi-dsdt-hpet.dsl"
>  
>  
> diff --git a/hw/i386/q35-acpi-dsdt.dsl b/hw/i386/q35-acpi-dsdt.dsl
> index e1cee5d..3fb4b2f 100644
> --- a/hw/i386/q35-acpi-dsdt.dsl
> +++ b/hw/i386/q35-acpi-dsdt.dsl
> @@ -48,23 +48,6 @@ DefinitionBlock (
>  /****************************************************************
>   * PCI Bus definition
>   ****************************************************************/
> -#define BOARD_SPECIFIC_PCI_RESOURSES \
> -     WordIO(ResourceProducer, MinFixed, MaxFixed, PosDecode, EntireRange, \
> -         0x0000, \
> -         0x0000, \
> -         0x0CD7, \
> -         0x0000, \
> -         0x0CD8, \
> -         ,, , TypeStatic) \
> -     /* 0xcd8-0xcf7 hole for CPU hotplug, hw/acpi/ich9.c:ICH9_PROC_BASE */ \
> -     WordIO(ResourceProducer, MinFixed, MaxFixed, PosDecode, EntireRange, \
> -         0x0000, \
> -         0x0D00, \
> -         0xFFFF, \
> -         0x0000, \
> -         0xF300, \
> -         ,, , TypeStatic)
> -
>      Scope(\_SB) {
>          Device(PCI0) {
>              Name(_HID, EisaId("PNP0A08"))
> @@ -131,7 +114,6 @@ DefinitionBlock (
>          }
>      }
>  
> -#include "acpi-dsdt-pci-crs.dsl"
>  #include "acpi-dsdt-hpet.dsl"
>  
>  
> diff --git a/hw/i386/ssdt-misc.dsl b/hw/i386/ssdt-misc.dsl
> index 2588e30..8d61f21 100644
> --- a/hw/i386/ssdt-misc.dsl
> +++ b/hw/i386/ssdt-misc.dsl
> @@ -18,23 +18,4 @@ ACPI_EXTRACT_ALL_CODE ssdp_misc_aml
>  
>  DefinitionBlock ("ssdt-misc.aml", "SSDT", 0x01, "BXPC", "BXSSDTSUSP", 0x1)
>  {
> -
> -/****************************************************************
> - * PCI memory ranges
> - ****************************************************************/
> -
> -    Scope(\) {
> -       ACPI_EXTRACT_NAME_DWORD_CONST acpi_pci32_start
> -       Name(P0S, 0x12345678)
> -       ACPI_EXTRACT_NAME_DWORD_CONST acpi_pci32_end
> -       Name(P0E, 0x12345678)
> -       ACPI_EXTRACT_NAME_BYTE_CONST acpi_pci64_valid
> -       Name(P1V, 0x12)
> -       ACPI_EXTRACT_NAME_BUFFER8 acpi_pci64_start
> -       Name(P1S, Buffer() { 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 })
> -       ACPI_EXTRACT_NAME_BUFFER8 acpi_pci64_end
> -       Name(P1E, Buffer() { 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 })
> -       ACPI_EXTRACT_NAME_BUFFER8 acpi_pci64_length
> -       Name(P1L, Buffer() { 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 })
> -    }
>  }
> -- 
> 1.8.3.1