Re: [PATCH v6 02/22] arm/acpi: Add a helper function to get the acpi table offset

[Julien Grall <julien.grall@arm.com>]

Hi Shannon,

On 17/03/2016 09:40, Shannon Zhao wrote:
> From: Shannon Zhao <shannon.zhao@linaro.org>
>
> These tables are aligned with 64bit.
>
> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> ---
>   xen/arch/arm/acpi/lib.c    | 15 +++++++++++++++
>   xen/include/asm-arm/acpi.h |  2 ++
>   2 files changed, 17 insertions(+)
>
> diff --git a/xen/arch/arm/acpi/lib.c b/xen/arch/arm/acpi/lib.c
> index db5c4d8..79f7edd 100644
> --- a/xen/arch/arm/acpi/lib.c
> +++ b/xen/arch/arm/acpi/lib.c
> @@ -60,3 +60,18 @@ bool_t __init acpi_psci_hvc_present(void)
>   {
>       return acpi_gbl_FADT.arm_boot_flags & ACPI_FADT_PSCI_USE_HVC;
>   }
> +
> +paddr_t __init acpi_get_table_offset(struct membank tbl_add[],
> +                                     EFI_MEM_RES index)

Without looking at the callers, the usage of this function is very 
unclear. For instance what does mean the offset in this case?

Furthermore, you are assuming that all the tables before the given index 
have all been created and will never be modified. So all the code flow 
to generate the ACPI tables is inflexible.

I'm fine for now with this kind of assumption, but this need to be spell 
out in the header.

> +{
> +    int i;
> +    paddr_t offset = 0;
> +
> +    for ( i = 0; i < index; i++ )
> +    {
> +        /* Aligned with 64bit (8 bytes) */
> +        offset += ROUNDUP(tbl_add[i].size, 8);
> +    }
> +
> +    return offset;
> +}
> diff --git a/xen/include/asm-arm/acpi.h b/xen/include/asm-arm/acpi.h
> index 7f59761..569fc31 100644
> --- a/xen/include/asm-arm/acpi.h
> +++ b/xen/include/asm-arm/acpi.h
> @@ -25,6 +25,7 @@
>
>   #include <xen/init.h>
>   #include <asm/page.h>
> +#include <asm/setup.h>
>
>   #define COMPILER_DEPENDENT_INT64   long long
>   #define COMPILER_DEPENDENT_UINT64  unsigned long long
> @@ -45,6 +46,7 @@ typedef enum {
>   bool_t __init acpi_psci_present(void);
>   bool_t __init acpi_psci_hvc_present(void);
>   void __init acpi_smp_init_cpus(void);
> +paddr_t acpi_get_table_offset(struct membank tbl_add[], EFI_MEM_RES index);
>
>   #ifdef CONFIG_ACPI
>   extern bool_t acpi_disabled;
>

Regards,

-- 
Julien Grall

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