Re: [XEN v4] xen/arm: vGICv3: Emulate properly 32-bit access on GICR_PENDBASER

[Ayan Kumar Halder <ayankuma@xxxxxxx>]

On 27/10/2022 10:44, Andre Przywara wrote:
> On Wed, 26 Oct 2022 19:30:04 +0100
> Ayan Kumar Halder <ayankuma@xxxxxxx> wrote:
>
> Hi,
Hi Andre,

Thanks for the feedback. Just one clarification.

> > If a guest is running in 32 bit mode and it tries to access
> > "GICR_PENDBASER + 4" mmio reg, it will be trapped to Xen. vreg_reg64_extract()
> > will return the value stored "v->arch.vgic.rdist_pendbase + 4".
> > This will be stored in a 64bit cpu register.
> > So now we have the top 32 bits of GICR_PENDBASER (a 64 bit MMIO register) stored
> > in the lower 32 bits of the 64bit cpu register.
> >
> > This 64bit cpu register is then modified bitwise with a mask (ie
> > GICR_PENDBASER_PTZ, it clears the 62nd bit). But the PTZ (which is bit 30 in the
> > 64 bit cpu register) is not cleared as expected by the specification.
> >
> > The correct thing to do here is to store the value of
> > "v->arch.vgic.rdist_pendbase" in a temporary 64 bit variable. This variable is
> > then modified bitwise with GICR_PENDBASER_PTZ mask. It is then passed to
> > vreg_reg64_extract() which will extract 32 bits from the given offset.
> >
> > Also, we have removed spin_lock_irqsave()/spin_unlock_irqrestore() to protect
> > v->arch.vgic.rdist_pendbase. The reason being v->arch.vgic.rdist_pendbase is
> > now being read/written in an atomic manner (using read_atomic()/write_atomic()).
> >
> > Signed-off-by: Ayan Kumar Halder <ayankuma@xxxxxxx>
> > Release-acked-by: Henry Wang <Henry.Wang@xxxxxxx>
> > ---
> >
> > Changes from:-
> >
> > v1 - 1. Extracted this fix from "[RFC PATCH v1 05/12] Arm: GICv3: Emulate
> > GICR_PENDBASER and GICR_PROPBASER on AArch32" into a separate patch with an
> > appropriate commit message.
> >
> > v2 - 1. Removed spin_lock_irqsave(). Used read_atomic() to read
> > v->arch.vgic.rdist_pendbase in an atomic context.
> > 2. Rectified the commit message to state that the cpu register is 64 bit.
> > (because currently, GICv3 is supported on Arm64 only). Reworded to make it
> > clear.
> >
> > v3 - 1. Added read_atomic()/write_atomic() for access to 
> > v->arch.vgic.rdist_pendbase
> > in __vgic_v3_rdistr_rd_mmio_write().
> >
> >   xen/arch/arm/vgic-v3.c | 19 ++++++-------------
> >   1 file changed, 6 insertions(+), 13 deletions(-)
> >
> > diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
> > index 0c23f6df9d..1adbdc0e54 100644
> > --- a/xen/arch/arm/vgic-v3.c
> > +++ b/xen/arch/arm/vgic-v3.c
> > @@ -249,16 +249,15 @@ static int __vgic_v3_rdistr_rd_mmio_read(struct vcpu *v, 
> > mmio_info_t *info,
> >   
> >       case VREG64(GICR_PENDBASER):
> >       {
> > -        unsigned long flags;
> > +        uint64_t val;
> >   
> >           if ( !v->domain->arch.vgic.has_its )
> >               goto read_as_zero_64;
> >           if ( !vgic_reg64_check_access(dabt) ) goto bad_width;
> >   
> > -        spin_lock_irqsave(&v->arch.vgic.lock, flags);
> > -        *r = vreg_reg64_extract(v->arch.vgic.rdist_pendbase, info);
> > -        *r &= ~GICR_PENDBASER_PTZ;       /* WO, reads as 0 */
> > -        spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
> > +        val = read_atomic(&v->arch.vgic.rdist_pendbase);
> > +        val &= ~GICR_PENDBASER_PTZ;      /* WO, reads as 0 */
> > +        *r = vreg_reg64_extract(val, info);
> That part looks fine now.
>
> >           return 1;
> >       }
> >   
> > @@ -566,25 +565,19 @@ static int __vgic_v3_rdistr_rd_mmio_write(struct vcpu *v, mmio_info_t *info,
> >   
> >       case VREG64(GICR_PENDBASER):
> >       {
> > -        unsigned long flags;
> > -
> >           if ( !v->domain->arch.vgic.has_its )
> >               goto write_ignore_64;
> >           if ( !vgic_reg64_check_access(dabt) ) goto bad_width;
> >   
> > -        spin_lock_irqsave(&v->arch.vgic.lock, flags);
> > -
> I don't think you can drop the lock here easily. If it would be just for
> the LPIs enabled check, that'd be fine, because you can never turn LPIs off
> again (but that would deserve an explicit comment then).
/* LPIs once enabled (ie via vgic_vcpu_enable_lpis()) are never disabled 
by Xen */
Let me know if this comment makes sense.

> But down below you do a read-modify-write operation of rdist_pendbase, and
> need to make sure no one else is attempting that at the same time.
AFAIU, Xen is non preemptable. So there cannot be a context switch 
between read-modify-write.
But I think you have a valid point.

If two callers are trying to read-modify-write on rdist_pendbase at the 
same time, it is possible that caller2 reads a stale value of 
rdist_pendbase (just before caller1 writes). So caller2 might program 
rdist_pendbase in such a way that caller1's update is gone.
I will fix this in v5.

- Ayan

> Cheers,
> Andre
>
> >           /* Writing PENDBASER with LPIs enabled is UNPREDICTABLE. */
> >           if ( !(v->arch.vgic.flags & VGIC_V3_LPIS_ENABLED) )
> >           {
> > -            reg = v->arch.vgic.rdist_pendbase;
> > +            reg = read_atomic(&v->arch.vgic.rdist_pendbase);
> >               vreg_reg64_update(&reg, r, info);
> >               reg = sanitize_pendbaser(reg);
> > -            v->arch.vgic.rdist_pendbase = reg;
> > +            write_atomic(&v->arch.vgic.rdist_pendbase, reg);
> >           }
> >   
> > -        spin_unlock_irqrestore(&v->arch.vgic.lock, false);
> > -
> >           return 1;
> >       }
> >