[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.