From mboxrd@z Thu Jan  1 00:00:00 1970
From: Paolo Bonzini <pbonzini@redhat.com>
Subject: Re: [PATCH v2 5/6] VMX: Validate capability MSRs
Date: Tue, 17 Jun 2014 10:00:22 +0200
Message-ID: <539FF596.60706@redhat.com>
References: <cover.1402988683.git.jan.kiszka@siemens.com> <a5a2ad8df890d8edaa80555abf0746d428504761.1402988683.git.jan.kiszka@siemens.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-15; format=flowed
Content-Transfer-Encoding: 7bit
Cc: kvm <kvm@vger.kernel.org>, Bandan Das <bsd@redhat.com>
To: Jan Kiszka <jan.kiszka@siemens.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mx1.redhat.com ([209.132.183.28]:15651 "EHLO mx1.redhat.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1752893AbaFQIA2 (ORCPT <rfc822;kvm@vger.kernel.org>);
	Tue, 17 Jun 2014 04:00:28 -0400
In-Reply-To: <a5a2ad8df890d8edaa80555abf0746d428504761.1402988683.git.jan.kiszka@siemens.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

Il 17/06/2014 09:04, Jan Kiszka ha scritto:
> +		default1 = vmx_ctl_msr[n].default1;
> +		ok = (ctrl.set & default1) == default1 &&
> +			((ctrl.set ^ ctrl.clr) & ~ctrl.clr) == 0;

Thanks, now I can understand what's going on. :)  It can still be 
simplified though.

This is just (ctrl.set & ~ctrl.clr) == 0:

	(a ^ b) & ~b == 0
->	(a & ~b) ^ (b & ~b) == 0
->	a & ~b == 0

This expresses just as well the concept that set=1, clr=0 is an 
impossible combination.

Also, it's a bit easier to read if the second line is written as a 
separate statement:

	ok = ok && (ctrl.set & ~ctrl.clr) == 0;

> +		if (ok && basic.ctrl) {
> +			true_ctrl.val = rdmsr(vmx_ctl_msr[n].true_index);
> +			ok = ctrl.clr == true_ctrl.clr &&
> +				((ctrl.set ^ true_ctrl.set) & ~default1) == 0;

This is

	((ctrl.set ^ true_ctrl.set) & ~default1 == 0
->	((ctrl.set & ~default1) ^ (true_ctrl.set & ~default1) == 0
->	((ctrl.set & ~default1) == (true_ctrl.set & ~default1)

A bit longer, but clearer: the difference between ctrl.set and 
true_ctrl.set is only that true_ctrl.set can clear some default-1 bits.

Or you can simplify it further:

->      (ctrl.set | default1) == (true_ctrl.set | default1)
->      ctrl.set == (true_ctrl.set | default1)

Also clearer: the difference between ctrl.set and true_ctrl.set is only 
that default-1 bits must be 1 in ctrl.set.  Pick the one you prefer.

Again, using a separate statement makes it easier to read in my opinion; 
in fact I would also write both statements as

	ok = ok && ...

even though it's redundant for the clr test.

Can you submit v3 of this patch only?

Paolo