[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
Hi,
Please CC all relevant maintainers.
On 08/06/17 14:03, Manish Jaggi wrote:
Spurious newline
This patch supports ITS in hardware domain, supports ITS in Xen
when booting with ACPI.
Signed-off-by: Manish Jaggi <mjaggi@xxxxxxxxxx>
---
Changes since v1:
- Moved its specific code to gic-v3-its.c
- fixed macros
It sounds like you haven't addressed all my comments. I will repeat them
for this time. But next time, I will not bother reviewing your patch.
xen/arch/arm/domain_build.c | 6 ++--
xen/arch/arm/gic-v3-its.c | 75
+++++++++++++++++++++++++++++++++++++++-
xen/arch/arm/gic-v3.c | 10 ++++--
xen/include/asm-arm/gic_v3_its.h | 6 ++++
4 files changed, 91 insertions(+), 6 deletions(-)
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 3abacc0..d6d6c94 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -20,7 +20,7 @@
#include <asm/psci.h>
#include <asm/setup.h>
#include <asm/cpufeature.h>
-
Why did you drop this newline?
+#include <asm-arm/gic_v3_its.h>
Nack. I asked on v1 to separate code between GICv3 and ITS, it is not
for directly calling gicv3 code directly in the common code.
If you need to call GICv3 specific code, then introduce a callback in
gic_hw_operations.
#include <asm/gic.h>
#include <xen/irq.h>
#include <xen/grant_table.h>
@@ -1804,7 +1804,9 @@ static int estimate_acpi_efi_size(struct domain
*d, struct kernel_info *kinfo)
madt_size = sizeof(struct acpi_table_madt)
+ sizeof(struct acpi_madt_generic_interrupt) *
d->max_vcpus
- + sizeof(struct acpi_madt_generic_distributor);
+ + sizeof(struct acpi_madt_generic_distributor)
+ + gicv3_its_madt_generic_translator_size();
See my comment above.
+
if ( d->arch.vgic.version == GIC_V3 )
madt_size += sizeof(struct acpi_madt_generic_redistributor)
* d->arch.vgic.nr_regions;
diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
index 1fb06ca..937b970 100644
--- a/xen/arch/arm/gic-v3-its.c
+++ b/xen/arch/arm/gic-v3-its.c
@@ -25,14 +25,18 @@
#include <xen/rbtree.h>
#include <xen/sched.h>
#include <xen/sizes.h>
+#include <xen/iocap.h>
The include are ordered alphabetically, please respect it.
#include <asm/gic.h>
#include <asm/gic_v3_defs.h>
#include <asm/gic_v3_its.h>
#include <asm/io.h>
#include <asm/page.h>
+#include <xen/acpi.h>
+#include <acpi/actables.h>
+#include <xen/pfn.h>
Ditto.
#define ITS_CMD_QUEUE_SZ SZ_1M
-
Again, we don't drop newline for no reason.
+#define ACPI_GICV3_ITS_MEM_SIZE (SZ_64K)
/*
* No lock here, as this list gets only populated upon boot while scanning
* firmware tables for all host ITSes, and only gets iterated afterwards.
@@ -920,6 +924,55 @@ int gicv3_lpi_change_vcpu(struct domain *d, paddr_t
vdoorbell,
return 0;
}
+int gicv3_its_deny_access(const struct domain *d)
+{
+ int rc = 0;
+ unsigned long mfn, nr;
+ const struct host_its *its_data;
+
+ list_for_each_entry(its_data, &host_its_list, entry)
+ {
+ mfn = paddr_to_pfn(its_data->addr);
+ nr = PFN_UP(ACPI_GICV3_ITS_MEM_SIZE);
+ rc = iomem_deny_access(d, mfn, mfn + nr);
+ if ( rc )
+ goto end;
Hmmm, why not using a break here rather than a goto?
+ }
+end:
+ return rc;
+}
+
+u32 gicv3_its_madt_generic_translator_size(void)
+{
+ const struct host_its *its_data;
+ u32 size = 0;
+
+ list_for_each_entry(its_data, &host_its_list, entry)
+ {
Pointless {
+ size += sizeof(struct acpi_madt_generic_translator);
+ }
Same here + add a newline.
+ 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.
+ 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?
+ 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.
+{
+ struct acpi_madt_generic_translator *its_entry;
+ struct host_its *its_data;
+
+ its_data = xzalloc(struct host_its);
+ if (!its_data)
Coding style.
+ 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.
Also newline.
+ return 0;
+}
Newline here.
/* 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.
}
-
Again, why did you drop this newline?
#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.
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?
+ 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?
+
/*
* 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.
}
#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.
bool gicv3_its_host_has_its(void);
@@ -167,6 +169,10 @@ int gicv3_its_make_hwdom_dt_nodes(const struct
domain *d,
const struct dt_device_node *gic,
void *fdt);
+u32 gicv3_its_make_hwdom_madt(u8 *base_ptr, u32 offset);
+u32 gicv3_its_madt_generic_translator_size(void);
+/* Deny iomem access for its */
+int gicv3_its_deny_access(const struct domain *d);
Same here.
/*
* Map a device on the host by allocating an ITT on the host (ITS).
* "nr_event" specifies how many events (interrupts) this device will
need.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
