[PATCH v3 40/62] arm/acpi: Estimate memory required for acpi/efi tables

On Thu, 31 Dec 2015, Shannon Zhao wrote:

On 2015/11/27 0:39, Stefano Stabellini wrote:

quoted

On Tue, 17 Nov 2015, shannon.zhao at linaro.org wrote:

quoted

From: Shannon Zhao <redacted>

Estimate the memory required for loading acpi/efi tables in Dom0. Alloc
the pages to store the new created EFI and ACPI tables and free these
pages when destroying domain.

Could you please explain what you are page aligning exactly and why?

At least it should be 64bit aligned of the table start address. If not,
guest will throw out an alignment fault.

ACPI: Using GIC for interrupt routing
Unhandled fault: alignment fault (0x96000021) at 0xffffff800006c19c
Internal error: : 96000021 [#1] PREEMPT SMP
Modules linked in:
CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.3.0+ #551
Hardware name: (null) (DT)
task: ffffffc008870000 ti: ffffffc00884c000 task.ti: ffffffc00884c000
PC is at acpi_get_phys_id+0x264/0x290
LR is at acpi_get_phys_id+0x178/0x290

OK, but between page alignment and 64-bit alignment there is a very big
difference.

quoted

quoted

Signed-off-by: Parth Dixit <redacted>
Signed-off-by: Shannon Zhao <redacted>
---
 xen/arch/arm/domain.c       |  4 +++
 xen/arch/arm/domain_build.c | 80 ++++++++++++++++++++++++++++++++++++++++++++-
 xen/common/efi/boot.c       | 21 ++++++++++++
 xen/include/asm-arm/setup.h |  2 ++
 4 files changed, 106 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 880d0a6..10c58c4 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c

@@ -638,6 +638,10 @@ void arch_domain_destroy(struct domain *d)
     domain_vgic_free(d);
     domain_vuart_free(d);
     free_xenheap_page(d->shared_info);
+#ifdef CONFIG_ACPI
+    free_xenheap_pages(d->arch.efi_acpi_table,
+                       get_order_from_bytes(d->arch.efi_acpi_len));
+#endif
 }
 
 void arch_domain_shutdown(struct domain *d)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 0c3441a..b5ed44c 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c

@@ -12,6 +12,8 @@
 #include <xen/libfdt/libfdt.h>
 #include <xen/guest_access.h>
 #include <xen/iocap.h>
+#include <xen/acpi.h>
+#include <acpi/actables.h>
 #include <asm/device.h>
 #include <asm/setup.h>
 #include <asm/platform.h>

@@ -1354,6 +1356,78 @@ static int prepare_dtb(struct domain *d, struct kernel_info *kinfo)
     return -EINVAL;
 }
 
