[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [RFC PATCH 05/24] ARM: GICv3 ITS: introduce ITS command handling
Hi Andre,
On 28/09/16 19:24, Andre Przywara wrote:
To be able to easily send commands to the ITS, create the respective
wrapper functions, which take care of the ring buffer.
The first two commands we implement provide methods to map a collection
to a redistributor (aka host core) and to flush the command queue (SYNC).
Start using these commands for mapping one collection to each host CPU.
Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
---
xen/arch/arm/gic-its.c | 101 ++++++++++++++++++++++++++++++++++++++++++
xen/arch/arm/gic-v3.c | 17 +++++++
xen/include/asm-arm/gic-its.h | 32 +++++++++++++
3 files changed, 150 insertions(+)
diff --git a/xen/arch/arm/gic-its.c b/xen/arch/arm/gic-its.c
index c8a7a7e..88397bc 100644
--- a/xen/arch/arm/gic-its.c
+++ b/xen/arch/arm/gic-its.c
@@ -33,6 +33,10 @@ static struct {
int host_lpi_bits;
} lpi_data;
+/* Physical redistributor address */
+static DEFINE_PER_CPU(uint64_t, rdist_addr);
The type should be paddr_t.
+/* Redistributor ID */
+static DEFINE_PER_CPU(uint64_t, rdist_id);
/* Pending table for each redistributor */
static DEFINE_PER_CPU(void *, pending_table);
@@ -40,6 +44,86 @@ static DEFINE_PER_CPU(void *, pending_table);
min_t(unsigned int, lpi_data.host_lpi_bits, CONFIG_HOST_LPI_BITS)
#define MAX_HOST_LPIS (BIT(MAX_HOST_LPI_BITS) - 8192)
+#define ITS_COMMAND_SIZE 32
+
+static int its_send_command(struct host_its *hw_its, void *its_cmd)
The its_cmd could be const as you don't modify it.
+{
+ int readp, writep;
Please use uint32_t (or maybe uint64_t) here.
+
+ spin_lock(&hw_its->cmd_lock);
+
+ readp = readl_relaxed(hw_its->its_base + GITS_CREADR) & GENMASK(19, 5);
+ writep = readl_relaxed(hw_its->its_base + GITS_CWRITER) & GENMASK(19, 5);
Please introduce a define for the GENMASK(19, 5) rather than hardcoding
it in multiple place.
+
+ if ( ((writep + ITS_COMMAND_SIZE) % PAGE_SIZE) == readp )
+ {
+ spin_unlock(&hw_its->cmd_lock);
+ return -EBUSY;
+ }
+
+ memcpy(hw_its->cmd_buf + writep, its_cmd, ITS_COMMAND_SIZE);
+ __flush_dcache_area(hw_its->cmd_buf + writep, ITS_COMMAND_SIZE);
Why the flush here? From patch #4, the GIC has been configured to be
able to snoop the cache. So a dsb(ish) would be enough here.
+ writep = (writep + ITS_COMMAND_SIZE) % PAGE_SIZE;
+
+ writeq_relaxed(writep & GENMASK(19, 5), hw_its->its_base + GITS_CWRITER);
+
+ spin_unlock(&hw_its->cmd_lock);
+
+ return 0;
This function return either -EBUSY or 0. Would not it be better to
return a bool instead?
+}
+
+static uint64_t encode_rdbase(struct host_its *hw_its, int cpu, uint64_t reg)
+{
+ reg &= ~GENMASK(51, 16);
+
+ if ( hw_its->pta )
+ reg |= per_cpu(rdist_addr, cpu) & GENMASK(51, 16);
+ else
+ reg |= per_cpu(rdist_id, cpu) << 16;
I would prefer if we setup the target address at initialize per-cpu
rather than doing it every time we send a sync command (or else).
+
+ return reg;
+}
+
+static int its_send_cmd_sync(struct host_its *its, int cpu)
+{
+ uint64_t cmd[4];
+
+ cmd[0] = GITS_CMD_SYNC;
+ cmd[1] = 0x00;
+ cmd[2] = encode_rdbase(its, cpu, 0x0);
+ cmd[3] = 0x00;
+
+ return its_send_command(its, cmd);
+}
+
+static int its_send_cmd_mapc(struct host_its *its, int collection_id, int cpu)
+{
+ uint64_t cmd[4];
+
+ cmd[0] = GITS_CMD_MAPC;
+ cmd[1] = 0x00;
+ cmd[2] = encode_rdbase(its, cpu, (collection_id & GENMASK(15, 0)) |
BIT(63));
+ cmd[3] = 0x00;
+
+ return its_send_command(its, cmd);
+}
+
+/* Set up the (1:1) collection mapping for the given host CPU. */
+void gicv3_its_setup_collection(int cpu)
+{
+ struct host_its *its;
+
+ list_for_each_entry(its, &host_its_list, entry)
+ {
+ /* Only send commands to ITS that have been initialized already. */
+ if ( !its->cmd_buf )
+ continue;
+
+ its_send_cmd_mapc(its, cpu, cpu);
+ its_send_cmd_sync(its, cpu);
Looking at the implementation of its_send_cmd_*, the functions may
return an error if the command queue is full. However you don't check
the return, and continue as it was fine. We will get in trouble much later.
Furthermore, sending the SYNC command does not meaning the ITS has
executed the command. You have to ensure that GITS_CREADR ==
GITS_CWRITER and I didn't find this code within this series.
+ }
+}
+
#define BASER_ATTR_MASK \
((0x3UL << GITS_BASER_SHAREABILITY_SHIFT) | \
(0x7UL << GITS_BASER_OUTER_CACHEABILITY_SHIFT) | \
@@ -147,6 +231,13 @@ int gicv3_its_init(struct host_its *hw_its)
if ( !hw_its->its_base )
return -ENOMEM;
+ /* Make sure the ITS is disabled before programming the BASE registers. */
+ reg = readl_relaxed(hw_its->its_base + GITS_CTLR);
+ writel_relaxed(reg & ~GITS_CTLR_ENABLE, hw_its->its_base + GITS_CTLR);
The spec (6.2.1 in IHI 0069C) requires the ITS to be disabled and
quiescent before programming the BASE registers. So I don't think this
check is enough here.
+
+ reg = readq_relaxed(hw_its->its_base + GITS_TYPER);
+ hw_its->pta = reg & GITS_TYPER_PTA;
+
for (i = 0; i < 8; i++)
{
void __iomem *basereg = hw_its->its_base + GITS_BASER0 + i * 8;
@@ -174,9 +265,18 @@ int gicv3_its_init(struct host_its *hw_its)
if ( IS_ERR(hw_its->cmd_buf) )
return PTR_ERR(hw_its->cmd_buf);
+ its_send_cmd_mapc(hw_its, smp_processor_id(), smp_processor_id());
+ its_send_cmd_sync(hw_its, smp_processor_id());
See my comments on the previous its_send_* functions
+
return 0;
}
+void gicv3_set_redist_addr(paddr_t address, int redist_id)
The second parameter should probably be unsigned, maybe uint64_t?
+{
+ this_cpu(rdist_addr) = address;
+ this_cpu(rdist_id) = redist_id;
+}
+
uint64_t gicv3_lpi_allocate_pendtable(void)
{
uint64_t reg, attr;
@@ -265,6 +365,7 @@ void gicv3_its_dt_init(const struct dt_device_node *node)
its_data->addr = addr;
its_data->size = size;
its_data->dt_node = its;
+ spin_lock_init(&its_data->cmd_lock);
printk("GICv3: Found ITS @0x%lx\n", addr);
diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index 5cf4618..b9387a3 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -638,6 +638,8 @@ static void gicv3_rdist_init_lpis(void __iomem * rdist_base)
table_reg = gicv3_lpi_get_proptable();
if ( table_reg )
writeq_relaxed(table_reg, rdist_base + GICR_PROPBASER);
+
+ gicv3_its_setup_collection(smp_processor_id());
}
static int __init gicv3_populate_rdist(void)
@@ -684,7 +686,22 @@ static int __init gicv3_populate_rdist(void)
this_cpu(rbase) = ptr;
if ( typer & GICR_TYPER_PLPIS )
+ {
+ paddr_t rdist_addr;
+
+ rdist_addr = gicv3.rdist_regions[i].base;
+ rdist_addr += ptr - gicv3.rdist_regions[i].map_base;
+
+ /* The ITS refers to redistributors either by their
physical
Coding style:
/*
* Foo
+ * address or by their ID. Determine those two values and
+ * let the ITS code store them in per host CPU variables to
+ * later be able to address those redistributors.
+ */
+ gicv3_set_redist_addr(rdist_addr,
+ (typer >> 8) & GENMASK(15, 0));
Please avoid hardcoding mask and use a define.
+
gicv3_rdist_init_lpis(ptr);
+ }
printk("GICv3: CPU%d: Found redistributor in region %d @%p\n",
smp_processor_id(), i, ptr);
diff --git a/xen/include/asm-arm/gic-its.h b/xen/include/asm-arm/gic-its.h
index b2a003f..b49d274 100644
--- a/xen/include/asm-arm/gic-its.h
+++ b/xen/include/asm-arm/gic-its.h
@@ -37,6 +37,7 @@
/* Register bits */
#define GITS_CTLR_ENABLE 0x1
+#define GITS_TYPER_PTA BIT(19)
#define GITS_IIDR_VALUE 0x34c
#define GITS_BASER_VALID BIT(63)
@@ -59,6 +60,22 @@
(31UL << GITS_BASER_ENTRY_SIZE_SHIFT)
|\
GITS_BASER_INDIRECT)
+/* ITS command definitions */
+#define ITS_CMD_SIZE 32
+
+#define GITS_CMD_MOVI 0x01
+#define GITS_CMD_INT 0x03
+#define GITS_CMD_CLEAR 0x04
+#define GITS_CMD_SYNC 0x05
+#define GITS_CMD_MAPD 0x08
+#define GITS_CMD_MAPC 0x09
+#define GITS_CMD_MAPTI 0x0a
+#define GITS_CMD_MAPI 0x0b
+#define GITS_CMD_INV 0x0c
+#define GITS_CMD_INVALL 0x0d
+#define GITS_CMD_MOVALL 0x0e
+#define GITS_CMD_DISCARD 0x0f
+
#ifndef __ASSEMBLY__
#include <xen/device_tree.h>
@@ -69,7 +86,9 @@ struct host_its {
paddr_t addr;
paddr_t size;
void __iomem *its_base;
+ spinlock_t cmd_lock;
void *cmd_buf;
+ bool pta;
};
extern struct list_head host_its_list;
@@ -89,6 +108,12 @@ uint64_t gicv3_lpi_allocate_pendtable(void);
int gicv3_lpi_init_host_lpis(int nr_lpis);
int gicv3_its_init(struct host_its *hw_its);
+/* Set the physical address and ID for each redistributor as read from DT. */
+void gicv3_set_redist_addr(paddr_t address, int redist_id);
+
+/* Map a collection for this host CPU to each host ITS. */
+void gicv3_its_setup_collection(int cpu);
+
#else
static inline void gicv3_its_dt_init(const struct dt_device_node *node)
@@ -110,6 +135,13 @@ static inline int gicv3_its_init(struct host_its *hw_its)
{
return 0;
}
Newline here
+static inline void gicv3_set_redist_addr(paddr_t address, int redist_id)
+{
+}
Ditto
+static inline void gicv3_its_setup_collection(int cpu)
+{
+}
+
#endif /* CONFIG_HAS_ITS */
#endif /* __ASSEMBLY__ */
Regards,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
