Re: [PATCH v4 11/17] xen/arm: ITS: Add GICR register emulation

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

Hi Vijay,

On 10/07/2015 09:42, vijay.kilari@gmail.com wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
>
> Emulate LPI related changes to GICR registers

It looks like to me that the LPI configuration table emulation doesn't 
contain any LPI specific code. So it looks like to me that this code 
should live in GICv3.

Anyway, I don't mind to defer this for Xen 4.7. But please add a TODO.

[...]

> +static int gicv3_dist_supports_lpis(void)
> +{
> +    return readl_relaxed(GICD + GICD_TYPER) & GICD_TYPER_LPIS_SUPPORTED;
> +}
> +
>   static int __cpuinit gicv3_cpu_init(void)
>   {
>       int i;
> @@ -1293,10 +1298,20 @@ static int __init gicv3_init(void)
>              gicv3.rdist_regions[0].size, gicv3.rdist_regions[0].map_base,
>              gicv3_info.maintenance_irq);
>
> +    reg = readl_relaxed(GICD + GICD_TYPER);
> +
> +    gicv3.rdist_data.id_bits = ((reg >> 19) & 0x1f) + 1;

Can you please introduce define for value 19 and 0x1f rather than 
hardcoding them?

> +    gicv3_info.nr_id_bits = gicv3.rdist_data.id_bits;
> +
>       spin_lock_init(&gicv3.lock);
>
>       spin_lock(&gicv3.lock);
>
> +    if ( gicv3_dist_supports_lpis() )
> +        gicv3_info.lpi_supported = 1;
> +    else
> +        gicv3_info.lpi_supported = 0;
> +

gicv3_info.lpi_supported = !!gicv3_gist_supports_lpis();

>       gicv3_dist_init();
>       res = gicv3_cpu_init();
>       gicv3_hyp_init();
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 4f3801b..3ebadcf 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -73,6 +73,16 @@ unsigned int gic_number_lines(void)
>       return gic_hw_ops->info->nr_lines;
>   }
>
> +unsigned int gic_nr_id_bits(void)
> +{
> +    return gic_hw_ops->info->nr_id_bits;
> +}
> +
> +bool_t gic_lpi_supported(void)
> +{
> +    return gic_hw_ops->info->lpi_supported;
> +}
> +
>   void gic_save_state(struct vcpu *v)
>   {
>       ASSERT(!local_irq_is_enabled());
> diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
> index abf60e2..bbcc7bb 100644
> --- a/xen/arch/arm/vgic-v3-its.c
> +++ b/xen/arch/arm/vgic-v3-its.c
> @@ -49,6 +49,36 @@ static void dump_cmd(its_cmd_block *cmd)
>   }
>   #endif
>
> +static void vits_disable_lpi(struct vcpu *v, uint32_t vlpi)
> +{
> +    struct pending_irq *p;
> +
> +    p = irq_to_pending(v, vlpi);
> +    clear_bit(GIC_IRQ_GUEST_ENABLED, &p->status);
> +    gic_remove_from_queues(v, vlpi);
> +}
> +
> +static void vits_enable_lpi(struct vcpu *v, uint32_t vlpi, uint8_t priority)

The argument priority is never used. Please drop it.

> +{
> +    struct pending_irq *p;
> +    unsigned long flags;
> +
> +    /* Get plpi for the given vlpi */

This comment seems wrong.

> +    p = irq_to_pending(v, vlpi);
> +
> +    set_bit(GIC_IRQ_GUEST_ENABLED, &p->status);
> +
> +    spin_lock_irqsave(&v->arch.vgic.lock, flags);
> +
> +    /*XXX: raise on right vcpu */

/* XXX: ... */

I guess you added this comment because I mentioned possible locking 
problem here?

If so, did you think how this would affect the vLPI handling to not do 
the correct locking for Xen 4.6?

Note for the others: AFAICT the locking of the pending_irq structure is 
done using the lock of the vCPU where the IRQ is routed. Stefano, please 
confirm if it's true.

> +    if ( !list_empty(&p->inflight) &&
> +         !test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) )
> +        gic_raise_guest_irq(v, irq_to_virq(p->desc), p->priority);
> +
> +    spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
> +}
> +
> +/* ITS device table helper functions */

