[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
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@xxxxxxxxxx wrote:
From: Manish Jaggi <manish.jaggi@xxxxxxxxxx>
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@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).
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@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).
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,
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
