Re: [PATCH v4 06/27] ARM: GICv3 ITS: introduce ITS command handling

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

Hi Andre,

On 04/03/2017 09:28 PM, Andre Przywara wrote:
> +#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.

[...]

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

[...]

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

[...]

> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index 54d2235..a559e5e 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -665,8 +665,25 @@ static int __init gicv3_populate_rdist(void)
>
>                  if ( typer & GICR_TYPER_PLPIS )
>                  {
> +                    paddr_t rdist_addr;
> +                    unsigned int procnum;
>                      int ret;
>
> +                    /*
> +                     * The ITS refers to redistributors either by their physical
> +                     * address or by their ID. Which one to use is an ITS
> +                     * choice. So determine those two values here (which we
> +                     * can do only here in GICv3 code) and tell the
> +                     * ITS code about it, so it can use them later to be able
> +                     * to address those redistributors accordingly.
> +                     */

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
> @@ -103,6 +103,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 3500b042..f4f3c9b 100644
> --- a/xen/include/asm-arm/gic_v3_its.h
> +++ b/xen/include/asm-arm/gic_v3_its.h
> @@ -42,11 +42,11 @@
>  #define GITS_CTLR_QUIESCENT             BIT(31)
>  #define GITS_CTLR_ENABLE                BIT(0)
>
> +#define GITS_TYPER_PTA                  BIT_ULL(19)
>  #define GITS_TYPER_DEVIDS_SHIFT         13
>  #define GITS_TYPER_DEVIDS_MASK          (0x1fUL << GITS_TYPER_DEVIDS_SHIFT)
>  #define GITS_TYPER_DEVICE_ID_BITS(r)    (((r & GITS_TYPER_DEVIDS_MASK) >> \
>                                                 GITS_TYPER_DEVIDS_SHIFT) + 1)
> -

Spurious change.

Cheers,

-- 
Julien Grall

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