[kvm-unit-tests PATCH 0/7] Fixes for clang builds

[kvm-unit-tests PATCH 0/7] Fixes for clang builds

[morbo]

From: Bill Wendling <morbo@google.com>

This is a series of patches which allow clang to build the
kvm-unit-tests.

Bill Wendling (7):
  x86: emulator: use "SSE2" for the target
  pci: use uint32_t for unsigned long values
  x86: realmode: syscall: add explicit size suffix to ambiguous
    instructions
  svm: change operand to output-only for matching constraint
  svm: convert neg shift to unsigned shift
  x86: VMX: use inline asm to get stack pointer
  x86: VMX: the "noclone" attribute is gcc-specific

 lib/linux/pci_regs.h |  4 ++--
 x86/emulator.c       |  2 +-
 x86/realmode.c       |  6 +++---
 x86/svm.c            |  6 +++---
 x86/syscall.c        |  2 +-
 x86/vmx_tests.c      | 11 ++++++++---
 6 files changed, 18 insertions(+), 13 deletions(-)

-- 
2.25.0.265.gbab2e86ba0-goog

[kvm-unit-tests PATCH 1/7] x86: emulator: use "SSE2" for the target

[morbo]

From: Bill Wendling <morbo@google.com>

The movdqu and movapd instructions are SSE2 instructions. Clang
interprets the __attribute__((target("sse"))) as allowing SSE only
instructions. Using SSE2 instructions cause an error.

Signed-off-by: Bill Wendling <morbo@google.com>
---
 x86/emulator.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/x86/emulator.c b/x86/emulator.c
index 8fe03b8..2990550 100644
--- a/x86/emulator.c
+++ b/x86/emulator.c
@@ -658,7 +658,7 @@ static bool sseeq(sse_union *v1, sse_union *v2)
     return ok;
 }
 
