From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58929) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YgRxL-0004oM-Rm for qemu-devel@nongnu.org; Fri, 10 Apr 2015 01:58:21 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YgRxH-0002WS-QC for qemu-devel@nongnu.org; Fri, 10 Apr 2015 01:58:19 -0400 Received: from szxga01-in.huawei.com ([58.251.152.64]:11235) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YgRxG-0002WA-J3 for qemu-devel@nongnu.org; Fri, 10 Apr 2015 01:58:15 -0400 Message-ID: <5527665B.6000505@huawei.com> Date: Fri, 10 Apr 2015 13:57:47 +0800 From: Shannon Zhao MIME-Version: 1.0 References: <1428055432-12120-1-git-send-email-zhaoshenglong@huawei.com> <1428055432-12120-7-git-send-email-zhaoshenglong@huawei.com> <87d23d1vfe.fsf@linaro.org> In-Reply-To: <87d23d1vfe.fsf@linaro.org> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH v4 06/20] hw/arm/virt-acpi-build: Generation of DSDT table for virt devices List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?UTF-8?B?QWxleCBCZW5uw6ll?= Cc: peter.maydell@linaro.org, hangaohuai@huawei.com, mst@redhat.com, a.spyridakis@virtualopensystems.com, msalter@redhat.com, claudio.fontana@huawei.com, qemu-devel@nongnu.org, peter.huangpeng@huawei.com, hanjun.guo@linaro.org, imammedo@redhat.com, pbonzini@redhat.com, lersek@redhat.com, christoffer.dall@linaro.org, shannon.zhao@linaro.org On 2015/4/9 17:51, Alex Bennée wrote: > > Shannon Zhao writes: > >> From: Shannon Zhao >> >> DSDT consists of the usual common table header plus a definition >> block in AML encoding which describes all devices in the platform. >> >> After initializing DSDT with header information the namespace is >> created which is followed by the device encodings. The devices are >> described using the Resource Template for the 32-Bit Fixed Memory >> Range and the Extended Interrupt Descriptors. >> >> Signed-off-by: Shannon Zhao >> Signed-off-by: Shannon Zhao >> --- >> hw/arm/virt-acpi-build.c | 137 +++++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 137 insertions(+) >> >> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c >> index 388838a..516c1d0 100644 >> --- a/hw/arm/virt-acpi-build.c >> +++ b/hw/arm/virt-acpi-build.c >> @@ -57,6 +57,139 @@ >> #define ACPI_BUILD_DPRINTF(fmt, ...) >> #endif >> >> +static void acpi_dsdt_add_cpus(Aml *scope, int max_cpus) >> +{ >> + Aml *dev, *crs; >> + int i; >> + char name[5]; > > name, dev and crs could be declared inside the for() loop. > >> + for (i = 0; i < max_cpus; i++) { >> + snprintf(name, 5, "CPU%u", i); > > What happens here if you have 10 or 100 CPUs? > >> + dev = aml_device("%s", name); > > Also aml_device seems to take a format string so why not simply: > > dev = aml_device("CPU%u", i) > Ok, it's better. >> + aml_append(dev, aml_name_decl("_HID", aml_string("ACPI0007"))); >> + aml_append(dev, aml_name_decl("_UID", aml_int(i))); >> + crs = aml_resource_template(); >> + aml_append(dev, aml_name_decl("_CRS", crs)); >> + aml_append(scope, dev); >> + } >> +} >> + >> +static void acpi_dsdt_add_uart(Aml *scope, const hwaddr *uart_addr, >> + const int *uart_irq) >> +{ >> + Aml *dev, *crs; >> + >> + dev = aml_device("COM0"); >> + aml_append(dev, aml_name_decl("_HID", aml_string("ARMH0011"))); >> + aml_append(dev, aml_name_decl("_UID", aml_int(0))); >> + >> + crs = aml_resource_template(); >> + aml_append(crs, >> + aml_memory32_fixed(uart_addr[0], uart_addr[1], 0x01)); >> + aml_append(crs, >> + aml_interrupt(0x01, *uart_irq + 32)); >> + aml_append(dev, aml_name_decl("_CRS", crs)); >> + aml_append(scope, dev); >> +} >> + >> +static void acpi_dsdt_add_rtc(Aml *scope, const hwaddr *rtc_addr, >> + const int *rtc_irq) >> +{ >> + Aml *dev, *crs; >> + >> + dev = aml_device("RTC0"); >> + aml_append(dev, aml_name_decl("_HID", aml_string("LNRO0013"))); >> + aml_append(dev, aml_name_decl("_UID", aml_int(0))); >> + >> + crs = aml_resource_template(); >> + aml_append(crs, >> + aml_memory32_fixed(rtc_addr[0], rtc_addr[1], 0x01)); >> + aml_append(crs, >> + aml_interrupt(0x01, *rtc_irq + 32)); >> + aml_append(dev, aml_name_decl("_CRS", crs)); >> + aml_append(scope, dev); >> +} >> + >> +static void acpi_dsdt_add_flash(Aml *scope, const hwaddr *flash_addr) >> +{ >> + Aml *dev, *crs; >> + hwaddr base = flash_addr[0]; >> + hwaddr size = flash_addr[1]; >> + >> + dev = aml_device("FLS0"); >> + aml_append(dev, aml_name_decl("_HID", aml_string("LNRO0015"))); >> + aml_append(dev, aml_name_decl("_UID", aml_int(0))); >> + >> + crs = aml_resource_template(); >> + aml_append(crs, >> + aml_memory32_fixed(base, size, 0x01)); >> + aml_append(dev, aml_name_decl("_CRS", crs)); >> + aml_append(scope, dev); >> + >> + dev = aml_device("FLS1"); >> + aml_append(dev, aml_name_decl("_HID", aml_string("LNRO0015"))); >> + aml_append(dev, aml_name_decl("_UID", aml_int(1))); >> + crs = aml_resource_template(); >> + aml_append(crs, >> + aml_memory32_fixed(base + size, size, 0x01)); >> + aml_append(dev, aml_name_decl("_CRS", crs)); >> + aml_append(scope, dev); >> +} >> + >> +static void acpi_dsdt_add_virtio(Aml *scope, const hwaddr *mmio_addrs, >> + const int *mmio_irq, int num) >> +{ >> + Aml *dev, *crs; >> + hwaddr base = mmio_addrs[0]; >> + hwaddr size = mmio_addrs[1]; > > What ensures all these hw addresses are in 32 bit space on 64 bit platforms? > As we're generating the ACPI tables for machine virt, from a15memmap[] we know the hw addresses are below 1GB. >> + int irq = *mmio_irq + 32; >> + int i; >> + char name[5]; >> + >> + for (i = 0; i < num; i++) { >> + snprintf(name, 5, "VR%02u", i); >> + dev = aml_device("%s", name); > > Again why not call aml_device directly? > >> + aml_append(dev, aml_name_decl("_HID", aml_string("LNRO0005"))); >> + aml_append(dev, aml_name_decl("_UID", aml_int(i))); >> + >> + crs = aml_resource_template(); >> + aml_append(crs, >> + aml_memory32_fixed(base, size, 0x01)); >> + aml_append(crs, >> + aml_interrupt(0x01, irq + i)); >> + aml_append(dev, aml_name_decl("_CRS", crs)); >> + aml_append(scope, dev); >> + base += size; >> + } >> +} >> + >> +/* DSDT */ >> +static void >> +build_dsdt(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info) >> +{ >> + Aml *scope, *dsdt; >> + acpi_dsdt_info *info = guest_info->dsdt_info; >> + >> + dsdt = init_aml_allocator(); >> + /* Reserve space for header */ >> + acpi_data_push(dsdt->buf, sizeof(AcpiTableHeader)); >> + >> + scope = aml_scope("\\_SB"); >> + acpi_dsdt_add_cpus(scope, guest_info->max_cpus); >> + acpi_dsdt_add_uart(scope, info->uart_addr, info->uart_irq); >> + acpi_dsdt_add_rtc(scope, info->rtc_addr, info->rtc_irq); >> + acpi_dsdt_add_flash(scope, info->flash_addr); >> + acpi_dsdt_add_virtio(scope, info->virtio_mmio_addr, >> + info->virtio_mmio_irq, info->virtio_mmio_num); >> + >> + aml_append(dsdt, scope); >> + /* copy AML table into ACPI tables blob and patch header there */ >> + g_array_append_vals(table_data, dsdt->buf->data, dsdt->buf->len); >> + build_header(linker, table_data, >> + (void *)(table_data->data + table_data->len - dsdt->buf->len), >> + "DSDT", dsdt->buf->len, 1); >> + free_aml_allocator(); >> +} >> + >> typedef >> struct AcpiBuildState { >> /* Copy of table in RAM (for patching). */ >> @@ -72,6 +205,7 @@ static >> void virt_acpi_build(VirtGuestInfo *guest_info, AcpiBuildTables *tables) >> { >> GArray *table_offsets; >> + GArray *tables_blob = tables->table_data; >> >> table_offsets = g_array_new(false, true /* clear */, >> sizeof(uint32_t)); >> @@ -89,6 +223,9 @@ void virt_acpi_build(VirtGuestInfo *guest_info, AcpiBuildTables *tables) >> * DSDT >> */ >> >> + /* DSDT is pointed to by FADT */ >> + build_dsdt(tables_blob, tables->linker, guest_info); >> + >> /* Cleanup memory that's no longer used. */ >> g_array_free(table_offsets, true); >> } >