This comment doesn't belong to this patch but patch #6.

>   static int vits_entry(struct domain *d, paddr_t entry, void *addr,
>                         uint32_t size, bool_t set)
>   {
> @@ -595,6 +625,141 @@ err:
>       return 0;
>   }
>
> +static int vgic_v3_gits_lpi_mmio_read(struct vcpu *v, mmio_info_t *info)
> +{
> +    uint32_t offset;
> +    struct hsr_dabt dabt = info->dabt;
> +    struct cpu_user_regs *regs = guest_cpu_user_regs();
> +    register_t *r = select_user_reg(regs, dabt.reg);
> +
> +    offset = info->gpa -
> +             (v->domain->arch.vits->propbase & BIT_48_12_MASK);
> +
> +    if ( offset < v->domain->arch.vits->prop_size )

As said on v3, this check is not necessary. You already register the 
handler on a valid range. So please drop this check.


> +    {
> +        DPRINTK("%pv: vITS: LPI Table read offset 0x%x\n", v, offset);
> +        spin_lock(&v->domain->arch.vits->prop_lock);
> +        *r = *((u8*)v->domain->arch.vits->prop_page + offset);

You didn't answer my question on v3... I.e "what about other access? a 
64/32/16 bits access are valid and will return the wrong value."

> +        spin_unlock(&v->domain->arch.vits->prop_lock);
> +        return 1;
> +    }
> +    else
> +        dprintk(XENLOG_G_ERR, "%pv: vITS: LPI Table read with wrong offset 0x%x\n",
> +                v, offset);
> +
> +    return 0;
> +}
> +
> +static int vgic_v3_gits_lpi_mmio_write(struct vcpu *v, mmio_info_t *info)
> +{
> +    uint32_t offset;
> +    uint32_t vid;
> +    uint8_t cfg, *p;
> +    bool_t enable;
> +    struct hsr_dabt dabt = info->dabt;
> +    struct cpu_user_regs *regs = guest_cpu_user_regs();
> +    register_t *r = select_user_reg(regs, dabt.reg);
> +
> +    offset = info->gpa -
> +             (v->domain->arch.vits->propbase & BIT_48_12_MASK);
> +
> +    vid = offset + NR_GIC_LPI;

I'm not sure how you test this series... Based on patch #6, NR_GIC_LPI 
(which is defined as 4096) represents the number of LPIs and not the 
BASE of LPIs (i.e FIRST_GIC_LPI = 8196)

> +    if ( offset < v->domain->arch.vits->prop_size )

This check is not necessary. Please drop it.

> +    {
> +        DPRINTK("%pv: vITS: LPI Table write offset 0x%x\n", v, offset);
> +
> +        spin_lock(&v->domain->arch.vits->prop_lock);
> +        p = ((u8*)v->domain->arch.vits->prop_page + offset);
> +        cfg = *p;
> +        enable = (cfg & *r) & 0x1;

Please use a define rather than 0x1. It would have been more clear that 
we are checking the enable bit.

> +
> +        if ( !enable )
> +             vits_enable_lpi(v, vid,  (*r & LPI_PRIORITY_MASK));
> +        else
> +             vits_disable_lpi(v, vid);
> +
> +        /* Update virtual prop page */
> +        *p = (*r & 0xff);
> +        spin_unlock(&v->domain->arch.vits->prop_lock);

What about other access than 8-bit (i.e 16-bit, 32-bit, 64-bit)?

> +        return 1;
> +    }
> +    else
> +        dprintk(XENLOG_G_ERR, "%pv: vITS: LPI Table invalid write @ 0x%x\n",
> +                v, offset);
> +
> +    return 0;
> +}
> +
> +static const struct mmio_handler_ops vgic_gits_lpi_mmio_handler = {
> +    .read_handler  = vgic_v3_gits_lpi_mmio_read,
> +    .write_handler = vgic_v3_gits_lpi_mmio_write,
> +};
> +
> +int vits_unmap_lpi_prop(struct vcpu *v)

It looks like to me that the function should take a domain rather than a 
vcpu.