-static __attribute__((target("sse"))) void test_sse(sse_union *mem)
+static __attribute__((target("sse2"))) void test_sse(sse_union *mem)
 {
     sse_union v;
 
-- 
2.25.0.265.gbab2e86ba0-goog

[kvm-unit-tests PATCH 2/7] pci: use uint32_t for unsigned long values

[morbo]

From: Bill Wendling <morbo@google.com>

The "pci_bar_*" functions use 64-bit masks, but the results are assigned
to 32-bit variables. Use 32-bit masks, since we're interested only in
the least significant 4-bits.

Signed-off-by: Bill Wendling <morbo@google.com>
---
 lib/linux/pci_regs.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/linux/pci_regs.h b/lib/linux/pci_regs.h
index 1becea8..3bc2b92 100644
--- a/lib/linux/pci_regs.h
+++ b/lib/linux/pci_regs.h
@@ -96,8 +96,8 @@
 #define  PCI_BASE_ADDRESS_MEM_TYPE_1M	0x02	/* Below 1M [obsolete] */
 #define  PCI_BASE_ADDRESS_MEM_TYPE_64	0x04	/* 64 bit address */
 #define  PCI_BASE_ADDRESS_MEM_PREFETCH	0x08	/* prefetchable? */
-#define  PCI_BASE_ADDRESS_MEM_MASK	(~0x0fUL)
-#define  PCI_BASE_ADDRESS_IO_MASK	(~0x03UL)
+#define  PCI_BASE_ADDRESS_MEM_MASK	(~0x0fU)
+#define  PCI_BASE_ADDRESS_IO_MASK	(~0x03U)
 /* bit 1 is reserved if address_space = 1 */
 
 /* Header type 0 (normal devices) */
-- 
2.25.0.265.gbab2e86ba0-goog

[kvm-unit-tests PATCH 3/7] x86: realmode: syscall: add explicit size suffix to ambiguous instructions

[morbo]

From: Bill Wendling <morbo@google.com>

Clang requires explicit size suffixes for potentially ambiguous
instructions:

x86/realmode.c:1647:2: error: ambiguous instructions require an explicit suffix (could be 'cmpb', 'cmpw', or 'cmpl')
        MK_INSN_PERF(perf_memory_load, "cmp $0, (%edi)");
        ^
x86/realmode.c:1591:10: note: expanded from macro 'MK_INSN_PERF'
                      "1:" insn "\n"                            \
                       ^
<inline asm>:8:3: note: instantiated into assembly here
1:cmp $0, (%edi)
  ^

The 'w' and 'l' suffixes generate code that's identical to the gcc
version without them.

Signed-off-by: Bill Wendling <morbo@google.com>
---
 x86/realmode.c | 6 +++---
 x86/syscall.c  | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/x86/realmode.c b/x86/realmode.c
index f5967ef..31f84d0 100644
--- a/x86/realmode.c
+++ b/x86/realmode.c
@@ -1644,7 +1644,7 @@ static void test_perf_memory_load(void)
 {
 	u32 cyc, tmp;
 
-	MK_INSN_PERF(perf_memory_load, "cmp $0, (%edi)");
+	MK_INSN_PERF(perf_memory_load, "cmpw $0, (%edi)");
 
 	init_inregs(&(struct regs){ .edi = (u32)&tmp });
 
@@ -1657,7 +1657,7 @@ static void test_perf_memory_store(void)
 {
 	u32 cyc, tmp;
 
-	MK_INSN_PERF(perf_memory_store, "mov %ax, (%edi)");
+	MK_INSN_PERF(perf_memory_store, "movw %ax, (%edi)");
 
 	init_inregs(&(struct regs){ .edi = (u32)&tmp });
 
@@ -1670,7 +1670,7 @@ static void test_perf_memory_rmw(void)
 {
 	u32 cyc, tmp;
 
-	MK_INSN_PERF(perf_memory_rmw, "add $1, (%edi)");
+	MK_INSN_PERF(perf_memory_rmw, "addw $1, (%edi)");
 
 	init_inregs(&(struct regs){ .edi = (u32)&tmp });
 
diff --git a/x86/syscall.c b/x86/syscall.c
index b4f5ac0..b7e29d6 100644
--- a/x86/syscall.c
+++ b/x86/syscall.c
@@ -38,7 +38,7 @@ static void handle_db(struct ex_regs *regs)
 
 /* expects desired ring 3 flags in rax */
 asm("syscall32_target:\n"
-    "   cmp $0, code_segment_upon_db(%rip)\n"
+    "   cmpl $0, code_segment_upon_db(%rip)\n"
     "   jne back_to_test\n"
     "   mov %eax,%r11d\n"
     "   sysretl\n");
-- 
2.25.0.265.gbab2e86ba0-goog

[kvm-unit-tests PATCH 4/7] svm: change operand to output-only for matching constraint

[morbo]

From: Bill Wendling <morbo@google.com>

According to GNU extended asm documentation, "the two operands [of
matching constraints] must include one input-only operand and one
output-only operand." So remove the read/write modifier from the output
constraint.

Signed-off-by: Bill Wendling <morbo@google.com>
---
 x86/svm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/x86/svm.c b/x86/svm.c
index aa3d995..ae85194 100644
--- a/x86/svm.c
+++ b/x86/svm.c
@@ -288,8 +288,8 @@ static void test_run(struct test *test, struct vmcb *vmcb)
             "cli \n\t"
             "stgi"
             : // inputs clobbered by the guest:
-	      "+D" (the_test),            // first argument register
-	      "+b" (the_vmcb)             // callee save register!
+	      "=D" (the_test),            // first argument register
+	      "=b" (the_vmcb)             // callee save register!
             : [test] "0" (the_test),
 	      [vmcb_phys] "1"(the_vmcb),
 	      [PREPARE_GIF_CLEAR] "i" (offsetof(struct test, prepare_gif_clear))
-- 
2.25.0.265.gbab2e86ba0-goog

[kvm-unit-tests PATCH 5/7] svm: convert neg shift to unsigned shift

[morbo]

From: Bill Wendling <morbo@google.com>

Shifting a negative signed value is undefined. Use a shift of an
unsigned value instead.

Signed-off-by: Bill Wendling <morbo@google.com>
---
 x86/svm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/x86/svm.c b/x86/svm.c
index ae85194..17be4b0 100644
--- a/x86/svm.c
+++ b/x86/svm.c
@@ -1148,7 +1148,7 @@ static bool npt_rw_l1mmio_check(struct test *test)
 }
 
 #define TSC_ADJUST_VALUE    (1ll << 32)
-#define TSC_OFFSET_VALUE    (-1ll << 48)
+#define TSC_OFFSET_VALUE    (~0ull << 48)
 static bool ok;
 
 static void tsc_adjust_prepare(struct test *test)
-- 
2.25.0.265.gbab2e86ba0-goog

[kvm-unit-tests PATCH 6/7] x86: VMX: use inline asm to get stack pointer

[morbo]

From: Bill Wendling <morbo@google.com>

The only supported use of a local register variable is to specify
registers for input and output operands when calling Extended asm. Using
it to automatically collect the value in the register isn't supported as
the contents of the register aren't guaranteed. Instead use inline asm
to get the stack pointer explicitly.

Signed-off-by: Bill Wendling <morbo@google.com>
---
 x86/vmx_tests.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
index a7abd63..ad8c002 100644
--- a/x86/vmx_tests.c
+++ b/x86/vmx_tests.c
@@ -2165,8 +2165,9 @@ static void into_guest_main(void)
 		.offset = (uintptr_t)&&into,
 		.selector = KERNEL_CS32,
 	};
-	register uintptr_t rsp asm("rsp");
+	uintptr_t rsp = 0;
 
+	asm volatile ("mov %%rsp, %0" : "=r" (rsp));
 	if (fp.offset != (uintptr_t)&&into) {
 		printf("Code address too high.\n");
 		return;
@@ -3261,8 +3262,9 @@ static void try_compat_invvpid(void *unused)
 		.offset = (uintptr_t)&&invvpid,
 		.selector = KERNEL_CS32,
 	};
-	register uintptr_t rsp asm("rsp");
+	register uintptr_t rsp = 0;
 
+	asm volatile ("mov %%rsp, %0" : "=r" (rsp));
 	TEST_ASSERT_MSG(fp.offset == (uintptr_t)&&invvpid,
 			"Code address too high.");
 	TEST_ASSERT_MSG(rsp == (u32)rsp, "Stack address too high.");
-- 
2.25.0.265.gbab2e86ba0-goog

[kvm-unit-tests PATCH 7/7] x86: VMX: the "noclone" attribute is gcc-specific

[morbo]

From: Bill Wendling <morbo@google.com>

Don't use the "noclone" attribute for clang as it's not supported.

Signed-off-by: Bill Wendling <morbo@google.com>
---
 x86/vmx_tests.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
index ad8c002..ec88016 100644
--- a/x86/vmx_tests.c
+++ b/x86/vmx_tests.c
@@ -4976,7 +4976,10 @@ extern unsigned char test_mtf1;
 extern unsigned char test_mtf2;
 extern unsigned char test_mtf3;
 
-__attribute__((noclone)) static void test_mtf_guest(void)
+#ifndef __clang__
+__attribute__((noclone))
+#endif
+static void test_mtf_guest(void)
 {
 	asm ("vmcall;\n\t"
 	     "out %al, $0x80;\n\t"
-- 
2.25.0.265.gbab2e86ba0-goog

Re: [kvm-unit-tests PATCH 2/7] pci: use uint32_t for unsigned long values

[Andrew Jones]

On Tue, Feb 25, 2020 at 11:44:22PM -0800, morbo@google.com wrote:
> From: Bill Wendling <morbo@google.com>
> 
> The "pci_bar_*" functions use 64-bit masks, but the results are assigned
> to 32-bit variables. Use 32-bit masks, since we're interested only in
> the least significant 4-bits.
> 
> Signed-off-by: Bill Wendling <morbo@google.com>
> ---
>  lib/linux/pci_regs.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/linux/pci_regs.h b/lib/linux/pci_regs.h
> index 1becea8..3bc2b92 100644
> --- a/lib/linux/pci_regs.h
> +++ b/lib/linux/pci_regs.h
> @@ -96,8 +96,8 @@
>  #define  PCI_BASE_ADDRESS_MEM_TYPE_1M	0x02	/* Below 1M [obsolete] */
>  #define  PCI_BASE_ADDRESS_MEM_TYPE_64	0x04	/* 64 bit address */
>  #define  PCI_BASE_ADDRESS_MEM_PREFETCH	0x08	/* prefetchable? */
> -#define  PCI_BASE_ADDRESS_MEM_MASK	(~0x0fUL)
> -#define  PCI_BASE_ADDRESS_IO_MASK	(~0x03UL)
> +#define  PCI_BASE_ADDRESS_MEM_MASK	(~0x0fU)
> +#define  PCI_BASE_ADDRESS_IO_MASK	(~0x03U)
>  /* bit 1 is reserved if address_space = 1 */
>  
>  /* Header type 0 (normal devices) */
> -- 
> 2.25.0.265.gbab2e86ba0-goog
>

This file comes directly from the Linux source. If it's not changed
there, then it shouldn't change here.

Thanks,
drew 

Re: [kvm-unit-tests PATCH 7/7] x86: VMX: the "noclone" attribute is gcc-specific

[Oliver Upton]

On Tue, Feb 25, 2020 at 11:45 PM <morbo@google.com> wrote:
>
> From: Bill Wendling <morbo@google.com>
>
> Don't use the "noclone" attribute for clang as it's not supported.
>
> Signed-off-by: Bill Wendling <morbo@google.com>
Signed-off-by: Oliver Upton <oupton@google.com>
> ---
>  x86/vmx_tests.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
> index ad8c002..ec88016 100644
> --- a/x86/vmx_tests.c
> +++ b/x86/vmx_tests.c
> @@ -4976,7 +4976,10 @@ extern unsigned char test_mtf1;
>  extern unsigned char test_mtf2;
>  extern unsigned char test_mtf3;
>
> -__attribute__((noclone)) static void test_mtf_guest(void)
> +#ifndef __clang__
> +__attribute__((noclone))
> +#endif
> +static void test_mtf_guest(void)
>  {
>         asm ("vmcall;\n\t"
>              "out %al, $0x80;\n\t"
> --
> 2.25.0.265.gbab2e86ba0-goog
>

Re: [kvm-unit-tests PATCH 7/7] x86: VMX: the "noclone" attribute is gcc-specific

[Oliver Upton]

Blech...

On Wed, Feb 26, 2020 at 12:21 AM Oliver Upton <oupton@google.com> wrote:
>
> On Tue, Feb 25, 2020 at 11:45 PM <morbo@google.com> wrote:
> >
> > From: Bill Wendling <morbo@google.com>
> >
> > Don't use the "noclone" attribute for clang as it's not supported.
> >
> > Signed-off-by: Bill Wendling <morbo@google.com>
> Signed-off-by: Oliver Upton <oupton@google.com>

s/Signed-off-by/Reviewed-by/

> > ---
> >  x86/vmx_tests.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
> > index ad8c002..ec88016 100644
> > --- a/x86/vmx_tests.c
> > +++ b/x86/vmx_tests.c
> > @@ -4976,7 +4976,10 @@ extern unsigned char test_mtf1;
> >  extern unsigned char test_mtf2;
> >  extern unsigned char test_mtf3;
> >
> > -__attribute__((noclone)) static void test_mtf_guest(void)
> > +#ifndef __clang__
> > +__attribute__((noclone))
> > +#endif
> > +static void test_mtf_guest(void)
> >  {
> >         asm ("vmcall;\n\t"
> >              "out %al, $0x80;\n\t"
> > --
> > 2.25.0.265.gbab2e86ba0-goog
> >

Re: [kvm-unit-tests PATCH 2/7] pci: use uint32_t for unsigned long values

[Bill Wendling]

I suspect then that how it's used in the kvm-unit-tests isn't correct.
Or at least it's not how it's used in the Linux source code. Would it
be better to cast all uses of these masks to uint32_t?

-bw


On Tue, Feb 25, 2020 at 11:59 PM Andrew Jones <drjones@redhat.com> wrote:
>
> On Tue, Feb 25, 2020 at 11:44:22PM -0800, morbo@google.com wrote:
> > From: Bill Wendling <morbo@google.com>
> >
> > The "pci_bar_*" functions use 64-bit masks, but the results are assigned
> > to 32-bit variables. Use 32-bit masks, since we're interested only in
> > the least significant 4-bits.
> >
> > Signed-off-by: Bill Wendling <morbo@google.com>
> > ---
> >  lib/linux/pci_regs.h | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/lib/linux/pci_regs.h b/lib/linux/pci_regs.h
> > index 1becea8..3bc2b92 100644
> > --- a/lib/linux/pci_regs.h
> > +++ b/lib/linux/pci_regs.h
> > @@ -96,8 +96,8 @@
> >  #define  PCI_BASE_ADDRESS_MEM_TYPE_1M        0x02    /* Below 1M [obsolete] */
> >  #define  PCI_BASE_ADDRESS_MEM_TYPE_64        0x04    /* 64 bit address */
> >  #define  PCI_BASE_ADDRESS_MEM_PREFETCH       0x08    /* prefetchable? */
> > -#define  PCI_BASE_ADDRESS_MEM_MASK   (~0x0fUL)
> > -#define  PCI_BASE_ADDRESS_IO_MASK    (~0x03UL)
> > +#define  PCI_BASE_ADDRESS_MEM_MASK   (~0x0fU)
> > +#define  PCI_BASE_ADDRESS_IO_MASK    (~0x03U)
> >  /* bit 1 is reserved if address_space = 1 */
> >
> >  /* Header type 0 (normal devices) */
> > --
> > 2.25.0.265.gbab2e86ba0-goog
> >
>
> This file comes directly from the Linux source. If it's not changed
> there, then it shouldn't change here.
>
> Thanks,
> drew
>

Re: [kvm-unit-tests PATCH 2/7] pci: use uint32_t for unsigned long values

[Andrew Jones]

On Wed, Feb 26, 2020 at 01:02:39AM -0800, Bill Wendling wrote:
> I suspect then that how it's used in the kvm-unit-tests isn't correct.
> Or at least it's not how it's used in the Linux source code. Would it
> be better to cast all uses of these masks to uint32_t?

Yup. If clang is upset about kvm-unit-test's use of these masks, then
we need to change how they're used, as we can't change how they're
defined.

There's only one occurrence of each though, and in the same function,
pci_bar_mask(). I'd probably just change the return type of that
function to uint64_t, rather than add casts.

Thanks,
drew

> 
> -bw
> 
> 
> On Tue, Feb 25, 2020 at 11:59 PM Andrew Jones <drjones@redhat.com> wrote:
> >
> > On Tue, Feb 25, 2020 at 11:44:22PM -0800, morbo@google.com wrote:
> > > From: Bill Wendling <morbo@google.com>
> > >
> > > The "pci_bar_*" functions use 64-bit masks, but the results are assigned
> > > to 32-bit variables. Use 32-bit masks, since we're interested only in
> > > the least significant 4-bits.
> > >
> > > Signed-off-by: Bill Wendling <morbo@google.com>
> > > ---
> > >  lib/linux/pci_regs.h | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/lib/linux/pci_regs.h b/lib/linux/pci_regs.h
> > > index 1becea8..3bc2b92 100644
> > > --- a/lib/linux/pci_regs.h
> > > +++ b/lib/linux/pci_regs.h
> > > @@ -96,8 +96,8 @@
> > >  #define  PCI_BASE_ADDRESS_MEM_TYPE_1M        0x02    /* Below 1M [obsolete] */
> > >  #define  PCI_BASE_ADDRESS_MEM_TYPE_64        0x04    /* 64 bit address */
> > >  #define  PCI_BASE_ADDRESS_MEM_PREFETCH       0x08    /* prefetchable? */
> > > -#define  PCI_BASE_ADDRESS_MEM_MASK   (~0x0fUL)
> > > -#define  PCI_BASE_ADDRESS_IO_MASK    (~0x03UL)
> > > +#define  PCI_BASE_ADDRESS_MEM_MASK   (~0x0fU)
> > > +#define  PCI_BASE_ADDRESS_IO_MASK    (~0x03U)
> > >  /* bit 1 is reserved if address_space = 1 */
> > >
> > >  /* Header type 0 (normal devices) */
> > > --
> > > 2.25.0.265.gbab2e86ba0-goog
> > >
> >
> > This file comes directly from the Linux source. If it's not changed
> > there, then it shouldn't change here.
> >
> > Thanks,
> > drew
> >
> 

[kvm-unit-tests PATCH v2 0/7] Fixes for clang builds

[Bill Wendling]

Changes to "pci: cast masks to uint32_t for unsigned long values" to
cast the masks instead of changing the values in the header.

Bill Wendling (2):
  x86: emulator: use "SSE2" for the target
  pci: cast masks to uint32_t for unsigned long values

Eric Hankland (2):
  x86: pmu: Test WRMSR on a running counter
  x86: pmu: Test perfctr overflow after WRMSR on a running counter

Oliver Upton (1):
  x86: VMX: Add tests for monitor trap flag

Paolo Bonzini (2):
  x86: provide enabled and disabled variation of the PCID test
  vmx: tweak XFAILS for #DB test

-- 
2.25.0.265.gbab2e86ba0-goog

[kvm-unit-tests PATCH 1/7] x86: emulator: use "SSE2" for the target

[Bill Wendling]

The movdqu and movapd instructions are SSE2 instructions. Clang
interprets the __attribute__((target("sse"))) as allowing SSE only
instructions. Using SSE2 instructions cause an error.

Signed-off-by: Bill Wendling <morbo@google.com>
---
 x86/emulator.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/x86/emulator.c b/x86/emulator.c
index 8fe03b8..2990550 100644
--- a/x86/emulator.c
+++ b/x86/emulator.c
@@ -658,7 +658,7 @@ static bool sseeq(sse_union *v1, sse_union *v2)
     return ok;
 }
 
-static __attribute__((target("sse"))) void test_sse(sse_union *mem)
+static __attribute__((target("sse2"))) void test_sse(sse_union *mem)
 {
     sse_union v;
 
-- 
2.25.0.265.gbab2e86ba0-goog

[kvm-unit-tests PATCH v2 1/7] x86: pmu: Test WRMSR on a running counter

[Bill Wendling]

From: Eric Hankland <ehankland@google.com>

Ensure that the value of the counter was successfully set to 0 after
writing it while the counter was running.

Signed-off-by: Eric Hankland <ehankland@google.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Bill Wendling <morbo@google.com>
---
 x86/pmu.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/x86/pmu.c b/x86/pmu.c
index a4e483b..c8096b8 100644
--- a/x86/pmu.c
+++ b/x86/pmu.c
@@ -420,6 +420,21 @@ static void check_rdpmc(void)
 	report_prefix_pop();
 }
 
+static void check_running_counter_wrmsr(void)
+{
+	pmu_counter_t evt = {
+		.ctr = MSR_IA32_PERFCTR0,
+		.config = EVNTSEL_OS | EVNTSEL_USR | gp_events[1].unit_sel,
+		.count = 0,
+	};
+
+	start_event(&evt);
+	loop();
+	wrmsr(MSR_IA32_PERFCTR0, 0);
+	stop_event(&evt);
+	report(evt.count < gp_events[1].min, "running counter wrmsr");
+}
+
 int main(int ac, char **av)
 {
 	struct cpuid id = cpuid(10);
@@ -454,6 +469,7 @@ int main(int ac, char **av)
 	check_counters_many();
 	check_counter_overflow();
 	check_gp_counter_cmask();
+	check_running_counter_wrmsr();
 
 	return report_summary();
 }
-- 
2.25.0.265.gbab2e86ba0-goog

[kvm-unit-tests PATCH 2/7] pci: use uint32_t for unsigned long values

[Bill Wendling]

The "pci_bar_*" functions use 64-bit masks, but the results are assigned
to 32-bit variables. Use 32-bit masks, since we're interested only in
the least significant 4-bits.

Signed-off-by: Bill Wendling <morbo@google.com>
---
 lib/linux/pci_regs.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/linux/pci_regs.h b/lib/linux/pci_regs.h
index 1becea8..3bc2b92 100644
--- a/lib/linux/pci_regs.h
+++ b/lib/linux/pci_regs.h
@@ -96,8 +96,8 @@
 #define  PCI_BASE_ADDRESS_MEM_TYPE_1M	0x02	/* Below 1M [obsolete] */
 #define  PCI_BASE_ADDRESS_MEM_TYPE_64	0x04	/* 64 bit address */
 #define  PCI_BASE_ADDRESS_MEM_PREFETCH	0x08	/* prefetchable? */
-#define  PCI_BASE_ADDRESS_MEM_MASK	(~0x0fUL)
-#define  PCI_BASE_ADDRESS_IO_MASK	(~0x03UL)
+#define  PCI_BASE_ADDRESS_MEM_MASK	(~0x0fU)
+#define  PCI_BASE_ADDRESS_IO_MASK	(~0x03U)
 /* bit 1 is reserved if address_space = 1 */
 
 /* Header type 0 (normal devices) */
-- 
2.25.0.265.gbab2e86ba0-goog

[kvm-unit-tests PATCH v2 2/7] x86: provide enabled and disabled variation of the PCID test

[Bill Wendling]

From: Paolo Bonzini <pbonzini@redhat.com>

The PCID test checks for exceptions when PCID=0 or INVPCID=0 in
CPUID.  Cover that by adding a separate testcase with different
CPUID.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Bill Wendling <morbo@google.com>
---
 x86/unittests.cfg | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/x86/unittests.cfg b/x86/unittests.cfg
index aae1523..f2401eb 100644
--- a/x86/unittests.cfg
+++ b/x86/unittests.cfg
@@ -228,7 +228,12 @@ extra_params = --append "10000000 `date +%s`"
 
 [pcid]
 file = pcid.flat
-extra_params = -cpu qemu64,+pcid
+extra_params = -cpu qemu64,+pcid,+invpcid
+arch = x86_64
+
+[pcid-disabled]
+file = pcid.flat
+extra_params = -cpu qemu64,-pcid,-invpcid
 arch = x86_64
 
 [rdpru]
-- 
2.25.0.265.gbab2e86ba0-goog

[kvm-unit-tests PATCH v2 3/7] x86: pmu: Test perfctr overflow after WRMSR on a running counter

[Bill Wendling]

From: Eric Hankland <ehankland@google.com>

Ensure that a WRMSR on a running counter will correctly update when the
counter should overflow (similar to the existing overflow test case but
with the counter being written to while it is running).

Signed-off-by: Eric Hankland <ehankland@google.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Bill Wendling <morbo@google.com>
---
 x86/pmu.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/x86/pmu.c b/x86/pmu.c
index c8096b8..f45621a 100644
--- a/x86/pmu.c
+++ b/x86/pmu.c
@@ -422,17 +422,34 @@ static void check_rdpmc(void)
 
 static void check_running_counter_wrmsr(void)
 {
+	uint64_t status;
 	pmu_counter_t evt = {
 		.ctr = MSR_IA32_PERFCTR0,
 		.config = EVNTSEL_OS | EVNTSEL_USR | gp_events[1].unit_sel,
 		.count = 0,
 	};
 
+	report_prefix_push("running counter wrmsr");
+
 	start_event(&evt);
 	loop();
 	wrmsr(MSR_IA32_PERFCTR0, 0);
 	stop_event(&evt);
-	report(evt.count < gp_events[1].min, "running counter wrmsr");
+	report(evt.count < gp_events[1].min, "cntr");
+
+	/* clear status before overflow test */
+	wrmsr(MSR_CORE_PERF_GLOBAL_OVF_CTRL,
+	      rdmsr(MSR_CORE_PERF_GLOBAL_STATUS));
+
+	evt.count = 0;
+	start_event(&evt);
+	wrmsr(MSR_IA32_PERFCTR0, -1);
+	loop();
+	stop_event(&evt);
+	status = rdmsr(MSR_CORE_PERF_GLOBAL_STATUS);
+	report(status & 1, "status");
+
+	report_prefix_pop();
 }
 
 int main(int ac, char **av)
-- 
2.25.0.265.gbab2e86ba0-goog

[kvm-unit-tests PATCH 3/7] x86: realmode: syscall: add explicit size suffix to ambiguous instructions

[Bill Wendling]

Clang requires explicit size suffixes for potentially ambiguous
instructions:

x86/realmode.c:1647:2: error: ambiguous instructions require an explicit suffix (could be 'cmpb', 'cmpw', or 'cmpl')
        MK_INSN_PERF(perf_memory_load, "cmp $0, (%edi)");
        ^
x86/realmode.c:1591:10: note: expanded from macro 'MK_INSN_PERF'
                      "1:" insn "\n"                            \
                       ^
<inline asm>:8:3: note: instantiated into assembly here
1:cmp $0, (%edi)
  ^

The 'w' and 'l' suffixes generate code that's identical to the gcc
version without them.

Signed-off-by: Bill Wendling <morbo@google.com>
---
 x86/realmode.c | 6 +++---
 x86/syscall.c  | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/x86/realmode.c b/x86/realmode.c
index f5967ef..31f84d0 100644
--- a/x86/realmode.c
+++ b/x86/realmode.c
@@ -1644,7 +1644,7 @@ static void test_perf_memory_load(void)
 {
 	u32 cyc, tmp;
 
-	MK_INSN_PERF(perf_memory_load, "cmp $0, (%edi)");
+	MK_INSN_PERF(perf_memory_load, "cmpw $0, (%edi)");
 
 	init_inregs(&(struct regs){ .edi = (u32)&tmp });
 
@@ -1657,7 +1657,7 @@ static void test_perf_memory_store(void)
 {
 	u32 cyc, tmp;
 
-	MK_INSN_PERF(perf_memory_store, "mov %ax, (%edi)");
+	MK_INSN_PERF(perf_memory_store, "movw %ax, (%edi)");
 
 	init_inregs(&(struct regs){ .edi = (u32)&tmp });
 
@@ -1670,7 +1670,7 @@ static void test_perf_memory_rmw(void)
 {
 	u32 cyc, tmp;
 
-	MK_INSN_PERF(perf_memory_rmw, "add $1, (%edi)");
+	MK_INSN_PERF(perf_memory_rmw, "addw $1, (%edi)");
 
 	init_inregs(&(struct regs){ .edi = (u32)&tmp });
 
diff --git a/x86/syscall.c b/x86/syscall.c
index b4f5ac0..b7e29d6 100644
--- a/x86/syscall.c
+++ b/x86/syscall.c
@@ -38,7 +38,7 @@ static void handle_db(struct ex_regs *regs)
 
 /* expects desired ring 3 flags in rax */
 asm("syscall32_target:\n"
-    "   cmp $0, code_segment_upon_db(%rip)\n"
+    "   cmpl $0, code_segment_upon_db(%rip)\n"
     "   jne back_to_test\n"
     "   mov %eax,%r11d\n"
     "   sysretl\n");
-- 
2.25.0.265.gbab2e86ba0-goog

[kvm-unit-tests PATCH 4/7] svm: change operand to output-only for matching constraint

[Bill Wendling]

According to GNU extended asm documentation, "the two operands [of
matching constraints] must include one input-only operand and one
output-only operand." So remove the read/write modifier from the output
constraint.

Signed-off-by: Bill Wendling <morbo@google.com>
---
 x86/svm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/x86/svm.c b/x86/svm.c
index aa3d995..ae85194 100644
--- a/x86/svm.c
+++ b/x86/svm.c
@@ -288,8 +288,8 @@ static void test_run(struct test *test, struct vmcb *vmcb)
             "cli \n\t"
             "stgi"
             : // inputs clobbered by the guest:
-	      "+D" (the_test),            // first argument register
-	      "+b" (the_vmcb)             // callee save register!
+	      "=D" (the_test),            // first argument register
+	      "=b" (the_vmcb)             // callee save register!
             : [test] "0" (the_test),
 	      [vmcb_phys] "1"(the_vmcb),
 	      [PREPARE_GIF_CLEAR] "i" (offsetof(struct test, prepare_gif_clear))
-- 
2.25.0.265.gbab2e86ba0-goog

[kvm-unit-tests PATCH v2 4/7] vmx: tweak XFAILS for #DB test

[Bill Wendling]

From: Paolo Bonzini <pbonzini@redhat.com>

These were already fixed by KVM_CAP_EXCEPTION_PAYLOAD, but they were failing
on old QEMUs that did not support it.  The recent KVM patch "KVM: x86: Deliver
exception payload on KVM_GET_VCPU_EVENTS" however fixed them even there, so
it is about time to flip the arguments to check_db_exit and avoid ugly XPASS
results with newer versions of QEMU.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Bill Wendling <morbo@google.com>
---
 x86/vmx_tests.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
index b31c360..1323dc5 100644
--- a/x86/vmx_tests.c
+++ b/x86/vmx_tests.c
@@ -8332,7 +8332,7 @@ static void vmx_db_test(void)
 	 * modified DR6, but fails miserably.
 	 */
 	single_step_guest("Software synthesized single-step", starting_dr6, 0);
-	check_db_exit(true, true, false, &post_wbinvd, DR_STEP, starting_dr6);
+	check_db_exit(false, false, false, &post_wbinvd, DR_STEP, starting_dr6);
 
 	/*
 	 * L0 synthesized #DB trap for single-step in MOVSS shadow is
@@ -8342,7 +8342,7 @@ static void vmx_db_test(void)
 	 */
 	single_step_guest("Software synthesized single-step in MOVSS shadow",
 			  starting_dr6, BIT(12) | DR_STEP | DR_TRAP0);
-	check_db_exit(true, true, true, &post_movss_wbinvd, DR_STEP | DR_TRAP0,
+	check_db_exit(true, false, true, &post_movss_wbinvd, DR_STEP | DR_TRAP0,
 		      starting_dr6);
 
 	/*
-- 
2.25.0.265.gbab2e86ba0-goog

[kvm-unit-tests PATCH 5/7] svm: convert neg shift to unsigned shift

[Bill Wendling]

Shifting a negative signed value is undefined. Use a shift of an
unsigned value instead.

Signed-off-by: Bill Wendling <morbo@google.com>
---
 x86/svm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/x86/svm.c b/x86/svm.c
index ae85194..17be4b0 100644
--- a/x86/svm.c
+++ b/x86/svm.c
@@ -1148,7 +1148,7 @@ static bool npt_rw_l1mmio_check(struct test *test)
 }
 
 #define TSC_ADJUST_VALUE    (1ll << 32)
-#define TSC_OFFSET_VALUE    (-1ll << 48)
+#define TSC_OFFSET_VALUE    (~0ull << 48)
 static bool ok;
 
 static void tsc_adjust_prepare(struct test *test)
-- 
2.25.0.265.gbab2e86ba0-goog

[kvm-unit-tests PATCH v2 5/7] x86: VMX: Add tests for monitor trap flag

[Bill Wendling]

From: Oliver Upton <oupton@google.com>

Test to verify that MTF VM-exits into host are synthesized when the
'monitor trap flag' processor-based VM-execution control is set under
various conditions.

Expect an MTF VM-exit if instruction execution produces no events other
than MTF. Should instruction execution produce a concurrent debug-trap
and MTF event, expect an MTF VM-exit with the 'pending debug exceptions'
VMCS field set. Expect an MTF VM-exit to follow event delivery should
instruction execution generate a higher-priority event, such as a
general-protection fault. Lastly, expect an MTF VM-exit to follow
delivery of a debug-trap software exception (INT1/INT3/INTO/INT n).

Signed-off-by: Oliver Upton <oupton@google.com>
Reviewed-by: Jim Mattson <jmattson@google.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Bill Wendling <morbo@google.com>
---
 x86/vmx.h       |   1 +
 x86/vmx_tests.c | 157 ++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 158 insertions(+)

diff --git a/x86/vmx.h b/x86/vmx.h
index 6214400..6adf091 100644
--- a/x86/vmx.h
+++ b/x86/vmx.h
@@ -399,6 +399,7 @@ enum Ctrl0 {
 	CPU_NMI_WINDOW		= 1ul << 22,
 	CPU_IO			= 1ul << 24,
 	CPU_IO_BITMAP		= 1ul << 25,
+	CPU_MTF			= 1ul << 27,
 	CPU_MSR_BITMAP		= 1ul << 28,
 	CPU_MONITOR		= 1ul << 29,
 	CPU_PAUSE		= 1ul << 30,
diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
index 1323dc5..a7abd63 100644
--- a/x86/vmx_tests.c
+++ b/x86/vmx_tests.c
@@ -4970,6 +4970,162 @@ static void test_vmx_preemption_timer(void)
 	vmcs_write(EXI_CONTROLS, saved_exit);
 }
 
+extern unsigned char test_mtf1;
+extern unsigned char test_mtf2;
+extern unsigned char test_mtf3;
+
+__attribute__((noclone)) static void test_mtf_guest(void)
+{
+	asm ("vmcall;\n\t"
+	     "out %al, $0x80;\n\t"
+	     "test_mtf1:\n\t"
+	     "vmcall;\n\t"
+	     "out %al, $0x80;\n\t"
+	     "test_mtf2:\n\t"
+	     /*
+	      * Prepare for the 'MOV CR3' test. Attempt to induce a
+	      * general-protection fault by moving a non-canonical address into
+	      * CR3. The 'MOV CR3' instruction does not take an imm64 operand,
+	      * so we must MOV the desired value into a register first.
+	      *
+	      * MOV RAX is done before the VMCALL such that MTF is only enabled
+	      * for the instruction under test.
+	      */
+	     "mov $0x8000000000000000, %rax;\n\t"
+	     "vmcall;\n\t"
+	     "mov %rax, %cr3;\n\t"
+	     "test_mtf3:\n\t"
+	     "vmcall;\n\t"
+	     /*
+	      * ICEBP/INT1 instruction. Though the instruction is now
+	      * documented, don't rely on assemblers enumerating the
+	      * instruction. Resort to hand assembly.
+	      */
+	     ".byte 0xf1;\n\t");
+}
+
+static void test_mtf_gp_handler(struct ex_regs *regs)
+{
+	regs->rip = (unsigned long) &test_mtf3;
+}
+
+static void test_mtf_db_handler(struct ex_regs *regs)
+{
+}
+
+static void enable_mtf(void)
+{
+	u32 ctrl0 = vmcs_read(CPU_EXEC_CTRL0);
+
+	vmcs_write(CPU_EXEC_CTRL0, ctrl0 | CPU_MTF);
+}
+
+static void disable_mtf(void)
+{
+	u32 ctrl0 = vmcs_read(CPU_EXEC_CTRL0);
+
+	vmcs_write(CPU_EXEC_CTRL0, ctrl0 & ~CPU_MTF);
+}
+
+static void enable_tf(void)
+{
+	unsigned long rflags = vmcs_read(GUEST_RFLAGS);
+
+	vmcs_write(GUEST_RFLAGS, rflags | X86_EFLAGS_TF);
+}
+
+static void disable_tf(void)
+{
+	unsigned long rflags = vmcs_read(GUEST_RFLAGS);
+
+	vmcs_write(GUEST_RFLAGS, rflags & ~X86_EFLAGS_TF);
+}
+
+static void report_mtf(const char *insn_name, unsigned long exp_rip)
+{
+	unsigned long rip = vmcs_read(GUEST_RIP);
+
+	assert_exit_reason(VMX_MTF);
+	report(rip == exp_rip, "MTF VM-exit after %s instruction. RIP: 0x%lx (expected 0x%lx)",
+	       insn_name, rip, exp_rip);
+}
+
+static void vmx_mtf_test(void)
+{
+	unsigned long pending_dbg;
+	handler old_gp, old_db;
+
+	if (!(ctrl_cpu_rev[0].clr & CPU_MTF)) {
+		printf("CPU does not support the 'monitor trap flag' processor-based VM-execution control.\n");
+		return;
+	}
+
+	test_set_guest(test_mtf_guest);
+
+	/* Expect an MTF VM-exit after OUT instruction */
+	enter_guest();
+	skip_exit_vmcall();
+
+	enable_mtf();
+	enter_guest();
+	report_mtf("OUT", (unsigned long) &test_mtf1);
+	disable_mtf();
+
+	/*
+	 * Concurrent #DB trap and MTF on instruction boundary. Expect MTF
+	 * VM-exit with populated 'pending debug exceptions' VMCS field.
+	 */
+	enter_guest();
+	skip_exit_vmcall();
+
+	enable_mtf();
+	enable_tf();
+
+	enter_guest();
+	report_mtf("OUT", (unsigned long) &test_mtf2);
+	pending_dbg = vmcs_read(GUEST_PENDING_DEBUG);
+	report(pending_dbg & DR_STEP,
+	       "'pending debug exceptions' field after MTF VM-exit: 0x%lx (expected 0x%lx)",
+	       pending_dbg, (unsigned long) DR_STEP);
+
+	disable_mtf();
+	disable_tf();
+	vmcs_write(GUEST_PENDING_DEBUG, 0);
+
+	/*
+	 * #GP exception takes priority over MTF. Expect MTF VM-exit with RIP
+	 * advanced to first instruction of #GP handler.
+	 */
+	enter_guest();
+	skip_exit_vmcall();
+
+	old_gp = handle_exception(GP_VECTOR, test_mtf_gp_handler);
+
+	enable_mtf();
+	enter_guest();
+	report_mtf("MOV CR3", (unsigned long) get_idt_addr(&boot_idt[GP_VECTOR]));
+	disable_mtf();
+
+	/*
+	 * Concurrent MTF and privileged software exception (i.e. ICEBP/INT1).
+	 * MTF should follow the delivery of #DB trap, though the SDM doesn't
+	 * provide clear indication of the relative priority.
+	 */
+	enter_guest();
+	skip_exit_vmcall();
+
+	handle_exception(GP_VECTOR, old_gp);
+	old_db = handle_exception(DB_VECTOR, test_mtf_db_handler);
+
+	enable_mtf();
+	enter_guest();
+	report_mtf("INT1", (unsigned long) get_idt_addr(&boot_idt[DB_VECTOR]));
+	disable_mtf();
+
+	enter_guest();
+	handle_exception(DB_VECTOR, old_db);
+}
+
 /*
  * Tests for VM-execution control fields
  */
@@ -9505,5 +9661,6 @@ struct vmx_test vmx_tests[] = {
 	TEST(atomic_switch_max_msrs_test),
 	TEST(atomic_switch_overflow_msrs_test),
 	TEST(rdtsc_vmexit_diff_test),
+	TEST(vmx_mtf_test),
 	{ NULL, NULL, NULL, NULL, NULL, {0} },
 };
-- 
2.25.0.265.gbab2e86ba0-goog

[kvm-unit-tests PATCH v2 6/7] x86: emulator: use "SSE2" for the target

[Bill Wendling]

The movdqu and movapd instructions are SSE2 instructions. Clang
interprets the __attribute__((target("sse"))) as allowing SSE only
instructions. Using SSE2 instructions cause an error.

Signed-off-by: Bill Wendling <morbo@google.com>
---
 x86/emulator.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/x86/emulator.c b/x86/emulator.c
index 8fe03b8..2990550 100644
--- a/x86/emulator.c
+++ b/x86/emulator.c
@@ -658,7 +658,7 @@ static bool sseeq(sse_union *v1, sse_union *v2)
     return ok;
 }
 
-static __attribute__((target("sse"))) void test_sse(sse_union *mem)
+static __attribute__((target("sse2"))) void test_sse(sse_union *mem)
 {
     sse_union v;
 
-- 
2.25.0.265.gbab2e86ba0-goog

[kvm-unit-tests PATCH 6/7] x86: VMX: use inline asm to get stack pointer

[Bill Wendling]

The only supported use of a local register variable is to specify
registers for input and output operands when calling Extended asm. Using
it to automatically collect the value in the register isn't supported as
the contents of the register aren't guaranteed. Instead use inline asm
to get the stack pointer explicitly.

Signed-off-by: Bill Wendling <morbo@google.com>
---
 x86/vmx_tests.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
index a7abd63..ad8c002 100644
--- a/x86/vmx_tests.c
+++ b/x86/vmx_tests.c
@@ -2165,8 +2165,9 @@ static void into_guest_main(void)
 		.offset = (uintptr_t)&&into,
 		.selector = KERNEL_CS32,
 	};
-	register uintptr_t rsp asm("rsp");
+	uintptr_t rsp = 0;
 
+	asm volatile ("mov %%rsp, %0" : "=r" (rsp));
 	if (fp.offset != (uintptr_t)&&into) {
 		printf("Code address too high.\n");
 		return;
@@ -3261,8 +3262,9 @@ static void try_compat_invvpid(void *unused)
 		.offset = (uintptr_t)&&invvpid,
 		.selector = KERNEL_CS32,
 	};
-	register uintptr_t rsp asm("rsp");
+	register uintptr_t rsp = 0;
 
+	asm volatile ("mov %%rsp, %0" : "=r" (rsp));
 	TEST_ASSERT_MSG(fp.offset == (uintptr_t)&&invvpid,
 			"Code address too high.");
 	TEST_ASSERT_MSG(rsp == (u32)rsp, "Stack address too high.");
-- 
2.25.0.265.gbab2e86ba0-goog

[kvm-unit-tests PATCH v2 7/7] pci: cast masks to uint32_t for unsigned long values

[Bill Wendling]

The "pci_bar_*" functions use 64-bit masks, but the results are assigned
to 32-bit variables. Cast mask usages to 32-bit, since we're interested
only in the least significant 4-bits.

Signed-off-by: Bill Wendling <morbo@google.com>
---
 lib/pci.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/pci.c b/lib/pci.c
index daa33e1..1b85411 100644
--- a/lib/pci.c
+++ b/lib/pci.c
@@ -107,7 +107,8 @@ pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id)
 uint32_t pci_bar_mask(uint32_t bar)
 {
 	return (bar & PCI_BASE_ADDRESS_SPACE_IO) ?
-		PCI_BASE_ADDRESS_IO_MASK : PCI_BASE_ADDRESS_MEM_MASK;
+		(uint32_t)PCI_BASE_ADDRESS_IO_MASK :
+		(uint32_t)PCI_BASE_ADDRESS_MEM_MASK;
 }
 
 uint32_t pci_bar_get(struct pci_dev *dev, int bar_num)
-- 
2.25.0.265.gbab2e86ba0-goog

[kvm-unit-tests PATCH 7/7] x86: VMX: the "noclone" attribute is gcc-specific

[Bill Wendling]

Don't use the "noclone" attribute for clang as it's not supported.

Signed-off-by: Bill Wendling <morbo@google.com>
---
 x86/vmx_tests.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
index ad8c002..ec88016 100644
--- a/x86/vmx_tests.c
+++ b/x86/vmx_tests.c
@@ -4976,7 +4976,10 @@ extern unsigned char test_mtf1;
 extern unsigned char test_mtf2;
 extern unsigned char test_mtf3;
 
-__attribute__((noclone)) static void test_mtf_guest(void)
+#ifndef __clang__
+__attribute__((noclone))
+#endif
+static void test_mtf_guest(void)
 {
 	asm ("vmcall;\n\t"
 	     "out %al, $0x80;\n\t"
-- 
2.25.0.265.gbab2e86ba0-goog

Re: [kvm-unit-tests PATCH v2 0/7] Fixes for clang builds

[Oliver Upton]

Hi Bill,

On Wed, Feb 26, 2020 at 1:44 AM Bill Wendling <morbo@google.com> wrote:
>
> Changes to "pci: cast masks to uint32_t for unsigned long values" to
> cast the masks instead of changing the values in the header.
>
> Bill Wendling (2):
>   x86: emulator: use "SSE2" for the target
>   pci: cast masks to uint32_t for unsigned long values
>
> Eric Hankland (2):
>   x86: pmu: Test WRMSR on a running counter
>   x86: pmu: Test perfctr overflow after WRMSR on a running counter
>
> Oliver Upton (1):
>   x86: VMX: Add tests for monitor trap flag
>
> Paolo Bonzini (2):
>   x86: provide enabled and disabled variation of the PCID test
>   vmx: tweak XFAILS for #DB test
>

These patches have already been pushed. Was there any reason to squash
your v1 patches into the commits that introduced the clang issues?
Otherwise, I'd strongly suggest re-posting the v1 series with just the
clang fixes.

Thanks!

--
Best,
Oliver

[kvm-unit-tests PATCH v3 0/7] Fixes for clang builds

[Bill Wendling]

Changes to "pci: cast masks to uint32_t for unsigned long values" to
cast the masks instead of changing the values in the header.

This is a re-send of the v2 patches, which included patches that
shouldn't have been included.

Bill Wendling (7):
  x86: emulator: use "SSE2" for the target
  pci: cast masks to uint32_t for unsigned long values
  x86: realmode: syscall: add explicit size suffix to ambiguous
    instructions
  svm: change operand to output-only for matching constraint
  svm: convert neg shift to unsigned shift
  x86: VMX: use inline asm to get stack pointer
  x86: VMX: the "noclone" attribute is gcc-specific

 lib/pci.c       |  3 ++-
 x86/emulator.c  |  2 +-
 x86/realmode.c  |  6 +++---
 x86/svm.c       |  6 +++---
 x86/syscall.c   |  2 +-
 x86/vmx_tests.c | 11 ++++++++---
 6 files changed, 18 insertions(+), 12 deletions(-)

-- 
2.25.1.481.gfbce0eb801-goog

[kvm-unit-tests PATCH v3 1/7] x86: emulator: use "SSE2" for the target

[Bill Wendling]

The movdqu and movapd instructions are SSE2 instructions. Clang
interprets the __attribute__((target("sse"))) as allowing SSE only
instructions. Using SSE2 instructions cause an error.

Signed-off-by: Bill Wendling <morbo@google.com>
---
 x86/emulator.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/x86/emulator.c b/x86/emulator.c
index 8fe03b8..2990550 100644
--- a/x86/emulator.c
+++ b/x86/emulator.c
@@ -658,7 +658,7 @@ static bool sseeq(sse_union *v1, sse_union *v2)
     return ok;
 }
 
-static __attribute__((target("sse"))) void test_sse(sse_union *mem)
+static __attribute__((target("sse2"))) void test_sse(sse_union *mem)
 {
     sse_union v;
 
-- 
2.25.1.481.gfbce0eb801-goog

[kvm-unit-tests PATCH v3 2/7] pci: cast masks to uint32_t for unsigned long values

[Bill Wendling]

The "pci_bar_*" functions use 64-bit masks, but the results are assigned
to 32-bit variables. Cast mask usages to 32-bit, since we're interested
only in the least significant 4-bits.

Signed-off-by: Bill Wendling <morbo@google.com>
---
 lib/pci.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/pci.c b/lib/pci.c
index daa33e1..1b85411 100644
--- a/lib/pci.c
+++ b/lib/pci.c
@@ -107,7 +107,8 @@ pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id)
 uint32_t pci_bar_mask(uint32_t bar)
 {
 	return (bar & PCI_BASE_ADDRESS_SPACE_IO) ?
-		PCI_BASE_ADDRESS_IO_MASK : PCI_BASE_ADDRESS_MEM_MASK;
+		(uint32_t)PCI_BASE_ADDRESS_IO_MASK :
+		(uint32_t)PCI_BASE_ADDRESS_MEM_MASK;
 }
 
 uint32_t pci_bar_get(struct pci_dev *dev, int bar_num)
-- 
2.25.1.481.gfbce0eb801-goog

[kvm-unit-tests PATCH v3 3/7] x86: realmode: syscall: add explicit size suffix to ambiguous instructions

[Bill Wendling]

Clang requires explicit size suffixes for potentially ambiguous
instructions:

x86/realmode.c:1647:2: error: ambiguous instructions require an explicit suffix (could be 'cmpb', 'cmpw', or 'cmpl')
        MK_INSN_PERF(perf_memory_load, "cmp $0, (%edi)");
        ^
x86/realmode.c:1591:10: note: expanded from macro 'MK_INSN_PERF'
                      "1:" insn "\n"                            \
                       ^
<inline asm>:8:3: note: instantiated into assembly here
1:cmp $0, (%edi)
  ^

The 'w' and 'l' suffixes generate code that's identical to the gcc
version without them.

Signed-off-by: Bill Wendling <morbo@google.com>
---
 x86/realmode.c | 6 +++---
 x86/syscall.c  | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/x86/realmode.c b/x86/realmode.c
index f5967ef..31f84d0 100644
--- a/x86/realmode.c
+++ b/x86/realmode.c
@@ -1644,7 +1644,7 @@ static void test_perf_memory_load(void)
 {
 	u32 cyc, tmp;
 
-	MK_INSN_PERF(perf_memory_load, "cmp $0, (%edi)");
+	MK_INSN_PERF(perf_memory_load, "cmpw $0, (%edi)");
 
 	init_inregs(&(struct regs){ .edi = (u32)&tmp });
 
@@ -1657,7 +1657,7 @@ static void test_perf_memory_store(void)
 {
 	u32 cyc, tmp;
 
-	MK_INSN_PERF(perf_memory_store, "mov %ax, (%edi)");
+	MK_INSN_PERF(perf_memory_store, "movw %ax, (%edi)");
 
 	init_inregs(&(struct regs){ .edi = (u32)&tmp });
 
@@ -1670,7 +1670,7 @@ static void test_perf_memory_rmw(void)
 {
 	u32 cyc, tmp;
 
-	MK_INSN_PERF(perf_memory_rmw, "add $1, (%edi)");
+	MK_INSN_PERF(perf_memory_rmw, "addw $1, (%edi)");
 
 	init_inregs(&(struct regs){ .edi = (u32)&tmp });
 
diff --git a/x86/syscall.c b/x86/syscall.c
index b4f5ac0..b7e29d6 100644
--- a/x86/syscall.c
+++ b/x86/syscall.c
@@ -38,7 +38,7 @@ static void handle_db(struct ex_regs *regs)
 
 /* expects desired ring 3 flags in rax */
 asm("syscall32_target:\n"
-    "   cmp $0, code_segment_upon_db(%rip)\n"
+    "   cmpl $0, code_segment_upon_db(%rip)\n"
     "   jne back_to_test\n"
     "   mov %eax,%r11d\n"
     "   sysretl\n");
-- 
2.25.1.481.gfbce0eb801-goog

[kvm-unit-tests PATCH v3 4/7] svm: change operand to output-only for matching constraint

[Bill Wendling]

According to GNU extended asm documentation, "the two operands [of
matching constraints] must include one input-only operand and one
output-only operand." So remove the read/write modifier from the output
constraint.

Signed-off-by: Bill Wendling <morbo@google.com>
---
 x86/svm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/x86/svm.c b/x86/svm.c
index aa3d995..ae85194 100644
--- a/x86/svm.c
+++ b/x86/svm.c
@@ -288,8 +288,8 @@ static void test_run(struct test *test, struct vmcb *vmcb)
             "cli \n\t"
             "stgi"
             : // inputs clobbered by the guest:
-	      "+D" (the_test),            // first argument register
-	      "+b" (the_vmcb)             // callee save register!
+	      "=D" (the_test),            // first argument register
+	      "=b" (the_vmcb)             // callee save register!
             : [test] "0" (the_test),
 	      [vmcb_phys] "1"(the_vmcb),
 	      [PREPARE_GIF_CLEAR] "i" (offsetof(struct test, prepare_gif_clear))
-- 
2.25.1.481.gfbce0eb801-goog

[kvm-unit-tests PATCH v3 5/7] svm: convert neg shift to unsigned shift

[Bill Wendling]

Shifting a negative signed value is undefined. Use a shift of an
unsigned value instead.

Signed-off-by: Bill Wendling <morbo@google.com>
---
 x86/svm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/x86/svm.c b/x86/svm.c
index ae85194..17be4b0 100644
--- a/x86/svm.c
+++ b/x86/svm.c
@@ -1148,7 +1148,7 @@ static bool npt_rw_l1mmio_check(struct test *test)
 }
 
 #define TSC_ADJUST_VALUE    (1ll << 32)
-#define TSC_OFFSET_VALUE    (-1ll << 48)
+#define TSC_OFFSET_VALUE    (~0ull << 48)
 static bool ok;
 
 static void tsc_adjust_prepare(struct test *test)
-- 
2.25.1.481.gfbce0eb801-goog

[kvm-unit-tests PATCH v3 6/7] x86: VMX: use inline asm to get stack pointer

[Bill Wendling]

The only supported use of a local register variable is to specify
registers for input and output operands when calling Extended asm. Using
it to automatically collect the value in the register isn't supported as
the contents of the register aren't guaranteed. Instead use inline asm
to get the stack pointer explicitly.

Signed-off-by: Bill Wendling <morbo@google.com>
---
 x86/vmx_tests.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
index a7abd63..ad8c002 100644
--- a/x86/vmx_tests.c
+++ b/x86/vmx_tests.c
@@ -2165,8 +2165,9 @@ static void into_guest_main(void)
 		.offset = (uintptr_t)&&into,
 		.selector = KERNEL_CS32,
 	};
-	register uintptr_t rsp asm("rsp");
+	uintptr_t rsp = 0;
 
+	asm volatile ("mov %%rsp, %0" : "=r" (rsp));
 	if (fp.offset != (uintptr_t)&&into) {
 		printf("Code address too high.\n");
 		return;
@@ -3261,8 +3262,9 @@ static void try_compat_invvpid(void *unused)
 		.offset = (uintptr_t)&&invvpid,
 		.selector = KERNEL_CS32,
 	};
-	register uintptr_t rsp asm("rsp");
+	register uintptr_t rsp = 0;
 
+	asm volatile ("mov %%rsp, %0" : "=r" (rsp));
 	TEST_ASSERT_MSG(fp.offset == (uintptr_t)&&invvpid,
 			"Code address too high.");
 	TEST_ASSERT_MSG(rsp == (u32)rsp, "Stack address too high.");
-- 
2.25.1.481.gfbce0eb801-goog

[kvm-unit-tests PATCH v3 7/7] x86: VMX: the "noclone" attribute is gcc-specific

[Bill Wendling]

Don't use the "noclone" attribute for clang as it's not supported.

Signed-off-by: Bill Wendling <morbo@google.com>
---
 x86/vmx_tests.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
index ad8c002..ec88016 100644
--- a/x86/vmx_tests.c
+++ b/x86/vmx_tests.c
@@ -4976,7 +4976,10 @@ extern unsigned char test_mtf1;
 extern unsigned char test_mtf2;
 extern unsigned char test_mtf3;
 
-__attribute__((noclone)) static void test_mtf_guest(void)
+#ifndef __clang__
+__attribute__((noclone))
+#endif
+static void test_mtf_guest(void)
 {
 	asm ("vmcall;\n\t"
 	     "out %al, $0x80;\n\t"
-- 
2.25.1.481.gfbce0eb801-goog

Re: [kvm-unit-tests PATCH v3 7/7] x86: VMX: the "noclone" attribute is gcc-specific

[Oliver Upton]

Hi Bill,

Perhaps it would be better to drill down the noclone attribute by GCC
version as well. May I suggest the following?

diff --git a/lib/compiler.h b/lib/compiler.h
new file mode 100644
index 000000000000..5cbcda94b0fe
--- /dev/null
+++ b/lib/compiler.h
@@ -0,0 +1,12 @@
+#ifndef _LIB_COMPILER_H_
+#define _LIB_COMPILER_H_
+
+#if GCC_VERSION >= 40500
+#define __noclone      __attribute__((__noclone__, __optimize__("no-tracer")))
+#endif /* GCC_VERSION >= 40500 */
+
+#if !defined(__noclone)
+#define __noclone
+#endif
+
+#endif /* _LIB_COMPILER_H_ */
diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
index 0e2c2f8a7d34..2dfc010d5d49 100644
--- a/x86/vmx_tests.c
+++ b/x86/vmx_tests.c
@@ -5,6 +5,7 @@
  */

 #include <asm/debugreg.h>
+#include <compiler.h>

 #include "vmx.h"
 #include "msr.h"
@@ -4974,7 +4975,7 @@ extern unsigned char test_mtf1;
 extern unsigned char test_mtf2;
 extern unsigned char test_mtf3;

-__attribute__((noclone)) static void test_mtf_guest(void)
+__noclone static void test_mtf_guest(void)
 {
        asm ("vmcall;\n\t"
             "out %al, $0x80;\n\t"
-- 
Thanks,
Oliver

On Wed, Feb 26, 2020 at 12:13 PM Bill Wendling <morbo@google.com> wrote:
>
> Don't use the "noclone" attribute for clang as it's not supported.
>
> Signed-off-by: Bill Wendling <morbo@google.com>
> ---
>  x86/vmx_tests.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
> index ad8c002..ec88016 100644
> --- a/x86/vmx_tests.c
> +++ b/x86/vmx_tests.c
> @@ -4976,7 +4976,10 @@ extern unsigned char test_mtf1;
>  extern unsigned char test_mtf2;
>  extern unsigned char test_mtf3;
>
> -__attribute__((noclone)) static void test_mtf_guest(void)
> +#ifndef __clang__
> +__attribute__((noclone))
> +#endif
> +static void test_mtf_guest(void)
>  {
>         asm ("vmcall;\n\t"
>              "out %al, $0x80;\n\t"
> --
> 2.25.1.481.gfbce0eb801-goog
>

Re: [kvm-unit-tests PATCH 2/7] pci: use uint32_t for unsigned long values

[Paolo Bonzini]

On 26/02/20 10:44, Bill Wendling wrote:
> The "pci_bar_*" functions use 64-bit masks, but the results are assigned
> to 32-bit variables. Use 32-bit masks, since we're interested only in
> the least significant 4-bits.
> 
> Signed-off-by: Bill Wendling <morbo@google.com>
> ---
>  lib/linux/pci_regs.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/linux/pci_regs.h b/lib/linux/pci_regs.h
> index 1becea8..3bc2b92 100644
> --- a/lib/linux/pci_regs.h
> +++ b/lib/linux/pci_regs.h
> @@ -96,8 +96,8 @@
>  #define  PCI_BASE_ADDRESS_MEM_TYPE_1M	0x02	/* Below 1M [obsolete] */
>  #define  PCI_BASE_ADDRESS_MEM_TYPE_64	0x04	/* 64 bit address */
>  #define  PCI_BASE_ADDRESS_MEM_PREFETCH	0x08	/* prefetchable? */
> -#define  PCI_BASE_ADDRESS_MEM_MASK	(~0x0fUL)
> -#define  PCI_BASE_ADDRESS_IO_MASK	(~0x03UL)
> +#define  PCI_BASE_ADDRESS_MEM_MASK	(~0x0fU)
> +#define  PCI_BASE_ADDRESS_IO_MASK	(~0x03U)
>  /* bit 1 is reserved if address_space = 1 */
>  
>  /* Header type 0 (normal devices) */
> 

Removing the "U" is even better because it will then sign-extend
automatically.

Paolo

Re: [kvm-unit-tests PATCH 5/7] svm: convert neg shift to unsigned shift

[Paolo Bonzini]

On 26/02/20 10:44, Bill Wendling wrote:
> Shifting a negative signed value is undefined. Use a shift of an
> unsigned value instead.
> 
> Signed-off-by: Bill Wendling <morbo@google.com>

In practice only UBSAN complains; GCC even documents that it does not
treat it as undefined (which would be insane, especially if the
multiplication by 2 does not overflow as in this case):

     GCC does not use the latitude given in C99 only to treat certain
     aspects of signed '<<' as undefined, but this is subject to change.

But in the interest of purity we should instead compile with -fwrapv.

Paolo

> ---
>  x86/svm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/x86/svm.c b/x86/svm.c
> index ae85194..17be4b0 100644
> --- a/x86/svm.c
> +++ b/x86/svm.c
> @@ -1148,7 +1148,7 @@ static bool npt_rw_l1mmio_check(struct test *test)
>  }
>  
>  #define TSC_ADJUST_VALUE    (1ll << 32)
> -#define TSC_OFFSET_VALUE    (-1ll << 48)
> +#define TSC_OFFSET_VALUE    (~0ull << 48)
>  static bool ok;
>  
>  static void tsc_adjust_prepare(struct test *test)
> 

Re: [kvm-unit-tests PATCH 7/7] x86: VMX: the "noclone" attribute is gcc-specific

[Paolo Bonzini]

On 26/02/20 10:44, Bill Wendling wrote:
> Don't use the "noclone" attribute for clang as it's not supported.
> 
> Signed-off-by: Bill Wendling <morbo@google.com>
> ---
>  x86/vmx_tests.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
> index ad8c002..ec88016 100644
> --- a/x86/vmx_tests.c
> +++ b/x86/vmx_tests.c
> @@ -4976,7 +4976,10 @@ extern unsigned char test_mtf1;
>  extern unsigned char test_mtf2;
>  extern unsigned char test_mtf3;
>  
> -__attribute__((noclone)) static void test_mtf_guest(void)
> +#ifndef __clang__
> +__attribute__((noclone))
> +#endif
> +static void test_mtf_guest(void)
>  {
>  	asm ("vmcall;\n\t"
>  	     "out %al, $0x80;\n\t"
> 

It's not needed at all, let's drop it.

Paolo

Re: [kvm-unit-tests PATCH v2 0/7] Fixes for clang builds

[Paolo Bonzini]

On 26/02/20 10:44, Bill Wendling wrote:
> Changes to "pci: cast masks to uint32_t for unsigned long values" to
> cast the masks instead of changing the values in the header.

Applied 1-3-4 and a tweaked version of 2 and 7.

For 5, let's use -fwrapv instead.

For 6, clang people should stop implementing GCC extensions in half and
declaring themselves compatible.  It's just a sanity check, you can wrap
it all with #ifndef __clang__.

Paolo

Re: [kvm-unit-tests PATCH 2/7] pci: use uint32_t for unsigned long values

[Andrew Jones]

On Fri, Feb 28, 2020 at 12:04:38PM +0100, Paolo Bonzini wrote:
> On 26/02/20 10:44, Bill Wendling wrote:
> > The "pci_bar_*" functions use 64-bit masks, but the results are assigned
> > to 32-bit variables. Use 32-bit masks, since we're interested only in
> > the least significant 4-bits.
> > 
> > Signed-off-by: Bill Wendling <morbo@google.com>
> > ---
> >  lib/linux/pci_regs.h | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/lib/linux/pci_regs.h b/lib/linux/pci_regs.h
> > index 1becea8..3bc2b92 100644
> > --- a/lib/linux/pci_regs.h
> > +++ b/lib/linux/pci_regs.h
> > @@ -96,8 +96,8 @@
> >  #define  PCI_BASE_ADDRESS_MEM_TYPE_1M	0x02	/* Below 1M [obsolete] */
> >  #define  PCI_BASE_ADDRESS_MEM_TYPE_64	0x04	/* 64 bit address */
> >  #define  PCI_BASE_ADDRESS_MEM_PREFETCH	0x08	/* prefetchable? */
> > -#define  PCI_BASE_ADDRESS_MEM_MASK	(~0x0fUL)
> > -#define  PCI_BASE_ADDRESS_IO_MASK	(~0x03UL)
> > +#define  PCI_BASE_ADDRESS_MEM_MASK	(~0x0fU)
> > +#define  PCI_BASE_ADDRESS_IO_MASK	(~0x03U)
> >  /* bit 1 is reserved if address_space = 1 */
> >  
> >  /* Header type 0 (normal devices) */
> > 
> 
> Removing the "U" is even better because it will then sign-extend
> automatically.
>

We don't want this patch at all though. We shouldn't change pci_regs.h
since it comes from linux and someday we may update again and lose
any changes we make. We should change how these masks are used instead.

Thanks,
drew

Re: [kvm-unit-tests PATCH 2/7] pci: use uint32_t for unsigned long values

[Paolo Bonzini]

On 28/02/20 13:46, Andrew Jones wrote:
> On Fri, Feb 28, 2020 at 12:04:38PM +0100, Paolo Bonzini wrote:
>> On 26/02/20 10:44, Bill Wendling wrote:
>>> The "pci_bar_*" functions use 64-bit masks, but the results are assigned
>>> to 32-bit variables. Use 32-bit masks, since we're interested only in
>>> the least significant 4-bits.
>>>
>>> Signed-off-by: Bill Wendling <morbo@google.com>
>>> ---
>>>  lib/linux/pci_regs.h | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/lib/linux/pci_regs.h b/lib/linux/pci_regs.h
>>> index 1becea8..3bc2b92 100644
>>> --- a/lib/linux/pci_regs.h
>>> +++ b/lib/linux/pci_regs.h
>>> @@ -96,8 +96,8 @@
>>>  #define  PCI_BASE_ADDRESS_MEM_TYPE_1M	0x02	/* Below 1M [obsolete] */
>>>  #define  PCI_BASE_ADDRESS_MEM_TYPE_64	0x04	/* 64 bit address */
>>>  #define  PCI_BASE_ADDRESS_MEM_PREFETCH	0x08	/* prefetchable? */
>>> -#define  PCI_BASE_ADDRESS_MEM_MASK	(~0x0fUL)
>>> -#define  PCI_BASE_ADDRESS_IO_MASK	(~0x03UL)
>>> +#define  PCI_BASE_ADDRESS_MEM_MASK	(~0x0fU)
>>> +#define  PCI_BASE_ADDRESS_IO_MASK	(~0x03U)
>>>  /* bit 1 is reserved if address_space = 1 */
>>>  
>>>  /* Header type 0 (normal devices) */
>>>
>>
>> Removing the "U" is even better because it will then sign-extend
>> automatically.
>>
> 
> We don't want this patch at all though. We shouldn't change pci_regs.h
> since it comes from linux and someday we may update again and lose
> any changes we make. We should change how these masks are used instead.

I will try to get the change into Linux.

Paolo

Re: [kvm-unit-tests PATCH 7/7] x86: VMX: the "noclone" attribute is gcc-specific

[Oliver Upton]

On Fri, Feb 28, 2020 at 3:16 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 26/02/20 10:44, Bill Wendling wrote:
> > Don't use the "noclone" attribute for clang as it's not supported.
> >
> > Signed-off-by: Bill Wendling <morbo@google.com>
> > ---
> >  x86/vmx_tests.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
> > index ad8c002..ec88016 100644
> > --- a/x86/vmx_tests.c
> > +++ b/x86/vmx_tests.c
> > @@ -4976,7 +4976,10 @@ extern unsigned char test_mtf1;
> >  extern unsigned char test_mtf2;
> >  extern unsigned char test_mtf3;
> >
> > -__attribute__((noclone)) static void test_mtf_guest(void)
> > +#ifndef __clang__
> > +__attribute__((noclone))
> > +#endif
> > +static void test_mtf_guest(void)
> >  {
> >       asm ("vmcall;\n\t"
> >            "out %al, $0x80;\n\t"
> >
>
> It's not needed at all, let's drop it.
>
> Paolo
>

If we wanted to be absolutely certain that the extern labels used for
assertions about the guest RIP are correct, we may still want it.
Alternatively, I could rewrite the test such that the guest will
report the instruction boundary where it anticipates MTF whenever it
makes the vmcall to request the host turns on MTF.

--
Thanks,
Oliver

Re: [kvm-unit-tests PATCH 7/7] x86: VMX: the "noclone" attribute is gcc-specific

[Paolo Bonzini]

On 28/02/20 19:19, Oliver Upton wrote:
> If we wanted to be absolutely certain that the extern labels used for
> assertions about the guest RIP are correct, we may still want it.
> Alternatively, I could rewrite the test such that the guest will
> report the instruction boundary where it anticipates MTF whenever it
> makes the vmcall to request the host turns on MTF.

Right, but it's used only once, and as a function pointer at that, so
there's only so much that the compiler can do to "optimize".  Let's
think about how to fix it if it breaks.

Paolo

Re: [kvm-unit-tests PATCH 7/7] x86: VMX: the "noclone" attribute is gcc-specific

[Oliver Upton]

On Fri, Feb 28, 2020 at 10:24 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 28/02/20 19:19, Oliver Upton wrote:
> > If we wanted to be absolutely certain that the extern labels used for
> > assertions about the guest RIP are correct, we may still want it.
> > Alternatively, I could rewrite the test such that the guest will
> > report the instruction boundary where it anticipates MTF whenever it
> > makes the vmcall to request the host turns on MTF.
>
> Right, but it's used only once, and as a function pointer at that, so
> there's only so much that the compiler can do to "optimize".  Let's
> think about how to fix it if it breaks.
>
> Paolo
>

Very true. Unchecked paranoia of the compiler isn't warranted here.

Thanks :)

--
Best,
Oliver