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

Re: [Xen-devel] [PATCH 0/4] ARM: ACPI: ITS: Add ITS Support for ACPI hardware domain

On 10/08/17 12:21, Manish Jaggi wrote:
Hi Julien,

On 6/21/2017 6:53 PM, Julien Grall wrote:
Hi Manish,

On 21/06/17 02:01, Manish Jaggi wrote:
This patch series adds the support of ITS for ACPI hardware domain.
It is tested on staging branch with has ITS v12 patchset by Andre.

I have tried to incorporate the review comments on the RFC v1/v2 patch.
The single patch in RFC is now split into 4 patches.

I will comment here rather than on each patches.

Patch1: ARM: ITS: Add translation_id to host_its
 Adds translation_id in host_its data structure, which is populated from
 translation_id read from firmwar MADT. This value is then programmed
 local MADT created for hardware domain in patch 4.

I don't see any reason to store value that will only be used for
generating the MADT which BTW is just a copy for the ITS. Instead we
should copy over the MADT entries.

There are two approaches,

If I use the standard API  acpi_table_parse_madt which would iterate
over ACPI_MADT_TYPE_GENERIC_TRANSLATOR entries, I have to maintain the
addr and translation_id in some data structure, to be filled later in
the hwdomain copy of madt generic translator.

If I don't use the standard API I have to add code to manually parse all
the translator entries.

There are a 3rd approach I suggested and ignored... The ITS entries for Dom0 is exactly the same as the host entries. So you only need to do a verbatim copy of the entry...

Which of the two you find cleaner?
This would also avoid to introduce a fake ID for DT as you currently
do in patch #2.

This can be avoided by storing translator_id only for acpi.

+static int add_to_host_its_list(u64 addr, u64 size,
+                      u32 translation_id, const void *node)
+    struct host_its *its_data;
+    its_data = xzalloc(struct host_its);
+    if ( !its_data )
+        return -1;
+    if ( node )
+        its_data->dt_node = node;
+    else
+        its_data->translation_id = translation_id;
+    its_data->addr = addr;
+    its_data->size = size;
+    printk("GICv3: Found ITS @0x%lx\n", addr);
+    list_add_tail(&its_data->entry, &host_its_list);
+    return 0;

What do you think?

I don't want to see the translation_id stored for no use at all but creating the DOM0 ACPI tables. Is that clearer?


Julien Grall

Xen-devel mailing list



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