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

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



Hi Manish,

On 02/01/18 09:28, manish.jaggi@xxxxxxxxxx wrote:
From: Manish Jaggi <manish.jaggi@xxxxxxxxxx>

  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@xxxxxxxxxx>
---
  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@xxxxxxxxxx>
+ * 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,

--
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®.