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

[Ian Campbell <ian.campbell@citrix.com>]

On Fri, 2015-07-10 at 13:12 +0530, vijay.kilari@gmail.com wrote:
> [...]

> +int vits_unmap_lpi_prop(struct vcpu *v)

Why is this function called "unmap"?

> +    /* Register mmio handlers for this region */
> +    register_mmio_handler(v->domain, &vgic_gits_lpi_mmio_handler,
> +                          maddr, lpi_size);

Something, somewhere, must ensure that we never register this MMIO
handler for any guest which does not have a VITS associated with it.

As it stands after this series I believe that means that the GITS_*
register handling, the GITS LPI MMIO region and the GITS_TRANSLATER
mapping should be setup for dom0 if-and-only-if a physical ITS is
present in the system. 

In particular they should never, as it stands today, be mapped for a
domU.

I am unable to spot the code which arranges all that. There is some if's
in the GICR_* handlers, but I don't think that covers everything.

Obviously this will change with the addition of PCI passthrough later
on, but that change should be done in that series and not preemptively
here.
>      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 */

"Provide" and an extra space before "0".

> +            aff |= (v->vcpu_id << GICR_TYPER_PROCESSOR_SHIFT);
> +        }
>          *r = aff;
> -
>          if ( v->arch.vgic.flags & VGIC_V3_RDIST_LAST )
>              *r |= GICR_TYPER_LAST;
> -

Spurious whitespace changes.

>          return 1;
>      case GICR_STATUSR:
>          /* Not implemented */
> @@ -125,10 +142,21 @@ 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 */
> +        if ( gic_lpi_supported() )
> +        {
> +            if ( dabt.size != DABT_DOUBLE_WORD ) goto bad_width;
> +            /* Remove shareability attribute we don't want dom to flush */

This code doesn't appear to match the code, nothing is removing an
attribute here. Perhaps it belongs next to the place which initialises,
or handles writes to, v->domain->arch.vits->propbase?

> @@ -203,7 +231,18 @@ 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 */
> +        if ( gic_lpi_supported() )
> +        {
> +            /*
> +             * Enable LPI's for ITS. Direct injection of LPI
> +             * by writing to GICR_{SET,CLR}LPIR are not supported

"is not supported"

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

Extra space after the &.
> @@ -694,6 +755,14 @@ static int vgic_v3_distr_mmio_read(struct vcpu *v, mmio_info_t *info)
>          *r = ((ncpus - 1) << GICD_TYPE_CPUS_SHIFT |
>                DIV_ROUND_UP(v->domain->arch.vgic.nr_spis, 32));
>  
> +        if ( gic_lpi_supported() )
> +        {
> +            irq_bits = gic_nr_id_bits();
> +            *r |= GICD_TYPE_LPIS;
> +        }
> +        else
> +            irq_bits = get_count_order(vgic_num_irqs(v->domain));

I think gic_nr_id_bits should return the correct thing whether or not
LPIs are supported, i.e.

        if ( gic_lpi_supported() )
            *r |= GICD_TYPE_LPIS;
        irq_bits = gic_nr_id_bits();

should be sufficient.


> 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;

Wrong indentation

Ian.