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@xxxxxxxxxx>
>
> 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.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
