| Message ID | 1412099359-5316-6-git-send-email-namit@cs.technion.ac.il (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
2014-09-30 20:49+0300, Nadav Amit: > Even after the recent fix, the assertion on paging_tmpl.h is triggered. > Apparently, the assertion wants to check that the PAE is always set on > long-mode, but does it in incorrect way. Note that the assertion is not > enabled unless the code is debugged by defining MMU_DEBUG. I think it was only supposed to be used together with (vcpu->cr3 & CR3_NONPAE_RESERVED_BITS) == 0) to checked if CR3 does not contain ones where it shouldn't when in short mode without PAE, because SDM says the lower 12 bits of the address are assumed to be 0. and when we (incorrectly) removed the second part of condition, it started to bug. I'd remove the new assert, it does not nothing useful, but is correct Reviewed-by: Radim Kr?má? <rkrcmar@redhat.com> > - ASSERT(!is_long_mode(vcpu) && is_pae(vcpu)); > + ASSERT(!is_long_mode(vcpu) || is_pae(vcpu)); -- 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 Oct 1, 2014, at 7:26 PM, Radim Kr?má? <rkrcmar@redhat.com> wrote: > 2014-09-30 20:49+0300, Nadav Amit: >> Even after the recent fix, the assertion on paging_tmpl.h is triggered. >> Apparently, the assertion wants to check that the PAE is always set on >> long-mode, but does it in incorrect way. Note that the assertion is not >> enabled unless the code is debugged by defining MMU_DEBUG. > > I think it was only supposed to be used together with > (vcpu->cr3 & CR3_NONPAE_RESERVED_BITS) == 0) > to checked if CR3 does not contain ones where it shouldn't when in short > mode without PAE, because SDM says > the lower 12 bits of the address are assumed to be 0. > and when we (incorrectly) removed the second part of condition, it > started to bug. > > I'd remove the new assert, it does not nothing useful, but is correct > Reviewed-by: Radim Kr?má? <rkrcmar@redhat.com> > >> - ASSERT(!is_long_mode(vcpu) && is_pae(vcpu)); >> + ASSERT(!is_long_mode(vcpu) || is_pae(vcpu)); I am ok with removing the assertion. Due to the multiple changes, I lost track what it was supposed to do. Anyhow, removing the second part was required since there are no reserved bits in non-pae (they are ignored - not reserved). Nadav
2014-10-01 20:14+0300, Nadav Amit: > On Oct 1, 2014, at 7:26 PM, Radim Kr?má? <rkrcmar@redhat.com> wrote: > > 2014-09-30 20:49+0300, Nadav Amit: > >> Even after the recent fix, the assertion on paging_tmpl.h is triggered. > >> Apparently, the assertion wants to check that the PAE is always set on > >> long-mode, but does it in incorrect way. Note that the assertion is not > >> enabled unless the code is debugged by defining MMU_DEBUG. > > > > I think it was only supposed to be used together with > > (vcpu->cr3 & CR3_NONPAE_RESERVED_BITS) == 0) > > to checked if CR3 does not contain ones where it shouldn't when in short > > mode without PAE, because SDM says > > the lower 12 bits of the address are assumed to be 0. > > and when we (incorrectly) removed the second part of condition, it > > started to bug. > > > > I'd remove the new assert, it does not nothing useful, but is correct > > Reviewed-by: Radim Kr?má? <rkrcmar@redhat.com> > > > >> - ASSERT(!is_long_mode(vcpu) && is_pae(vcpu)); > >> + ASSERT(!is_long_mode(vcpu) || is_pae(vcpu)); > > I am ok with removing the assertion. Due to the multiple changes, I lost track what it was supposed to do. (It didn't say reserved when it was introduced and refactoring was done by different author.) > Anyhow, removing the second part was required since there are no reserved bits in non-pae (they are ignored - not reserved). Thanks, I thought that "assumed" is "shit will hit the fan unless", and that this assert made it instant and clear. -- 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
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Il 01/10/2014 19:14, Nadav Amit ha scritto: >>>>> + ASSERT(!is_long_mode(vcpu) || is_pae(vcpu)); > I am ok with removing the assertion. Due to the multiple changes, I > lost track what it was supposed to do. Anyhow, removing the second > part was required since there are no reserved bits in non-pae (they > are ignored - not reserved). It becomes a bit clearer if you apply De Morgan: ASSERT(!(is_long_mode(vcpu) && !is_pae(vcpu))); or almost equivalently WARN_ON(is_long_mode(vcpu) && !is_pae(vcpu)); We should change ASSERT to positive logic (MMU_WARN_ON for example). Paolo -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQEcBAEBAgAGBQJUNQEgAAoJEBRUblpOawnX9hwH/AghSII0PKLF1RDC9GqvKBb6 m+UYFmYxcJjidhsvcZuNg/pRuRfJYNOFoJWO13eTYUL/eSnxXEZqy1nQTneFUFjm WKFebVc5FWc8DAXpEehrHuKUn/QmOEDj8qo41Bf0kHiptzh6W1jIEjH1AaPIwta3 5MFmFN6T+BmU1pBqOgGY5OJkAQnM9WmnsjsFDRJEFW520GP1Xvws+XxRA31Q6Qol 1qLvK2kSeuCUlGDwNWTFT0w79wQwpwuXCJfII5vzRp02pVgDKtl6sNLyKKGthxtv ONPpn0Uq0mFRbxasPk8glwqaZtqRNJKG+jysSYEf3aBmMhP4hAcGoADP6C5Umj8= =7S50 -----END PGP SIGNATURE----- -- 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
diff --git a/arch/x86/kvm/paging_tmpl.h b/arch/x86/kvm/paging_tmpl.h index 806d58e..faf7298 100644 --- a/arch/x86/kvm/paging_tmpl.h +++ b/arch/x86/kvm/paging_tmpl.h @@ -298,7 +298,7 @@ retry_walk: } #endif walker->max_level = walker->level; - ASSERT(!is_long_mode(vcpu) && is_pae(vcpu)); + ASSERT(!is_long_mode(vcpu) || is_pae(vcpu)); accessed_dirty = PT_GUEST_ACCESSED_MASK; pt_access = pte_access = ACC_ALL;
Even after the recent fix, the assertion on paging_tmpl.h is triggered. Apparently, the assertion wants to check that the PAE is always set on long-mode, but does it in incorrect way. Note that the assertion is not enabled unless the code is debugged by defining MMU_DEBUG. Signed-off-by: Nadav Amit <namit@cs.technion.ac.il> --- arch/x86/kvm/paging_tmpl.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)