Re: [PATCH unit-tests] Add async page fault test

[Gleb Natapov <gleb@redhat.com>]

On Wed, May 09, 2012 at 11:29:11AM +0300, Avi Kivity wrote:
> On 05/08/2012 02:24 PM, Gleb Natapov wrote:
> > Signed-off-by: Gleb Natapov <gleb@redhat.com>
> 
> Please describe the regression you're testing for.  We could even link
> it to the fix with the commit hash.
> 
The test does not really tests for a regression, because there wasn't
one. It test that prefault does not generates spurious #PFs. Will write
that.

> 
> >  void vfree(void *mem)
> >  {
> >      unsigned long size = ((unsigned long *)mem)[-1];
> > diff --git a/lib/x86/vm.h b/lib/x86/vm.h
> > index 71ab4a8..ff4842f 100644
> > --- a/lib/x86/vm.h
> > +++ b/lib/x86/vm.h
> > @@ -22,6 +22,7 @@ void vfree(void *mem);
> >  void *vmap(unsigned long long phys, unsigned long size);
> >  void *alloc_vpage(void);
> >  void *alloc_vpages(ulong nr);
> > +unsigned long virt_to_phys_cr3(void *mem);
> 
> uint64_t.
virt_to_phys() also unsigned long. And get_pte() that virt_to_phys_cr3()
uses also. I guess the code is not ready for more then 2^32 memory in
32bit VM.

> 
> > @@ -0,0 +1,98 @@
> > +/*
> > + * Async PF test. For the test to actually do anywathing it ineeds to be started
> > + * in memory cgroup with 512M of memory and with more then 1G memory provided 
> > + * to the guest.
> > + */
> 
> Please include instructions or a script on how to do that.
> 
OK.

> Alterative ways of doing this:
> - file-backed memory using FUSE to control paging
Not sure how that can be done.

> - add madvise(MADV_DONTNEED) support to testdev, and have the guest
> trigger page-in itself.
MADV_DONTNEED will drop page, not swap it out.

> 
> I'm not asking to change this test, just providing ideas for the future
> in case fine-grained control is needed.  It also doesn't thrash the disk.
> 
> > +#include "x86/msr.h"
> > +#include "x86/processor.h"
> > +#include "x86/apic-defs.h"
> > +#include "x86/apic.h"
> > +#include "x86/desc.h"
> > +#include "x86/isr.h"
> > +#include "x86/vm.h"
> > +
> > +#include "libcflat.h"
> > +#include <stdint.h>
> > +
> > +#define KVM_PV_REASON_PAGE_NOT_PRESENT 1
> > +#define KVM_PV_REASON_PAGE_READY 2
> > +
> > +#define MSR_KVM_ASYNC_PF_EN 0x4b564d02
> > +
> > +#define KVM_ASYNC_PF_ENABLED                    (1 << 0)
> > +#define KVM_ASYNC_PF_SEND_ALWAYS                (1 << 1)
> > +
> > +volatile uint32_t apf_reason __attribute__((aligned(64)));
> > +char *buf;
> > +volatile uint64_t  i;
> > +volatile unsigned long phys;
> 
> uint64_t.
> 
> > +
> > +static void pf_isr(struct ex_regs *r)
> > +{
> > +	void* virt = (void*)((ulong)(buf+i) & ~4095ul);
> > +
> > +	switch (get_apf_reason()) {
> > +		case 0:
> 
> default:
I'd rather make deafult: fail the test. It shouldn't happen.

> 
> > +			printf("unexpected #PF at %p\n", read_cr2());
> > +			fail = true;
> > +			break;
> > +		case KVM_PV_REASON_PAGE_NOT_PRESENT:
> > +			phys = virt_to_phys_cr3(virt);
> > +			install_pte(phys_to_virt(read_cr3()), 1, virt, phys, 0);
> > +			write_cr3(read_cr3());
> 
> What's the point of these?
> 
Shouldn't we reload page tables after changing them?

> > +			printf("Got not present #PF token %x virt addr %p phys addr %p\n", read_cr2(), virt, phys);
> > +			while(phys) {
> > +				irq_enable();
> > +				halt();
> 
> Racy... you need safe_halt() here.
The code generated is sti; hlt; cli. By I as well may add safe_halt() to
do sti; hlt explicit.

> 
> > +				irq_disable();
> > +			}
> > +			break;
> > +		case KVM_PV_REASON_PAGE_READY:
> > +			printf("Got present #PF token %x\n", read_cr2());
> > +			if ((uint32_t)read_cr2() == ~0)
> > +				break;
> > +			install_pte(phys_to_virt(read_cr3()), 1, virt, phys | PTE_PRESENT | PTE_WRITE, 0);
> > +			write_cr3(read_cr3());
> > +			phys = 0;
> > +			break;
> > +	}
> > +}
> > +
> >
> 
> -- 
> error compiling committee.c: too many arguments to function

--
			Gleb.