Re: [Qemu-devel] [PATCH v2 2/3] x86: kvm: Add MTRR support for kvm_get|put_msrs()

[Alex Williamson]

On Thu, 2014-08-14 at 23:20 +0200, Laszlo Ersek wrote:
> You're going to use my name in contexts that I won't wish to be privy
> to. :) I like everything about this patch except:
> 
> > +        case MSR_MTRRphysBase(0) ... MSR_MTRRphysMask(MSR_MTRRcap_VCNT):
> 
> ... the off-by-one in this case range. Everything is cool and the range
> conforms to
> <https://gcc.gnu.org/onlinedocs/gcc-4.9.1/gcc/Case-Ranges.html> (ie. the
> range is inclusive), but the *argument* of the MSR_MTRRphysMask() macro
> is off-by-one. You should say
> 
>     case MSR_MTRRphysBase(0) ... MSR_MTRRphysMask(MSR_MTRRcap_VCNT - 1):
> 
> Peek up to the for loops: the greatest argument you ever pass to
> MSR_MTRRphysMask() is (MSR_MTRRcap_VCNT - 1).
> 
> Of course this causes no visible bug, because we don't use those
> register indices at all (and if we *did* use them, then we'd add new
> case labels for them, and then gcc would be required by the standard to
> complain about duplicated case labels [*]).

Nope, legitimate bug.  v3 on the way...