Re: [PATCH v4 03/17] xen/arm: ITS: Port ITS driver to Xen

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

Hi Vijay,

On 10/07/2015 09:42, vijay.kilari@gmail.com wrote:
> ---
> v4: Major changes
>    - Redistributor refactoring patch is merged
>    - Fixed comments from v3 related to coding style and
>      removing duplicate code.
>    - Target address is stored from bits[48:16] to avoid
>      shifting of target address while building ITS commands
>    - Removed non-static functions
>    - Removed usage of command builder functions
>    - Changed its_cmd_block union to include mix of bit and unsigned
>      variable types to define ITS command structure

You also replaced all ratelimited printk by normal printk. On Xen, error 
and warning are not ratelimited by default for Xen (only guest printk is 
ratelimited).

The ratelimited is used in the ITS for the command queue. Which will be
partially exposed to the guest via the LPI configuration table 
virtualisation.

I'd like to see at least the macro its_err_ratelimited kept, even though 
it uses XENLOG_ERR today. This is at least to remind us that it needs to 
be fixed in 4.7 in order to avoid possible security issue.

[..]

> +static void its_send_single_command(struct its_node *its,
> +                                    its_cmd_block *src,
> +                                    struct its_collection *sync_col)
> +{
> +    its_cmd_block *cmd, *sync_cmd, *next_cmd;
> +    unsigned long flags;
> +
> +    BUILD_BUG_ON(sizeof(its_cmd_block) != 32);
> +
> +    spin_lock_irqsave(&its->lock, flags);
> +
> +    cmd = its_allocate_entry(its);
> +    if ( !cmd )
> +    {

Can you keep /* We are soooo screwed .... */? I'm not sure why you 
removed it since v3, but it was useful shows that it's can unlikely happen.

> +        its_err("ITS can't allocate, dropping command\n");
> +        spin_unlock_irqrestore(&its->lock, flags);
> +        return;
> +    }


[..]


> +int its_lpi_init(u32 id_bits)

You are calling its_lpi_init from the vgic driver. I'm struggling to 
understand why... The vGIC driver is only here to virtualize the GIC for 
a domain not initialize the physical GIC.

Why can't you call its_lpi_init from its_init as Linux does? This would 
avoid to export this function.

[..]

> +static void its_cpu_init_collection(void)
> +{
> +    struct its_node *its;
> +    int cpu;
> +
> +    spin_lock(&its_lock);
> +    cpu = smp_processor_id();
> +
> +    list_for_each_entry(its, &its_nodes, entry)
> +    {
> +        u64 target;

Newline here please.

> +        /*
> +         * We now have to bind each collection to its target
> +         * redistributor.
> +         */
> +        if ( readq_relaxed(its->base + GITS_TYPER) & GITS_TYPER_PTA )
> +        {
> +            /*
> +             * This ITS wants the physical address of the
> +             * redistributor.
> +             */
> +            target = gic_data_rdist().phys_base;
> +            /*
> +             * If Target address is GICR adddress, then it is aligned to 64K

I think the "If Target address is GICR address..." is not necessary. We 
are already in the if for the Target address will be the re-dist address.

> +             * and hence ITS command field is only consider 32 bit skipping
> +             * lower 16 bits.So take bit[48:16]

I would say : "The ITS command is only considering [48:16] of the GICR 
address".

> +             */
> +            its->collections[cpu].target_address = target >> 16;

You could have done target >>= 16; To avoid duplicating 
its->collections[cpu].target_address in both if.

> +        }
> +        else
> +        {
> +            /*
> +             * This ITS wants a linear CPU number.
> +             */
> +            target = readq_relaxed(gic_data_rdist().rbase + GICR_TYPER);
> +            target = GICR_TYPER_CPU_NUMBER(target);
> +            its->collections[cpu].target_address = target;
> +        }
> +
> +        /* Perform collection mapping */

And then

its->collections[cpu].target_address = target;

> +        its->collections[cpu].col_id = cpu;
> +
> +        its_send_mapc(its, &its->collections[cpu], 1);
> +        its_send_invall(its, &its->collections[cpu]);
> +    }
> +
> +    spin_unlock(&its_lock);
> +}

[..]

> +int __init its_init(struct rdist_prop *rdists)
> +{
> +    struct dt_device_node *np = NULL;
> +
> +    static const struct dt_device_match its_device_ids[] __initconst =
> +    {
> +        DT_MATCH_GIC_ITS,
> +        { /* sentinel */ },
> +    };
> +
> +    for (np = dt_find_matching_node(NULL, its_device_ids); np;
> +             np = dt_find_matching_node(np, its_device_ids))
> +        its_probe(np);
> +
> +    if ( list_empty(&its_nodes) )
> +    {
> +        its_warn("ITS: No ITS available, not enabling LPIs\n");
> +        return -ENXIO;
> +    }
> +
> +    gic_rdists = rdists;
> +    its_alloc_lpi_tables();

FIY, I think that its_lpi_init should be called here as it's done on Linux.

> +
> +    return 0;
> +}
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index 30682cf..b5c59f6 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -53,6 +53,7 @@ static struct {
>       paddr_t dbase;            /* Address of distributor registers */
>       paddr_t dbase_size;
>       void __iomem *map_dbase;  /* Mapped address of distributor registers */
> +    struct rdist_prop rdist_data;
>       struct rdist_region *rdist_regions;
>       uint32_t  rdist_stride;
>       unsigned int rdist_count; /* Number of rdist regions count */
> @@ -63,10 +64,10 @@ static struct {
>   static struct gic_info gicv3_info;
>
>   /* per-cpu re-distributor base */
> -static DEFINE_PER_CPU(void __iomem*, rbase);
> +DEFINE_PER_CPU(struct rdist, rdist);
>
>   #define GICD                   (gicv3.map_dbase)
> -#define GICD_RDIST_BASE        (this_cpu(rbase))
> +#define GICD_RDIST_BASE        (per_cpu(rdist, smp_processor_id()).rbase)

You could use this_cpu here:

this_cpu(rdist).rbase

>   #define GICD_RDIST_SGI_BASE    (GICD_RDIST_BASE + SZ_64K)
>
>   /*
> @@ -613,6 +614,7 @@ static int __init gicv3_populate_rdist(void)
>       uint32_t aff;
>       uint32_t reg;
>       uint64_t typer;
> +    uint64_t offset;
>       uint64_t mpidr = cpu_logical_map(smp_processor_id());
>
>       /*
> @@ -648,9 +650,12 @@ static int __init gicv3_populate_rdist(void)
>
>               if ( (typer >> 32) == aff )
>               {
> -                this_cpu(rbase) = ptr;
> -                printk("GICv3: CPU%d: Found redistributor in region %d @%p\n",
> -                        smp_processor_id(), i, ptr);
> +                offset = ptr - gicv3.rdist_regions[i].map_base;
> +                per_cpu(rdist, smp_processor_id()).rbase = ptr;

Please use: this_cpu(rdist).rbase = ptr;

> +                per_cpu(rdist, smp_processor_id()).phys_base =  gicv3.rdist_regions[i].base + offset;

this_cpu(rdist).phys_base;

> +                printk("GICv3: CPU%d: Found redistributor in region %d @%"PRIpaddr"\n",
> +                        smp_processor_id(), i,
> +                        per_cpu(rdist, smp_processor_id()).phys_base);

this_cpu(rdist).phys_base;

>                   return 0;
>               }
>               if ( gicv3.rdist_stride )

> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index 9e2acb7..e9d5f36 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -161,6 +161,7 @@
>       DT_MATCH_COMPATIBLE("arm,gic-400")
>
>   #define DT_MATCH_GIC_V3 DT_MATCH_COMPATIBLE("arm,gic-v3")
> +#define DT_MATCH_GIC_ITS DT_MATCH_COMPATIBLE("arm,gic-v3-its")

I don't think you need to export this.

>
>   /*
>    * GICv3 registers that needs to be saved/restored
> diff --git a/xen/include/asm-arm/gic_v3_defs.h b/xen/include/asm-arm/gic_v3_defs.h
> index 556f114..051a95e 100644
> --- a/xen/include/asm-arm/gic_v3_defs.h
> +++ b/xen/include/asm-arm/gic_v3_defs.h
> @@ -48,6 +48,7 @@
>   /* Additional bits in GICD_TYPER defined by GICv3 */
>   #define GICD_TYPE_ID_BITS_SHIFT 19
>
> +#define GICD_TYPER_LPIS_SUPPORTED    (1U << 17)
>   #define GICD_CTLR_RWP                (1UL << 31)
>   #define GICD_CTLR_ARE_NS             (1U << 4)
>   #define GICD_CTLR_ENABLE_G1A         (1U << 1)
> @@ -59,11 +60,12 @@
>   #define GICR_WAKER_ProcessorSleep    (1U << 1)
>   #define GICR_WAKER_ChildrenAsleep    (1U << 2)
>
> -#define GICD_PIDR2_ARCH_REV_MASK     (0xf0)
> +#define GIC_PIDR2_ARCH_REV_MASK      (0xf0)
> +#define GICD_PIDR2_ARCH_REV_MASK     GIC_PIDR2_ARCH_REV_MASK
>   #define GICD_PIDR2_ARCH_REV_SHIFT    (0x4)
>   #define GICD_PIDR2_ARCH_GICV3        (0x3)
>
> -#define GICR_PIDR2_ARCH_REV_MASK     GICD_PIDR2_ARCH_REV_MASK
> +#define GICR_PIDR2_ARCH_REV_MASK     GIC_PIDR2_ARCH_REV_MASK
>   #define GICR_PIDR2_ARCH_REV_SHIFT    GICD_PIDR2_ARCH_REV_SHIFT
>   #define GICR_PIDR2_ARCH_GICV3        GICD_PIDR2_ARCH_GICV3
>
> @@ -113,10 +115,24 @@
>   #define GICR_ICFGR1                  (0x0C04)
>   #define GICR_NSACR                   (0x0E00)
>
> +#define GICR_CTLR_ENABLE_LPIS        (1UL << 0)
> +#define GICR_TYPER_CPU_NUMBER(r)     (((r) >> 8) & 0xffff)
> +
> +#define GICR_PROPBASER_InnerShareable    (1U << 10)
> +#define GICR_PROPBASER_SHAREABILITY_MASK (3UL << 10)
> +#define GICR_PROPBASER_nC                (1U << 7)
> +#define GICR_PROPBASER_WaWb              (5U << 7)
> +#define GICR_PROPBASER_CACHEABILITY_MASK (7U << 7)
> +#define GICR_PROPBASER_IDBITS_MASK       (0x1f)
>   #define GICR_TYPER_PLPIS             (1U << 0)
>   #define GICR_TYPER_VLPIS             (1U << 1)
>   #define GICR_TYPER_LAST              (1U << 4)
>
> +#define GICR_PENDBASER_InnerShareable    (1U << 10)
> +#define GICR_PENDBASER_SHAREABILITY_MASK (3UL << 10)
> +#define GICR_PENDBASER_nC                (1U << 7)
> +#define GICR_PENDBASER_CACHEABILITY_MASK (7U << 7)
> +
>   #define DEFAULT_PMR_VALUE            0xff
>
>   #define GICH_VMCR_EOI                (1 << 9)
> @@ -152,6 +168,103 @@
>   #define ICH_SGI_IRQ_SHIFT            24
>   #define ICH_SGI_IRQ_MASK             0xf
>   #define ICH_SGI_TARGETLIST_MASK      0xffff
> +#define LPI_PROP_GROUP1                 (1 << 1)
>
> +/*
> + * ITS registers, offsets from ITS_base
> + */

It would make sense to move all the GITS_* defines in the gic-its.h 
which you've introduced within this patch.

> +#define GITS_CTLR                       0x0000
> +#define GITS_IIDR                       0x0004
> +#define GITS_TYPER                      0x0008
> +#define GITS_CBASER                     0x0080
> +#define GITS_CWRITER                    0x0088
> +#define GITS_CREADR                     0x0090
> +#define GITS_BASER0                     0x0100
> +#define GITS_BASER1                     0x0108
> +#define GITS_BASER                      0x0100
> +#define GITS_BASERN                     0x013c
> +#define GITS_PIDR0                      GICR_PIDR0
> +#define GITS_PIDR1                      GICR_PIDR1
> +#define GITS_PIDR2                      GICR_PIDR2
> +#define GITS_PIDR3                      GICR_PIDR3
> +#define GITS_PIDR4                      GICR_PIDR4
> +#define GITS_PIDR5                      GICR_PIDR5
> +#define GITS_PIDR7                      GICR_PIDR7

[..]

> +struct rdist {
> +    void __iomem *rbase;
> +    void *pend_page;
> +    paddr_t phys_base;
> +};
> +
> +struct rdist_prop {
> +    void *prop_page;
> +    uint64_t flags;
> +};

On v3, I asked the meaning of "prop" in both the name of the structure 
and the field. Can you explain what does it mean?

It would also be nice to document the 2 structures.

Regards,

-- 
Julien Grall