Re: [Xen-devel] [PATCH RFC 30/35] arm : acpi map XSDT table to dom0

[Julien Grall <julien.grall@xxxxxxxxxx>]

Hi Parth,



On 04/02/2015 14:02, parth.dixit@xxxxxxxxxx wrote:
> From: Parth Dixit <parth.dixit@xxxxxxxxxx>
>
> XSDT table cannot be passed as is to dom0 because new tables specific to xen
> need to be added to its table entries
>
> Signed-off-by: Parth Dixit <parth.dixit@xxxxxxxxxx>
> ---
>   xen/arch/arm/arm64/acpi/arm-core.c | 65 ++++++++++++++++++++++++++++++++++++--
>   1 file changed, 63 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/arm/arm64/acpi/arm-core.c 
> b/xen/arch/arm/arm64/acpi/arm-core.c
> index 9a26202..a210596 100644
> --- a/xen/arch/arm/arm64/acpi/arm-core.c
> +++ b/xen/arch/arm/arm64/acpi/arm-core.c
> @@ -256,13 +256,74 @@ static void set_psci_fadt(void)
>       fadt->header.checksum = (u8)( fadt->header.checksum-checksum );
>   }
>
> +#define NR_NEW_XEN_TABLES 2
Please add a comment to specify what is the 2 new tables.

> +
> +static int map_xsdt_table(struct domain *d, u64 *xsdt)
> +{
> +    int res;
> +    u64 size;
> +    u64 addr = *xsdt;
> +    u64 *new_xsdt;
> +    struct acpi_table_header *table;
> +    u64 *table_entry;
> +    u8 checksum;
> +
> +    /* map xsdt table */
> +    table = acpi_os_map_memory(addr, sizeof(struct acpi_table_header) );
No space before the last )

Also acpi_os_map_memory can fail.


> +    size = table->length ;
No space before the ;

> +    acpi_os_unmap_memory(table, sizeof(struct acpi_table_header) );
Ditto

> +
> +    table = acpi_os_map_memory(addr, size);
acpi_os_map_memory can fail

> +    size += ( NR_NEW_XEN_TABLES*sizeof(acpi_native_uint) );

(NR_NEW_XEN_TABLELS * sizeof(...));

> +    new_xsdt = ACPI_ALLOCATE_ZEROED(size);
> +    if( new_xsdt == NULL)
if ( ... )

> +        return -ENOMEM;
> +    ACPI_MEMCPY(new_xsdt, table,table->length);
Missing space after the comma.

> +    acpi_os_unmap_memory(table, table->length);
> +
> +    table = (struct acpi_table_header *) new_xsdt;
> +    table->length = size;
> +    *xsdt = addr = virt_to_maddr(new_xsdt);
> +
> +    /* map xsdt region */
> +    res = map_ram_regions(d,
> +                        paddr_to_pfn(addr & PAGE_MASK),
> +                        DIV_ROUND_UP(size, PAGE_SIZE),
> +                        paddr_to_pfn(addr & PAGE_MASK));
> +    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;
> +    }
> +
> +    table_entry = ACPI_CAST_PTR(u64,
> +        ( ACPI_CAST_PTR(u8, new_xsdt) + sizeof(struct acpi_table_header) ) );
Too much space in general.

> +    table_entry +=
> +        ( ( (size - sizeof(struct acpi_table_header) ) /
> +            sizeof(acpi_native_uint) ) );
Ditto

> +
> +    checksum = acpi_tb_checksum(ACPI_CAST_PTR(u8, table), table->length);
> +    table->checksum = (u8)( table->checksum - checksum );
(table->checksum - checksum);

Also I'm not sure the the u8 cast is useful.

> +    return 0;
> +}
> +
>   int acpi_map_tables(struct domain *d)
>   {
>       int i,res;
> -    u64 addr, size;
> +    u64 addr, size, rsdp;
> +    struct acpi_table_rsdp *rsdt;
> +
> +    addr = rsdp = acpi_os_get_root_pointer();
Why do you set addr, but only use below rather than rsdp?

> +    rsdt = acpi_os_map_memory(addr, sizeof(struct acpi_table_rsdp) );
No space before )


> +    addr = rsdt->xsdt_physical_address;
> +    map_xsdt_table(d, &addr);
> +    rsdt->xsdt_physical_address = addr;
> +    acpi_os_unmap_memory(rsdt, sizeof(struct acpi_table_rsdp) );
No space before )

Regards,

--
Julien Grall

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