[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [PATCH 06/28] ARM: GICv3 ITS: introduce ITS command handling
Hi Andre,
Continuing the review where I left it yesterday.
On 30/01/2017 18:31, Andre Przywara wrote:
[...]
+/* Wait for an ITS to become quiescient (all ITS operations completed). */
+static int gicv3_its_wait_quiescient(struct host_its *hw_its)
+{
+ uint32_t reg;
+ s_time_t deadline = NOW() + MILLISECS(1000);
+
+ reg = readl_relaxed(hw_its->its_base + GITS_CTLR);
+ if ( (reg & (GITS_CTLR_QUIESCENT | GITS_CTLR_ENABLE)) ==
GITS_CTLR_QUIESCENT )
It would be clearer if you rewrite this:
(reg & GITS_CTLR_QUIESCENT) && !(reg & GITS_CTLR_ENABLE).
+ return 0;
+
+ writel_relaxed(reg & ~GITS_CTLR_ENABLE, hw_its->its_base + GITS_CTLR);
It feels a bit odd to disable the ITS in a function containing "wait" in
the name. You may want to rename the function to reflect the behavior.
+
+ do {
+ reg = readl_relaxed(hw_its->its_base + GITS_CTLR);
+ if ( reg & GITS_CTLR_QUIESCENT )
+ return 0;
+
+ cpu_relax();
+ udelay(1);
+ } while ( NOW() <= deadline );
+
+ dprintk(XENLOG_ERR, "ITS not quiescient\n");
+ return -ETIMEDOUT;
+}
+
static unsigned int max_its_device_bits = CONFIG_MAX_PHYS_ITS_DEVICE_BITS;
integer_param("max_its_device_bits", max_its_device_bits);
int gicv3_its_init(struct host_its *hw_its)
{
uint64_t reg;
- int i;
+ int i, ret;
hw_its->its_base = ioremap_nocache(hw_its->addr, hw_its->size);
if ( !hw_its->its_base )
return -ENOMEM;
+ ret = gicv3_its_wait_quiescient(hw_its);
+ if ( ret )
+ return ret;
+
+ reg = readq_relaxed(hw_its->its_base + GITS_TYPER);
+ if ( reg & GITS_TYPER_PTA )
+ hw_its->flags |= HOST_ITS_USES_PTA;
+
for ( i = 0; i < GITS_BASER_NR_REGS; i++ )
{
void __iomem *basereg = hw_its->its_base + GITS_BASER0 + i * 8;
@@ -196,6 +322,20 @@ int gicv3_its_init(struct host_its *hw_its)
return -ENOMEM;
writeq_relaxed(0, hw_its->its_base + GITS_CWRITER);
[...]
diff --git a/xen/arch/arm/gic-v3-lpi.c b/xen/arch/arm/gic-v3-lpi.c
index e2fc901..5911b91 100644
--- a/xen/arch/arm/gic-v3-lpi.c
+++ b/xen/arch/arm/gic-v3-lpi.c
@@ -30,11 +30,31 @@ static struct {
unsigned int host_lpi_bits;
} lpi_data;
+/* Physical redistributor address */
+static DEFINE_PER_CPU(paddr_t, redist_addr);
+/* Redistributor ID */
+static DEFINE_PER_CPU(int, redist_id);
s/int/unsigned int/
/* Pending table for each redistributor */
static DEFINE_PER_CPU(void *, pending_table);
Rather than defining 3 per-cpu variables, could we merge all in a single
structure?
#define MAX_PHYS_LPIS (BIT_ULL(lpi_data.host_lpi_bits) - LPI_OFFSET)
+/* Stores this redistributor's physical address and ID in a per-CPU variable */
+void gicv3_set_redist_address(paddr_t address, int redist_id)
s/int/unsigned int/
+{
+ this_cpu(redist_addr) = address;
+ this_cpu(redist_id) = redist_id;
+}
+
+/* Returns a redistributor's ID (either as an address or as an ID) */
+uint64_t gicv3_get_redist_address(int cpu, bool use_pta)
s/int/unsigned int/
+{
+ if ( use_pta )
+ return per_cpu(redist_addr, cpu) & GENMASK(51, 16);
+ else
+ return per_cpu(redist_id, cpu) << 16;
What if the function is called before the CPU has been setup? If it
cannot happen, please document it.
+}
+
uint64_t gicv3_lpi_allocate_pendtable(void)
{
uint64_t reg;
diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index 440c079..5f825a6 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -644,7 +644,7 @@ static int gicv3_rdist_init_lpis(void __iomem * rdist_base)
return -ENOMEM;
writeq_relaxed(table_reg, rdist_base + GICR_PROPBASER);
- return 0;
+ return gicv3_its_setup_collection(smp_processor_id());
}
static int __init gicv3_populate_rdist(void)
@@ -692,7 +692,21 @@ static int __init gicv3_populate_rdist(void)
if ( typer & GICR_TYPER_PLPIS )
{
- int ret;
+ paddr_t rdist_addr;
+ int procnum, ret;
procnum should be unsigned.
+
+ rdist_addr = gicv3.rdist_regions[i].base;
+ rdist_addr += ptr - gicv3.rdist_regions[i].map_base;
+ procnum = (typer & GICR_TYPER_PROC_NUM_MASK);
+ procnum >>= GICR_TYPER_PROC_NUM_SHIFT;
+
+ /*
+ * 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.
+ */
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.
+ gicv3_set_redist_address(rdist_addr, procnum);
ret = gicv3_rdist_init_lpis(ptr);
if ( ret && ret != -ENODEV )
diff --git a/xen/include/asm-arm/gic_v3_defs.h
b/xen/include/asm-arm/gic_v3_defs.h
index b307322..878bae2 100644
--- a/xen/include/asm-arm/gic_v3_defs.h
+++ b/xen/include/asm-arm/gic_v3_defs.h
@@ -101,6 +101,8 @@
#define GICR_TYPER_PLPIS (1U << 0)
#define GICR_TYPER_VLPIS (1U << 1)
#define GICR_TYPER_LAST (1U << 4)
+#define GICR_TYPER_PROC_NUM_SHIFT 8
+#define GICR_TYPER_PROC_NUM_MASK (0xffff << GICR_TYPER_PROC_NUM_SHIFT)
/* For specifying the inner cacheability type only */
#define GIC_BASER_CACHE_nCnB 0ULL
diff --git a/xen/include/asm-arm/gic_v3_its.h b/xen/include/asm-arm/gic_v3_its.h
index ff5572f..8288185 100644
--- a/xen/include/asm-arm/gic_v3_its.h
+++ b/xen/include/asm-arm/gic_v3_its.h
@@ -40,6 +40,9 @@
#define GITS_CTLR_QUIESCENT BIT(31)
#define GITS_CTLR_ENABLE BIT(0)
+#define GITS_TYPER_PTA BIT_ULL(19)
+#define GITS_TYPER_IDBITS_SHIFT 8
+
#define GITS_IIDR_VALUE 0x34c
#define GITS_BASER_INDIRECT BIT_ULL(62)
@@ -67,10 +70,27 @@
#define GITS_CBASER_SIZE_MASK 0xff
+/* 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>
#define HOST_ITS_FLUSH_CMD_QUEUE (1U << 0)
+#define HOST_ITS_USES_PTA (1U << 1)
/* data structure for each hardware ITS */
struct host_its {
@@ -79,6 +99,7 @@ struct host_its {
paddr_t addr;
paddr_t size;
void __iomem *its_base;
+ spinlock_t cmd_lock;
I was expecting to see a code to initialize the spinlock. So please
initialize the spinlock.
void *cmd_buf;
unsigned int flags;
};
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
