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

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

2018-01-22 Thread Julien Grall


Hi,

On 19/01/18 06:10, Manish Jaggi wrote:



On 01/17/2018 12:22 AM, Julien Grall wrote:
  IORT for hardware domain is generated using the requesterId and 
deviceId map.


  Signed-off-by: Manish Jaggi 
---
  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 
  #include 
  #include 
+#include 
  #include 
  #include 
  #include 
@@ -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.


I didn't ask to replace iort_size with madt_size. But rename madt_size 
to table_size or some other name that could be reused for both.


This is very similar to when you store the error return of a function. 
You are not going to name ret_foo, ret_bar, ret_fish... You are just 
going to use one variable and re-use it.


Anyway, I am not going to fight with that and just send a patch to clean 
that up once it has been merged.


[...]

  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 00..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 
+ * 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.


Well yes. But does it mean you have to add more wrong code? ;)

Cheers,

--
Julien Grall

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

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

2018-01-18 Thread Manish Jaggi




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.ja...@linaro.org wrote:

From: Manish Jaggi 

  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 
---
  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 
  #include 
  #include 
+#include 
  #include 
  #include 
  #include 
@@ -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(_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 00..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 
+ * 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 
+#include 
+
+/*
+ * 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, _deviceid_map_list, entry)
+    count++;
+
+    pcirc_array = xzalloc_bytes(sizeof(uint64_t)*count);
+    if ( !pcirc_array )
+    return -ENOMEM;
+
+    itsg_array =

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

2018-01-16 Thread Julien Grall


Hi Manish,

On 02/01/18 09:28, manish.ja...@linaro.org wrote:

From: Manish Jaggi 

  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 
---
  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 
  #include 
  #include 
+#include 
  #include 
  #include 
  #include 
@@ -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(_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 00..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 
+ * 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 
+#include 
+
+/*
+ * 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, _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, _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;
+}
+

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

2018-01-02 Thread manish . jaggi

From: Manish Jaggi 

 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 
---
 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 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -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(_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 00..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 
+ * 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 
+#include 
+
+/*
+ * 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, _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, _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
+ *

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

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

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

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

4 matches

Site Navigation

Mail list logo

Footer information