Re: [Xen-devel] [PATCH RFC 31/35] arm : acpi map status override table to dom0

[Julien Grall <julien.grall@xxxxxxxxxx>]

Hi Parth,

You don't only map the status override table. You also create it.
I would update the commit title to reflect it.


On 04/02/2015 14:02, parth.dixit@xxxxxxxxxx wrote:
> From: Parth Dixit <parth.dixit@xxxxxxxxxx>
>
> hide UART used by xen by indicating it in STAO table
> and map it to dom0
>
> Signed-off-by: Parth Dixit <parth.dixit@xxxxxxxxxx>
> ---
>   xen/arch/arm/arm64/acpi/arm-core.c | 50 ++++++++++++++++++++++++++++++++++++++
>   1 file changed, 50 insertions(+)
>
> diff --git a/xen/arch/arm/arm64/acpi/arm-core.c 
> b/xen/arch/arm/arm64/acpi/arm-core.c
> index a210596..9fd02f9 100644
> --- a/xen/arch/arm/arm64/acpi/arm-core.c
> +++ b/xen/arch/arm/arm64/acpi/arm-core.c
> @@ -256,6 +256,48 @@ static void set_psci_fadt(void)
>       fadt->header.checksum = (u8)( fadt->header.checksum-checksum );
>   }
>
> +static int map_stao_table(struct domain *d, u64 *mstao)
> +{
> +    u64 addr;
> +    u64 size;
> +    int res;
> +    u8 checksum;
> +    struct acpi_table_stao *stao=NULL;
stao = NULL

> +
> +    stao = ACPI_ALLOCATE_ZEROED(sizeof(struct acpi_table_stao) );
No space before the last )

> +    if( stao == NULL )
> +        return -ENOMEM;
> +
> +    ACPI_MEMCPY(stao->header.signature,ACPI_SIG_STAO, 4);
Space after comma

> +    stao->header.length = sizeof(struct acpi_table_header) + 1;
> +    stao->header.checksum = 0;
> +    ACPI_MEMCPY(stao->header.oem_id, "LINARO", 6);
> +    ACPI_MEMCPY(stao->header.oem_table_id, "RTSMVEV8", 8);
I though the plan was to use a Xen OEM ID?

> +    stao->header.revision = 1;
> +    ACPI_MEMCPY(stao->header.asl_compiler_id, "INTL", 4);
> +    stao->header.asl_compiler_revision = 0x20140828;
Where does this revision comes from?

> +    stao->uart = 1;
What about the devpath?

> +    size = sizeof(struct acpi_table_stao);
> +    checksum = acpi_tb_checksum(ACPI_CAST_PTR(u8, stao), size);
> +    stao->header.checksum = (u8)( stao->header.checksum - checksum );
No space before the last )

> +    *mstao = addr = virt_to_maddr(stao);
> +
> +    res = map_ram_regions(d,
> +                          paddr_to_pfn(addr & PAGE_MASK),
> +                          DIV_ROUND_UP(size, PAGE_SIZE),
> +                          paddr_to_pfn(addr & PAGE_MASK));
I'm concerned with this mapping, as long as most of the others. 
map_ram_regions is mapping Read/Write the region. In this case, the STAO 
size may not be aligned to PAGE_SIZE.
So we may end up to map to DOM0 RW Xen memory. Even if DOM0 is a trusted 
domain, we should never let DOM0 write in Xen memory.
IIRC, the plan was to map at least RO all the ACPI tables.

> +    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
>
>   static int map_xsdt_table(struct domain *d, u64 *xsdt)
> @@ -304,6 +346,14 @@ static int map_xsdt_table(struct domain *d, u64 *xsdt)
>           ( ( (size - sizeof(struct acpi_table_header) ) /
>               sizeof(acpi_native_uint) ) );
>
> +    /* map stao table */
> +    map_stao_table(d, &addr);
> +    if(res)
if ( ... )

> +            return res;
> +
> +    table_entry--;
> +    *table_entry = addr ;
> +
No space before ;

>       checksum = acpi_tb_checksum(ACPI_CAST_PTR(u8, table), table->length);
>       table->checksum = (u8)( table->checksum - checksum );
No space before the last )

Regards,

--
Julien Grall

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