[PATCH 3/6] kvm: arm64: Convert kvm_set_s2pte_readonly() from inline asm to cmpxchg()

[Catalin Marinas]

Hi Christoffer,

On Tue, Aug 01, 2017 at 01:16:18PM +0200, Christoffer Dall wrote:
> On Tue, Jul 25, 2017 at 02:53:05PM +0100, Catalin Marinas wrote:
> > +	pteval_t old_pteval, pteval;
> > +
> > +	do {
> > +		pteval = old_pteval = READ_ONCE(pte_val(*pte));
> > +		pteval &= ~PTE_S2_RDWR;
> > +		pteval |= PTE_S2_RDONLY;
> > +	} while (cmpxchg_relaxed(&pte_val(*pte), old_pteval, pteval) !=
> > +		 old_pteval);
> 
> I'm wondering if the READ_ONCE for old_pteval is strictly necessary, or
> if that's really for the pteval.  Actually, I'm a little unsure whether
> this is equivalent to
> 
> 	old_pteval = READ_ONCE(pte_val(*pte));
> 	pteval = old_pteval;
> 
> or
> 
> 	old_pteval = READ_ONCE(pte_val(*pte));
> 	pteval = READ_ONCE(pte_val(*pte));
> 
> I think it's the former, which I also think is correct,

I think so too.

> but the reason
> I'm going down this road is that we have a use of cmpxchg() in the VGIC
> code, which doesn't use READ_ONCE for the old value (~
> vgic-mmio-v3.c:404), and I also found other occurences of this in the
> kernel, so I'm wondering if the VGIC code is broken or we're being
> overly careful here, or if this is necessary because hardware can update
> the value behind our backs in this case?

We use it because the compiler may decide it's short on registers and
instead of saving old_pteval on the stack it reads it again from memory
just before cmpxchg, so we would miss any update to *pte done by the
hardware. In practice, I've never seen (on arm64) gcc generating two
loads to *pte without READ_ONCE but maybe I haven't tried hard enough.

We should probably fix the VGIC code as well as a precaution, just in
case the compiler tries to get smarter in the future.

> In any case, for this patch:
> 
> Reviewed-by: Christoffer Dall <cdall at linaro.org>

Thanks.

-- 
Catalin