Re: [Xen-devel] [PATCH v6 12/22] arm/acpi: Prepare EFI memory descriptor for Dom0

[Julien Grall <julien.grall@xxxxxxx>]

Hi Shannon,

On 17/03/2016 09:40, Shannon Zhao wrote:
> From: Shannon Zhao <shannon.zhao@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> Signed-off-by: Shannon Zhao <shannon.zhao@xxxxxxxxxx>
> ---
> 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,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel