lkml.org 
[lkml]   [2012]   [Dec]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH v2 2/5] KVM: MMU: adjust page size early if gfn used as page table
    On 12/12/2012 08:57 AM, Marcelo Tosatti wrote:
    > On Mon, Dec 10, 2012 at 05:13:03PM +0800, Xiao Guangrong wrote:
    >> We have two issues in current code:
    >> - if target gfn is used as its page table, guest will refault then kvm will use
    >> small page size to map it. We need two #PF to fix its shadow page table
    >>
    >> - sometimes, say a exception is triggered during vm-exit caused by #PF
    >> (see handle_exception() in vmx.c), we remove all the shadow pages shadowed
    >> by the target gfn before go into page fault path, it will cause infinite
    >> loop:
    >> delete shadow pages shadowed by the gfn -> try to use large page size to map
    >> the gfn -> retry the access ->...
    >>
    >> To fix these, We can adjust page size early if the target gfn is used as page
    >> table
    >>
    >> Signed-off-by: Xiao Guangrong <xiaoguangrong@linux.vnet.ibm.com>
    >> ---
    >> arch/x86/kvm/mmu.c | 13 ++++---------
    >> arch/x86/kvm/paging_tmpl.h | 33 ++++++++++++++++++++++++++++++++-
    >> 2 files changed, 36 insertions(+), 10 deletions(-)
    >>
    >> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
    >> index 2a3c890..54fc61e 100644
    >> --- a/arch/x86/kvm/mmu.c
    >> +++ b/arch/x86/kvm/mmu.c
    >> @@ -2380,15 +2380,10 @@ static int set_spte(struct kvm_vcpu *vcpu, u64 *sptep,
    >> if (pte_access & ACC_WRITE_MASK) {
    >>
    >> /*
    >> - * There are two cases:
    >> - * - the one is other vcpu creates new sp in the window
    >> - * between mapping_level() and acquiring mmu-lock.
    >> - * - the another case is the new sp is created by itself
    >> - * (page-fault path) when guest uses the target gfn as
    >> - * its page table.
    >> - * Both of these cases can be fixed by allowing guest to
    >> - * retry the access, it will refault, then we can establish
    >> - * the mapping by using small page.
    >> + * Other vcpu creates new sp in the window between
    >> + * mapping_level() and acquiring mmu-lock. We can
    >> + * allow guest to retry the access, the mapping can
    >> + * be fixed if guest refault.
    >> */
    >> if (level > PT_PAGE_TABLE_LEVEL &&
    >> has_wrprotected_page(vcpu->kvm, gfn, level))
    >> diff --git a/arch/x86/kvm/paging_tmpl.h b/arch/x86/kvm/paging_tmpl.h
    >> index ec481e9..32d77ff 100644
    >> --- a/arch/x86/kvm/paging_tmpl.h
    >> +++ b/arch/x86/kvm/paging_tmpl.h
    >> @@ -491,6 +491,36 @@ out_gpte_changed:
    >> return 0;
    >> }
    >>
    >> + /*
    >> + * To see whether the mapped gfn can write its page table in the current
    >> + * mapping.
    >> + *
    >> + * It is the helper function of FNAME(page_fault). When guest uses large page
    >> + * size to map the writable gfn which is used as current page table, we should
    >> + * force kvm to use small page size to map it because new shadow page will be
    >> + * created when kvm establishes shadow page table that stop kvm using large
    >> + * page size. Do it early can avoid unnecessary #PF and emulation.
    >> + *
    >> + * Note: the PDPT page table is not checked for PAE-32 bit guest. It is ok
    >> + * since the PDPT is always shadowed, that means, we can not use large page
    >> + * size to map the gfn which is used as PDPT.
    >> + */
    >> +static bool
    >> +FNAME(mapped_gfn_can_write_current_pagetable)(struct guest_walker *walker)
    >> +{
    >> + int level;
    >> + gfn_t mask = ~(KVM_PAGES_PER_HPAGE(walker->level) - 1);
    >> +
    >> + if (!(walker->pte_access & ACC_WRITE_MASK))
    >> + return false;
    >> +
    >> + for (level = walker->level; level <= walker->max_level; level++)
    >> + if (!((walker->gfn ^ walker->table_gfn[level - 1]) & mask))
    >> + return true;
    >
    > XOR won't work. Just check with sums and integer comparison, ie.
    > walker->gfn + KVM_PAGES_PER_HPAGE(walker->level).

    It can not work since walker->gfn is not large-page-size aligned. For example,
    guest uses large page size to map 0x123000000 to physical address 0-2M, if
    guest faults on 0x123001000, walker->gfn = 0x1000.

    The code "if (!((walker->gfn ^ walker->table_gfn[level - 1]) & mask))" is the
    same as "if (walker->gfn & mask == walker->table_gfn[level - 1] & mask)" - if
    any page in the large page area used as page table, we should use 4K page size
    to fix it.

    In above example, if table_gfn is in the area [0, 2M), kvm is forced to use
    4k page size.

    >
    > Moreover, its confusing to have it checked at this level. What about
    > doing at reexecute_instruction?

    Hmm, this patch is trying to fix a bug described in the changelog:

    ======
    - sometimes, say a exception is triggered during vm-exit caused by #PF
    (see handle_exception() in vmx.c), we remove all the shadow pages shadowed
    by the target gfn before go into page fault path, it will cause infinite
    loop:
    delete shadow pages shadowed by the gfn -> try to use large page size to map
    the gfn -> retry the access ->...
    ======

    Which is caused by this code:

    if (is_page_fault(intr_info)) {
    /* EPT won't cause page fault directly */
    BUG_ON(enable_ept);
    cr2 = vmcs_readl(EXIT_QUALIFICATION);
    trace_kvm_page_fault(cr2, error_code);

    if (kvm_event_needs_reinjection(vcpu))
    kvm_mmu_unprotect_page_virt(vcpu, cr2);
    return kvm_mmu_page_fault(vcpu, cr2, error_code, NULL, 0);
    }

    This bug is introduced in commit c219346325.

    Another way to fix it is doing this change:
    @@ -2395,7 +2395,7 @@ static int set_spte(struct kvm_vcpu *vcpu, u64 *sptep,
    */
    if (level > PT_PAGE_TABLE_LEVEL &&
    has_wrprotected_page(vcpu->kvm, gfn, level))
    - goto done;
    + return 1;

    The disadvantage of this way is, it causes unnecessary emulation. For example,
    if 0-2M is mapped in guest and only page 0 used as page table, any write to
    [4K, 2M) will need be emulated.

    Your idea?


    \
     
     \ /
      Last update: 2012-12-12 20:41    [W:2.461 / U:0.028 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site