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

[Parth Dixit <parth.dixit@xxxxxxxxxx>]

+shannon

On 8 June 2015 at 22:14, Julien Grall <julien.grall.oss@xxxxxxxxx> wrote:
> 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