From mboxrd@z Thu Jan  1 00:00:00 1970
From: Gleb Natapov <gleb@redhat.com>
Subject: Re: [PATCH v4 06/13] nEPT: Add EPT tables support to paging_tmpl.h
Date: Tue, 30 Jul 2013 17:36:17 +0300
Message-ID: <20130730143616.GF25505@redhat.com>
References: <1374750001-28527-1-git-send-email-gleb@redhat.com>
 <1374750001-28527-7-git-send-email-gleb@redhat.com>
 <51F78F5B.7070405@redhat.com>
 <20130730115637.GC25505@redhat.com>
 <51F7ADF2.8020506@redhat.com>
 <20130730142232.GE25505@redhat.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: kvm@vger.kernel.org,
	Xiao Guangrong <xiaoguangrong@linux.vnet.ibm.com>,
	Jun Nakajima <jun.nakajima@Intel.com>,
	Yang Zhang <yang.z.zhang@Intel.com>
To: Paolo Bonzini <pbonzini@redhat.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mx1.redhat.com ([209.132.183.28]:59589 "EHLO mx1.redhat.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1750959Ab3G3Ogz (ORCPT <rfc822;kvm@vger.kernel.org>);
	Tue, 30 Jul 2013 10:36:55 -0400
Content-Disposition: inline
In-Reply-To: <20130730142232.GE25505@redhat.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

On Tue, Jul 30, 2013 at 05:22:32PM +0300, Gleb Natapov wrote:
> > >>
> > >> BUILD_BUG_ON(!!PT_GUEST_ACCESSED_MASK != !!PT_GUEST_DIRTY_MASK);
> > >> #if PT_GUEST_ACCESSED_MASK
> > >> BUILD_BUG_ON(PT_GUEST_DIRTY_SHIFT <= PT_GUEST_ACCESSED_SHIFT);
> > >> BUILD_BUG_ON(PT_GUEST_DIRTY_SHIFT <= PT_WRITABLE_SHIFT);
> > >> BUILD_BUG_ON(PT_GUEST_DIRTY_MASK != (1 << PT_GUEST_DIRTY_SHIFT));
> > >> BUILD_BUG_ON(PT_GUEST_ACCESSED_MASK != (1 << PT_GUEST_ACCESSED_SHIFT));
> > >
> > > This will not work if I define _SHIFT to be 8/9 now.
> > 
> > They will because I put them under an appropriate "#if". :)
> > 
> True, missed that.
> 
> > OTOH if you define _SHIFT to be 8/9 you can move the #if so that it only
> > covers the last two checks.
> > 
> > > But we do not use
> > > BUILD_BUG_ON() to check values from the same define "namespace". It is
> > > implied that they are correct and when they change all "namespace"
> > > remains to be consistent. If you look at BUILD_BUG_ON() that we have
> > > (and this patch adds) they are from the form:
> > >   PT_WRITABLE_MASK != ACC_WRITE_MASK
> > >   ACC_WRITE_MASK != VMX_EPT_WRITABLE_MASK
> > >   VMX_EPT_READABLE_MASK != PT_PRESENT_MASK
> > >   VMX_EPT_WRITABLE_MASK != PT_WRITABLE_MASK
> > > 
> > > i.e they compare value from different "namespaces".
> > 
> > Yes, all these BUILD_BUG_ONs make sense.
> > 
> > But I think BUILD_BUG_ON() is more generically for consistency checks
> > and enforcing invariants that the code expects.  Our invariants are:
> > 
> > * A/D bits are enabled or disabled in pairs
> > 
> > * dirty is the left of accessed and writable
> > 
> > * masks should either be zero or match the corresponding shifts
> > 
> > The alternative to BUILD_BUG_ON would be a comment that explains the
> > invariants, but there's no need to use English if C can do it better! :)
> > 
> OK, will add this in separate patch.
> 
Not really, BUILD_BUG_ON() is not meant to be used outside of a
function.

--
			Gleb.