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

[Julien Grall <julien.grall@xxxxxxx>]

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.

Cheers,

--
Julien Grall

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