[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [PATCH v2 05/27] ARM: GICv3 ITS: introduce ITS command handling
Hi Andre,
On 16/03/17 11:20, 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-v3-its.c | 181 +++++++++++++++++++++++++++++++++++++-
xen/arch/arm/gic-v3-lpi.c | 22 +++++
xen/arch/arm/gic-v3.c | 19 +++-
xen/include/asm-arm/gic_v3_defs.h | 2 +
xen/include/asm-arm/gic_v3_its.h | 38 ++++++++
5 files changed, 260 insertions(+), 2 deletions(-)
diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
index e5601ed..5c11b0d 100644
--- a/xen/arch/arm/gic-v3-its.c
+++ b/xen/arch/arm/gic-v3-its.c
@@ -19,11 +19,14 @@
*/
#include <xen/lib.h>
+#include <xen/delay.h>
#include <xen/mm.h>
#include <xen/sizes.h>
+#include <asm/gic.h>
#include <asm/gic_v3_defs.h>
#include <asm/gic_v3_its.h>
#include <asm/io.h>
+#include <asm/page.h>
#define ITS_CMD_QUEUE_SZ SZ_64K
@@ -34,6 +37,145 @@ bool gicv3_its_host_has_its(void)
return !list_empty(&host_its_list);
}
+#define BUFPTR_MASK GENMASK(19, 5)
+static int its_send_command(struct host_its *hw_its, const void *its_cmd)
+{
+ s_time_t deadline = NOW() + MILLISECS(1);
It would be nice to explain where does this value comes from.
+ uint64_t readp, writep;
+ int ret = -EBUSY;
+
+ /* No ITS commands from an interrupt handler (at the moment). */
+ ASSERT(!in_irq());
+
+ spin_lock(&hw_its->cmd_lock);
+
+ do {
+ readp = readq_relaxed(hw_its->its_base + GITS_CREADR) & BUFPTR_MASK;
+ writep = readq_relaxed(hw_its->its_base + GITS_CWRITER) & BUFPTR_MASK;
+
+ if ( ((writep + ITS_CMD_SIZE) % ITS_CMD_QUEUE_SZ) != readp )
+ {
+ ret = 0;
+ break;
+ }
+
+ /*
+ * If the command queue is full, wait for a bit in the hope it drains
+ * before giving up.
+ */
+ spin_unlock(&hw_its->cmd_lock);
+ cpu_relax();
+ udelay(1);
+ spin_lock(&hw_its->cmd_lock);
+ } while ( NOW() <= deadline );
+
+ if ( ret )
+ {
+ spin_unlock(&hw_its->cmd_lock);
+ printk(XENLOG_WARNING "ITS: command queue full.\n");
This function could be called from a domain. So please ratelimit the
message (see printk_ratelimit).
+ return ret;
+ }
+
+ memcpy(hw_its->cmd_buf + writep, its_cmd, ITS_CMD_SIZE);
+ if ( hw_its->flags & HOST_ITS_FLUSH_CMD_QUEUE )
+ clean_and_invalidate_dcache_va_range(hw_its->cmd_buf + writep,
+ ITS_CMD_SIZE);
+ else
+ dsb(ishst);
+
+ writep = (writep + ITS_CMD_SIZE) % ITS_CMD_QUEUE_SZ;
+ writeq_relaxed(writep & BUFPTR_MASK, hw_its->its_base + GITS_CWRITER);
+
+ spin_unlock(&hw_its->cmd_lock);
+
+ return 0;
+}
+
+/* Wait for an ITS to finish processing all commands. */
+static int gicv3_its_wait_commands(struct host_its *hw_its)
+{
+ s_time_t deadline = NOW() + MILLISECS(100);
Same remark as above. Why 1ms and 100ms here?
+ uint64_t readp, writep;
+
+ do {
+ spin_lock(&hw_its->cmd_lock);
+ readp = readq_relaxed(hw_its->its_base + GITS_CREADR) & BUFPTR_MASK;
+ writep = readq_relaxed(hw_its->its_base + GITS_CWRITER) & BUFPTR_MASK;
+ spin_unlock(&hw_its->cmd_lock);
+
+ if ( readp == writep )
+ return 0;
+
+ cpu_relax();
+ udelay(1);
+ } while ( NOW() <= deadline );
+
+ return -ETIMEDOUT;
+}
[...]
+static int its_send_cmd_mapc(struct host_its *its, uint32_t collection_id,
+ unsigned 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)));
I requested to drop the mask here and I didn't see any answer explaining
why you would not do it. So this should be dropped unless you give me a
reason to keep it.
+ cmd[2] |= GITS_VALID_BIT;
+ cmd[3] = 0x00;
+
+ return its_send_command(its, cmd);
+}
+
+/* Set up the (1:1) collection mapping for the given host CPU. */
+int gicv3_its_setup_collection(unsigned int cpu)
+{
+ struct host_its *its;
+ int ret;
+
+ list_for_each_entry(its, &host_its_list, entry)
+ {
+ if ( !its->cmd_buf )
+ continue;
Why this check?
[...]
+/*
+ * Before an ITS gets initialized, it should be in a quiescent state, where
+ * all outstanding commands and transactions have finished.
+ * So if the ITS is already enabled, turn it off and wait for all outstanding
+ * operations to get processed by polling the QUIESCENT bit.
+ */
+static int gicv3_disable_its(struct host_its *hw_its)
+{
+ uint32_t reg;
+ s_time_t deadline = NOW() + MILLISECS(100);
Why 100ms?
[...]
static int gicv3_lpi_allocate_pendtable(uint64_t *reg)
{
uint64_t val;
diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index cc1e219..38dafe7 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -666,7 +666,21 @@ static int __init gicv3_populate_rdist(void)
if ( typer & GICR_TYPER_PLPIS )
{
- int ret;
+ paddr_t rdist_addr;
+ int procnum, ret;
As mentioned in v1, procnum should be unsigned. If you disagree, please
explain.
+
+ /*
+ * The ITS refers to redistributors either by their
physical
+ * 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.
+ */
Again, this comment does not look useful and is misleading as the code
to get/set the redistributor information is living in gic-v3-lpi.c and
not gic-v3-its.c.
[...]
diff --git a/xen/include/asm-arm/gic_v3_its.h b/xen/include/asm-arm/gic_v3_its.h
index d5facf0..8b493fb 100644
--- a/xen/include/asm-arm/gic_v3_its.h
+++ b/xen/include/asm-arm/gic_v3_its.h
[...]
#include <xen/device_tree.h>
#define HOST_ITS_FLUSH_CMD_QUEUE (1U << 0)
+#define HOST_ITS_USES_PTA (1U << 1)
/* data structure for each hardware ITS */
struct host_its {
@@ -87,6 +106,7 @@ struct host_its {
paddr_t addr;
paddr_t size;
void __iomem *its_base;
+ spinlock_t cmd_lock;
Again, initialization, clean-up of a field should be done in the same
that added the field. Otherwise, this is a call to miss a bit of the
code and makes more difficult for the reviewer.
So please initialize cmd_lock in this patch and patch #6.
void *cmd_buf;
unsigned int flags;
};
@@ -107,6 +127,13 @@ int gicv3_lpi_init_rdist(void __iomem * rdist_base);
int gicv3_lpi_init_host_lpis(unsigned int nr_lpis);
int gicv3_its_init(void);
+/* Store the physical address and ID for each redistributor as read from DT. */
+void gicv3_set_redist_address(paddr_t address, unsigned int redist_id);
+uint64_t gicv3_get_redist_address(unsigned int cpu, bool use_pta);
+
+/* Map a collection for this host CPU to each host ITS. */
+int gicv3_its_setup_collection(unsigned int cpu);
+
#else
static LIST_HEAD(host_its_list);
@@ -134,6 +161,17 @@ static inline int gicv3_its_init(void)
{
return 0;
}
+
+static inline void gicv3_set_redist_address(paddr_t address,
+ unsigned int redist_id)
+{
+}
+
+static inline int gicv3_its_setup_collection(unsigned int cpu)
+{
This function should never be called as it is gated by the presence of
ITS. I would add a BUG() with a comment to ensure this is the case.
+ return 0;
+}
+
#endif /* CONFIG_HAS_ITS */
#endif
Cheers
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
