Re: [PATCH v3 05/26] ARM: GICv3 ITS: introduce ITS command handling

[Julien Grall <julien.grall@arm.com>]

Hi Andre,

I will be nice and repeating my comments. I am hoping they will be fixed 
next version.

On 31/03/17 19:05, Andre Przywara wrote:

[...]

>  #define ITS_CMD_QUEUE_SZ                SZ_1M
>
> @@ -34,6 +37,147 @@ bool gicv3_its_host_has_its(void)
>      return !list_empty(&host_its_list);
>  }
>
> +#define BUFPTR_MASK                     GENMASK_ULL(19, 5)
> +static int its_send_command(struct host_its *hw_its, const void *its_cmd)
> +{
> +    /* Some small grace period in case the command queue is congested. */

This comment is a nice improvement. But as mention in the previous 
version, should make it clear that it is a guess. People will likely ask 
why you choose 1ms whilst Linux is using 1s.

> +    s_time_t deadline = NOW() + MILLISECS(1);
> +    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).

You replied this morning, you will fixed in in v4. I am hoping this will 
be the case.

> +        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)
> +{
> +    /* Define an upper limit for our wait time. */

See my remark on the previous timeout comment.

> +    s_time_t deadline = NOW() + MILLISECS(100);
> +    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;
> +}
> +

[...]

> +/* 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 )

This check should be dropped.

[...]

> +/*
> + * 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;
> +    /* A similar generous wait limit as we use for the command queue wait. */

See my above comments about the timeout.

> +    s_time_t deadline = NOW() + MILLISECS(100);
> +
> +    reg = readl_relaxed(hw_its->its_base + GITS_CTLR);
> +    if ( !(reg & GITS_CTLR_ENABLE) && (reg & GITS_CTLR_QUIESCENT) )
> +        return 0;
> +
> +    writel_relaxed(reg & ~GITS_CTLR_ENABLE, hw_its->its_base + GITS_CTLR);
> +
> +    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 quiescent.\n");

dprintk will disappear on non-debug build. But this looks quite useful. 
So I would use printk.

[...]

> +uint64_t gicv3_get_redist_address(unsigned int cpu, bool use_pta)
> +{
> +    if ( use_pta )
> +        return per_cpu(lpi_redist, cpu).redist_addr & GENMASK_ULL(51, 16);
> +    else
> +        return per_cpu(lpi_redist, cpu).redist_id << 16;
> +}
> +
>  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 b84bc40..0e21cb2 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;

procnum should be unsigned.

> +
> +                    /*
> +                     * 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.
> +                     */

I said it on v2 this morning and will repeat it for record. This comment 
is not useful in itself here because redist_address could be used by 
other code. It would be more useful on top of the call to initialize ITS 
as it would explain why it is done there and not before.

[...]

> diff --git a/xen/include/asm-arm/gic_v3_defs.h b/xen/include/asm-arm/gic_v3_defs.h
> index 7cdebc5..b01b6ed 100644
> --- a/xen/include/asm-arm/gic_v3_defs.h
> +++ b/xen/include/asm-arm/gic_v3_defs.h


[...]

>  /* data structure for each hardware ITS */
>  struct host_its {
> @@ -88,6 +107,7 @@ struct host_its {
>      paddr_t size;
>      void __iomem *its_base;
>      unsigned int devid_bits;
> +    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;
>  };
> @@ -108,6 +128,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);
> @@ -135,6 +162,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
>

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel