Re: [PATCH v9 12/28] ARM: vGIC: advertise LPI support

[Andre Przywara <andre.przywara@arm.com>]

Hi,

On 16/05/17 14:03, Julien Grall wrote:
> Hi Andre,
> 
> On 11/05/17 18:53, 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.
> 
> s/GICR_TYPE/GICR_TYPER/
> 
> Also, I think it would be worth explaining that you populate
> GICR_TYPER.Process_Number because the ITS will use it later on.
> 
>> Advertise 24 bits worth of LPIs to the guest.
> 
> Again this is not valid anymore. You said you would drop it on the
> previous version. So why it has not been done?
> 
>>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>> ---
>>  xen/arch/arm/vgic-v3.c | 70
>> ++++++++++++++++++++++++++++++++++++++++++++++----
>>  1 file changed, 65 insertions(+), 5 deletions(-)
>>
>> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
>> index 38c123c..6dbdb2e 100644
>> --- a/xen/arch/arm/vgic-v3.c
>> +++ b/xen/arch/arm/vgic-v3.c
>> @@ -170,8 +170,19 @@ 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;
>> +    {
>> +        unsigned long flags;
>> +
>> +        if ( !v->domain->arch.vgic.has_its )
>> +            goto read_as_zero_32;
>> +        if ( dabt.size != DABT_WORD ) goto bad_width;
>> +
>> +        spin_lock_irqsave(&v->arch.vgic.lock, flags);
>> +        *r = vgic_reg32_extract(!!(v->arch.vgic.flags &
>> VGIC_V3_LPIS_ENABLED),
>> +                                info);
>> +        spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
>> +        return 1;
>> +    }
>>
>>      case VREG32(GICR_IIDR):
>>          if ( dabt.size != DABT_WORD ) goto bad_width;
>> @@ -183,16 +194,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;
> 
> Why the mask here? This sound like a bug to me if vcpu_id does not fit
> it and you would make it worst by the mask.
> 
> But this is already addressed by max_vcpus in the vgic_ops. So please
> drop the pointless mask.
> 
> Lastly, I would have expected to try to address my remark everywhere
> regarding hardcoding offset. In this case,

Fixed.

>>
>>          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;
>> @@ -426,6 +441,28 @@ 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);
>> +
>> +    /* rdists_enabled is protected by the domain lock. */
>> +    ASSERT(spin_is_locked(&v->domain->arch.vgic.lock));
>> +
>> +    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)
>> @@ -436,8 +473,26 @@ 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;
>> +    {
>> +        unsigned long flags;
>> +
>> +        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 */
> 
> Getting back to the locking. I don't see any place where we get the
> domain vgic lock before vCPU vgic lock.

Because that seems to be the natural locking order, given that one
domain can have multiple VCPUs: We take the domain lock first, then the
VCPU lock. This *seems* to be documented in
xen/include/asm-arm/domain.h, where it says in a comment next to the
domain lock:
================
* If both class of lock is required then this lock must be
* taken first. ....
================

> So this raises the question why
> this ordering and not moving this lock into vgic_vcpu_enable_lpis.

Do you see any issues with that?

> At least this require documentation in the code and explanation in the
> commit message.

In this case I would try to comment on this, but would refrain from
proper locking order documentation (where?) until the rework.

>> +        spin_lock_irqsave(&v->arch.vgic.lock, flags);
>> +
>> +        /* 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_irqrestore(&v->arch.vgic.lock, flags);
>> +        vgic_unlock(v);
>> +
>> +        return 1;
>> +    }
>>
>>      case VREG32(GICR_IIDR):
>>          /* RO */
>> @@ -1058,6 +1113,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 = v->domain->arch.vgic.intid_bits;
>> +        }
> 
> As I said on the previous version, I would have expected the field
> intid_bits to be used even if ITS is not enabled.
> 
> The current code make very difficult to understand the purpose of
> intid_bits and know it is only used when ITS is enabled.
> 
> intid_bits should correctly be initialized in vgic_v3_domain_init and
> directly used it.

OK, I changed this, removed the wrong irq_bits assignment above (this
number is independent from the number of actually implemented SPIs) and
always putting through the hardware value for Dom0 and "10" for DomUs
(to cover all SPIs, we don't need more right now for guests. And yes, I
added a TODO there as well ;-)

Cheers,
Andre.

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

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