Re: [PATCH v2 5/6] VMX: Validate capability MSRs

[Paolo Bonzini <pbonzini@redhat.com>]

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