Re: [Xen-devel] [PATCH v2 31/41] arm : acpi estimate memory required for acpi/efi tables

[Julien Grall <julien.grall.oss@xxxxxxxxx>]

Hi Parth,

I think it misses a ":" between acpi and estimate.

On 17/05/2015 21:03, Parth Dixit wrote:
> Estimate the memory required for loading acpi/efi tablee
> in DOM0. Initialize the size of acpi/efi tables.
It needs more description...

> Signed-off-by: Parth Dixit <parth.dixit@xxxxxxxxxx>
> ---
>   xen/arch/arm/domain_build.c | 54 ++++++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 53 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 9d98f64..f2ca525 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -69,6 +69,9 @@ struct meminfo __initdata acpi_mem;
>   #ifdef CONFIG_ACPI
>   /* Reserve DOM0 FDT size in ACPI case only */
>   #define DOM0_FDT_MIN_SIZE 4096
> +#define NR_NEW_XEN_TABLES 2
New table of what?

> +/* Constant to indicate "Xen" in unicode u16 format */
> +static const u16 XEN_EFI_FW_VENDOR[] ={0x0058,0x0065,0x006E,0x0000};
I think you have to rework the order of your patch in this series. This 
kind of variable should appear where you add the EFI table and not where 
you estimate the size.
It would be easier to understand what it's used for.

>   #endif
>
>   struct vcpu *__init alloc_dom0_vcpu0(struct domain *dom0)
> @@ -1222,6 +1225,51 @@ static int handle_node(struct domain *d, struct 
> kernel_info *kinfo,
>       return res;
>   }
>   #ifdef CONFIG_ACPI
> +static int estimate_acpi_size(struct domain *d,struct kernel_info *kinfo, 
> struct membank tbl_add[])
> +{
> +    int size = 0;
> +    u64 addr;
> +    struct acpi_table_header *table;
> +    struct acpi_table_rsdp *rsdp_tbl;
> +
> +    set_acpi_size(size);
> +    tbl_add[TBL_EFIT].size = sizeof(struct efi_system_table)
> +        + sizeof(struct efi_config_table)
> +        + sizeof(XEN_EFI_FW_VENDOR);
> +
> +    tbl_add[TBL_MMAP].size = sizeof(struct efi_memory_desc)
> +        *(kinfo->mem.nr_banks + acpi_mem.nr_banks + TBL_MMAX);
> +    tbl_add[TBL_XENV].size = sizeof(struct acpi_table_xenv);
> +    tbl_add[TBL_STAO].size = sizeof(struct acpi_table_stao);
> +
> +    addr = acpi_os_get_root_pointer();
> +    if( !addr )
> +        return -ENODEV;
> +
> +    rsdp_tbl = acpi_os_map_memory(addr, sizeof(struct acpi_table_rsdp));
> +    if( !rsdp_tbl )
> +        return -ENOMEM;
> +
> +    table = acpi_os_map_memory(rsdp_tbl->xsdt_physical_address,
> +                               sizeof(struct acpi_table_header));
> +    if ( !table )
> +        return -ENOMEM;
> +
> +    tbl_add[TBL_XSDT].size = table->length
> +        +( NR_NEW_XEN_TABLES*sizeof(acpi_native_uint) );
Coding style:

+ (NR_NEW_XEN_TABLES * sizeof(acpi_native_uint);

This is also needs some explanation of the acpi_native_uint. We should 
be able to understand the code without have to search on the web. A 
reference to the doc would works too...
> +    tbl_add[TBL_XSDT].start = rsdp_tbl->xsdt_physical_address;
> +    acpi_os_unmap_memory(table, sizeof(struct acpi_table_header));
> +    acpi_os_unmap_memory(rsdp_tbl, sizeof(struct acpi_table_rsdp));
> +    size = tbl_add[TBL_EFIT].size
The size is used to set acpi_size but this is EFI table... Please be 
consistent.
Regards,

--
Julien Grall

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