> +{
> +    paddr_t maddr, addr;
> +    unsigned long mfn;
> +    uint32_t lpi_size, id_bits;
> +    int i;
> +
> +    maddr = v->domain->arch.vits->propbase & BIT_48_12_MASK;

maddr means machine address which doesn't seem to be the case here. 
Please use the right name.

> +    id_bits = ((v->domain->arch.vits->propbase & GICR_PROPBASER_IDBITS_MASK)+1);
> +
> +    DPRINTK("%pv: vITS: Unmap guest LPI conf table maddr 0x%lx lpi_size 0x%x\n",
> +            v, maddr, lpi_size);
> +
> +    spin_lock(&v->domain->arch.vits->prop_lock);

I don't understand why you need to take the prop_lock here. You only use 
them in the handler which you don't have yet registered.

> +    if ( id_bits > gic_nr_id_bits() )
> +        id_bits = gic_nr_id_bits();

What happen if the property configuration table is smaller?

> +
> +    lpi_size = 1UL << id_bits;
> +
> +    v->domain->arch.vits->prop_size = lpi_size;
> +    /* Allocate Virtual LPI Property table */
> +    /* TODO: To re-use guest property table? */
> +    v->domain->arch.vits->prop_page =
> +        alloc_xenheap_pages(get_order_from_bytes(lpi_size), 0);
> +    if ( !v->domain->arch.vits->prop_page )
> +    {
> +        dprintk(XENLOG_G_ERR, "%pv: vITS: Fail to allocate LPI Prop page\n", v);
> +        return 0;
> +    }
> +
> +    addr = maddr;
> +    for ( i = 0; i < lpi_size / PAGE_SIZE; i++ )
> +    {
> +        vits_entry(v->domain, addr,
> +                   (void *)(v->domain->arch.vits->prop_page + i * PAGE_SIZE),
> +                   PAGE_SIZE, 0);
> +        addr += PAGE_SIZE;

It's good to copy the value from the guest memory to our copy. It would 
be better to also replicate the value in the vITS structure. I.e 
enable/disable the LPIs when necessary.

Though, it may means a long process in this function.

[...]

> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
> index 4af5a84..25b69a0 100644
> --- a/xen/arch/arm/vgic-v3.c
> +++ b/xen/arch/arm/vgic-v3.c
> @@ -30,6 +30,7 @@
>   #include <asm/mmio.h>
>   #include <asm/gic_v3_defs.h>
>   #include <asm/gic.h>
> +#include <asm/gic-its.h>
>   #include <asm/vgic.h>
>
>   /* GICD_PIDRn register values for ARM implementations */
> @@ -93,7 +94,18 @@ static int __vgic_v3_rdistr_rd_mmio_read(struct vcpu *v, mmio_info_t *info,
>       switch ( gicr_reg )
>       {
>       case GICR_CTLR:
> -        /* We have not implemented LPI's, read zero */
> +        /*
> +         * Enable LPI's for ITS. Direct injection of LPI
> +         * by writing to GICR_{SET,CLR}LPIR are not supported
> +         */

This comment should be on write but not read. The read function only 
return the value stored in gicr_ctlr.

> +        if ( gic_lpi_supported() )
> +        {
> +            if ( dabt.size != DABT_WORD ) goto bad_width;
> +            vgic_lock(v);
> +            *r = v->domain->arch.vgic.gicr_ctlr;
> +            vgic_unlock(v);
> +            return 1;
> +        }

I think this could be simplified to

    if ( dabt.size != DABT_WORD ) goto bad_width;
    vgic_lock(v);
    *r = v->domain->arch.vgic.gicr_ctlr;
    vgic_unlock(v);
    return 0;

This is because the write emulation will ensure the validity of gicr_ctlr.

>           goto read_as_zero_32;
>       case GICR_IIDR:
>           if ( dabt.size != DABT_WORD ) goto bad_width;
> @@ -106,11 +118,16 @@ static int __vgic_v3_rdistr_rd_mmio_read(struct vcpu *v, mmio_info_t *info,
>                  MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 2) << 48 |
>                  MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 1) << 40 |
>                  MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 0) << 32);
> +        if ( gic_lpi_supported() )
> +        {
> +            /* Set LPI support */
> +            aff |= GICR_TYPER_PLPIS;
> +            /* GITS_TYPER.PTA is  0. Provice vcpu number as ta */

s/Provice/Provide/

And please use "Target Address" rather than "ta". Easier to read.

> +            aff |= (v->vcpu_id << GICR_TYPER_PROCESSOR_SHIFT);
> +        }

The variable aff is used to store the affinity nothing else.

Please use *r instead of aff. Note that you will have to move the code 
after *r = aff;

>           *r = aff;
> -
>           if ( v->arch.vgic.flags & VGIC_V3_RDIST_LAST )
>               *r |= GICR_TYPER_LAST;
> -
>           return 1;

[...]

> @@ -224,10 +263,32 @@ static int __vgic_v3_rdistr_rd_mmio_write(struct vcpu *v, mmio_info_t *info,
>           /* LPI is not implemented */
>           goto write_ignore_64;
>       case GICR_PROPBASER:
> -        /* LPI is not implemented */
> +        if ( gic_lpi_supported() )
> +        {
> +            if ( dabt.size != DABT_DOUBLE_WORD ) goto bad_width;
> +            vgic_lock(v);
> +            /* LPI configuration tables are shared across cpus. Should be same */
> +            /* TODO: Manage change in property table */
> +            if ( v->domain->arch.vits->propbase != 0 )

Well it's potentially valid to have propbase equals to 0. You may want 
to add explain this restriction in the comment as a new TODO.

[..]

> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index 73a6f7e..a5f66f6 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -154,6 +154,10 @@ void domain_vgic_free(struct domain *d)
>       xfree(d->arch.vgic.shared_irqs);
>       xfree(d->arch.vgic.pending_irqs);
>       xfree(d->arch.vgic.allocated_irqs);
> +#ifdef CONFIG_ARM_64
> +    free_xenheap_pages(d->arch.vits->prop_page,
> +                       get_order_from_bytes(d->arch.vits->prop_size));
> +#endif

Anything allocated for the vITS should be deallocated in the vITS code 
not in the common vGIC code.

A new callback in the vgic structure should be added in order to free 
vgic specific code.

I would be nice if you don't do it for xen 4.6, but you need at least to 
add a /* TODO: Move in vits code */.

>   }
>
>   int vcpu_vgic_init(struct vcpu *v)
> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> index 67e4695..49db7f0 100644
> --- a/xen/include/asm-arm/domain.h
> +++ b/xen/include/asm-arm/domain.h
> @@ -102,6 +102,7 @@ struct arch_domain
>           paddr_t dbase; /* Distributor base address */
>           paddr_t cbase; /* CPU base address */
>   #ifdef CONFIG_ARM_64
> +	int gicr_ctlr;

The indentation look wrong here.

>           /* GIC V3 addressing */
>           paddr_t dbase_size; /* Distributor base size */
>           /* List of contiguous occupied by the redistributors */

[...]

> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index fdd96c8..69bf1ff 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -100,6 +100,7 @@
>   #define GICD_TYPE_CPUS_SHIFT 5
>   #define GICD_TYPE_CPUS  0x0e0
>   #define GICD_TYPE_SEC   0x400
> +#define GICD_TYPE_LPIS  (0x1UL << 17)
>
>   #define GICC_CTL_ENABLE 0x1
>   #define GICC_CTL_EOI    (0x1 << 9)
> @@ -283,6 +284,10 @@ extern void gic_dump_info(struct vcpu *v);
>
>   /* Number of interrupt lines */
>   extern unsigned int gic_number_lines(void);
> +/* Number of interrupt id bits supported */
> +extern unsigned int gic_nr_id_bits(void);
> +/* LPI support info */
> +bool_t gic_lpi_supported(void);
>
>   void gicv3_eoi_irq(struct irq_desc *irqd);
>   void gicv3_dir_irq(struct irq_desc *irqd);
> @@ -302,6 +307,10 @@ struct gic_info {
>       unsigned int maintenance_irq;
>       /* Pointer to the device tree node representing the interrupt controller */
>       const struct dt_device_node *node;
> +    /* Number of IRQ ID bits supported */
> +    uint32_t nr_id_bits;
> +    /* LPIs are support information */
> +    bool_t lpi_supported;

[..]

>   struct rdist_prop {
>       void *prop_page;
> +    int id_bits;

The nr_id_bits (in gic_info) and id_bits here looks the same. Please 
remove one of them.

IHMO, the former (i.e in gic_info) should be removed.

Regards,

-- 
Julien Grall