From mboxrd@z Thu Jan 1 00:00:00 1970 From: Radim =?utf-8?B?S3LEjW3DocWZ?= Subject: Re: [PATCH 5/6] KVM: x86: Wrong assertion on paging_tmpl.h Date: Wed, 1 Oct 2014 19:54:16 +0200 Message-ID: <20141001175416.GC12083@potion.brq.redhat.com> References: <1412099359-5316-1-git-send-email-namit@cs.technion.ac.il> <1412099359-5316-6-git-send-email-namit@cs.technion.ac.il> <20141001162603.GE12085@potion.brq.redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: Nadav Amit , Paolo Bonzini , kvm@vger.kernel.org To: Nadav Amit Return-path: Received: from mx1.redhat.com ([209.132.183.28]:47403 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753488AbaJARyZ (ORCPT ); Wed, 1 Oct 2014 13:54:25 -0400 Content-Disposition: inline In-Reply-To: Sender: kvm-owner@vger.kernel.org List-ID: 2014-10-01 20:14+0300, Nadav Amit: > On Oct 1, 2014, at 7:26 PM, Radim Kr=C4=8Dm=C3=A1=C5=99 wrote: > > 2014-09-30 20:49+0300, Nadav Amit: > >> Even after the recent fix, the assertion on paging_tmpl.h is trigg= ered. > >> Apparently, the assertion wants to check that the PAE is always se= t 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. > >=20 > > I think it was only supposed to be used together with > > (vcpu->cr3 & CR3_NONPAE_RESERVED_BITS) =3D=3D 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. > >=20 > > I'd remove the new assert, it does not nothing useful, but is corre= ct > > Reviewed-by: Radim Kr=C4=8Dm=C3=A1=C5=99 > >=20 > >> - ASSERT(!is_long_mode(vcpu) && is_pae(vcpu)); > >> + ASSERT(!is_long_mode(vcpu) || is_pae(vcpu)); >=20 > I am ok with removing the assertion. Due to the multiple changes, I l= ost 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 rese= rved 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.