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

[André Przywara <andre.przywara@xxxxxxx>]

On 06/04/17 12:42, Julien Grall wrote:
> Hi,
> 
> On 06/04/17 11:21, Andre Przywara wrote:
>> 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;
> 
> This will be called until VGIC_V3_LPIS_ENABLED will be set for the vCPU.
> However rdist_propbase is part of struct domain. So ...
> 
>>         }
>>     ....
>> 2) This function above can only be called once:
> 
> ... as this function will be called multiple per domain (once per vCPU).
> You could end up having nr_lpis not in sync with propbase.
> 
>>     ....
>>         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 ;-)
> 
> I am afraid that this is not safe. Tĥis is fine to have propbase stored
> in the domain because the spec (8.11.9 in ARM IHI 0069C) says:
> 
> "Setting different values in different copies of GICR_PROPBASER on
> Redistributors that are required to use a common LPI Configuration table".
> 
> But you need to keep Xen internal state consistent.

Ah, the lovely GIC architecture. So technically GICR_PROPBASER is per
redistributor (so per VCPU in our case), that's why I was mechanically
using the VCPU lock. But as you rightly mention, the spec goes into
great details why it actually isn't ;-) So indeed I need to use a lock
per domain, also prevent changing propbaser once at least *one* VCPU has
enabled its redistributor. Looks like I need a counter.

Thanks for pointing this out!

Cheers,
Andre


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