Re: [RFC PATCH v2 15/22] xen/arm: its: Add support to emulate GICR register for LPIs

[Julien Grall <julien.grall@linaro.org>]

Hello Vijay,

On 19/03/15 14:38, vijay.kilari@gmail.com wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> 
> With this patch add emulation of GICR registers for LPIs.
> Also add LPI property table emulation.
> 
> Domain's LPI property table is unmapped during domain init
> on LPIPROPBASE update and trapped on LPI property
> table read and write
> 
> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> ---
>  xen/arch/arm/vgic-v3-its.c        |  144 +++++++++++++++++++++++++++++++++++++
>  xen/arch/arm/vgic-v3.c            |   64 +++++++++++++----
>  xen/include/asm-arm/domain.h      |    1 +
>  xen/include/asm-arm/gic-its.h     |    1 +
>  xen/include/asm-arm/gic.h         |    2 +
>  xen/include/asm-arm/gic_v3_defs.h |    2 +
>  6 files changed, 200 insertions(+), 14 deletions(-)
> 
> diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
> index 4d8945f..f1d68d9 100644
> --- a/xen/arch/arm/vgic-v3-its.c
> +++ b/xen/arch/arm/vgic-v3-its.c
> @@ -869,6 +869,150 @@ err:
>      return 0;
>  }
>  
> +/* Search device structure and get corresponding plpi */
> +int vgic_its_get_pid(struct vcpu *v, uint32_t vlpi, uint32_t *plpi)

static int ....

Also you either return 0 or 1 so the return type should be bool_t.

> +{
> +    struct domain *d = v->domain;
> +    struct its_device *dev;
> +    int i = 0;
> +
> +    spin_lock(&d->arch.vits_devs.lock);
> +    list_for_each_entry( dev, &d->arch.vits_devs.dev_list, entry )
> +    {
> +        i = 0;
> +        while ((i = find_next_bit(dev->vlpi_map, dev->nr_lpis, i)) < dev->nr_lpis )
> +        {
> +            if ( dev->vlpi_entries[i].vlpi == vlpi )
> +            {
> +                *plpi = dev->vlpi_entries[i].plpi;
> +                spin_unlock(&d->arch.vits_devs.lock);
> +                return 0;
> +            }
> +            i++;
> +        }
> +    }
> +    spin_unlock(&d->arch.vits_devs.lock);
> +

The cost of this function seems high (2 imbricated loops). How often
will it be call?

> +    return 1;
> +}
> +
> +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);
> +    uint8_t cfg;
> +
> +    offset = info->gpa -
> +             (v->domain->arch.lpi_conf->propbase & 0xfffffffff000UL);
> +
> +    if ( offset < SZ_64K )

This check is pointless, you have registered the handler on the a valid
range.

> +    {
> +        DPRINTK("vITS: LPI Table read offset 0x%x\n", offset );
> +        cfg = readb_relaxed(v->domain->arch.lpi_conf->prop_page + offset);

Why do you use readb_relaxed? Those helpers have been created for using
reading MMIO not Xen memory...

Also what about the other access sizes? a 64/32/64 bits access are valid
and will return the wrong value.

> +        *r = cfg;
> +        return 1;
> +    }
> +    else
> +        dprintk(XENLOG_ERR, "vITS: LPI Table read with wrong offset 0x%x\n",
> +                offset);
> +
> +    return 0;
> +}
> +
> +static int vgic_v3_gits_lpi_mmio_write(struct vcpu *v, mmio_info_t *info)
> +{
> +    uint32_t offset;
> +    uint32_t pid, vid;
> +    uint8_t cfg;
> +    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.lpi_conf->propbase & 0xfffffffff000UL);
> +
> +    vid = offset + NR_GIC_LPI;

I think NR_GIC_LPI is misnamed and should be renamed to GIC_LPI_OFFSET.

> +    if ( offset < SZ_64K )

Ditto for the check.

> +    {
> +        DPRINTK("vITS: LPI Table write offset 0x%x\n", offset );
> +        if ( vgic_its_get_pid(v, vid, &pid) )
> +        {
> +            dprintk(XENLOG_ERR, "vITS: pID not found for vid %d\n", vid);

Please don't use XENLOG_ERR, see why on my comments in a previous patch.

> +            return 0;
> +        }
> +      
> +        cfg = readb_relaxed(v->domain->arch.lpi_conf->prop_page + offset);

Same question as before for readb_relaxed.

> +        enable = (cfg & *r) & 0x1;
> +
> +        if ( !enable )
> +             vgic_its_enable_lpis(v, pid);
> +        else
> +             vgic_its_disable_lpis(v, pid);

If I'm not mistaken pid = physical LPI and vid = virtual LPI. So you
should use vid instead of pid for vgic_its_{enable,disable}_lpis.

> +        /* Update virtual prop page */
> +        writeb_relaxed((*r & 0xff),
> +                        v->domain->arch.lpi_conf->prop_page + offset);

Same question as readb_relaxed here.

Also what about the other access size? 64/32/16 bits accesses are valid.

> +        
> +        return 1;
> +    }
> +    else
> +        dprintk(XENLOG_ERR, "vITS: LPI Table write with wrong offset 0x%x\n",
> +                offset);
> +
> +    return 0; 
> +}
> +
> +static const struct mmio_handler_ops vgic_gits_lpi_mmio_handler = {
> +    .read_handler  = vgic_v3_gits_lpi_mmio_read,Although, 
> +    .write_handler = vgic_v3_gits_lpi_mmio_write,
> +};

It looks like to me that the LPI emulation should be in the GICv3 code
not ITS.

> +
> +int vgic_its_unmap_lpi_prop(struct vcpu *v)
> +{
> +    paddr_t maddr;
> +    uint32_t lpi_size;
> +    int i;
> +    
> +    maddr = v->domain->arch.lpi_conf->propbase & 0xfffffffff000UL;
> +    lpi_size = 1UL << ((v->domain->arch.lpi_conf->propbase & 0x1f) + 1);
> +
> +    DPRINTK("vITS: Unmap guest LPI conf table maddr 0x%lx lpi_size 0x%x\n", 
> +             maddr, lpi_size);
> +
> +    if ( lpi_size < SZ_64K )

Why this restriction? The IDbits can encode up to 32 bits interrupt
identifier.

You have to check this value against GICD_TYPER.IDbits.

> +    {
> +        dprintk(XENLOG_ERR, "vITS: LPI Prop page < 64K\n");

No XENLOG_ERR

> +        return 0;
> +    }
> +
> +    /* XXX: As per 4.8.9 each re-distributor shares a common LPI configuration table 

Coding style:
/*
 *

XXX means TODO for me. So what did you forget to add?

4.8.9 from which spec?

> +     * So one set of mmio handlers to manage configuration table is enough
> +     */
> +    for ( i = 0; i < lpi_size / PAGE_SIZE; i++ )
> +        guest_physmap_remove_page(v->domain, paddr_to_pfn(maddr),
> +                                gmfn_to_mfn(v->domain, paddr_to_pfn(maddr)), 0);

No validation at all on the address pass for the guest? gmfn_to_mfn can
return an invalid MFN and I'm not sure what would happen if the guest is
trying to pass other things than RAM.

You may also need to free this unmapped page.

> +    /* Register mmio handlers for this region */
> +    register_mmio_handler(v->domain, &vgic_gits_lpi_mmio_handler,
> +                          maddr, lpi_size);
> +
> +    /* Allocate Virtual LPI Property table */
> +    v->domain->arch.lpi_conf->prop_page =
> +        alloc_xenheap_pages(get_order_from_bytes(lpi_size), 0);

I wasn't able to find a place where you free the pages allocated...

> +    if ( !v->domain->arch.lpi_conf->prop_page )
> +    {
> +        dprintk(XENLOG_ERR, "vITS: Failed to allocate LPI Prop page\n");

No XENLOG_ERR.

> +        return 0;
> +    }
> +
> +    memset(v->domain->arch.lpi_conf->prop_page, 0xa2, lpi_size);

Why?

What about if the guest decides to set another priority? Same if the
guest decides to provide an LPI page with some LPIs enabled.

> +
> +    return 1;
> +}
> +
>  struct vgic_its *its_to_vits(struct vcpu *v, paddr_t phys_base)
>  {
>      struct vgic_its *vits = NULL;
> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
> index ec79c2a..e9ec7fa 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 */
> @@ -99,20 +100,30 @@ 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 */
> -        goto read_as_zero_32;
> +        /*
> +         * Enable LPI's for ITS. Direct injection of LPI
> +         * by writing to GICR_{SET,CLR}LPIR are not supported
> +         */

This comment would be more meaningful on the write emulation not read one.

> +        if ( dabt.size != DABT_WORD ) goto bad_width;
> +        vgic_lock(v);
> +        *r = v->domain->arch.vgic.gicr_ctlr;
> +        vgic_unlock(v);
> +        return 1;
>      case GICR_IIDR:
>          if ( dabt.size != DABT_WORD ) goto bad_width;
>          *r = GICV3_GICR_IIDR_VAL;
>          return 1;
>      case GICR_TYPER:
> -        if ( dabt.size != DABT_DOUBLE_WORD ) goto bad_width;
> -        /* TBD: Update processor id in [23:8] when ITS support is added */
> +        if ( dabt.size != DABT_WORD && dabt.size != DABT_DOUBLE_WORD )

Why do you change the access size check? You don't even support WORD
access in this code...

> +            goto bad_width;
> +        /* XXX: Update processor id in [23:8] if GITS_TYPER: PTA is not set */

As said on a previous patch, it would be better if GITS_TYPER is defined
for a specific value in the emulation. So we don't have to worry about
GITS_TYPER.PTA is 0 or 1.

IHMO, GITS_TYPER.PTA = 0 would make the code a lot simpler.

>          aff = (MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 3) << 56 |
>                 MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 2) << 48 |
>                 MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 1) << 40 |
>                 MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 0) << 32);
>          *r = aff;
> +        /* Set LPI support */
> +        aff |= (GICR_TYPER_DISTRIBUTED_IMP | GICR_TYPER_PLPIS);

Funny, how can Linux works? You don't even expose those 2 bits because
you set aff not *r...

What is GICR_TYPER_DISTRIBUTED_IMP? It points to Bit 3.

Although, the spec define bit 3 as Direct LPI. As you don't implement
the register GICR_SETLPIR/GICR_CLRLPI,... this bit should be set 0.

Finally we still have to support GICv3 on platform where ITS is not
present. So, for instance GICR_TYPER_PLPIS should not always be set.

>  
>          if ( v->arch.vgic.flags & VGIC_V3_RDIST_LAST )
>              *r |= GICR_TYPER_LAST;
> @@ -131,10 +142,13 @@ static int __vgic_v3_rdistr_rd_mmio_read(struct vcpu *v, mmio_info_t *info,
>          /* WO. Read as zero */
>          goto read_as_zero_64;
>      case GICR_PROPBASER:
> -        /* LPI's not implemented */
> -        goto read_as_zero_64;
> +        if ( dabt.size != DABT_DOUBLE_WORD ) goto bad_width;
> +        /* Remove shareability attribute we don't want dom to flush */

The comment seems misplaced. I don't see a such things implemented in
the read.

> +        *r = v->domain->arch.lpi_conf->propbase;

A lock is missing.

> +        return 1;
>      case GICR_PENDBASER:
> -        /* LPI's not implemented */
> +        if ( dabt.size != DABT_DOUBLE_WORD ) goto bad_width;
> +        *r = v->domain->arch.lpi_conf->pendbase[v->vcpu_id];

It sounds like pendbase should be stored per vcpu not in a domain array.

Also a lock is missing.

>          goto read_as_zero_64;
>      case GICR_INVLPIR:
>          /* WO. Read as zero */
> @@ -209,8 +223,15 @@ static int __vgic_v3_rdistr_rd_mmio_write(struct vcpu *v, mmio_info_t *info,
>      switch ( gicr_reg )
>      {
>      case GICR_CTLR:
> -        /* LPI's not implemented */
> -        goto write_ignore_32;
> +        /*
> +         * Enable LPI's for ITS. Direct injection of LPI
> +         * by writing to GICR_{SET,CLR}LPIR are not supported
> +         */

This comment should be placed in read emulation of GICR_TYPER  not
write/read of GICR_CTLR.

> +        if ( dabt.size != DABT_WORD ) goto bad_width;
> +        vgic_lock(v);
> +        v->domain->arch.vgic.gicr_ctlr = (*r) & GICR_CTL_ENABLE;

GICR_CTL_ENABLE should be named GICR_CTL_ENABLE_LPIS. Anyway, there is
already a define GICR_CTL_ENABLE_LPIS which has been added by you. So
please use it.

Futhermore, if the ITS is not present, this bit should be RES0.

> +        vgic_unlock(v);
> +        return 1;Although, 
>      case GICR_IIDR:
>          /* RO */
>          goto write_ignore_32;
> @@ -230,11 +251,26 @@ static int __vgic_v3_rdistr_rd_mmio_write(struct vcpu *v, mmio_info_t *info,
>          /* LPI is not implemented */

Odd, even after your series, there is lots of place with the comment /*
LPI is not implemented */. Did you intend to implement them? Or is it
because they deal with Direct LPI which we don't support? If it's the
latter, then the comment should be updated.

>          goto write_ignore_64;
>      case GICR_PROPBASER:
> -        /* LPI is not implemented */
> -        goto write_ignore_64;
> +        if ( dabt.size != DABT_DOUBLE_WORD ) goto bad_width;
> +        vgic_lock(v);

When GICR_CTLR.EnableLPIs == 1, it's change is unpredictable. That means
we should deny a such case by crashing the domain.

> +        /* LPI configuration tables are shared across cpus. Should be same */
> +        if ( (v->domain->arch.lpi_conf->propbase != 0) && 
> +             ((v->domain->arch.lpi_conf->propbase & 0xfffffffff000UL) !=  (*r & 0xfffffffff000UL)) )

Multiple problems here:
  * r == 0 is perfectly valid
  * the guest can change probase at anytime when GICR_CTLR.EnableLPIs ==
0. The value only matter when this bit is set to 1

> +        {
> +            dprintk(XENLOG_ERR,

no XENLOG_ERR.Although,

> +                "vGICv3: vITS: Wrong configuration of LPI_PROPBASER\n");

This is part of the vGICv3 not vITS. Also please follow the same pattern
as the other message within the redistributor emulation.

> +            return 0;
> +        }     
> +        v->domain->arch.lpi_conf->propbase = *r;
> +        vgic_unlock(v);
> +        return vgic_its_unmap_lpi_prop(v);
>      case GICR_PENDBASER:
> -        /* LPI is not implemented */
> -        goto write_ignore_64;
> +        /* Just hold pendbaser value for guest read */

Faking the emulation is not a good things. A guest may try to use this
page and it won't work correctly.

It would take a long time for the developer to understand the problem.

If you think it's not important right now, we should at least notify the
guest in some way.

> +        if ( dabt.size != DABT_DOUBLE_WORD ) goto bad_width;
> +        vgic_lock(v);
> +        v->domain->arch.lpi_conf->pendbase[v->vcpu_id] = *r;
> +        vgic_unlock(v);

You know that taking the VCPU lock doesn't protect concurrent access on
the pendbase?

> +        return 1;
>      case GICR_INVLPIR:
>          /* LPI is not implemented */
>          goto write_ignore_64;
> @@ -703,7 +739,7 @@ static int vgic_v3_distr_mmio_read(struct vcpu *v, mmio_info_t *info)
>                ((v->domain->arch.vgic.nr_spis / 32) & GICD_TYPE_LINES));

Sounds like you forgot to update irq_bits.

>          *r |= (irq_bits - 1) << GICD_TYPE_ID_BITS_SHIFT;
> -

Please keep the blank line after *r |= GICD_TYPE_LPIS.

> +        *r |= GICD_TYPE_LPIS;


It's wrong on platform without ITS support in Xen.


>          return 1;
>      }
>      case GICD_STATUSR:
> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> index bc7aee9..7202f93 100644
> --- a/xen/include/asm-arm/domain.h
> +++ b/xen/include/asm-arm/domain.h
> @@ -101,6 +101,7 @@ struct arch_domain
>          paddr_t dbase; /* Distributor base address */
>          paddr_t cbase; /* CPU base address */
>  #ifdef CONFIG_ARM_64Although, 
> +	int gicr_ctlr;

The indentation is wrong.

>          /* 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-its.h b/xen/include/asm-arm/gic-its.h
> index 82cfbdc..e1a5fa0 100644
> --- a/xen/include/asm-arm/gic-its.h
> +++ b/xen/include/asm-arm/gic-its.h
> @@ -229,6 +229,7 @@ unsigned long *its_lpi_alloc_chunks(int nirqs, int *base, int *nr_ids);
>  uint32_t its_get_pta_type(void);
>  uint32_t its_get_nr_its(void);
>  struct its_node * its_get_phys_node(uint32_t dev_id);
> +int vgic_its_unmap_lpi_prop(struct vcpu *v);
>  #endif /* __ASM_ARM_GIC_ITS_H__ */
>  
>  /*
> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index 6f5767f..f15174b 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -20,6 +20,7 @@
>  
>  #define NR_GIC_LOCAL_IRQS  NR_LOCAL_IRQS
>  #define NR_GIC_SGI         16
> +#define NR_GIC_LPI         8192

The naming is wrong.

>  #define MAX_RDIST_COUNT    4
>  
>  #define GICD_CTLR       (0x000)
> @@ -96,6 +97,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)
> diff --git a/xen/include/asm-arm/gic_v3_defs.h b/xen/include/asm-arm/gic_v3_defs.h
> index f8bac52..125fc28 100644
> --- a/xen/include/asm-arm/gic_v3_defs.h
> +++ b/xen/include/asm-arm/gic_v3_defs.h
> @@ -45,6 +45,7 @@
>  #define GICC_SRE_EL2_DIB             (1UL << 2)
>  #define GICC_SRE_EL2_ENEL1           (1UL << 3)
>  
> +#define GICR_CTL_ENABLE              (1U << 0)

This definition is misplaced...

>  /* Additional bits in GICD_TYPER defined by GICv3 */
>  #define GICD_TYPE_ID_BITS_SHIFT 19
>  
> @@ -133,6 +134,7 @@
>  
>  #define GICR_TYPER_PLPIS             (1U << 0)
>  #define GICR_TYPER_VLPIS             (1U << 1)
> +#define GICR_TYPER_DISTRIBUTED_IMP   (1U << 3)
>  #define GICR_TYPER_LAST              (1U << 4)
>  
>  #define DEFAULT_PMR_VALUE            0xff
> 

Regards,

-- 
Julien Grall