Re: [PATCH RFC 32/35] arm : acpi map xen environment table to dom0

[Julien Grall <julien.grall@linaro.org>]

Hi Parth,

As for the STAO table, this is not only mapping but also creating the table.

On 04/02/2015 14:02, parth.dixit@linaro.org wrote:
> From: Parth Dixit <parth.dixit@linaro.org>
>
> xen environment table contains the grant table address,size and event

, size

> channel interrupt information required by dom0.
>
> Signed-off-by: Parth Dixit <parth.dixit@linaro.org>
> ---
>   xen/arch/arm/arm64/acpi/arm-core.c | 52 +++++++++++++++++++++++++++++++++++++-
>   1 file changed, 51 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/arm64/acpi/arm-core.c b/xen/arch/arm/arm64/acpi/arm-core.c
> index 9fd02f9..9e9285c 100644
> --- a/xen/arch/arm/arm64/acpi/arm-core.c
> +++ b/xen/arch/arm/arm64/acpi/arm-core.c
> @@ -33,7 +33,6 @@
>   #include <asm/cputype.h>
>   #include <asm/acpi.h>
>   #include <asm/p2m.h>
> -

Spurious change.

>   /*
>    * We never plan to use RSDT on arm/arm64 as its deprecated in spec but this
>    * variable is still required by the ACPI core
> @@ -297,6 +296,49 @@ static int map_stao_table(struct domain *d, u64 *mstao)
>       return 0;
>   }
>
> +static int map_xenv_table(struct domain *d, u64 *mxenv)
> +{
> +    u64 addr;
> +    u64 size;
> +    int res;
> +    u8 checksum;
> +    struct acpi_table_xenv *xenv=NULL;

*xenv = NULL

> +
> +    xenv = ACPI_ALLOCATE_ZEROED( sizeof(struct acpi_table_xenv) );

No space necessary after the first ( and before the last )

> +    if( xenv == NULL )
> +            return -ENOMEM;
> +
> +    ACPI_MEMCPY(xenv->header.signature, ACPI_SIG_XENV, 4);
> +    xenv->header.length = sizeof(struct acpi_table_header)+12;

Where does the 12 comes from?

> +    xenv->header.checksum = 0;
> +    ACPI_MEMCPY(xenv->header.oem_id, "XenVMM", 6);
> +    ACPI_MEMCPY(xenv->header.oem_table_id, "RTSMVEV8", 8);

RTSMVEV8? Why?

> +    xenv->header.revision = 1;
> +    ACPI_MEMCPY(xenv->header.asl_compiler_id, "INTL", 4);
> +    xenv->header.asl_compiler_revision = 0x20140828;

Why this compiler revision?

> +    xenv->gnt_start = 0x00000010000000;
> +    xenv->gnt_size = 0x20000;

This is hardcoded. Even if you precise it in the cover letter, you 
should make clear in the patch that we have hardcoded.

> +    xenv->evt_intr = 31;
> +    xenv->evt_intr_flag =3;

Ditto

Also intr_flag = 3; and please use define rather than a value.

> +    size = sizeof(struct acpi_table_xenv);
> +    checksum = acpi_tb_checksum(ACPI_CAST_PTR(u8, xenv), size);
> +    xenv->header.checksum = (u8)( xenv->header.checksum - checksum );

No space after the ( and before )

> +    *mxenv = addr = virt_to_maddr(xenv);
> +
> +    res = map_ram_regions(d,
> +                        paddr_to_pfn(addr & PAGE_MASK),
> +                        DIV_ROUND_UP(size, PAGE_SIZE),
> +                        paddr_to_pfn(addr & PAGE_MASK));

Same remark as for the Xen Environment table (see patch #31).

> +    if ( res )
> +    {
> +         printk(XENLOG_ERR "Unable to map 0x%"PRIx64
> +                " - 0x%"PRIx64" in domain \n",
> +                addr & PAGE_MASK, PAGE_ALIGN(addr + size) - 1);
> +         return res;
> +    }
> +
> +    return 0;
> +}
>
>   #define NR_NEW_XEN_TABLES 2
>
> @@ -346,6 +388,14 @@ static int map_xsdt_table(struct domain *d, u64 *xsdt)
>           ( ( (size - sizeof(struct acpi_table_header) ) /
>               sizeof(acpi_native_uint) ) );
>
> +    /* map xen env table */
> +    res = map_xenv_table(d, &addr);
> +    if(res)
> +            return res;
> +
> +    table_entry--;
> +    *table_entry = addr ;
> +

No space before ;

>       /* map stao table */
>       map_stao_table(d, &addr);
>       if(res)
>

Regards,

-- 
Julien Grall