[PATCH] kvm: arm64: Enable hardware updates of the Access Flag for Stage 2 page tables

[Christoffer Dall]

On Mon, May 09, 2016 at 05:50:56PM +0100, Catalin Marinas wrote:
> On Mon, May 09, 2016 at 05:33:10PM +0200, Christoffer Dall wrote:
> > On Wed, Apr 13, 2016 at 05:57:37PM +0100, Catalin Marinas wrote:
> > > The ARMv8.1 architecture extensions introduce support for hardware
> > > updates of the access and dirty information in page table entries. With
> > > VTCR_EL2.HA enabled (bit 21), when the CPU accesses an IPA with the
> > > PTE_AF bit cleared in the stage 2 page table, instead of raising an
> > > Access Flag fault to EL2 the CPU sets the actual page table entry bit
> > > (10). To ensure that kernel modifications to the page table do not
> > > inadvertently revert a bit set by hardware updates, certain Stage 2
> > > software pte/pmd operations must be performed atomically.
> > > 
> > > The main user of the AF bit is the kvm_age_hva() mechanism. The
> > > kvm_age_hva_handler() function performs a "test and clear young" action
> > > on the pte/pmd. This needs to be atomic in respect of automatic hardware
> > > updates of the AF bit. Since the AF bit is in the same position for both
> > > Stage 1 and Stage 2, the patch reuses the existing
> > > ptep_test_and_clear_young() functionality if
> > > __HAVE_ARCH_PTEP_TEST_AND_CLEAR_YOUNG is defined. Otherwise, the
> > > existing pte_young/pte_mkold mechanism is preserved.
> > > 
> > > The kvm_set_s2pte_readonly() (and the corresponding pmd equivalent) have
> > > to perform atomic modifications in order to avoid a race with updates of
> > > the AF bit. The arm64 implementation has been re-written using
> > > exclusives.
> > > 
> > > Currently, kvm_set_s2pte_writable() (and pmd equivalent) take a pointer
> > > argument and modify the pte/pmd in place. However, these functions are
> > > only used on local variables rather than actual page table entries, so
> > > it makes more sense to follow the pte_mkwrite() approach for stage 1
> > > attributes. The change to kvm_s2pte_mkwrite() makes it clear that these
> > > functions do not modify the actual page table entries.
> > 
> > so if one CPU takes a permission fault and is in the process of updating
> > the page table, what prevents another CPU from setting the access flag
> > (on a read, done by HW) on that entry between us reading the old pte and
> > replacing it with the new pte?
> 
> There isn't anything that would prevent this. However, as in the stage 1
> scenarios, explicitly writing a PTE as a result of a permission fault
> should also mark the entry as accessed (young, either as the default
> protection in PROT_DEFAULT or explicitly via pte_mkyoung).
> 
> > Don't we loose the AF information in that case too?
> 
> Since the hardware never clears the AF bit automatically, there is no
> information to lose in this race as long as the kvm_set_s2pte_writable()
> sets the AF bit. AFAICT, the PAGE_S2 and PAGE_S2_DEVICE definitions
> already use PROT_DEFAULT which has the AF bit set.

duh, yeah, I didn't consider the overall use case we're targeting.
Thanks for reminding me.

> 
> > >  static inline void kvm_set_s2pte_readonly(pte_t *pte)
> > >  {
> > > -	pte_val(*pte) = (pte_val(*pte) & ~PTE_S2_RDWR) | PTE_S2_RDONLY;
> > > +	pteval_t pteval;
> > > +	unsigned long tmp;
> > > +
> > > +	asm volatile("//	kvm_set_s2pte_readonly\n"
> > > +	"	prfm	pstl1strm, %2\n"
> > > +	"1:	ldxr	%0, %2\n"
> > > +	"	and	%0, %0, %3		// clear PTE_S2_RDWR\n"
> > > +	"	orr	%0, %0, %4		// set PTE_S2_RDONLY\n"
> > > +	"	stxr	%w1, %0, %2\n"
> > > +	"	cbnz	%w1, 1b\n"
> > > +	: "=&r" (pteval), "=&r" (tmp), "+Q" (pte_val(*pte))
> > 
> > so the +Q means "pass the memory address of the value and by the way the
> > content, not the address itself, can be updated by the assembly code" ?
> 
> Yes. I think "info gcc" isn't clear enough on this constraint (at least
> to me) but that's something we learnt from the tools guys in the early
> days of the aarch64 kernel port.
> 
Just finding anything about the meaning of this turned out to be its own
challenge.

In any case:

Reviewed-by: Christoffer Dall <christoffer.dall at linaro.org>

And I have applied it to kvmarm/next.

-Christoffer