[kvm-unit-tests PATCH] x86/memory: pass host clwb and clflushopt support information

[kvm-unit-tests PATCH] x86/memory: pass host clwb and clflushopt support information

[Haozhong Zhang]

Intel VMX cannot intercept guest clwb and clflushopt. When clwb and
clflushopt are not exposed in guest cpuid, clwb and clflushopt
instructions in this test case can still succeed without #UD on the
host CPU which has clwb and clflushopt support, though failures with
UD are expected.

In order to avoid false alarms in such cases, introduce the following
two arguments "has_clwb" and "has_clflushopt" to allow users to
specify whether clwb and clflushopt are supported on the host CPU.

Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
---
 x86/memory.c | 29 +++++++++++++++++++++++++++--
 1 file changed, 27 insertions(+), 2 deletions(-)

diff --git a/x86/memory.c b/x86/memory.c
index cd1eb46..03ff7d3 100644
--- a/x86/memory.c
+++ b/x86/memory.c
@@ -23,10 +23,29 @@ static void handle_ud(struct ex_regs *regs)
 	regs->rip += isize;
 }
 
+/*
+ * Intel VMX cannot intercept guest clwb and clflushopt. When clwb and
+ * clflushopt are not exposed in guest cpuid, clwb and clflushopt
+ * instructions in this test case can still succeed without #UD on
+ * the host CPU which has clwb and clflushopt support. In order to avoid
+ * false alarms in such cases, introduce the following two arguments
+ * to allow users to specify whether clwb and clflushopt are supported on
+ * the host CPU:
+ * - has_clwb:       indicates clwb is supported on the host CPU
+ * - has_clflushopt: indicates clflushopt is supported on the host CPU
+ */
 int main(int ac, char **av)
 {
 	struct cpuid cpuid7, cpuid1;
 	int xfail;
+	int host_has_clwb = 0, host_has_clflushopt = 0; /* 0: unknown */
+	int i;
+
+	for (i = 1; i < ac; i++)
+		if (!strcmp(av[i], "has_clwb"))
+			host_has_clwb = 1;
+		else if (!strcmp(av[i], "has_clflushopt"))
+			host_has_clflushopt = 1;
 
 	setup_idt();
 	handle_exception(UD_VECTOR, handle_ud);
@@ -63,13 +82,19 @@ int main(int ac, char **av)
 	ud = 0;
 	/* clflushopt (%rbx): */
 	asm volatile(".byte 0x66, 0x0f, 0xae, 0x3b" : : "b" (&target));
-	report_xfail("clflushopt", xfail, ud == 0);
+	if (host_has_clflushopt)
+		report("clflushopt", ud == 0);
+	else
+		report_xfail("clflushopt", xfail, ud == 0);
 
 	xfail = !(cpuid7.b & (1U << 24)); /* CLWB */
 	ud = 0;
 	/* clwb (%rbx): */
 	asm volatile(".byte 0x66, 0x0f, 0xae, 0x33" : : "b" (&target));
-	report_xfail("clwb", xfail, ud == 0);
+	if (host_has_clwb)
+		report("clwb", ud == 0);
+	else
+		report_xfail("clwb", xfail, ud == 0);
 
 	ud = 0;
 	/* clwb requires a memory operand, the following is NOT a valid
-- 
2.14.1

Re: [kvm-unit-tests PATCH] x86/memory: pass host clwb and clflushopt support information

[Eduardo Habkost]

On Tue, Dec 12, 2017 at 04:35:24PM +0800, Haozhong Zhang wrote:
> Intel VMX cannot intercept guest clwb and clflushopt. When clwb and
> clflushopt are not exposed in guest cpuid, clwb and clflushopt
> instructions in this test case can still succeed without #UD on the
> host CPU which has clwb and clflushopt support, though failures with
> UD are expected.
> 
> In order to avoid false alarms in such cases, introduce the following
> two arguments "has_clwb" and "has_clflushopt" to allow users to
> specify whether clwb and clflushopt are supported on the host CPU.
> 
> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> ---
>  x86/memory.c | 29 +++++++++++++++++++++++++++--
>  1 file changed, 27 insertions(+), 2 deletions(-)
> 
> diff --git a/x86/memory.c b/x86/memory.c
> index cd1eb46..03ff7d3 100644
> --- a/x86/memory.c
> +++ b/x86/memory.c
> @@ -23,10 +23,29 @@ static void handle_ud(struct ex_regs *regs)
>  	regs->rip += isize;
>  }
>  
> +/*
> + * Intel VMX cannot intercept guest clwb and clflushopt. When clwb and
> + * clflushopt are not exposed in guest cpuid, clwb and clflushopt
> + * instructions in this test case can still succeed without #UD on
> + * the host CPU which has clwb and clflushopt support. In order to avoid
> + * false alarms in such cases, introduce the following two arguments
> + * to allow users to specify whether clwb and clflushopt are supported on
> + * the host CPU:
> + * - has_clwb:       indicates clwb is supported on the host CPU
> + * - has_clflushopt: indicates clflushopt is supported on the host CPU
> + */

Why not simply use "-cpu host" to make sure the guest CPUID flags
match host CPUID?


>  int main(int ac, char **av)
>  {
>  	struct cpuid cpuid7, cpuid1;
>  	int xfail;
> +	int host_has_clwb = 0, host_has_clflushopt = 0; /* 0: unknown */
> +	int i;
> +
> +	for (i = 1; i < ac; i++)
> +		if (!strcmp(av[i], "has_clwb"))
> +			host_has_clwb = 1;
> +		else if (!strcmp(av[i], "has_clflushopt"))
> +			host_has_clflushopt = 1;
>  
>  	setup_idt();
>  	handle_exception(UD_VECTOR, handle_ud);
> @@ -63,13 +82,19 @@ int main(int ac, char **av)
>  	ud = 0;
>  	/* clflushopt (%rbx): */
>  	asm volatile(".byte 0x66, 0x0f, 0xae, 0x3b" : : "b" (&target));
> -	report_xfail("clflushopt", xfail, ud == 0);
> +	if (host_has_clflushopt)
> +		report("clflushopt", ud == 0);
> +	else
> +		report_xfail("clflushopt", xfail, ud == 0);
>  
>  	xfail = !(cpuid7.b & (1U << 24)); /* CLWB */
>  	ud = 0;
>  	/* clwb (%rbx): */
>  	asm volatile(".byte 0x66, 0x0f, 0xae, 0x33" : : "b" (&target));
> -	report_xfail("clwb", xfail, ud == 0);
> +	if (host_has_clwb)
> +		report("clwb", ud == 0);
> +	else
> +		report_xfail("clwb", xfail, ud == 0);
>  
>  	ud = 0;
>  	/* clwb requires a memory operand, the following is NOT a valid
> -- 
> 2.14.1
> 

-- 
Eduardo

Re: [kvm-unit-tests PATCH] x86/memory: pass host clwb and clflushopt support information

[Haozhong Zhang]

On 12/18/17 16:36 -0200, Eduardo Habkost wrote:
> On Tue, Dec 12, 2017 at 04:35:24PM +0800, Haozhong Zhang wrote:
> > Intel VMX cannot intercept guest clwb and clflushopt. When clwb and
> > clflushopt are not exposed in guest cpuid, clwb and clflushopt
> > instructions in this test case can still succeed without #UD on the
> > host CPU which has clwb and clflushopt support, though failures with
> > UD are expected.
> > 
> > In order to avoid false alarms in such cases, introduce the following
> > two arguments "has_clwb" and "has_clflushopt" to allow users to
> > specify whether clwb and clflushopt are supported on the host CPU.
> > 
> > Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> > ---
> >  x86/memory.c | 29 +++++++++++++++++++++++++++--
> >  1 file changed, 27 insertions(+), 2 deletions(-)
> > 
> > diff --git a/x86/memory.c b/x86/memory.c
> > index cd1eb46..03ff7d3 100644
> > --- a/x86/memory.c
> > +++ b/x86/memory.c
> > @@ -23,10 +23,29 @@ static void handle_ud(struct ex_regs *regs)
> >  	regs->rip += isize;
> >  }
> >  
> > +/*
> > + * Intel VMX cannot intercept guest clwb and clflushopt. When clwb and
> > + * clflushopt are not exposed in guest cpuid, clwb and clflushopt
> > + * instructions in this test case can still succeed without #UD on
> > + * the host CPU which has clwb and clflushopt support. In order to avoid
> > + * false alarms in such cases, introduce the following two arguments
> > + * to allow users to specify whether clwb and clflushopt are supported on
> > + * the host CPU:
> > + * - has_clwb:       indicates clwb is supported on the host CPU
> > + * - has_clflushopt: indicates clflushopt is supported on the host CPU
> > + */
> 
> Why not simply use "-cpu host" to make sure the guest CPUID flags
> match host CPUID?
>

Can I understand that testing these two cases with host/guest CPUID
mismatch (specially clwb and clflushopt flags) is invalid? If yes,
please ignore this patch.

Thanks,
Haozhong

> 
> >  int main(int ac, char **av)
> >  {
> >  	struct cpuid cpuid7, cpuid1;
> >  	int xfail;
> > +	int host_has_clwb = 0, host_has_clflushopt = 0; /* 0: unknown */
> > +	int i;
> > +
> > +	for (i = 1; i < ac; i++)
> > +		if (!strcmp(av[i], "has_clwb"))
> > +			host_has_clwb = 1;
> > +		else if (!strcmp(av[i], "has_clflushopt"))
> > +			host_has_clflushopt = 1;
> >  
> >  	setup_idt();
> >  	handle_exception(UD_VECTOR, handle_ud);
> > @@ -63,13 +82,19 @@ int main(int ac, char **av)
> >  	ud = 0;
> >  	/* clflushopt (%rbx): */
> >  	asm volatile(".byte 0x66, 0x0f, 0xae, 0x3b" : : "b" (&target));
> > -	report_xfail("clflushopt", xfail, ud == 0);
> > +	if (host_has_clflushopt)
> > +		report("clflushopt", ud == 0);
> > +	else
> > +		report_xfail("clflushopt", xfail, ud == 0);
> >  
> >  	xfail = !(cpuid7.b & (1U << 24)); /* CLWB */
> >  	ud = 0;
> >  	/* clwb (%rbx): */
> >  	asm volatile(".byte 0x66, 0x0f, 0xae, 0x33" : : "b" (&target));
> > -	report_xfail("clwb", xfail, ud == 0);
> > +	if (host_has_clwb)
> > +		report("clwb", ud == 0);
> > +	else
> > +		report_xfail("clwb", xfail, ud == 0);
> >  
> >  	ud = 0;
> >  	/* clwb requires a memory operand, the following is NOT a valid
> > -- 
> > 2.14.1
> > 
> 
> -- 
> Eduardo

Re: [kvm-unit-tests PATCH] x86/memory: pass host clwb and clflushopt support information

[Eduardo Habkost]

On Tue, Dec 19, 2017 at 10:54:16AM +0800, Haozhong Zhang wrote:
> On 12/18/17 16:36 -0200, Eduardo Habkost wrote:
> > On Tue, Dec 12, 2017 at 04:35:24PM +0800, Haozhong Zhang wrote:
> > > Intel VMX cannot intercept guest clwb and clflushopt. When clwb and
> > > clflushopt are not exposed in guest cpuid, clwb and clflushopt
> > > instructions in this test case can still succeed without #UD on the
> > > host CPU which has clwb and clflushopt support, though failures with
> > > UD are expected.
> > > 
> > > In order to avoid false alarms in such cases, introduce the following
> > > two arguments "has_clwb" and "has_clflushopt" to allow users to
> > > specify whether clwb and clflushopt are supported on the host CPU.
> > > 
> > > Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> > > ---
> > >  x86/memory.c | 29 +++++++++++++++++++++++++++--
> > >  1 file changed, 27 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/x86/memory.c b/x86/memory.c
> > > index cd1eb46..03ff7d3 100644
> > > --- a/x86/memory.c
> > > +++ b/x86/memory.c
> > > @@ -23,10 +23,29 @@ static void handle_ud(struct ex_regs *regs)
> > >  	regs->rip += isize;
> > >  }
> > >  
> > > +/*
> > > + * Intel VMX cannot intercept guest clwb and clflushopt. When clwb and
> > > + * clflushopt are not exposed in guest cpuid, clwb and clflushopt
> > > + * instructions in this test case can still succeed without #UD on
> > > + * the host CPU which has clwb and clflushopt support. In order to avoid
> > > + * false alarms in such cases, introduce the following two arguments
> > > + * to allow users to specify whether clwb and clflushopt are supported on
> > > + * the host CPU:
> > > + * - has_clwb:       indicates clwb is supported on the host CPU
> > > + * - has_clflushopt: indicates clflushopt is supported on the host CPU
> > > + */
> > 
> > Why not simply use "-cpu host" to make sure the guest CPUID flags
> > match host CPUID?
> >
> 
> Can I understand that testing these two cases with host/guest CPUID
> mismatch (specially clwb and clflushopt flags) is invalid? If yes,
> please ignore this patch.

I wouldn't say it's invalid to test what happens when the host
and guest CPUID don't match.  The question is: is it useful to do
so?  Are we testing different code paths when we do that?

The inability to trigger #UD if the host CPUID includes the flag
sounds like a bug/limitation we would like to get rid of as soon
as hardware allow us to, and not a feature we need to test for.

What's the right way to ensure memory.flat is always tested using
"-cpu host"?

-- 
Eduardo

Re: [kvm-unit-tests PATCH] x86/memory: pass host clwb and clflushopt support information

[Haozhong Zhang]

On 12/20/17 19:26 -0200, Eduardo Habkost wrote:
> On Tue, Dec 19, 2017 at 10:54:16AM +0800, Haozhong Zhang wrote:
> > On 12/18/17 16:36 -0200, Eduardo Habkost wrote:
> > > On Tue, Dec 12, 2017 at 04:35:24PM +0800, Haozhong Zhang wrote:
> > > > Intel VMX cannot intercept guest clwb and clflushopt. When clwb and
> > > > clflushopt are not exposed in guest cpuid, clwb and clflushopt
> > > > instructions in this test case can still succeed without #UD on the
> > > > host CPU which has clwb and clflushopt support, though failures with
> > > > UD are expected.
> > > > 
> > > > In order to avoid false alarms in such cases, introduce the following
> > > > two arguments "has_clwb" and "has_clflushopt" to allow users to
> > > > specify whether clwb and clflushopt are supported on the host CPU.
> > > > 
> > > > Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> > > > ---
> > > >  x86/memory.c | 29 +++++++++++++++++++++++++++--
> > > >  1 file changed, 27 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/x86/memory.c b/x86/memory.c
> > > > index cd1eb46..03ff7d3 100644
> > > > --- a/x86/memory.c
> > > > +++ b/x86/memory.c
> > > > @@ -23,10 +23,29 @@ static void handle_ud(struct ex_regs *regs)
> > > >  	regs->rip += isize;
> > > >  }
> > > >  
> > > > +/*
> > > > + * Intel VMX cannot intercept guest clwb and clflushopt. When clwb and
> > > > + * clflushopt are not exposed in guest cpuid, clwb and clflushopt
> > > > + * instructions in this test case can still succeed without #UD on
> > > > + * the host CPU which has clwb and clflushopt support. In order to avoid
> > > > + * false alarms in such cases, introduce the following two arguments
> > > > + * to allow users to specify whether clwb and clflushopt are supported on
> > > > + * the host CPU:
> > > > + * - has_clwb:       indicates clwb is supported on the host CPU
> > > > + * - has_clflushopt: indicates clflushopt is supported on the host CPU
> > > > + */
> > > 
> > > Why not simply use "-cpu host" to make sure the guest CPUID flags
> > > match host CPUID?
> > >
> > 
> > Can I understand that testing these two cases with host/guest CPUID
> > mismatch (specially clwb and clflushopt flags) is invalid? If yes,
> > please ignore this patch.
> 
> I wouldn't say it's invalid to test what happens when the host
> and guest CPUID don't match.  The question is: is it useful to do
> so?  Are we testing different code paths when we do that?
>

No, VMX cannot intercept guest clwb and clflushopt, so no KVM code
path is involved when guest executes those two instructions.

> The inability to trigger #UD if the host CPUID includes the flag
> sounds like a bug/limitation we would like to get rid of as soon
> as hardware allow us to, and not a feature we need to test for.
>

It's more the inability of VMX, which cannot intercept clwb and
clflushopt.

> What's the right way to ensure memory.flat is always tested using
> "-cpu host"?
>

I think so, at least '-cpu host' can mitigate the hardware inability.   

Haozhong

Re: [kvm-unit-tests PATCH] x86/memory: pass host clwb and clflushopt support information

[Jim Mattson]

This isn't anything new. The same thing happened with ADX instructions
on Broadwell, MOVBE on Haswell, F16C instructions on Ivebridge, etc.

On Wed, Dec 20, 2017 at 5:08 PM, Haozhong Zhang
<haozhong.zhang@intel.com> wrote:
> On 12/20/17 19:26 -0200, Eduardo Habkost wrote:
>> On Tue, Dec 19, 2017 at 10:54:16AM +0800, Haozhong Zhang wrote:
>> > On 12/18/17 16:36 -0200, Eduardo Habkost wrote:
>> > > On Tue, Dec 12, 2017 at 04:35:24PM +0800, Haozhong Zhang wrote:
>> > > > Intel VMX cannot intercept guest clwb and clflushopt. When clwb and
>> > > > clflushopt are not exposed in guest cpuid, clwb and clflushopt
>> > > > instructions in this test case can still succeed without #UD on the
>> > > > host CPU which has clwb and clflushopt support, though failures with
>> > > > UD are expected.
>> > > >
>> > > > In order to avoid false alarms in such cases, introduce the following
>> > > > two arguments "has_clwb" and "has_clflushopt" to allow users to
>> > > > specify whether clwb and clflushopt are supported on the host CPU.
>> > > >
>> > > > Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
>> > > > ---
>> > > >  x86/memory.c | 29 +++++++++++++++++++++++++++--
>> > > >  1 file changed, 27 insertions(+), 2 deletions(-)
>> > > >
>> > > > diff --git a/x86/memory.c b/x86/memory.c
>> > > > index cd1eb46..03ff7d3 100644
>> > > > --- a/x86/memory.c
>> > > > +++ b/x86/memory.c
>> > > > @@ -23,10 +23,29 @@ static void handle_ud(struct ex_regs *regs)
>> > > >         regs->rip += isize;
>> > > >  }
>> > > >
>> > > > +/*
>> > > > + * Intel VMX cannot intercept guest clwb and clflushopt. When clwb and
>> > > > + * clflushopt are not exposed in guest cpuid, clwb and clflushopt
>> > > > + * instructions in this test case can still succeed without #UD on
>> > > > + * the host CPU which has clwb and clflushopt support. In order to avoid
>> > > > + * false alarms in such cases, introduce the following two arguments
>> > > > + * to allow users to specify whether clwb and clflushopt are supported on
>> > > > + * the host CPU:
>> > > > + * - has_clwb:       indicates clwb is supported on the host CPU
>> > > > + * - has_clflushopt: indicates clflushopt is supported on the host CPU
>> > > > + */
>> > >
>> > > Why not simply use "-cpu host" to make sure the guest CPUID flags
>> > > match host CPUID?
>> > >
>> >
>> > Can I understand that testing these two cases with host/guest CPUID
>> > mismatch (specially clwb and clflushopt flags) is invalid? If yes,
>> > please ignore this patch.
>>
>> I wouldn't say it's invalid to test what happens when the host
>> and guest CPUID don't match.  The question is: is it useful to do
>> so?  Are we testing different code paths when we do that?
>>
>
> No, VMX cannot intercept guest clwb and clflushopt, so no KVM code
> path is involved when guest executes those two instructions.
>
>> The inability to trigger #UD if the host CPUID includes the flag
>> sounds like a bug/limitation we would like to get rid of as soon
>> as hardware allow us to, and not a feature we need to test for.
>>
>
> It's more the inability of VMX, which cannot intercept clwb and
> clflushopt.
>
>> What's the right way to ensure memory.flat is always tested using
>> "-cpu host"?
>>
>
> I think so, at least '-cpu host' can mitigate the hardware inability.
>
> Haozhong

Re: [kvm-unit-tests PATCH] x86/memory: pass host clwb and clflushopt support information

[Paolo Bonzini]

On 20/12/2017 22:26, Eduardo Habkost wrote:
> On Tue, Dec 19, 2017 at 10:54:16AM +0800, Haozhong Zhang wrote:
>> Can I understand that testing these two cases with host/guest CPUID
>> mismatch (specially clwb and clflushopt flags) is invalid? If yes,
>> please ignore this patch.
> 
> I wouldn't say it's invalid to test what happens when the host
> and guest CPUID don't match.  The question is: is it useful to do
> so?  Are we testing different code paths when we do that?
> 
> The inability to trigger #UD if the host CPUID includes the flag
> sounds like a bug/limitation we would like to get rid of as soon
> as hardware allow us to, and not a feature we need to test for.
> 
> What's the right way to ensure memory.flat is always tested using
> "-cpu host"?

We should just add it to x86/unittests.cfg, which lets you specify the
desired QEMU arguments.  Otherwise, it's not run at all as part of my
integration tests:

diff --git a/x86/unittests.cfg b/x86/unittests.cfg
index c6a383b..22c62d5 100644
--- a/x86/unittests.cfg
+++ b/x86/unittests.cfg
@@ -142,6 +142,11 @@ arch = x86_64
 #[init]
 #file = init.flat

+[memory]
+file = memory.flat
+extra_params = -cpu host
+arch = x86_64
+
 [msr]
 file = msr.flat

Thanks,

Paolo