|
[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 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@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?
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,
>
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |