[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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |