Avi Kivity wrote: > Real hardware disregards permission errors when computing page fault error > code bit 0 (page present). Do the same. > This generates (false positive) build warnings here: CC [M] /data/kvm-kmod/x86/coalesced_mmio.o /data/kvm-kmod/x86/paging_tmpl.h: In function ‘paging32_walk_addr’: /data/kvm-kmod/x86/paging_tmpl.h:122: warning: ‘pte_access’ may be used uninitialized in this function /data/kvm-kmod/x86/paging_tmpl.h: In function ‘paging64_walk_addr’: /data/kvm-kmod/x86/paging_tmpl.h:122: warning: ‘pte_access’ may be used uninitialized in this function > Signed-off-by: Avi Kivity <avi@xxxxxxxxxx> > --- > arch/x86/kvm/paging_tmpl.h | 50 +++++++++++++++++++++++++------------------ > 1 files changed, 29 insertions(+), 21 deletions(-) > > diff --git a/arch/x86/kvm/paging_tmpl.h b/arch/x86/kvm/paging_tmpl.h > index 56c7f4f..1bbeffc 100644 > --- a/arch/x86/kvm/paging_tmpl.h > +++ b/arch/x86/kvm/paging_tmpl.h > @@ -121,19 +121,23 @@ static int FNAME(walk_addr)(struct guest_walker *walker, > gfn_t table_gfn; > unsigned index, pt_access, pte_access; > gpa_t pte_gpa; > - int rsvd_fault = 0; > + bool eperm, present, rsvd_fault; > > trace_kvm_mmu_pagetable_walk(addr, write_fault, user_fault, > fetch_fault); > walk: > + present = true; > + eperm = rsvd_fault = false; > walker->level = vcpu->arch.mmu.root_level; > pte = vcpu->arch.cr3; > #if PTTYPE == 64 > if (!is_long_mode(vcpu)) { > pte = kvm_pdptr_read(vcpu, (addr >> 30) & 3); > trace_kvm_mmu_paging_element(pte, walker->level); > - if (!is_present_gpte(pte)) > - goto not_present; > + if (!is_present_gpte(pte)) { > + present = false; > + goto error; > + } > --walker->level; > } > #endif > @@ -151,31 +155,36 @@ walk: > walker->table_gfn[walker->level - 1] = table_gfn; > walker->pte_gpa[walker->level - 1] = pte_gpa; > > - if (kvm_read_guest(vcpu->kvm, pte_gpa, &pte, sizeof(pte))) > - goto not_present; > + if (kvm_read_guest(vcpu->kvm, pte_gpa, &pte, sizeof(pte))) { > + present = false; > + break; > + } > > trace_kvm_mmu_paging_element(pte, walker->level); > > - if (!is_present_gpte(pte)) > - goto not_present; > + if (!is_present_gpte(pte)) { > + present = false; > + break; goto error? > + } > > - rsvd_fault = is_rsvd_bits_set(vcpu, pte, walker->level); > - if (rsvd_fault) > - goto access_error; > + if (is_rsvd_bits_set(vcpu, pte, walker->level)) { > + rsvd_fault = true; > + break; goto error? > + } > > if (write_fault && !is_writable_pte(pte)) > if (user_fault || is_write_protection(vcpu)) > - goto access_error; > + eperm = true; > > if (user_fault && !(pte & PT_USER_MASK)) > - goto access_error; > + eperm = true; > > #if PTTYPE == 64 > if (fetch_fault && (pte & PT64_NX_MASK)) > - goto access_error; > + eperm = true; > #endif > > - if (!(pte & PT_ACCESSED_MASK)) { > + if (!eperm && !rsvd_fault && !(pte & PT_ACCESSED_MASK)) { We should never get here with rsvd_fault == true - redundant check? > trace_kvm_mmu_set_accessed_bit(table_gfn, index, > sizeof(pte)); > if (FNAME(cmpxchg_gpte)(vcpu->kvm, table_gfn, > @@ -214,6 +223,9 @@ walk: > --walker->level; > } > > + if (!present || eperm || rsvd_fault) > + goto error; > + We would only need to check for eperm here if we jumped above. Jan > if (write_fault && !is_dirty_gpte(pte)) { > bool ret; > > @@ -233,14 +245,10 @@ walk: > __func__, (u64)pte, pte_access, pt_access); > return 1; > > -not_present: > +error: > walker->error_code = 0; > - goto err; > - > -access_error: > - walker->error_code = PFERR_PRESENT_MASK; > - > -err: > + if (present) > + walker->error_code |= PFERR_PRESENT_MASK; > if (write_fault) > walker->error_code |= PFERR_WRITE_MASK; > if (user_fault) -- Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux -- 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