From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jan Kiszka Subject: Re: [PATCH 2/2] KVM: MMU: Keep going on permission error Date: Wed, 07 Jul 2010 14:33:17 +0200 Message-ID: <4C34740D.2070504@siemens.com> References: <1278424176-15310-1-git-send-email-avi@redhat.com> <1278424176-15310-3-git-send-email-avi@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: kvm@vger.kernel.org, Marcelo Tosatti To: Avi Kivity Return-path: Received: from goliath.siemens.de ([192.35.17.28]:23736 "EHLO goliath.siemens.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752838Ab0GGMdb (ORCPT ); Wed, 7 Jul 2010 08:33:31 -0400 In-Reply-To: <1278424176-15310-3-git-send-email-avi@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: Avi Kivity wrote: > Real hardware disregards permission errors when computing page fault = error > code bit 0 (page present). Do the same. >=20 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 =E2=80=98paging32_walk_ad= dr=E2=80=99: /data/kvm-kmod/x86/paging_tmpl.h:122: warning: =E2=80=98pte_access=E2=80= =99 may be used uninitialized in this function /data/kvm-kmod/x86/paging_tmpl.h: In function =E2=80=98paging64_walk_ad= dr=E2=80=99: /data/kvm-kmod/x86/paging_tmpl.h:122: warning: =E2=80=98pte_access=E2=80= =99 may be used uninitialized in this function > Signed-off-by: Avi Kivity > --- > arch/x86/kvm/paging_tmpl.h | 50 +++++++++++++++++++++++++---------= --------- > 1 files changed, 29 insertions(+), 21 deletions(-) >=20 > 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 =3D 0; > + bool eperm, present, rsvd_fault; > =20 > trace_kvm_mmu_pagetable_walk(addr, write_fault, user_fault, > fetch_fault); > walk: > + present =3D true; > + eperm =3D rsvd_fault =3D false; > walker->level =3D vcpu->arch.mmu.root_level; > pte =3D vcpu->arch.cr3; > #if PTTYPE =3D=3D 64 > if (!is_long_mode(vcpu)) { > pte =3D 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 =3D false; > + goto error; > + } > --walker->level; > } > #endif > @@ -151,31 +155,36 @@ walk: > walker->table_gfn[walker->level - 1] =3D table_gfn; > walker->pte_gpa[walker->level - 1] =3D pte_gpa; > =20 > - 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 =3D false; > + break; > + } > =20 > trace_kvm_mmu_paging_element(pte, walker->level); > =20 > - if (!is_present_gpte(pte)) > - goto not_present; > + if (!is_present_gpte(pte)) { > + present =3D false; > + break; goto error? > + } > =20 > - rsvd_fault =3D 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 =3D true; > + break; goto error? > + } > =20 > if (write_fault && !is_writable_pte(pte)) > if (user_fault || is_write_protection(vcpu)) > - goto access_error; > + eperm =3D true; > =20 > if (user_fault && !(pte & PT_USER_MASK)) > - goto access_error; > + eperm =3D true; > =20 > #if PTTYPE =3D=3D 64 > if (fetch_fault && (pte & PT64_NX_MASK)) > - goto access_error; > + eperm =3D true; > #endif > =20 > - if (!(pte & PT_ACCESSED_MASK)) { > + if (!eperm && !rsvd_fault && !(pte & PT_ACCESSED_MASK)) { We should never get here with rsvd_fault =3D=3D 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; > } > =20 > + 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; > =20 > @@ -233,14 +245,10 @@ walk: > __func__, (u64)pte, pte_access, pt_access); > return 1; > =20 > -not_present: > +error: > walker->error_code =3D 0; > - goto err; > - > -access_error: > - walker->error_code =3D PFERR_PRESENT_MASK; > - > -err: > + if (present) > + walker->error_code |=3D PFERR_PRESENT_MASK; > if (write_fault) > walker->error_code |=3D PFERR_WRITE_MASK; > if (user_fault) --=20 Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux