| Message ID | 1457042122-30727-1-git-send-email-pfeiner@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 03.03.2016 22:55, Peter Feiner wrote: > Tested with arch=i386, x86_64, ppc64, and arm64. > > Signed-off-by: Peter Feiner <pfeiner@google.com> > --- > Makefile | 2 +- > x86/hypercall.c | 2 +- > x86/vmx_tests.c | 4 ++-- > 3 files changed, 4 insertions(+), 4 deletions(-) ... > @@ -1392,6 +1390,7 @@ static void dbgctls_main(void) > asm volatile("mov %%dr7,%0" : "=r" (dr7)); > debugctl = rdmsr(MSR_IA32_DEBUGCTLMSR); > /* Commented out: KVM does not support DEBUGCTL so far */ > + assert(debugctl == debugctl); > report("Load debug controls", dr7 == 0x404 /* && debugctl == 0x2 */); > > dr7 = 0x408; > @@ -1413,6 +1412,7 @@ static void dbgctls_main(void) > asm volatile("mov %%dr7,%0" : "=r" (dr7)); > debugctl = rdmsr(MSR_IA32_DEBUGCTLMSR); > /* Commented out: KVM does not support DEBUGCTL so far */ > + assert(debugctl == debugctl); > report("Guest=host debug controls", dr7 == 0x402 /* && debugctl == 0x1 */); Now these assert()s are really ugly. Wouldn't it be better do comment out the "debugctl = rdmsr(MSR_IA32_DEBUGCTLMSR);" line instead? Thomas -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/03/2016 09:47, Thomas Huth wrote: >> > asm volatile("mov %%dr7,%0" : "=r" (dr7)); >> > debugctl = rdmsr(MSR_IA32_DEBUGCTLMSR); >> > /* Commented out: KVM does not support DEBUGCTL so far */ >> > + assert(debugctl == debugctl); >> > report("Guest=host debug controls", dr7 == 0x402 /* && debugctl == 0x1 */); > Now these assert()s are really ugly. Wouldn't it be better do comment > out the "debugctl = rdmsr(MSR_IA32_DEBUGCTLMSR);" line instead? This also looks better than the asserts: (void)debugctl; Thomas, if you're okay with it I can do the change locally. Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04.03.2016 09:49, Paolo Bonzini wrote: > > > On 04/03/2016 09:47, Thomas Huth wrote: >>>> asm volatile("mov %%dr7,%0" : "=r" (dr7)); >>>> debugctl = rdmsr(MSR_IA32_DEBUGCTLMSR); >>>> /* Commented out: KVM does not support DEBUGCTL so far */ >>>> + assert(debugctl == debugctl); >>>> report("Guest=host debug controls", dr7 == 0x402 /* && debugctl == 0x1 */); >> Now these assert()s are really ugly. Wouldn't it be better do comment >> out the "debugctl = rdmsr(MSR_IA32_DEBUGCTLMSR);" line instead? > > This also looks better than the asserts: > > (void)debugctl; > > Thomas, if you're okay with it I can do the change locally. I personally would rather prefer to put comments around "debugctl =", but your suggestion looks at least better to me than the solution with assert() ... (since assert() can also be #undef in normal C projects, so IMHO it should not be used for something that has influence on the behavior of the normal C code). Thomas -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Mar 4, 2016 at 1:00 AM, Thomas Huth <thuth@redhat.com> wrote: > On 04.03.2016 09:49, Paolo Bonzini wrote: >> >> >> On 04/03/2016 09:47, Thomas Huth wrote: >>>>> asm volatile("mov %%dr7,%0" : "=r" (dr7)); >>>>> debugctl = rdmsr(MSR_IA32_DEBUGCTLMSR); >>>>> /* Commented out: KVM does not support DEBUGCTL so far */ >>>>> + assert(debugctl == debugctl); >>>>> report("Guest=host debug controls", dr7 == 0x402 /* && debugctl == 0x1 */); >>> Now these assert()s are really ugly. Wouldn't it be better do comment >>> out the "debugctl = rdmsr(MSR_IA32_DEBUGCTLMSR);" line instead? >> >> This also looks better than the asserts: >> >> (void)debugctl; >> >> Thomas, if you're okay with it I can do the change locally. > > I personally would rather prefer to put comments around "debugctl =", > but your suggestion looks at least better to me than the solution with > assert() ... (since assert() can also be #undef in normal C projects, so > IMHO it should not be used for something that has influence on the > behavior of the normal C code). I wasn't too happy with the asserts either. I didn't just comment it all out because I wanted to leave in the rdmsr in case there were side effects (which there shouldn't be, but that's the point of testing!). I'm fine with the (void) as well. Paolo, I'll assume you're going to massage the patch unless you say otherwise. Thanks, Peter -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Makefile b/Makefile index 72e6711..09999c6 100644 --- a/Makefile +++ b/Makefile @@ -41,7 +41,7 @@ cc-option = $(shell if $(CC) $(1) -S -o /dev/null -xc /dev/null \ > /dev/null 2>&1; then echo "$(1)"; else echo "$(2)"; fi ;) CFLAGS += -g -CFLAGS += $(autodepend-flags) -Wall +CFLAGS += $(autodepend-flags) -Wall -Werror CFLAGS += $(call cc-option, -fomit-frame-pointer, "") CFLAGS += $(call cc-option, -fno-stack-protector, "") CFLAGS += $(call cc-option, -fno-stack-protector-all, "") diff --git a/x86/hypercall.c b/x86/hypercall.c index 3ac5ff9..75179a1 100644 --- a/x86/hypercall.c +++ b/x86/hypercall.c @@ -34,7 +34,6 @@ asm ("gp_tss: \n\t" "iretq\n\t" "jmp gp_tss\n\t" ); -#endif static inline int test_edge(void) @@ -47,6 +46,7 @@ test_edge(void) printf("Return from int 13, test_rip = %lx\n", test_rip); return test_rip == (1ul << 47); } +#endif int main(int ac, char **av) { diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c index d851692..b7dfcbb 100644 --- a/x86/vmx_tests.c +++ b/x86/vmx_tests.c @@ -1174,12 +1174,10 @@ static int vpid_exit_handler() u64 guest_rip; ulong reason; u32 insn_len; - u32 exit_qual; guest_rip = vmcs_read(GUEST_RIP); reason = vmcs_read(EXI_REASON) & 0xff; insn_len = vmcs_read(EXI_INST_LEN); - exit_qual = vmcs_read(EXI_QUALIFICATION); switch (reason) { case VMX_VMCALL: @@ -1392,6 +1390,7 @@ static void dbgctls_main(void) asm volatile("mov %%dr7,%0" : "=r" (dr7)); debugctl = rdmsr(MSR_IA32_DEBUGCTLMSR); /* Commented out: KVM does not support DEBUGCTL so far */ + assert(debugctl == debugctl); report("Load debug controls", dr7 == 0x404 /* && debugctl == 0x2 */); dr7 = 0x408; @@ -1413,6 +1412,7 @@ static void dbgctls_main(void) asm volatile("mov %%dr7,%0" : "=r" (dr7)); debugctl = rdmsr(MSR_IA32_DEBUGCTLMSR); /* Commented out: KVM does not support DEBUGCTL so far */ + assert(debugctl == debugctl); report("Guest=host debug controls", dr7 == 0x402 /* && debugctl == 0x1 */); dr7 = 0x408;
Tested with arch=i386, x86_64, ppc64, and arm64. Signed-off-by: Peter Feiner <pfeiner@google.com> --- Makefile | 2 +- x86/hypercall.c | 2 +- x86/vmx_tests.c | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-)