| Message ID | 20160928182457.12433-6-andre.przywara@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 28 Sep 2016, 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@arm.com> > --- > 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); > +/* 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) > +{ > + int readp, writep; uint64_t > + 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); It might be worth to #define ITS_CMD_RING_SIZE PAGE_SIZE for clarity > + 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); > + 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; > +} > + > +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); Again in my version of the spec is GENMASK(47, 16). > + else > + reg |= per_cpu(rdist_id, cpu) << 16; > + 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); > + } > +} > + > #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); > + > + reg = readq_relaxed(hw_its->its_base + GITS_TYPER); > + hw_its->pta = reg & GITS_TYPER_PTA; To avoid problems: 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()); Why do we need these two commands in addition to the ones issued by gicv3_its_setup_collection? > return 0; > } > > +void gicv3_set_redist_addr(paddr_t address, int redist_id) > +{ > + 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 > + * 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 #define the 8 > 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 In my version of the spec (PRD03-GENC-010745 24.0) 0x0a is MAPVI. > +#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; > } > +static inline void gicv3_set_redist_addr(paddr_t address, int redist_id) > +{ > +} > +static inline void gicv3_its_setup_collection(int cpu) > +{ > +} > + > #endif /* CONFIG_HAS_ITS */ > > #endif /* __ASSEMBLY__ */
On Wed, 26 Oct 2016, Stefano Stabellini wrote: > > +/* 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 > > In my version of the spec (PRD03-GENC-010745 24.0) 0x0a is MAPVI. For reference, I had an older version of the spec. I found the new version, which confirms Andre's numbers.
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@arm.com> > --- > 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,
Hi, On 27/10/16 00:55, Stefano Stabellini wrote: > On Wed, 28 Sep 2016, 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@arm.com> >> --- >> 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); >> +/* 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) >> +{ >> + int readp, writep; > > uint64_t A bit overkill, but probably right type-wise. > >> + 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); > > It might be worth to > > #define ITS_CMD_RING_SIZE PAGE_SIZE > > for clarity Or revisit this to allow bigger queues than one page. > >> + 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); >> + 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; >> +} >> + >> +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); > > Again in my version of the spec is GENMASK(47, 16). > > >> + else >> + reg |= per_cpu(rdist_id, cpu) << 16; >> + 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); >> + } >> +} >> + >> #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); >> + >> + reg = readq_relaxed(hw_its->its_base + GITS_TYPER); >> + hw_its->pta = reg & GITS_TYPER_PTA; > > To avoid problems: > > 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()); > > Why do we need these two commands in addition to the ones issued by > gicv3_its_setup_collection? gicv3_its_setup_collection() gets called for each redistributor. On the first CPU this is done _before_ we actually have set up the ITS, so we can't send any commands at this time. The function actually checks for an initialised command buffer, eventually doing nothing on the first core. However this function _here_ is only called once (on core 0). So we send the commands for core 0 now, actually at the earliest possible time. Cheers, Andre. >> return 0; >> } >> >> +void gicv3_set_redist_addr(paddr_t address, int redist_id) >> +{ >> + 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 >> + * 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 #define the 8 > > >> 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 > > In my version of the spec (PRD03-GENC-010745 24.0) 0x0a is MAPVI. > > >> +#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; >> } >> +static inline void gicv3_set_redist_addr(paddr_t address, int redist_id) >> +{ >> +} >> +static inline void gicv3_its_setup_collection(int cpu) >> +{ >> +} >> + >> #endif /* CONFIG_HAS_ITS */ >> >> #endif /* __ASSEMBLY__ */ >
Hi Julien, (forgot to hit the Send button yesterday ...) just going over the review comments again and found some leftovers. I fixed/addressed all comments of yours that I don't explicitly refer to here. ... On 02/11/16 15:05, Julien Grall wrote: > 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@arm.com> >> --- >> 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? I'd rather keep this in line with the general UNIX/Linux way of returning an int, with 0 on success and a negative error number on failure. This allows easier fixing when we introduce more error handling in the future (SErrors?). So callers just pass on the return value and it's up to the leaf functions to come up with the proper error value. >> +} >> + >> +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). I believe we can't do easily, because the PTA bit is per ITS, not per redistributor. I see that it's rather unlikely that we have ITSes with different PTA bit settings in one system, but architecturally it's possible. >> + >> + 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. Originally I didn't care so much about handling ITS command queue errors, because traditionally we couldn't do too much about them, since there is no good way of communicating failure back to the guest. But since we now only issue commands outside of a non-Dom0 guest context, I added the error handling you suggested and pass any error up the call chain. > >> + } >> +} >> + >> #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? Well, the processor ID is a 16-bit value and nicely aligns with Xen's VCPUIDs, which are declared as "int" in struct vcpu. So I'd rather keep it as int here. >> +{ >> + 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 Oh, right, I think I copied this commenting style wrongly from some other Xen VGIC code. Fixed that now. Cheers, Andre. >> + * 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, >
Hi Andre, On 31/01/2017 09:10, Andre Przywara wrote: > On 02/11/16 15:05, Julien Grall wrote: >> On 28/09/16 19:24, Andre Przywara wrote: >>> +} >>> + >>> +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). > > I believe we can't do easily, because the PTA bit is per ITS, not per > redistributor. I see that it's rather unlikely that we have ITSes with > different PTA bit settings in one system, but architecturally it's possible. How about storing the value per ITS then? [...] >>> +void gicv3_set_redist_addr(paddr_t address, int redist_id) >> >> The second parameter should probably be unsigned, maybe uint64_t? > > Well, the processor ID is a 16-bit value and nicely aligns with Xen's > VCPUIDs, which are declared as "int" in struct vcpu. > So I'd rather keep it as int here. I am not sure why you speak about the vCPUIDS where this code only deal with pCPUIDs. Anyway, the ID should never be signed and even though it has been defined as int in the structure, on ARM we are trying to use unsigned number as it makes much more sense. Furthermore, the per-cpu value rdist_id has been defined as uint64_t, hence my request to use uint64_t. The 2 type need to match for consistency. I am fine if you decide to use unsigned int for the per-cpu value. Regards,
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); +/* 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) +{ + int readp, writep; + + 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); + + 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); + 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; +} + +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; + + 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); + } +} + #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); + + 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()); + return 0; } +void gicv3_set_redist_addr(paddr_t address, int redist_id) +{ + 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 + * 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)); + 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; } +static inline void gicv3_set_redist_addr(paddr_t address, int redist_id) +{ +} +static inline void gicv3_its_setup_collection(int cpu) +{ +} + #endif /* CONFIG_HAS_ITS */ #endif /* __ASSEMBLY__ */
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@arm.com> --- 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(+)