About UMIP unit test.

About UMIP unit test.

[Yu Zhang]

Hi Jan,

   Previously I saw your UMIP patches merged in xen, and we'd like to 
try some unit test here in Intel. And I wonder do you have any unit test 
code for this feature, or any suggestions? :)

Thanks
Yu

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: About UMIP unit test.

[Andrew Cooper]

On 02/03/2017 02:33, Yu Zhang wrote:
> Hi Jan,
>
>   Previously I saw your UMIP patches merged in xen, and we'd like to
> try some unit test here in Intel. And I wonder do you have any unit
> test code for this feature, or any suggestions? :)

There is support in the x86 emulator to handle UMIP correctly in the
case that it is found set in CR4.  However, the logic to emulate UMIP on
non-UMIP-capable hardware has not been merged yet.

As for unit tests, if you use
http://xenbits.xen.org/gitweb/?p=xtf.git;a=blob;f=tests/cpuid-faulting/main.c;hb=HEAD
as an example, making a unit test should hopefully be trivial.

General docs can be found here http://xenbits.xen.org/docs/xtf/ and 'd
be happy to answer any other questions you have on XTF, should you
choose to use it.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[DO NOT APPLY PATCH XTF 0/2] UMIP test case

[Wei Liu]

I wrote this long time ago before UMIP was merged.

Yu, since you asked, I might as well post it for your reference on how to
do it with XTF.

This series is not yet tested in any way.

Wei Liu (2):
  x86: add UMIP feature bit
  Add UMIP test

 include/arch/x86/processor.h         |   1 +
 include/xen/arch-x86/cpufeatureset.h |   1 +
 tests/umip/Makefile                  |   9 ++++
 tests/umip/main.c                    | 102 +++++++++++++++++++++++++++++++++++
 4 files changed, 113 insertions(+)
 create mode 100644 tests/umip/Makefile
 create mode 100644 tests/umip/main.c

-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[DO NOT APPLY PATCH XTF 1/2] x86: add UMIP feature bit

[Wei Liu]

Strangely there was already one cpu_has_umip defined, so there is no
need for one in this patch.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 include/arch/x86/processor.h         | 1 +
 include/xen/arch-x86/cpufeatureset.h | 1 +
 2 files changed, 2 insertions(+)

diff --git a/include/arch/x86/processor.h b/include/arch/x86/processor.h
index 0106954..e2a2cbb 100644
--- a/include/arch/x86/processor.h
+++ b/include/arch/x86/processor.h
@@ -52,6 +52,7 @@
 #define X86_CR4_PCE             0x00000100  /* Performance counters at ipl 3  */
 #define X86_CR4_OSFXSR          0x00000200  /* Fast FPU save and restore      */
 #define X86_CR4_OSXMMEXCPT      0x00000400  /* Unmasked SSE exceptions        */
+#define X86_CR4_UMIP            0x00000800  /* UMIP                           */
 #define X86_CR4_VMXE            0x00002000  /* VMX                            */
 #define X86_CR4_SMXE            0x00004000  /* SMX                            */
 #define X86_CR4_FSGSBASE        0x00010000  /* {rd,wr}{fs,gs}base             */
diff --git a/include/xen/arch-x86/cpufeatureset.h b/include/xen/arch-x86/cpufeatureset.h
index 905e8e8..f66a4ab 100644
--- a/include/xen/arch-x86/cpufeatureset.h
+++ b/include/xen/arch-x86/cpufeatureset.h
@@ -144,6 +144,7 @@
 
 /* Intel-defined CPU features, CPUID level 0x00000007:0.ecx, word 6 */
 #define X86_FEATURE_PREFETCHWT1   (6*32+ 0) /* PREFETCHWT1 instruction */
+#define X86_FEATURE_UMIP          (6*32+ 2) /* User-Mode Instruction Prevention */
 #define X86_FEATURE_PKU           (6*32+ 3) /* Protection Keys for Userspace */
 #define X86_FEATURE_OSPKE         (6*32+ 4) /* OS Protection Keys Enable */
 
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[DO NOT APPLY PATCH XTF 2/2] Add UMIP test

[Wei Liu]

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 tests/umip/Makefile |   9 +++++
 tests/umip/main.c   | 102 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 111 insertions(+)
 create mode 100644 tests/umip/Makefile
 create mode 100644 tests/umip/main.c

diff --git a/tests/umip/Makefile b/tests/umip/Makefile
new file mode 100644
index 0000000..a74016c
--- /dev/null
+++ b/tests/umip/Makefile
@@ -0,0 +1,9 @@
+include $(ROOT)/build/common.mk
+
+NAME      := umip
+CATEGORY  := functional
+TEST-ENVS := hvm32
+
+obj-perenv += main.o
+
+include $(ROOT)/build/gen.mk
diff --git a/tests/umip/main.c b/tests/umip/main.c
new file mode 100644
index 0000000..ca74085
--- /dev/null
+++ b/tests/umip/main.c
@@ -0,0 +1,102 @@
+/**
+ * @file tests/umip/main.c
+ * @ref test-umip
+ *
+ * @page test-umip UMIP
+ *
+ * Test if UMIP (User-Mode Instruction Prevention) is functional.
+ *
+ * Returns SUCCESS if UMIP is functional, SKIP if UMIP is not
+ * available and FAILURE if UMIP doesn't cause SGDT, SLDT, SIDT, STR
+ * or SMSW to fault.
+ *
+ * @see tests/umip/main.c
+ */
+#include <xtf.h>
+
+#include <arch/x86/processor.h>
+
+#define EX(instr)                                                       \
+    bool seen_ ## instr ## _fault;                                      \
+    bool ex_ ##instr(struct cpu_regs *regs,                             \
+                     const struct extable_entry *ex)                    \
+    {                                                                   \
+        if ( regs->entry_vector == X86_EXC_GP )                         \
+        {                                                               \
+            seen_ ## instr ## _fault = true;                            \
+            regs->ip = ex->fixup;                                       \
+                                                                        \
+            return true;                                                \
+        }                                                               \
+                                                                        \
+        return false;                                                   \
+    }                                                                   \
+
+EX(sgdt)
+EX(sldt)
+EX(sidt)
+EX(str)
+EX(smsw)
+
+#undef EX
+
+void test_main(void)
+{
+    uint8_t buf[1024];
+
+    printk("Test if UMIP is functional\n");
+
+    if ( !cpu_has_umip )
+    {
+        xtf_skip("UMIP is not available\n");
+        return;
+    }
+
+    if ( !xtf_has_fep )
+        xtf_skip("FEP not available, some tests will be skipped\n");
+
+    write_cr4(read_cr4() | X86_CR4_UMIP);
+
+#define TEST(fep,instr)                                         \
+    do {                                                        \
+        seen_ ## instr ## _fault = false;                       \
+        asm volatile ("1: " fep #instr " %0; 2:"                \
+                      _ASM_EXTABLE_HANDLER(1b, 2b, ex_ ##instr) \
+                      : : "m"(buf));                            \
+        if ( !seen_ ## instr ## _fault )                        \
+        {                                                       \
+            xtf_failure(fep #instr " didn't cause fault\n");    \
+            return;                                             \
+        }                                                       \
+        seen_ ## instr ## _fault = false;                       \
+    } while (0)
+
+    TEST(, sgdt);
+    TEST(, sldt);
+    TEST(, sidt);
+    TEST(, str);
+    TEST(, smsw);
+
+    if ( xtf_has_fep )
+    {
+        TEST(_ASM_XEN_FEP, sgdt);
+        TEST(_ASM_XEN_FEP, sldt);
+        TEST(_ASM_XEN_FEP, sidt);
+        TEST(_ASM_XEN_FEP, str);
+        TEST(_ASM_XEN_FEP, smsw);
+    }
+
+#undef TEST
+
+    xtf_success(NULL);
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [DO NOT APPLY PATCH XTF 0/2] UMIP test case

[Andrew Cooper]

On 02/03/2017 08:42, Wei Liu wrote:
> I wrote this long time ago before UMIP was merged.
>
> Yu, since you asked, I might as well post it for your reference on how to
> do it with XTF.
>
> This series is not yet tested in any way.

Unfortunately, you execute all of the sensitive instructions in kernel
mode, where they wouldn't fault even with UMIP active.

For full testing of a feature like this, the test should include a check
that the ability to modify CR4.UMIP depends strictly on the visibility
of the feature, that uses in the kernel still continue to work, even
when active, and that behaviour returns back to normal after the feature
has been deactivated.

The first patch however, is fine to go in.

~Andrew

(I am also going to pretend I didn't see those macros :) )

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [DO NOT APPLY PATCH XTF 0/2] UMIP test case

[Wei Liu]

On Thu, Mar 02, 2017 at 09:05:27AM +0000, Andrew Cooper wrote:
> On 02/03/2017 08:42, Wei Liu wrote:
> > I wrote this long time ago before UMIP was merged.
> >
> > Yu, since you asked, I might as well post it for your reference on how to
> > do it with XTF.
> >
> > This series is not yet tested in any way.
> 
> Unfortunately, you execute all of the sensitive instructions in kernel
> mode, where they wouldn't fault even with UMIP active.
> 
> For full testing of a feature like this, the test should include a check
> that the ability to modify CR4.UMIP depends strictly on the visibility
> of the feature, that uses in the kernel still continue to work, even
> when active, and that behaviour returns back to normal after the feature
> has been deactivated.
> 

I'm pretty sure there are bugs. :-)

> The first patch however, is fine to go in.

Feel free to pick it up in your next batch.

> 
> ~Andrew
> 
> (I am also going to pretend I didn't see those macros :) )

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [DO NOT APPLY PATCH XTF 0/2] UMIP test case

[Yu Zhang]

Wah. Thank you,  Andrew & Wei. :-)

On 3/2/2017 5:05 PM, Andrew Cooper wrote:
> On 02/03/2017 08:42, Wei Liu wrote:
>> I wrote this long time ago before UMIP was merged.
>>
>> Yu, since you asked, I might as well post it for your reference on how to
>> do it with XTF.
>>
>> This series is not yet tested in any way.
> Unfortunately, you execute all of the sensitive instructions in kernel
> mode, where they wouldn't fault even with UMIP active.
>
> For full testing of a feature like this, the test should include a check
> that the ability to modify CR4.UMIP depends strictly on the visibility
> of the feature, that uses in the kernel still continue to work, even
> when active, and that behaviour returns back to normal after the feature
> has been deactivated.

So, before cr4 is written, a cpuid is needed first in the test code, right?

But besides the emulation of cpuid and cr4, my understanding is that 
hypervisor need to inject a GP fault if an instruction causes a VM exit, 
right? This should be different than the normal GP fault inside a VM, 
and how do we test this code path?

Our QA and I may need to have a study of the XTF, and may probably seek 
for your help in the future. :-)
Anyway, thanks a lot!

Yu

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [DO NOT APPLY PATCH XTF 0/2] UMIP test case

[Andrew Cooper]

On 02/03/17 12:30, Yu Zhang wrote:
> Wah. Thank you,  Andrew & Wei. :-)
>
> On 3/2/2017 5:05 PM, Andrew Cooper wrote:
>> On 02/03/2017 08:42, Wei Liu wrote:
>>> I wrote this long time ago before UMIP was merged.
>>>
>>> Yu, since you asked, I might as well post it for your reference on
>>> how to
>>> do it with XTF.
>>>
>>> This series is not yet tested in any way.
>> Unfortunately, you execute all of the sensitive instructions in kernel
>> mode, where they wouldn't fault even with UMIP active.
>>
>> For full testing of a feature like this, the test should include a check
>> that the ability to modify CR4.UMIP depends strictly on the visibility
>> of the feature, that uses in the kernel still continue to work, even
>> when active, and that behaviour returns back to normal after the feature
>> has been deactivated.
>
> So, before cr4 is written, a cpuid is needed first in the test code,
> right?

Yes, but that is performed automatically by the framework for
convenience.  The predicate cpu_has_umip will work fine for your test.

>
> But besides the emulation of cpuid and cr4, my understanding is that
> hypervisor need to inject a GP fault if an instruction causes a VM
> exit, right? This should be different than the normal GP fault inside
> a VM, and how do we test this code path?

Using the Force Emulation Prefix.  This breaks out to the hypervisor and
runs the instruction through the emulator.  You should test both that
the native instructions behave as expected, and that once forced, the
instructions also behave sensibly.

The FEP test is to cover alternative cases with would require emulation
normally (e.g. memory operand hitting an MMIO region, or the page marked
NX in EPT because of VM instrospection being active).

>
> Our QA and I may need to have a study of the XTF, and may probably
> seek for your help in the future. :-)
> Anyway, thanks a lot!

Haozhong (CC'd) has previously used XTF for a load of nested-virt
testing, if you'd like an alternative opinion, (although I do hope you
find XTF easy to use and useful).

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel