Re: [Xen-devel] [PATCH v6 22/36] ARM: vGIC: advertise LPI support

[Julien Grall <julien.grall@xxxxxxx>]

Hi Andre,

On 04/07/2017 06:32 PM, Andre Przywara wrote:
> To let a guest know about the availability of virtual LPIs, set the
> respective bits in the virtual GIC registers and let a guest control
> the LPI enable bit.
> Only report the LPI capability if the host has initialized at least
> one ITS.
> This removes a "TBD" comment, as we now populate the processor number
> in the GICR_TYPE register.
> Advertise 24 bits worth of LPIs to the guest.
Again, why 24 bits? This should be the number of LPIs supported by the host.

And likely this number should be set from vgic_v3_domain_init(....) and 
not hardcoded in the emulation.
> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
> ---
>  xen/arch/arm/vgic-v3.c | 59 +++++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 54 insertions(+), 5 deletions(-)
>
> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
> index 7b086b9..1c1d014 100644
> --- a/xen/arch/arm/vgic-v3.c
> +++ b/xen/arch/arm/vgic-v3.c
> @@ -169,8 +169,15 @@ static int __vgic_v3_rdistr_rd_mmio_read(struct vcpu *v, 
> mmio_info_t *info,
>      switch ( gicr_reg )
>      {
>      case VREG32(GICR_CTLR):
> -        /* We have not implemented LPI's, read zero */
> -        goto read_as_zero_32;
> +        if ( !v->domain->arch.vgic.has_its )
> +            goto read_as_zero_32;
> +        if ( dabt.size != DABT_WORD ) goto bad_width;
> +
> +        spin_lock(&v->arch.vgic.lock);
This lock is used in interrupt context (see vgic_vcpu_inject_irq). So 
you want to disable interrupt.
> +        *r = vgic_reg32_extract(!!(v->arch.vgic.flags & VGIC_V3_LPIS_ENABLED),
> +                                info);
> +        spin_unlock(&v->arch.vgic.lock);
> +        return 1;
>
>      case VREG32(GICR_IIDR):
>          if ( dabt.size != DABT_WORD ) goto bad_width;
> @@ -182,16 +189,20 @@ static int __vgic_v3_rdistr_rd_mmio_read(struct vcpu *v, 
> mmio_info_t *info,
>          uint64_t typer, aff;
>
>          if ( !vgic_reg64_check_access(dabt) ) goto bad_width;
> -        /* TBD: Update processor id in [23:8] when ITS support is added */
>          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);
>          typer = aff;
> +        /* We use the VCPU ID as the redistributor ID in bits[23:8] */
> +        typer |= (v->vcpu_id & 0xffff) << 8;
>
>          if ( v->arch.vgic.flags & VGIC_V3_RDIST_LAST )
>              typer |= GICR_TYPER_LAST;
>
> +        if ( v->domain->arch.vgic.has_its )
> +            typer |= GICR_TYPER_PLPIS;
> +
>          *r = vgic_reg64_extract(typer, info);
>
>          return 1;
> @@ -421,6 +432,25 @@ static uint64_t sanitize_pendbaser(uint64_t reg)
>      return reg;
>  }
>
> +static void vgic_vcpu_enable_lpis(struct vcpu *v)
> +{
> +    uint64_t reg = v->domain->arch.vgic.rdist_propbase;
> +    unsigned int nr_lpis = BIT((reg & 0x1f) + 1);
> +
> +    if ( nr_lpis < LPI_OFFSET )
> +        nr_lpis = 0;
> +    else
> +        nr_lpis -= LPI_OFFSET;
> +
> +    if ( !v->domain->arch.vgic.rdists_enabled )
> +    {
> +        v->domain->arch.vgic.nr_lpis = nr_lpis;
> +        v->domain->arch.vgic.rdists_enabled = true;
> +    }
> +
> +    v->arch.vgic.flags |= VGIC_V3_LPIS_ENABLED;
> +}
> +
>  static int __vgic_v3_rdistr_rd_mmio_write(struct vcpu *v, mmio_info_t *info,
>                                            uint32_t gicr_reg,
>                                            register_t r)
> @@ -431,8 +461,22 @@ static int __vgic_v3_rdistr_rd_mmio_write(struct vcpu *v, 
> mmio_info_t *info,
>      switch ( gicr_reg )
>      {
>      case VREG32(GICR_CTLR):
> -        /* LPI's not implemented */
> -        goto write_ignore_32;
> +        if ( !v->domain->arch.vgic.has_its )
> +            goto write_ignore_32;
> +        if ( dabt.size != DABT_WORD ) goto bad_width;
> +
> +        vgic_lock(v);                   /* protects rdists_enabled */
Can't you move this lock in vgic_vcpu_enable_lpis?

> +        spin_lock(&v->arch.vgic.lock);
See above for the v->arch.vgic.lock.

> +
> +        /* LPIs can only be enabled once, but never disabled again. */
> +        if ( (r & GICR_CTLR_ENABLE_LPIS) &&
> +             !(v->arch.vgic.flags & VGIC_V3_LPIS_ENABLED) )
> +            vgic_vcpu_enable_lpis(v);
> +
> +        spin_unlock(&v->arch.vgic.lock);
> +        vgic_unlock(v);
> +
> +        return 1;
>
>      case VREG32(GICR_IIDR):
>          /* RO */
> @@ -1049,6 +1093,11 @@ static int vgic_v3_distr_mmio_read(struct vcpu *v, 
> mmio_info_t *info,
>          typer = ((ncpus - 1) << GICD_TYPE_CPUS_SHIFT |
>                   DIV_ROUND_UP(v->domain->arch.vgic.nr_spis, 32));
>
> +        if ( v->domain->arch.vgic.has_its )
> +        {
> +            typer |= GICD_TYPE_LPIS;
> +            irq_bits = 24;
See my comment above about 24.

> +        }
>          typer |= (irq_bits - 1) << GICD_TYPE_ID_BITS_SHIFT;
>
>          *r = vgic_reg32_extract(typer, info);
Cheers,

--
Julien Grall

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