Re: [Xen-devel] [PATCH v5 30/30] ARM: vGIC: advertise LPI support

[Andre Przywara <andre.przywara@xxxxxxx>]

Hi,

On 06/04/17 02:04, Stefano Stabellini wrote:
> On Thu, 6 Apr 2017, 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.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
>> ---
>>  xen/arch/arm/vgic-v3.c | 46 +++++++++++++++++++++++++++++++++++++++++-----
>>  1 file changed, 41 insertions(+), 5 deletions(-)
>>
>> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
>> index 3b01247..ba0e79f 100644
>> --- a/xen/arch/arm/vgic-v3.c
>> +++ b/xen/arch/arm/vgic-v3.c
>> @@ -168,8 +168,12 @@ 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 ( dabt.size != DABT_WORD ) goto bad_width;
>> +        spin_lock(&v->arch.vgic.lock);
>> +        *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;
>> @@ -181,16 +185,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;
>> @@ -411,6 +419,17 @@ 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) - LPI_OFFSET;
>> +
>> +    if ( !v->domain->arch.vgic.nr_lpis )
>> +        v->domain->arch.vgic.nr_lpis = nr_lpis;
> 
> What if nr_lpis was already set and the nr_lpis has changed?

I think this can never happen:
1) vgic.rdist_propbase is only writeable when the redistributor has not
been enabled yet:
        /* Writing PROPBASER with LPIs enabled is UNPREDICTABLE. */
        if ( !(v->arch.vgic.flags & VGIC_V3_LPIS_ENABLED) )
        {
            reg = v->domain->arch.vgic.rdist_propbase;
            vgic_reg64_update(&reg, r, info);
            reg = sanitize_propbaser(reg);
            v->domain->arch.vgic.rdist_propbase = reg;
        }
        ....
2) This function above can only be called once:
        ....
        spin_lock(&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);
        ....

Does that sound safe? Sorry if the architecture is a bit awkward in this
regard ;-)

Cheers,
Andre.

>> +    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)
>> @@ -421,8 +440,20 @@ 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 ( dabt.size != DABT_WORD ) goto bad_width;
>> +        if ( !v->domain->arch.vgic.has_its )
>> +            return 1;
>> +
>> +        spin_lock(&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);
>> +
>> +        return 1;
>>  
>>      case VREG32(GICR_IIDR):
>>          /* RO */
>> @@ -1032,6 +1063,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;
>> +        }
>>          typer |= (irq_bits - 1) << GICD_TYPE_ID_BITS_SHIFT;
>>  
>>          *r = vgic_reg32_extract(typer, info);
>> -- 
>> 2.8.2
>>

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