Re: [Xen-devel] [PATCH resend 02/13] acpi: arm: query estimated size of hardware domain's IORT.

[Julien Grall <julien.grall@xxxxxxx>]

Title: Please drop the full stop.

On 03/13/2018 03:20 PM, mjaggi@xxxxxxxxxxxxxxxxxx wrote:
> From: Manish Jaggi <mjaggi@xxxxxxxxxxxxxxxxxx>
>
> Code to query estimated IORT size for hardware domain.
> IORT for hardware domain is generated using the requesterid and
> deviceid map. Xen code requires the size to be predeterminded.
Please expand: "Xen code requires the size to be predeterminded".

> Signed-off-by: Manish Jaggi <manish.jaggi@xxxxxxxxxx>
> ---
>   xen/arch/arm/acpi/Makefile          |   1 +
>   xen/arch/arm/acpi/gen-iort.c        | 101 ++++++++++++++++++++++++++++++++++++
>   xen/arch/arm/domain_build.c         |  16 ++++--
>   xen/include/asm-arm/acpi/gen-iort.h |  33 ++++++++++++
>   4 files changed, 148 insertions(+), 3 deletions(-)
>
> diff --git a/xen/arch/arm/acpi/Makefile b/xen/arch/arm/acpi/Makefile
> index eb7e8ce4f7..073339603c 100644
> --- a/xen/arch/arm/acpi/Makefile
> +++ b/xen/arch/arm/acpi/Makefile
> @@ -1,3 +1,4 @@
>   obj-y += lib.o
>   obj-y += boot.init.o
>   obj-y += ridmap.o
> +obj-y += gen-iort.o
> diff --git a/xen/arch/arm/acpi/gen-iort.c b/xen/arch/arm/acpi/gen-iort.c
> new file mode 100644
> index 0000000000..687c4f18ee
> --- /dev/null
> +++ b/xen/arch/arm/acpi/gen-iort.c
> @@ -0,0 +1,101 @@
> +/*
> + * xen/arch/arm/acpi/gen-iort.c
> + *
> + * Code to generate IORT for hardware domain using the requesterId
> + * and deviceId map.
Here we go. You again mix upper case and lower case for the same name. 
To be honest, I would much prefer if you stick with deviceId. I.e first 
letter of the second word is uppercase.
> + *
> + * Manish Jaggi <manish.jaggi@xxxxxxxxxx>
It is linaro.org.

> + * Copyright (c) 2018 Linaro.
> + *
> + * Ths program is free software; you can redistribute it and/or
> + * modify it under the terms and conditions of the GNU General Public
> + * License, version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <asm/acpi/ridmap.h>
> +#include <xen/acpi.h>
I am a bit surprised that only those 2 headers are necessary.

> +
> +/*
> + * Size of hardware domains' IORT is calculated based on the number of
> + * mappings in the requesterid - deviceid mapping list.
> + * Return value 0: Success
> + */
> +int estimate_iort_size(size_t *iort_size)
__init.

