Re: [Xen-devel] [PATCH v3 40/62] arm/acpi: Estimate memory required for acpi/efi tables
On 2015/11/30 23:14, Julien Grall wrote: > Hi Shannon, > > On 17/11/15 09:40, shannon.z...@linaro.org wrote: >> 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 >> #include >> #include >> +#include >> #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,0x}; >> + >> +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)); >> + >> +return size; >> +} >> +#endif >> + > > efi/boot.c only contains code related to the stub EFI. This is not the > case of this code. > I add these codes at this file because I want to reuse the definitions of EFI structures like EFI_SYSTEM_TABLE. Parth re-define those structures at previous patch set and one comment suggested to reuse the existing ones. If I add these codes in xen/arch/arm/domain_build.c, it will have some conflicts. E.g: In file included from /home/open-source/xen/xen/include/asm/efibind.h:2:0, from domain_build.c:29: /home/open-source/xen/xen/include/asm/arm64/efibind.h:86:20: error: conflicting types for 'UINT64' typedef uint64_t UINT64; ^ In file included from /home/open-source/xen/xen/include/acpi/acpi.h:57:0, from /home/open-source/xen/xen/include/xen/acpi.h:33, from domain_build.c:15: /home/open-source/xen/xen/include/acpi/actypes.h:130:35: note: previous declaration of 'UINT64' was here typedef COMPILER_DEPENDENT_UINT64 UINT64; > Moreover, this code won't compile on x86 because acpi_mem is only > defined for ARM. > > Regards, > Thanks, -- Shannon ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [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: > > On Tue, 17 Nov 2015, shannon.z...@linaro.org wrote: > >> From: Shannon Zhao> >> > >> 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 (0x9621) at 0xff86c19c > Internal error: : 9621 [#1] PREEMPT SMP > Modules linked in: > CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.3.0+ #551 > Hardware name: (null) (DT) > task: ffc00887 ti: ffc00884c000 task.ti: ffc00884c000 > 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. > > > >> > >> Signed-off-by: Parth Dixit > >> Signed-off-by: Shannon Zhao > >> --- > >> 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 > >> #include > >> #include > >> +#include > >> +#include > >> #include > >> #include > >> #include > >> @@ -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? > > > > > >> +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 > > > > > >> +} > >> +
Re: [Xen-devel] [PATCH v3 40/62] arm/acpi: Estimate memory required for acpi/efi tables
On Mon, 4 Jan 2016, Shannon Zhao wrote: > On 2015/11/30 23:14, Julien Grall wrote: > > Hi Shannon, > > > > On 17/11/15 09:40, shannon.z...@linaro.org wrote: > >> 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 > >> #include > >> #include > >> +#include > >> #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,0x}; > >> + > >> +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)); > >> + > >> +return size; > >> +} > >> +#endif > >> + > > > > efi/boot.c only contains code related to the stub EFI. This is not the > > case of this code. > > > > I add these codes at this file because I want to reuse the definitions > of EFI structures like EFI_SYSTEM_TABLE. Parth re-define those > structures at previous patch set and one comment suggested to reuse the > existing ones. If I add these codes in xen/arch/arm/domain_build.c, it > will have some conflicts. E.g: > > In file included from /home/open-source/xen/xen/include/asm/efibind.h:2:0, > from domain_build.c:29: > /home/open-source/xen/xen/include/asm/arm64/efibind.h:86:20: error: > conflicting types for 'UINT64' > typedef uint64_t UINT64; > ^ > In file included from /home/open-source/xen/xen/include/acpi/acpi.h:57:0, > from /home/open-source/xen/xen/include/xen/acpi.h:33, > from domain_build.c:15: > /home/open-source/xen/xen/include/acpi/actypes.h:130:35: note: previous > declaration of 'UINT64' was here > typedef COMPILER_DEPENDENT_UINT64 UINT64; I think that having this code in efi/boot.c might be OK, but let's see what Jan thinks. > > Moreover, this code won't compile on x86 because acpi_mem is only > > defined for ARM. > > > > Regards, > > > > Thanks, > -- > Shannon > ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 40/62] arm/acpi: Estimate memory required for acpi/efi tables
On 2015/11/27 0:39, Stefano Stabellini wrote: > On Tue, 17 Nov 2015, shannon.z...@linaro.org wrote: >> From: Shannon Zhao>> >> 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 (0x9621) at 0xff86c19c Internal error: : 9621 [#1] PREEMPT SMP Modules linked in: CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.3.0+ #551 Hardware name: (null) (DT) task: ffc00887 ti: ffc00884c000 task.ti: ffc00884c000 PC is at acpi_get_phys_id+0x264/0x290 LR is at acpi_get_phys_id+0x178/0x290 > >> >> Signed-off-by: Parth Dixit >> Signed-off-by: Shannon Zhao >> --- >> 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 >> #include >> #include >> +#include >> +#include >> #include >> #include >> #include >> @@ -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? > > >> +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 > > >> +} >> +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
Re: [Xen-devel] [PATCH v3 40/62] arm/acpi: Estimate memory required for acpi/efi tables
Hi Shannon, On 17/11/15 09:40, shannon.z...@linaro.org wrote: > 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 > #include > #include > +#include > #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,0x}; > + > +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)); > + > +return size; > +} > +#endif > + efi/boot.c only contains code related to the stub EFI. This is not the case of this code. Moreover, this code won't compile on x86 because acpi_mem is only defined for ARM. Regards, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel