[Xen-devel,RFC,02/11] acpi: arm: API to query estimated size of hardware domain's IORT

Message ID 20180102092809.1841-3-manish.jaggi@linaro.org
State New
Headers show
Series
  • acpi: arm: IORT Support for Xen
Related show

Commit Message

Manish Jaggi Jan. 2, 2018, 9:28 a.m.
From: Manish Jaggi <manish.jaggi@linaro.org>

 Code to query estimated IORT size for hardware domain.
 IORT for hardware domain is generated using the requesterId and deviceId map.

 Signed-off-by: Manish Jaggi <manish.jaggi@linaro.com>
---
 xen/arch/arm/domain_build.c     |  12 ++++-
 xen/drivers/acpi/arm/Makefile   |   1 +
 xen/drivers/acpi/arm/gen-iort.c | 101 ++++++++++++++++++++++++++++++++++++++++
 xen/include/acpi/gen-iort.h     |   6 +++
 4 files changed, 119 insertions(+), 1 deletion(-)

Comments

Julien Grall Jan. 16, 2018, 6:52 p.m. | #1
Hi Manish,

On 02/01/18 09:28, manish.jaggi@linaro.org wrote:
> From: Manish Jaggi <manish.jaggi@linaro.org>
> 
>   Code to query estimated IORT size for hardware domain.

Please avoid indenting the commit message.

>   IORT for hardware domain is generated using the requesterId and deviceId map.
> 
>   Signed-off-by: Manish Jaggi <manish.jaggi@linaro.com>
> ---
>   xen/arch/arm/domain_build.c     |  12 ++++-
>   xen/drivers/acpi/arm/Makefile   |   1 +
>   xen/drivers/acpi/arm/gen-iort.c | 101 ++++++++++++++++++++++++++++++++++++++++
>   xen/include/acpi/gen-iort.h     |   6 +++
>   4 files changed, 119 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index c74f4dd69d..f5d5e3d271 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 <acpi/gen-iort.h>
>   #include <asm/device.h>
>   #include <asm/setup.h>
>   #include <asm/platform.h>
> @@ -1799,7 +1800,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, madt_size, iort_size;

Rather than introduce a variable for 10 instructions, you can rename 
madt_size so it can be re-used. I would be ok for this to be in the same 
patch (providing a proper commit message).

>       u64 addr;
>       struct acpi_table_rsdp *rsdp_tbl;
>       struct acpi_table_header *table;
> @@ -1840,6 +1841,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(&iort_size) )

Coding style.

> +    {
> +        printk("Unable to get hwdom iort size\n");
> +        return -EINVAL;
> +    }
> +
> +    acpi_size += ROUNDUP(iort_size, 8);
> +
>       d->arch.efi_acpi_len = PAGE_ALIGN(ROUNDUP(efi_size, 8)
>                                         + ROUNDUP(acpi_size, 8));
>   
> diff --git a/xen/drivers/acpi/arm/Makefile b/xen/drivers/acpi/arm/Makefile
> index 046fad5e3d..13f1a9159f 100644
> --- a/xen/drivers/acpi/arm/Makefile
> +++ b/xen/drivers/acpi/arm/Makefile
> @@ -1 +1,2 @@
>   obj-y = ridmap.o
> +obj-y += gen-iort.o
> diff --git a/xen/drivers/acpi/arm/gen-iort.c b/xen/drivers/acpi/arm/gen-iort.c
> new file mode 100644
> index 0000000000..3fc32959c6
> --- /dev/null
> +++ b/xen/drivers/acpi/arm/gen-iort.c
> @@ -0,0 +1,101 @@
> +/*
> + * xen/drivers/acpi/arm/gen-iort.c
> + *
> + * Code to generate IORT for hardware domain using the requesterId
> + * and deviceId map.
> + *
> + * Manish Jaggi <manish.jaggi@linaro.com>
> + * Copyright (c) 2018 Linaro.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.

The license is wrong (see patch #1).

> + *
> + * 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 <acpi/ridmap.h>
> +#include <xen/acpi.h>
> +
> +/*
> + * Size of hardware domains iort is calulcated based on the number of

s/iort/IORT/
s/calulcated/calculated/

> + * mappings in the requesterId - deviceId mapping list.
> + */
> +int estimate_iort_size(size_t *iort_size)
> +{
> +    int count = 0;
> +    int pcirc_count = 0;
> +    int itsg_count = 0;
> +    uint64_t *pcirc_array;
> +    uint64_t *itsg_array;

What is the rationale to store the address directly rather than "void 
*"? This would avoid the cast in the code.

> +    struct rid_deviceid_map *rmap;
> +

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.

For the rest of the function, please be careful on the coding style. I 
am not going to point them all, but I expect you to fix them.

> +    list_for_each_entry(rmap, &rid_deviceid_map_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;
> +
> +    list_for_each_entry(rmap, &rid_deviceid_map_list, entry)
> +    {
> +        int i = 0;
> +
> +        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
> +     * = 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) );
> +
> +    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/include/acpi/gen-iort.h b/xen/include/acpi/gen-iort.h
> new file mode 100644
> index 0000000000..68e666fdce
> --- /dev/null
> +++ b/xen/include/acpi/gen-iort.h
> @@ -0,0 +1,6 @@
> +#ifndef _GEN_IORT_H
> +#define _GEN_IORT_H
> +
> +int estimate_iort_size(size_t *iort_size);
> +
> +#endif

Missing emacs magic.

> 

Cheers,
Manish Jaggi Jan. 19, 2018, 6:10 a.m. | #2
On 01/17/2018 12:22 AM, Julien Grall wrote:
> Hi Manish,
>
Hi Julien,
Thanks for reviewing this patch.
> On 02/01/18 09:28, manish.jaggi@linaro.org wrote:
>> From: Manish Jaggi <manish.jaggi@linaro.org>
>>
>>   Code to query estimated IORT size for hardware domain.
>
> Please avoid indenting the commit message.
ok.
>
>>   IORT for hardware domain is generated using the requesterId and 
>> deviceId map.
>>
>>   Signed-off-by: Manish Jaggi <manish.jaggi@linaro.com>
>> ---
>>   xen/arch/arm/domain_build.c     |  12 ++++-
>>   xen/drivers/acpi/arm/Makefile   |   1 +
>>   xen/drivers/acpi/arm/gen-iort.c | 101 
>> ++++++++++++++++++++++++++++++++++++++++
>>   xen/include/acpi/gen-iort.h     |   6 +++
>>   4 files changed, 119 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>> index c74f4dd69d..f5d5e3d271 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 <acpi/gen-iort.h>
>>   #include <asm/device.h>
>>   #include <asm/setup.h>
>>   #include <asm/platform.h>
>> @@ -1799,7 +1800,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, madt_size, iort_size;
>
> Rather than introduce a variable for 10 instructions, you can rename 
> madt_size so it can be re-used. I would be ok for this to be in the 
> same patch (providing a proper commit message).
Why would you want to replace iort_size with madt_size ?
What is the harm if adding a variable makes the code more verbose.
I am not able to appreciate your point here.
>
>>       u64 addr;
>>       struct acpi_table_rsdp *rsdp_tbl;
>>       struct acpi_table_header *table;
>> @@ -1840,6 +1841,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(&iort_size) )
>
> Coding style.
>
>> +    {
>> +        printk("Unable to get hwdom iort size\n");
>> +        return -EINVAL;
>> +    }
>> +
>> +    acpi_size += ROUNDUP(iort_size, 8);
>> +
>>       d->arch.efi_acpi_len = PAGE_ALIGN(ROUNDUP(efi_size, 8)
>>                                         + ROUNDUP(acpi_size, 8));
>>   diff --git a/xen/drivers/acpi/arm/Makefile 
>> b/xen/drivers/acpi/arm/Makefile
>> index 046fad5e3d..13f1a9159f 100644
>> --- a/xen/drivers/acpi/arm/Makefile
>> +++ b/xen/drivers/acpi/arm/Makefile
>> @@ -1 +1,2 @@
>>   obj-y = ridmap.o
>> +obj-y += gen-iort.o
>> diff --git a/xen/drivers/acpi/arm/gen-iort.c 
>> b/xen/drivers/acpi/arm/gen-iort.c
>> new file mode 100644
>> index 0000000000..3fc32959c6
>> --- /dev/null
>> +++ b/xen/drivers/acpi/arm/gen-iort.c
>> @@ -0,0 +1,101 @@
>> +/*
>> + * xen/drivers/acpi/arm/gen-iort.c
>> + *
>> + * Code to generate IORT for hardware domain using the requesterId
>> + * and deviceId map.
>> + *
>> + * Manish Jaggi <manish.jaggi@linaro.com>
>> + * Copyright (c) 2018 Linaro.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>
> The license is wrong (see patch #1).
Please see my comment in patch #1.
This license is used from an existing file in xen.
So there are a lot of wrong licenses in xen code.
>
>> + *
>> + * 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 <acpi/ridmap.h>
>> +#include <xen/acpi.h>
>> +
>> +/*
>> + * Size of hardware domains iort is calulcated based on the number of
>
> s/iort/IORT/
> s/calulcated/calculated/
Thanks.
>
>> + * mappings in the requesterId - deviceId mapping list.
>> + */
>> +int estimate_iort_size(size_t *iort_size)
>> +{
>> +    int count = 0;
>> +    int pcirc_count = 0;
>> +    int itsg_count = 0;
>> +    uint64_t *pcirc_array;
>> +    uint64_t *itsg_array;
>
> What is the rationale to store the address directly rather than "void 
> *"? This would avoid the cast in the code.
>
>> +    struct rid_deviceid_map *rmap;
>> +
>
> 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.
>
I will add more documentation that will explain why it is used this way.
> For the rest of the function, please be careful on the coding style. I 
> am not going to point them all, but I expect you to fix them.
>
>> +    list_for_each_entry(rmap, &rid_deviceid_map_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;
>> +
>> +    list_for_each_entry(rmap, &rid_deviceid_map_list, entry)
>> +    {
>> +        int i = 0;
>> +
>> +        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
>> +     * = 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) );
>> +
>> +    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/include/acpi/gen-iort.h b/xen/include/acpi/gen-iort.h
>> new file mode 100644
>> index 0000000000..68e666fdce
>> --- /dev/null
>> +++ b/xen/include/acpi/gen-iort.h
>> @@ -0,0 +1,6 @@
>> +#ifndef _GEN_IORT_H
>> +#define _GEN_IORT_H
>> +
>> +int estimate_iort_size(size_t *iort_size);
>> +
>> +#endif
>
> Missing emacs magic.
>
>>
>
> Cheers,
>

Patch

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index c74f4dd69d..f5d5e3d271 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 <acpi/gen-iort.h>
 #include <asm/device.h>
 #include <asm/setup.h>
 #include <asm/platform.h>
@@ -1799,7 +1800,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, madt_size, iort_size;
     u64 addr;
     struct acpi_table_rsdp *rsdp_tbl;
     struct acpi_table_header *table;
@@ -1840,6 +1841,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(&iort_size) )
+    {
+        printk("Unable to get hwdom iort size\n");
+        return -EINVAL;
+    }
+
+    acpi_size += ROUNDUP(iort_size, 8);
+
     d->arch.efi_acpi_len = PAGE_ALIGN(ROUNDUP(efi_size, 8)
                                       + ROUNDUP(acpi_size, 8));
 
diff --git a/xen/drivers/acpi/arm/Makefile b/xen/drivers/acpi/arm/Makefile
index 046fad5e3d..13f1a9159f 100644
--- a/xen/drivers/acpi/arm/Makefile
+++ b/xen/drivers/acpi/arm/Makefile
@@ -1 +1,2 @@ 
 obj-y = ridmap.o
+obj-y += gen-iort.o
diff --git a/xen/drivers/acpi/arm/gen-iort.c b/xen/drivers/acpi/arm/gen-iort.c
new file mode 100644
index 0000000000..3fc32959c6
--- /dev/null
+++ b/xen/drivers/acpi/arm/gen-iort.c
@@ -0,0 +1,101 @@ 
+/*
+ * xen/drivers/acpi/arm/gen-iort.c
+ *
+ * Code to generate IORT for hardware domain using the requesterId
+ * and deviceId map.
+ *
+ * Manish Jaggi <manish.jaggi@linaro.com>
+ * Copyright (c) 2018 Linaro.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * 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 <acpi/ridmap.h>
+#include <xen/acpi.h>
+
+/*
+ * Size of hardware domains iort is calulcated based on the number of
+ * mappings in the requesterId - deviceId mapping list.
+ */
+int estimate_iort_size(size_t *iort_size)
+{
+    int count = 0;
+    int pcirc_count = 0;
+    int itsg_count = 0;
+    uint64_t *pcirc_array;
+    uint64_t *itsg_array;
+    struct rid_deviceid_map *rmap;
+
+    list_for_each_entry(rmap, &rid_deviceid_map_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;
+
+    list_for_each_entry(rmap, &rid_deviceid_map_list, entry)
+    {
+        int i = 0;
+
+        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
+     * = 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) );
+
+    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/include/acpi/gen-iort.h b/xen/include/acpi/gen-iort.h
new file mode 100644
index 0000000000..68e666fdce
--- /dev/null
+++ b/xen/include/acpi/gen-iort.h
@@ -0,0 +1,6 @@ 
+#ifndef _GEN_IORT_H
+#define _GEN_IORT_H
+
+int estimate_iort_size(size_t *iort_size);
+
+#endif