From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH RFC 14/31] tools/libxc: Use featureset rather than guesswork Date: Tue, 5 Jan 2016 16:22:49 +0000 Message-ID: <568BEDD9.5040201@citrix.com> References: <1450301073-28191-1-git-send-email-andrew.cooper3@citrix.com> <1450301073-28191-15-git-send-email-andrew.cooper3@citrix.com> <1452009270.13361.339.camel@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1452009270.13361.339.camel@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Campbell , Xen-devel Cc: Wei Liu , Ian Jackson List-Id: xen-devel@lists.xenproject.org On 05/01/16 15:54, Ian Campbell wrote: > On Wed, 2015-12-16 at 21:24 +0000, Andrew Cooper wrote: >> It is conceptually wrong to base a VM's featureset on the features >> visible to >> the toolstack which happens to construct it. >> >> Instead, the featureset used is either an explicit one passed by the >> toolstack, or the default which Xen believes it can give to the guest. >> >> Signed-off-by: Andrew Cooper >> --- >> CC: Ian Campbell >> CC: Ian Jackson >> CC: Wei Liu >> --- >> tools/libxc/xc_cpuid_x86.c | 229 +++++++++++++------------------------ >> -------- >> 1 file changed, 64 insertions(+), 165 deletions(-) >> >> diff --git a/tools/libxc/xc_cpuid_x86.c b/tools/libxc/xc_cpuid_x86.c >> index 8bd3126..3f39306 100644 >> --- a/tools/libxc/xc_cpuid_x86.c >> +++ b/tools/libxc/xc_cpuid_x86.c >> @@ -24,6 +24,7 @@ >> #include "xc_private.h" >> #include >> #include >> +#include >> >> #define bitmaskof(idx) (1u << ((idx) & 31)) >> #define clear_bit(idx, dst) ((dst) &= ~bitmaskof(idx)) >> @@ -211,37 +212,19 @@ static void amd_xc_cpuid_policy(xc_interface *xch, >> regs[0] = DEF_MAX_AMDEXT; >> break; >> >> - case 0x80000001: { >> + case 0x80000001: >> if ( !info->pae ) >> clear_bit(X86_FEATURE_PAE, regs[3]); > This (and the others which actually change in this patch) are correct > because the locally defined clear_bit (contrary to what one might usually > expect) includes masking off the lower bits to get the offset in the given > word, is that right? > > That confused me a lot until I spotted the clear_bit in the context above. > What do you think about renaming it to clear_feature, or something else > without the false expectation? Just thought I'd mention it having scratched > my head for a bit ;-), feel free to say no. A later patch renames them so I can include the real set/test/clear_bit from xc_bitops.h The problem is that, until this patch, there would be a massive delta (which is destined for /dev/null) if I proactively renamed them. ~Andrew