[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [RFC v2][PATCH] arm-acpi: Add ITS Support for Dom0
On 09/06/2017 07:48, Manish Jaggi wrote:
On 6/8/2017 7:28 PM, Julien Grall wrote:
Hi,
Hello Julien,
Hello,
+ list_for_each_entry(its_data, &host_its_list, entry)
+ {
Pointless {
+ size += sizeof(struct acpi_madt_generic_translator);
+ }
Just for readability of code.
You have indentation for that. So I don't think it helps.
Same here + add a newline.
Sure.
+ return size;
+}
+
+u32 gicv3_its_make_hwdom_madt(u8 *base_ptr, u32 offset)
+{
+ struct acpi_madt_generic_translator *gic_its;
+ const struct host_its *its_data;
+ u32 table_len = offset, size;
+
+ /* Update GIC ITS information in hardware domain's MADT */
+ list_for_each_entry(its_data, &host_its_list, entry)
+ {
+ size = sizeof(struct acpi_madt_generic_translator);
+ gic_its = (struct acpi_madt_generic_translator *)(base_ptr +
table_len);
This line is likely too long.
I will check it.
+ gic_its->header.type = ACPI_MADT_TYPE_GENERIC_TRANSLATOR;
+ gic_its->header.length = size;
+ gic_its->base_address = its_data->addr;
On the previous patch you had:
gic_its->translation_id = its_data->translation_id;
I asked to explain why you need to have the same ID as the host. And
now you dropped it. This does not match the spec (Table 5-67 in ACPI
6.1):
"GIC ITS ID. In a system with multiple GIC ITS units, this value must
be unique to each one."
But here, the ITS ID will not be unique. So why did you dropped it?
The reason I dropped it from its_data as I was not setting it. So it
doesn't belong there.
Where would it belong then?
This function is used to generate ACPI tables for the hardware domain.
Will the below code be ok?
If you noticed, I didn't say this code is wrong. Instead I asked why you
use the same ID. Meaning, is there anything in the DSDT requiring this
value?
+ int tras_id = 0;
unsigned.
+ list_for_each_entry(its_data, &host_its_list, entry)
+ {
+ gic_its->translation_id = ++trans_id;
You start the translation ID at 1. Why?
+ table_len += size;
+ }
+ return table_len;
+}
+
/*
* Create the respective guest DT nodes from a list of host ITSes.
* This copies the reg property, so the guest sees the ITS at the same
address
@@ -992,6 +1045,26 @@ int gicv3_its_make_hwdom_dt_nodes(const struct
domain *d,
return res;
}
+int gicv3_its_acpi_init(struct acpi_subtable_header *header, const
unsigned long end)
ACPI is an option and is not able by default. Please make sure that
this code build without ACPI. Likely this means surrounding with
#ifdef CONFIG_ACPI.
I will get compiled but not called. Do you still want to put ifdef, i
can add that.
All ACPIs functions are protected by ifdef. So this one should be as well.
+{
+ struct acpi_madt_generic_translator *its_entry;
+ struct host_its *its_data;
+
+ its_data = xzalloc(struct host_its);
+ if (!its_data)
Coding style.
Sure.
+ return -1;
+
+ its_entry = (struct acpi_madt_generic_translator *)header;
+ its_data->addr = its_entry->base_address;
+ its_data->size = ACPI_GICV3_ITS_MEM_SIZE;
+
+ spin_lock_init(&its_data->cmd_lock);
+
+ printk("GICv3: Found ITS @0x%lx\n", its_data->addr);
+
+ list_add_tail(&its_data->entry, &host_its_list);
As said on v1, likely you could re-use factorize a part of
gicv3_its_dt_init to avoid implementing twice the initialization.
For this I have a different opinion.
Why didn't you state it on the previous version? I usually interpret a
non-answer as an acknowledgment.
gicv3_its_dt_init has a loop dt_for_each_child_node(node, its) while
gicv3_its_acpi_init is a callback.
Moreover, apart from xzalloc and list_add_tail most of the code is
different. so IMHO keeping them separate is better.
You still set addr and size as in the DT counterpart. Also, this is a
call to forget to initialize a field if we decided to extend the
structure host_its. So I still don't see any reason to open-code it and
take the risk to introduce bug in the future...
Also newline.
+ return 0;
+}
Newline here.
Sure.
/* Scan the DT for any ITS nodes and create a list of host ITSes out of
it. */
void gicv3_its_dt_init(const struct dt_device_node *node)
{
diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index c927306..f0f6d12 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -1333,9 +1333,8 @@ static int gicv3_iomem_deny_access(const struct
domain *d)
return iomem_deny_access(d, mfn, mfn + nr);
}
- return 0;
+ return gicv3_its_deny_access(d);
Copying my answer from v1 for convenience:
if ( vbase != INVALID_PADDR )
{
mfn = vbase >> PAGE_SHIFT;
nr = DIV_ROUND_UP(csize, PAGE_SIZE);
return iomem_deny_access(d, mfn, mfn + nr);
}
When GICv3 is able to support GICv2, vbase will be valid and the code
will bail out after denying access to the GICV. So the ITS regions
will not be denied.
Ok, got your point. Would the below code be ok?
LGTM.
diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index c927306..a3d1eff 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -1308,6 +1308,13 @@ static int gicv3_iomem_deny_access(const struct
domain *d)
if ( rc )
return rc;
+ if ( gicv3_its_host_has_its() )
+ {
+ rc = gicv3_its_deny_access(d);
+ if ( rc )
+ return rc;
+ }
+
for ( i = 0; i < gicv3.rdist_count; i++ )
{
mfn = gicv3.rdist_regions[i].base >> PAGE_SHIFT;
}
-
Again, why did you drop this newline?
I will fix it
#ifdef CONFIG_ACPI
static void __init
gic_acpi_add_rdist_region(paddr_t base, paddr_t size, bool
single_rdist)
@@ -1374,6 +1373,7 @@ static int gicv3_make_hwdom_madt(const struct
domain *d, u32 offset)
for ( i = 0; i < d->max_vcpus; i++ )
{
gicc = (struct acpi_madt_generic_interrupt *)(base_ptr +
table_len);
+
As said on v1, spurious change.
No. I wanted to add it intentionally. for a better code readability.
A general and simple rule is to separate code clean-up with new
functionality. This is not a new functionality and therefore should not
be there.
ACPI_MEMCPY(gicc, host_gicc, size);
gicc->cpu_interface_number = i;
gicc->uid = i;
@@ -1399,7 +1399,7 @@ static int gicv3_make_hwdom_madt(const struct
domain *d, u32 offset)
gicr->length = d->arch.vgic.rdist_regions[i].size;
table_len += size;
}
-
Again why did you drop the newline?
I will fix it
+ table_len = gicv3_its_make_hwdom_madt(base_ptr, table_len);
return table_len;
}
@@ -1567,6 +1567,9 @@ static void __init gicv3_acpi_init(void)
gicv3.rdist_stride = 0;
+ acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_TRANSLATOR,
+ gicv3_its_acpi_init, 0);
As said on v1, acpi_table_parse_madt may return an error. Why this is
not checked?
I missed that, I will add it,
+
/*
* In ACPI, 0 is considered as the invalid address. However the
rest
* of the initialization rely on the invalid address to be
@@ -1585,6 +1588,7 @@ static void __init gicv3_acpi_init(void)
else
vsize = GUEST_GICC_SIZE;
+
As said on v1, this is a spurious newline.
ok
}
#else
static void __init gicv3_acpi_init(void) { }
diff --git a/xen/include/asm-arm/gic_v3_its.h
b/xen/include/asm-arm/gic_v3_its.h
index d2a3e53..b72aec2 100644
--- a/xen/include/asm-arm/gic_v3_its.h
+++ b/xen/include/asm-arm/gic_v3_its.h
@@ -105,6 +105,7 @@
#include <xen/device_tree.h>
#include <xen/rbtree.h>
+#include <xen/acpi.h>
#define HOST_ITS_FLUSH_CMD_QUEUE (1U << 0)
#define HOST_ITS_USES_PTA (1U << 1)
@@ -134,6 +135,7 @@ extern struct list_head host_its_list;
/* Parse the host DT and pick up all host ITSes. */
void gicv3_its_dt_init(const struct dt_device_node *node);
+int gicv3_its_acpi_init(struct acpi_subtable_header *header, const
unsigned long end);
This will likely need an #ifdef CONFIG_ACPI. And also a stub would be
required if ITS is disabled.
Sorry didnt got your point. on ifdef.
Look how gicv3_its_dt_init has been introduced. There is a stub for when
ITS is not built-in. Furthermore, the function is ACPI specific are
therefore should be protected with #ifdef CONFIG_ACPI.
I can add a check gicv3_its_host_has_its() before calling
gicv3_its_acpi_init, would that be ok?
gicv3_its_host_has_its will always return false as it is rely on the
list of ITS to be populated. However, this will be populated by
gicv3_its_acpi_init...
Regards,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
