| Message ID | 20161216134348.16236-15-haozhong.zhang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 16/12/16 13:43, Haozhong Zhang wrote: > Fault #GP(0) is expected in this test. > > Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com> > --- > tests/vvmx/vmxon.c | 29 +++++++++++++++++++++++++++++ > 1 file changed, 29 insertions(+) > > diff --git a/tests/vvmx/vmxon.c b/tests/vvmx/vmxon.c > index 0664a48..ec7ee7e 100644 > --- a/tests/vvmx/vmxon.c > +++ b/tests/vvmx/vmxon.c > @@ -3,6 +3,7 @@ > #include "util.h" > > static uint8_t vmxon_region[PAGE_SIZE] __aligned(PAGE_SIZE); > +static uint8_t vmxon_region_2nd[PAGE_SIZE] __aligned(PAGE_SIZE); You introduce vmxon_region_2nd here, but reference it in the previous patch. However, I am not sure of its purpose. Why cant you reuse the previous host state area? > > /** > * vmxon with CR4.VMXE cleared > @@ -165,6 +166,31 @@ static bool test_vmxon_in_root_cpl0_novmcs(void) > VMXERR_VMFAIL_INVALID, 0, 0); > } > > +static unsigned long vmxon_in_root_user(void) > +{ > + exinfo_t fault; > + unsigned long ret = vmxon((uint64_t)vmxon_region_2nd, &fault); > + > + return (ret << 32) | fault; > +} I have a pending patch for exec_user_param() (originally written for a test which I subsequently reimplemented differently). I have just pushed it for you to use. You can have one common user_vmxon() and pass a variable host region to it, rather than having an almost-entirely duplicate function. ~Andrew
On 12/16/16 20:59 +0000, Andrew Cooper wrote: >On 16/12/16 13:43, Haozhong Zhang wrote: >> Fault #GP(0) is expected in this test. >> >> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com> >> --- >> tests/vvmx/vmxon.c | 29 +++++++++++++++++++++++++++++ >> 1 file changed, 29 insertions(+) >> >> diff --git a/tests/vvmx/vmxon.c b/tests/vvmx/vmxon.c >> index 0664a48..ec7ee7e 100644 >> --- a/tests/vvmx/vmxon.c >> +++ b/tests/vvmx/vmxon.c >> @@ -3,6 +3,7 @@ >> #include "util.h" >> >> static uint8_t vmxon_region[PAGE_SIZE] __aligned(PAGE_SIZE); >> +static uint8_t vmxon_region_2nd[PAGE_SIZE] __aligned(PAGE_SIZE); > >You introduce vmxon_region_2nd here, but reference it in the previous patch. > I must have made mistakes when separating patch 13 - 16. >However, I am not sure of its purpose. Why cant you reuse the previous >host state area? > Intel SDM says SW should not access or modify the VXMON rgion of a logical processor between vmxon and vmxoff. Though I have tested on real hardware whether reusing VMXON region would cause any trouble, I still want to exclude that possibility/noise from this test. >> >> /** >> * vmxon with CR4.VMXE cleared >> @@ -165,6 +166,31 @@ static bool test_vmxon_in_root_cpl0_novmcs(void) >> VMXERR_VMFAIL_INVALID, 0, 0); >> } >> >> +static unsigned long vmxon_in_root_user(void) >> +{ >> + exinfo_t fault; >> + unsigned long ret = vmxon((uint64_t)vmxon_region_2nd, &fault); >> + >> + return (ret << 32) | fault; >> +} > >I have a pending patch for exec_user_param() (originally written for a >test which I subsequently reimplemented differently). > >I have just pushed it for you to use. You can have one common >user_vmxon() and pass a variable host region to it, rather than having >an almost-entirely duplicate function. > Thank you! I can use the dedicated bits in exinfo_t as you suggested in previous patch. Haozhong
On 19/12/16 03:46, Haozhong Zhang wrote: >> However, I am not sure of its purpose. Why cant you reuse the previous >> host state area? >> > > Intel SDM says SW should not access or modify the VXMON rgion of a > logical processor between vmxon and vmxoff. Though I have tested on > real hardware whether reusing VMXON region would cause any trouble, I > still want to exclude that possibility/noise from this test. That is a good reason. Could you please add a comment to this effect? ~Andrew
On 12/19/16 16:07 +0000, Andrew Cooper wrote: >On 19/12/16 03:46, Haozhong Zhang wrote: >>> However, I am not sure of its purpose. Why cant you reuse the previous >>> host state area? >>> >> >> Intel SDM says SW should not access or modify the VXMON rgion of a >> logical processor between vmxon and vmxoff. Though I have tested on ^ I missed a 'not' here, hope you didn't misunderstand what I meant here >> real hardware whether reusing VMXON region would cause any trouble, I >> still want to exclude that possibility/noise from this test. > >That is a good reason. Could you please add a comment to this effect? > Sure, I'll add a comment around vmxon_region_2nd. Haozhong
diff --git a/tests/vvmx/vmxon.c b/tests/vvmx/vmxon.c index 0664a48..ec7ee7e 100644 --- a/tests/vvmx/vmxon.c +++ b/tests/vvmx/vmxon.c @@ -3,6 +3,7 @@ #include "util.h" static uint8_t vmxon_region[PAGE_SIZE] __aligned(PAGE_SIZE); +static uint8_t vmxon_region_2nd[PAGE_SIZE] __aligned(PAGE_SIZE); /** * vmxon with CR4.VMXE cleared @@ -165,6 +166,31 @@ static bool test_vmxon_in_root_cpl0_novmcs(void) VMXERR_VMFAIL_INVALID, 0, 0); } +static unsigned long vmxon_in_root_user(void) +{ + exinfo_t fault; + unsigned long ret = vmxon((uint64_t)vmxon_region_2nd, &fault); + + return (ret << 32) | fault; +} + +/** + * vmxon in VMX root w/ CPL = 3 and w/o current VMCS + * + * Expect: #GP(0) + */ +static bool test_vmxon_in_root_user_novmcs(void) +{ + clear_vmcs(vmxon_region_2nd, get_vmcs_revid()); + + unsigned long ret = exec_user(vmxon_in_root_user); + uint8_t err = (ret >> 32) & 0xff; + exinfo_t fault = ret & 0xffffffff; + + return handle_vmxinsn_err(__func__, err, fault, + VMXERR_FAULT, EXINFO_SYM(GP, 0), 0); +} + bool test_vmxon(void) { if ( !test_vmxon_novmxe() ) @@ -194,6 +220,9 @@ bool test_vmxon(void) if ( !test_vmxon_in_root_cpl0_novmcs() ) return false; + if ( !test_vmxon_in_root_user_novmcs() ) + return false; + return true; }
Fault #GP(0) is expected in this test. Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com> --- tests/vvmx/vmxon.c | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+)