Re: [PATCH 5/6] KVM: x86: Wrong assertion on paging_tmpl.h

["Radim Krčmář" <rkrcmar@redhat.com>]

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.