> +{
> +    int count = 0;
> +    int pcirc_count = 0;
> +    int itsg_count = 0;
All the 3 variables above should be unsigned int.

> +    uint64_t *pcirc_array;
> +    uint64_t *itsg_array;
Again, what is the rationale to store the address directly rather than 
"void *"? This would avoid the cast in the code.

> +    struct rid_devid_map *rmap;
I am sorry but I still don't see any comment about my comment on the 
previous version. For reminder:
"A bit more documention of this function would be appreciated. For 
instance, the rationale between browsing the list twice for allocation.
I actually do think this might be avoidable by storing a bit more 
information from the IORT. From the table you can easily deduced the 
number of root complex and ITS group. They could be store with the rest 
of information."
> +
> +    list_for_each_entry(rmap, &rid_devid_list, entry)
> +        count++;
> +
> +    pcirc_array = xzalloc_bytes(sizeof(uint64_t)*count);
> +    if ( !pcirc_array )
> +        return -ENOMEM;
> +
> +    itsg_array = xzalloc_bytes(sizeof(uint64_t)*count);
> +    if ( !itsg_array )
> +        return -ENOMEM;
If the allocation fail, you will leak pcirc_array.

> +
> +    list_for_each_entry(rmap, &rid_devid_list, entry)
> +    {
> +        int i = 0;
unsigned.

> +
> +        for ( i = 0; i <= pcirc_count; i++ )
> +        {
> +            if ( pcirc_array[i] == (uint64_t) rmap->pcirc_node )
> +                break;
> +            if ( i == pcirc_count )
> +            {
> +                pcirc_array[i] = (uint64_t) rmap->pcirc_node;
> +                pcirc_count++;
> +                break;
> +            }
> +        }
> +
> +        for ( i = 0; i <= itsg_count; i++ )
> +        {
> +            if ( itsg_array[i] == (uint64_t) rmap->its_node )
> +                break;
> +            if ( i == itsg_count )
> +            {
> +                itsg_array[i] = (uint64_t) rmap->its_node;
> +                itsg_count++;
> +                break;
> +            }
> +        }
> +    }
> +
> +    /* Size of IORT
Coding style.

> +     * = Size of IORT Table Header + Size of PCIRC Header Nodes +
> +     *   Size of PCIRC nodes + Size of ITS Header nodes + Size of ITS Nodes
> +     *   + Size of idmap nodes
> +     */
> +    *iort_size = sizeof(struct acpi_table_iort) +
> +                 pcirc_count*( (sizeof(struct acpi_iort_node) -1) +
> +                               sizeof(struct acpi_iort_root_complex) ) +
> +                 itsg_count*( (sizeof(struct acpi_iort_node) -1) +
> +                               sizeof(struct acpi_iort_its_group) ) +
> +                 count*( sizeof(struct acpi_iort_id_mapping) );
Coding style in general:
        - No space needed after ( in that cases
        - Space before and after +*-

> +
> +    xfree(itsg_array);
> +    xfree(pcirc_array);
> +
> +    return 0;
> +}
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 155c952349..33a46cab1e 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -14,6 +14,7 @@
>   #include <xen/acpi.h>
>   #include <xen/warning.h>
>   #include <acpi/actables.h>
> +#include <asm/acpi/gen-iort.h>
>   #include <asm/device.h>
>   #include <asm/setup.h>
>   #include <asm/platform.h>
> @@ -1801,7 +1802,7 @@ static int acpi_create_fadt(struct domain *d, struct 
> membank tbl_add[])
>   
>   static int estimate_acpi_efi_size(struct domain *d, struct kernel_info *kinfo)
>   {
> -    size_t efi_size, acpi_size, madt_size;
> +    size_t efi_size, acpi_size, table_size;
I am ok if you keep the renaming in that patch. But you should at least 
mention it in the commit message.
>       u64 addr;
>       struct acpi_table_rsdp *rsdp_tbl;
>       struct acpi_table_header *table;
> @@ -1811,8 +1812,8 @@ static int estimate_acpi_efi_size(struct domain *d, 
> struct kernel_info *kinfo)
>       acpi_size = ROUNDUP(sizeof(struct acpi_table_fadt), 8);
>       acpi_size += ROUNDUP(sizeof(struct acpi_table_stao), 8);
>   
> -    madt_size = gic_get_hwdom_madt_size(d);
> -    acpi_size += ROUNDUP(madt_size, 8);
> +    table_size = gic_get_hwdom_madt_size(d);
> +    acpi_size += ROUNDUP(table_size, 8);
>   
>       addr = acpi_os_get_root_pointer();
>       if ( !addr )
> @@ -1842,6 +1843,15 @@ static int estimate_acpi_efi_size(struct domain *d, 
> struct kernel_info *kinfo)
>       acpi_os_unmap_memory(table, sizeof(struct acpi_table_header));
>   
>       acpi_size += ROUNDUP(sizeof(struct acpi_table_rsdp), 8);
> +
> +    if ( estimate_iort_size(&table_size) )
> +    {
> +        printk("Unable to get hwdom iort size\n");
> +        return -EINVAL;
> +    }
> +
> +    acpi_size += ROUNDUP(table_size, 8);
> +
>       d->arch.efi_acpi_len = PAGE_ALIGN(ROUNDUP(efi_size, 8)
>                                         + ROUNDUP(acpi_size, 8));
>   
> diff --git a/xen/include/asm-arm/acpi/gen-iort.h b/xen/include/asm-arm/acpi/gen-iort.h
> new file mode 100644
> index 0000000000..3b2af1e871
> --- /dev/null
> +++ b/xen/include/asm-arm/acpi/gen-iort.h
Is it worth it to have a separate header for gen-iort? How about using 
acpi.h?
> @@ -0,0 +1,33 @@
> +/*
> + * xen/include/asm-arm/acpi/gen-iort.h
> + *
> + * Manish Jaggi <manish.jaggi@xxxxxxxxxx>
> + * Copyright (c) 2018 Linaro.
> + *
> + * Ths program is free software; you can redistribute it and/or
> + * modify it under the terms and conditions of the GNU General Public
> + * License, version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#ifndef _ACPI_GEN_IORT_H
> +#define _ACPI_GEN_IORT_H
> +
> +/*
> + * Returns the size of hardware domains IORT
> + */
> +int estimate_iort_size(size_t *iort_size);
> +
> +#endif
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel