| Message ID | 1347797235-20732-10-git-send-email-avi@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 09/16/2012 08:07 PM, Avi Kivity wrote: > + /* > + * On a write fault, fold the dirty bit into accessed_dirty by shifting it one > + * place right. > + * > + * On a read fault, do nothing. > + */ > + accessed_dirty &= pte >> (write_fault >> ilog2(PFERR_WRITE_MASK)); It is too trick: it depends on DIRTY_SHIFT = ACCESSED_SHIFT + 1. How about change it to: accessed_dirty &= pte >> (!!write_fault >> (DIRTY_SHIFT - ACCESSED_SHIFT)) ? Otherwise looks good to me. Reviewed-by: Xiao Guangrong <xiaoguangrong@linux.vnet.ibm.com> -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/18/2012 10:21 AM, Xiao Guangrong wrote: > On 09/16/2012 08:07 PM, Avi Kivity wrote: > >> + /* >> + * On a write fault, fold the dirty bit into accessed_dirty by shifting it one >> + * place right. >> + * >> + * On a read fault, do nothing. >> + */ >> + accessed_dirty &= pte >> (write_fault >> ilog2(PFERR_WRITE_MASK)); > > It is too trick: it depends on DIRTY_SHIFT = ACCESSED_SHIFT + 1. How about change it > to: > accessed_dirty &= pte >> (!!write_fault >> (DIRTY_SHIFT - ACCESSED_SHIFT)) ? !! forces a branch, unless gcc is really clever. So I changed it to shift = write_fault >> ilog2(PFERR_WRITE_MASK); shift *= PT_DIRTY_SHIFT - PT_ACCESSED_SHIFT; accessed_dirty &= pte >> shift; which should result in the same code.
diff --git a/arch/x86/kvm/paging_tmpl.h b/arch/x86/kvm/paging_tmpl.h index 95a64d1..5bb3d4f 100644 --- a/arch/x86/kvm/paging_tmpl.h +++ b/arch/x86/kvm/paging_tmpl.h @@ -151,7 +151,7 @@ static int FNAME(walk_addr_generic)(struct guest_walker *walker, pt_element_t pte; pt_element_t __user *uninitialized_var(ptep_user); gfn_t table_gfn; - unsigned index, pt_access, pte_access; + unsigned index, pt_access, pte_access, accessed_dirty; gpa_t pte_gpa; int offset; const int write_fault = access & PFERR_WRITE_MASK; @@ -180,6 +180,7 @@ retry_walk: ASSERT((!is_long_mode(vcpu) && is_pae(vcpu)) || (mmu->get_cr3(vcpu) & CR3_NONPAE_RESERVED_BITS) == 0); + accessed_dirty = PT_ACCESSED_MASK; pt_access = pte_access = ACC_ALL; ++walker->level; @@ -224,6 +225,7 @@ retry_walk: goto error; } + accessed_dirty &= pte; pte_access = pt_access & gpte_access(vcpu, pte); walker->ptes[walker->level - 1] = pte; @@ -251,11 +253,21 @@ retry_walk: if (!write_fault) protect_clean_gpte(&pte_access, pte); - ret = FNAME(update_accessed_dirty_bits)(vcpu, mmu, walker, write_fault); - if (unlikely(ret < 0)) - goto error; - else if (ret) - goto retry_walk; + /* + * On a write fault, fold the dirty bit into accessed_dirty by shifting it one + * place right. + * + * On a read fault, do nothing. + */ + accessed_dirty &= pte >> (write_fault >> ilog2(PFERR_WRITE_MASK)); + + if (unlikely(!accessed_dirty)) { + ret = FNAME(update_accessed_dirty_bits)(vcpu, mmu, walker, write_fault); + if (unlikely(ret < 0)) + goto error; + else if (ret) + goto retry_walk; + } walker->pt_access = pt_access; walker->pte_access = pte_access;
Keep track of accessed/dirty bits; if they are all set, do not enter the accessed/dirty update loop. Signed-off-by: Avi Kivity <avi@redhat.com> --- arch/x86/kvm/paging_tmpl.h | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-)