| Message ID | 1458207668-12012-13-git-send-email-zhaoshenglong@huawei.com |
|---|---|
| State | New |
| Headers | show |
Hi Shannon, On 17/03/2016 09:40, Shannon Zhao wrote: > From: Shannon Zhao <shannon.zhao@linaro.org> > > Create a few EFI memory descriptors to tell Dom0 the RAM region s/a few// > information, ACPI table regions and EFI tables reserved resions. s/resions/regions/ > > Signed-off-by: Parth Dixit <parth.dixit@linaro.org> > Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org> > --- > v6: remove acpi_diabled check > --- > xen/arch/arm/domain_build.c | 2 ++ > xen/arch/arm/efi/efi-dom0.c | 40 ++++++++++++++++++++++++++++++++++++++++ > xen/include/asm-arm/setup.h | 5 +++++ > 3 files changed, 47 insertions(+) > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > index 613551c..008fc76 100644 > --- a/xen/arch/arm/domain_build.c > +++ b/xen/arch/arm/domain_build.c > @@ -1688,6 +1688,8 @@ static int prepare_acpi(struct domain *d, struct kernel_info *kinfo) > acpi_map_other_tables(d); > acpi_create_efi_system_table(d->arch.efi_acpi_gpa, d->arch.efi_acpi_table, > tbl_add); > + acpi_create_efi_mmap_table(d->arch.efi_acpi_gpa, d->arch.efi_acpi_len, > + d->arch.efi_acpi_table, &kinfo->mem, tbl_add); > > return 0; > } > diff --git a/xen/arch/arm/efi/efi-dom0.c b/xen/arch/arm/efi/efi-dom0.c > index b8a062c..3ffde94 100644 > --- a/xen/arch/arm/efi/efi-dom0.c > +++ b/xen/arch/arm/efi/efi-dom0.c > @@ -23,6 +23,7 @@ > > #include "efi.h" > #include "efi-dom0.h" > +#include <xen/pfn.h> > #include <asm/setup.h> > #include <asm/acpi.h> > > @@ -92,3 +93,42 @@ void __init acpi_create_efi_system_table(paddr_t paddr, void *efi_acpi_table, > tbl_add[TBL_EFIT].start = table_addr; > tbl_add[TBL_EFIT].size = table_size; > } > + > +void __init acpi_create_efi_mmap_table(paddr_t paddr, paddr_t size, Please rename paddr and size to a more meaningful name. Like: acpi_gpa and acpi_len. Actually, you could directly pass the domain. So you will pass one argument rather 3 (paddr, size and efi_acpi_table). > + void *efi_acpi_table, > + const struct meminfo *mem, Please pass kinfo instead. > + struct membank tbl_add[]) > +{ > + EFI_MEMORY_DESCRIPTOR *memory_map; > + unsigned int i, offset; > + u8 *base_ptr; > + > + base_ptr = efi_acpi_table + acpi_get_table_offset(tbl_add, TBL_MMAP); > + memory_map = (EFI_MEMORY_DESCRIPTOR *)(base_ptr); NIT: the bracket around base_ptr are not necessary. > + > + offset = 0; > + for( i = 0; i < mem->nr_banks; i++, offset++ ) > + { > + memory_map[offset].Type = EfiConventionalMemory; > + memory_map[offset].PhysicalStart = mem->bank[i].start; > + memory_map[offset].NumberOfPages = PFN_UP(mem->bank[i].size); The page size use by Xen and UEFI may be different. Please use EFI_SIZE_TO_PAGES here. > + memory_map[offset].Attribute = EFI_MEMORY_WB; > + } > + > + for( i = 0; i < acpi_mem.nr_banks; i++, offset++ ) > + { > + memory_map[offset].Type = EfiACPIReclaimMemory; > + memory_map[offset].PhysicalStart = acpi_mem.bank[i].start; > + memory_map[offset].NumberOfPages = PFN_UP(acpi_mem.bank[i].size); Ditto You are also assuming that acpi_mem.bank[i].size will always be aligned to 4KB. If so, we may expose unwanted data to the guest. Based on how the field is set, I would add a BUG_ON to ensure this condition. > + memory_map[offset].Attribute = EFI_MEMORY_WB; > + } > + > + memory_map[offset].Type = EfiACPIReclaimMemory; > + memory_map[offset].PhysicalStart = paddr; > + memory_map[offset].NumberOfPages = PFN_UP(size); > + memory_map[offset].Attribute = EFI_MEMORY_WB; > + > + tbl_add[TBL_MMAP].start = paddr + acpi_get_table_offset(tbl_add, TBL_MMAP); > + tbl_add[TBL_MMAP].size = sizeof(EFI_MEMORY_DESCRIPTOR) > + * (mem->nr_banks + acpi_mem.nr_banks + 1); > +} > diff --git a/xen/include/asm-arm/setup.h b/xen/include/asm-arm/setup.h > index e423b15..b2899f2 100644 > --- a/xen/include/asm-arm/setup.h > +++ b/xen/include/asm-arm/setup.h > @@ -56,6 +56,11 @@ size_t estimate_efi_size(int mem_nr_banks); > void acpi_create_efi_system_table(paddr_t paddr, void *efi_acpi_table, > struct membank tbl_add[]); > > +void acpi_create_efi_mmap_table(paddr_t paddr, paddr_t size, > + void *efi_acpi_table, > + const struct meminfo *mem, > + struct membank tbl_add[]); > + > int construct_dom0(struct domain *d); > > void discard_initial_modules(void); > Regards,
On 2016年03月22日 00:51, Julien Grall wrote: >> + memory_map[offset].Attribute = EFI_MEMORY_WB; >> + } >> + >> + for( i = 0; i < acpi_mem.nr_banks; i++, offset++ ) >> + { >> + memory_map[offset].Type = EfiACPIReclaimMemory; >> + memory_map[offset].PhysicalStart = acpi_mem.bank[i].start; >> + memory_map[offset].NumberOfPages = >> PFN_UP(acpi_mem.bank[i].size); > > Ditto > > You are also assuming that acpi_mem.bank[i].size will always be aligned > to 4KB. If so, we may expose unwanted data to the guest. > > Based on how the field is set, I would add a BUG_ON to ensure this > condition. UEFI spec says "EFI memory descriptors of type EfiACPIReclaimMemory and EfiACPIMemoryNVS must be aligned on a 4 KiB boundary and must be a multiple of 4 KiB in size." So I think the size is aligned to 4kb, right? Thanks,
Hi Shannon, On 22/03/16 13:16, Shannon Zhao wrote: > On 2016年03月22日 00:51, Julien Grall wrote: >>> + memory_map[offset].Attribute = EFI_MEMORY_WB; >>> + } >>> + >>> + for( i = 0; i < acpi_mem.nr_banks; i++, offset++ ) >>> + { >>> + memory_map[offset].Type = EfiACPIReclaimMemory; >>> + memory_map[offset].PhysicalStart = acpi_mem.bank[i].start; >>> + memory_map[offset].NumberOfPages = >>> PFN_UP(acpi_mem.bank[i].size); >> >> Ditto >> >> You are also assuming that acpi_mem.bank[i].size will always be aligned >> to 4KB. If so, we may expose unwanted data to the guest. >> >> Based on how the field is set, I would add a BUG_ON to ensure this >> condition. > UEFI spec says > "EFI memory descriptors of type EfiACPIReclaimMemory and > EfiACPIMemoryNVS must be aligned on a 4 KiB boundary and must be a > multiple of 4 KiB in size." > > So I think the size is aligned to 4kb, right? Right. I was suggested to add a BUG_ON to document the constraint and ensure nobody will play with acpi_mem outside EFI. A such check would also be nice for mem->bank[i].size; Regards,
On 2016年03月23日 00:04, Julien Grall wrote: > Hi Shannon, > > On 22/03/16 13:16, Shannon Zhao wrote: >> On 2016年03月22日 00:51, Julien Grall wrote: >>>> + memory_map[offset].Attribute = EFI_MEMORY_WB; >>>> + } >>>> + >>>> + for( i = 0; i < acpi_mem.nr_banks; i++, offset++ ) >>>> + { >>>> + memory_map[offset].Type = EfiACPIReclaimMemory; >>>> + memory_map[offset].PhysicalStart = acpi_mem.bank[i].start; >>>> + memory_map[offset].NumberOfPages = >>>> PFN_UP(acpi_mem.bank[i].size); >>> >>> Ditto >>> >>> You are also assuming that acpi_mem.bank[i].size will always be aligned >>> to 4KB. If so, we may expose unwanted data to the guest. >>> >>> Based on how the field is set, I would add a BUG_ON to ensure this >>> condition. >> UEFI spec says >> "EFI memory descriptors of type EfiACPIReclaimMemory and >> EfiACPIMemoryNVS must be aligned on a 4 KiB boundary and must be a >> multiple of 4 KiB in size." >> >> So I think the size is aligned to 4kb, right? > > Right. I was suggested to add a BUG_ON to document the constraint and > ensure nobody will play with acpi_mem outside EFI. > > A such check would also be nice for mem->bank[i].size; sorry, I didn't get the idea. How to add a BUG_ON to check the size? Thanks,
Hi Shannon, On 24/03/16 15:06, Shannon Zhao wrote: > On 2016年03月23日 00:04, Julien Grall wrote: >> On 22/03/16 13:16, Shannon Zhao wrote: >>> On 2016年03月22日 00:51, Julien Grall wrote: >>>>> + memory_map[offset].Attribute = EFI_MEMORY_WB; >>>>> + } >>>>> + >>>>> + for( i = 0; i < acpi_mem.nr_banks; i++, offset++ ) >>>>> + { >>>>> + memory_map[offset].Type = EfiACPIReclaimMemory; >>>>> + memory_map[offset].PhysicalStart = acpi_mem.bank[i].start; >>>>> + memory_map[offset].NumberOfPages = >>>>> PFN_UP(acpi_mem.bank[i].size); >>>> >>>> Ditto >>>> >>>> You are also assuming that acpi_mem.bank[i].size will always be aligned >>>> to 4KB. If so, we may expose unwanted data to the guest. >>>> >>>> Based on how the field is set, I would add a BUG_ON to ensure this >>>> condition. >>> UEFI spec says >>> "EFI memory descriptors of type EfiACPIReclaimMemory and >>> EfiACPIMemoryNVS must be aligned on a 4 KiB boundary and must be a >>> multiple of 4 KiB in size." >>> >>> So I think the size is aligned to 4kb, right? >> >> Right. I was suggested to add a BUG_ON to document the constraint and >> ensure nobody will play with acpi_mem outside EFI. >> >> A such check would also be nice for mem->bank[i].size; > sorry, I didn't get the idea. How to add a BUG_ON to check the size? Something like: BUG_ON(acpi_mem.bank[i].size & EFI_PAGE_MASK); Regards,
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index 613551c..008fc76 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -1688,6 +1688,8 @@ static int prepare_acpi(struct domain *d, struct kernel_info *kinfo) acpi_map_other_tables(d); acpi_create_efi_system_table(d->arch.efi_acpi_gpa, d->arch.efi_acpi_table, tbl_add); + acpi_create_efi_mmap_table(d->arch.efi_acpi_gpa, d->arch.efi_acpi_len, + d->arch.efi_acpi_table, &kinfo->mem, tbl_add); return 0; } diff --git a/xen/arch/arm/efi/efi-dom0.c b/xen/arch/arm/efi/efi-dom0.c index b8a062c..3ffde94 100644 --- a/xen/arch/arm/efi/efi-dom0.c +++ b/xen/arch/arm/efi/efi-dom0.c @@ -23,6 +23,7 @@ #include "efi.h" #include "efi-dom0.h" +#include <xen/pfn.h> #include <asm/setup.h> #include <asm/acpi.h> @@ -92,3 +93,42 @@ void __init acpi_create_efi_system_table(paddr_t paddr, void *efi_acpi_table, tbl_add[TBL_EFIT].start = table_addr; tbl_add[TBL_EFIT].size = table_size; } + +void __init acpi_create_efi_mmap_table(paddr_t paddr, paddr_t size, + void *efi_acpi_table, + const struct meminfo *mem, + struct membank tbl_add[]) +{ + EFI_MEMORY_DESCRIPTOR *memory_map; + unsigned int i, offset; + u8 *base_ptr; + + base_ptr = efi_acpi_table + acpi_get_table_offset(tbl_add, TBL_MMAP); + memory_map = (EFI_MEMORY_DESCRIPTOR *)(base_ptr); + + offset = 0; + for( i = 0; i < mem->nr_banks; i++, offset++ ) + { + memory_map[offset].Type = EfiConventionalMemory; + memory_map[offset].PhysicalStart = mem->bank[i].start; + memory_map[offset].NumberOfPages = PFN_UP(mem->bank[i].size); + memory_map[offset].Attribute = EFI_MEMORY_WB; + } + + for( i = 0; i < acpi_mem.nr_banks; i++, offset++ ) + { + memory_map[offset].Type = EfiACPIReclaimMemory; + memory_map[offset].PhysicalStart = acpi_mem.bank[i].start; + memory_map[offset].NumberOfPages = PFN_UP(acpi_mem.bank[i].size); + memory_map[offset].Attribute = EFI_MEMORY_WB; + } + + memory_map[offset].Type = EfiACPIReclaimMemory; + memory_map[offset].PhysicalStart = paddr; + memory_map[offset].NumberOfPages = PFN_UP(size); + memory_map[offset].Attribute = EFI_MEMORY_WB; + + tbl_add[TBL_MMAP].start = paddr + acpi_get_table_offset(tbl_add, TBL_MMAP); + tbl_add[TBL_MMAP].size = sizeof(EFI_MEMORY_DESCRIPTOR) + * (mem->nr_banks + acpi_mem.nr_banks + 1); +} diff --git a/xen/include/asm-arm/setup.h b/xen/include/asm-arm/setup.h index e423b15..b2899f2 100644 --- a/xen/include/asm-arm/setup.h +++ b/xen/include/asm-arm/setup.h @@ -56,6 +56,11 @@ size_t estimate_efi_size(int mem_nr_banks); void acpi_create_efi_system_table(paddr_t paddr, void *efi_acpi_table, struct membank tbl_add[]); +void acpi_create_efi_mmap_table(paddr_t paddr, paddr_t size, + void *efi_acpi_table, + const struct meminfo *mem, + struct membank tbl_add[]); + int construct_dom0(struct domain *d); void discard_initial_modules(void);