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

[Ayan Kumar Halder <ayankuma@xxxxxxx>]

Hi Julien,

On 26/10/2022 17:45, Julien Grall wrote:
> On 26/10/2022 16:06, Ayan Kumar Halder wrote:
> > > ... you also need to ensure that the writers are atomically setting 
> > > rdist_pendbase. Please correct if I am wrong, but the callers are 
> > > not using write_atomic(). So how does that work?
> > I think read_atomic()/write_atomic() may not be the correct approach 
> > for the following reasons :-
> > 1. __vgic_v3_rdistr_rd_mmio_read is a static function. So 'val' has a 
> > global lifetime. Thus, all the following three lines need to be 
> > protected from concurrent access.
> I don't understand this argument. 'static' means the function is not 
> exported. The local variables will still reside on the stack.
> So why does the use of 'val' needs to be protected with the lock?
Yes, you are correct. I was misunderstanding this as a static variable.

Also, I understood from Stefano that pre-emption does not occur in Xen. 
So there will be no context switch.
So, the only race is between __vgic_v3_rdistr_rd_mmio_read() and 
__vgic_v3_rdistr_rd_mmio_write() for reading/writing rdist_pendbase.
- Ayan

> >          val = read_atomic(&v->arch.vgic.rdist_pendbase);
> >          val &= ~GICR_PENDBASER_PTZ;      /* WO, reads as 0 */
> >
> >          /* If a context switch happens here, then the 'val' below 
> > may potentially be incorrect. */
> >          *r = vreg_reg64_extract(val, info);
> >
> > 2. The same holds true for 'reg' as well in 
> > __vgic_v3_rdistr_rd_mmio_write()
> >              reg = v->arch.vgic.rdist_pendbase;
> >              blah, blah
> >              v->arch.vgic.rdist_pendbase = reg;
> Same here.
>
> Cheers,