+#ifdef CONFIG_ACPI
+static int estimate_acpi_efi_size(struct domain *d, struct kernel_info *kinfo)
+{
+    u64 efi_size, acpi_size = 0, addr;
+    u32 madt_size;
+    struct acpi_table_rsdp *rsdp_tbl;
+    struct acpi_table_header *table = NULL;
+
+    efi_size = estimate_efi_size(kinfo->mem.nr_banks);
+
+    acpi_size += PAGE_ALIGN(sizeof(struct acpi_table_fadt));
+    acpi_size += PAGE_ALIGN(sizeof(struct acpi_table_stao));
+
+    madt_size = sizeof(struct acpi_table_madt)
+                + sizeof(struct acpi_madt_generic_interrupt) * d->max_vcpus
+                + sizeof(struct acpi_madt_generic_distributor);
+    if ( d->arch.vgic.version == GIC_V3 )
+        madt_size += sizeof(struct acpi_madt_generic_redistributor)
+                     * d->arch.vgic.nr_regions;
+    acpi_size += PAGE_ALIGN(madt_size);

are the MADT and FADT tables guaranteed to be on separate pages or is it
just an estimate?

quoted

+    addr = acpi_os_get_root_pointer();
+    if ( !addr )
+    {
+        printk("Unable to get acpi root pointer\n");
+        return -EINVAL;
+    }
+    rsdp_tbl = acpi_os_map_memory(addr, sizeof(struct acpi_table_rsdp));
+    table = acpi_os_map_memory(rsdp_tbl->xsdt_physical_address,
+                               sizeof(struct acpi_table_header));
+    acpi_size += PAGE_ALIGN(table->length + sizeof(u64));
+    acpi_os_unmap_memory(table, sizeof(struct acpi_table_header));
+    acpi_os_unmap_memory(rsdp_tbl, sizeof(struct acpi_table_rsdp));
+
+    acpi_size += PAGE_ALIGN(sizeof(struct acpi_table_rsdp));
+    d->arch.efi_acpi_len = PAGE_ALIGN(efi_size) + PAGE_ALIGN(acpi_size);
+
+    return 0;
+}
+
+static int prepare_acpi(struct domain *d, struct kernel_info *kinfo)
+{
+    int rc = 0;
+    int order;
+
+    rc = estimate_acpi_efi_size(d, kinfo);
+    if ( rc != 0 )
+        return rc;
+
+    order = get_order_from_bytes(d->arch.efi_acpi_len);
+    d->arch.efi_acpi_table = alloc_xenheap_pages(order, 0);
+    if ( d->arch.efi_acpi_table == NULL )
+    {
+        printk("unable to allocate memory!\n");
+        return -1;

ENOMEM

quoted

+    }
+    memset(d->arch.efi_acpi_table, 0, d->arch.efi_acpi_len);
+
+    /* For ACPI, Dom0 doesn't use kinfo->gnttab_start to get the grant table
+     * region. So we use it as the ACPI table mapped address. */
+    d->arch.efi_acpi_gpa = kinfo->gnttab_start;
+
+    return 0;
+}
+#else
+static int prepare_acpi(struct domain *d, struct kernel_info *kinfo)
+{
+    /* Only booting with ACPI will hit here */
+    BUG_ON(1);
+    return -EINVAL;
+}
+#endif
 static void dtb_load(struct kernel_info *kinfo)
 {
     void * __user dtb_virt = (void * __user)(register_t)kinfo->dtb_paddr;

@@ -1540,7 +1614,11 @@ int construct_dom0(struct domain *d)
     allocate_memory(d, &kinfo);
     find_gnttab_region(d, &kinfo);
 
-    rc = prepare_dtb(d, &kinfo);
+    if ( acpi_disabled )
+        rc = prepare_dtb(d, &kinfo);
+    else
+        rc = prepare_acpi(d, &kinfo);
+
     if ( rc < 0 )
         return rc;
 

diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index 53c7452..78d8ae9 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c

@@ -13,6 +13,7 @@
 #include <xen/multiboot.h>
 #include <xen/pci_regs.h>
 #include <xen/pfn.h>
+#include <asm/acpi.h>
 #if EFI_PAGE_SIZE != PAGE_SIZE
 # error Cannot use xen/pfn.h here!
 #endif

@@ -1171,6 +1172,26 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
     for( ; ; ); /* not reached */
 }
 
+#ifdef CONFIG_ACPI
+/* Constant to indicate "Xen" in unicode u16 format */
+static const u16 XEN_EFI_FW_VENDOR[] ={0x0058,0x0065,0x006E,0x0000};
+
+int __init estimate_efi_size(int mem_nr_banks)
+{
+    int size;
+    int est_size = sizeof(EFI_SYSTEM_TABLE);
+    int ect_size = sizeof(EFI_CONFIGURATION_TABLE);
+    int emd_size = sizeof(EFI_MEMORY_DESCRIPTOR);
+    int fw_vendor_size = sizeof(XEN_EFI_FW_VENDOR);
+
+    size = PAGE_ALIGN(est_size + ect_size + fw_vendor_size)
+           + PAGE_ALIGN(emd_size *
+                        (mem_nr_banks + acpi_mem.nr_banks + TBL_MMAX));

Why are they on two separate pages? Is it mandated by the EFI spec?
Why are you adding TBL_MMAX to the multiplicand here?

I don't like that we are passing mem_nr_banks as argument but we are
accessing acpi_mem.nr_banks from the global variable. I would prefer if
they were both passed as arguments.

quoted

+    return size;
+}
+#endif
+
 #ifndef CONFIG_ARM /* TODO - runtime service support */
 
 static bool_t __initdata efi_rs_enable = 1;

diff --git a/xen/include/asm-arm/setup.h b/xen/include/asm-arm/setup.h
index 30ac53b..b759813 100644
--- a/xen/include/asm-arm/setup.h
+++ b/xen/include/asm-arm/setup.h

@@ -51,6 +51,8 @@ void arch_init_memory(void);
 
 void copy_from_paddr(void *dst, paddr_t paddr, unsigned long len);
 
+int estimate_efi_size(int mem_nr_banks);
+
 int construct_dom0(struct domain *d);
 
 void discard_initial_modules(void);

-- 
2.1.0

.

-- 
Shannon