diff mbox series

[v5,4/4] x86: Add test case for INVVPID with LAM

Message ID 20230530024356.24870-5-binbin.wu@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series x86: Add test cases for LAM | expand

Commit Message

Binbin Wu May 30, 2023, 2:43 a.m. UTC 
LAM applies to the linear address of INVVPID operand, however,
it doesn't apply to the linear address in the INVVPID descriptor.

The added cases use tagged operand or tagged target invalidation
address to make sure the behaviors are expected when LAM is on.

Also, INVVPID case using tagged operand can be used as the common
test cases for VMX instruction VMExits.

Signed-off-by: Binbin Wu <binbin.wu@linux.intel.com>
---
 x86/vmx_tests.c | 46 +++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 45 insertions(+), 1 deletion(-)

Comments

Chao Gao June 5, 2023, 1:22 p.m. UTC | #1 
On Tue, May 30, 2023 at 10:43:56AM +0800, Binbin Wu wrote:
>LAM applies to the linear address of INVVPID operand, however,
>it doesn't apply to the linear address in the INVVPID descriptor.
>
>The added cases use tagged operand or tagged target invalidation
>address to make sure the behaviors are expected when LAM is on.
>
>Also, INVVPID case using tagged operand can be used as the common
>test cases for VMX instruction VMExits.
>
>Signed-off-by: Binbin Wu <binbin.wu@linux.intel.com>
>---
> x86/vmx_tests.c | 46 +++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 45 insertions(+), 1 deletion(-)
>
>diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
>index 217befe..3f3f203 100644
>--- a/x86/vmx_tests.c
>+++ b/x86/vmx_tests.c
>@@ -3225,6 +3225,48 @@ static void invvpid_test_not_in_vmx_operation(void)
> 	TEST_ASSERT(!vmx_on());
> }
> 
>+/* LAM applies to the target address inside the descriptor of invvpid */

This isn't correct. LAM doesn't apply to that address. Right?

>+static void invvpid_test_lam(void)
>+{
>+	void *vaddr;
>+	struct invvpid_operand *operand;
>+	u64 lam_mask = LAM48_MASK;
>+	bool fault;
>+
>+	if (!this_cpu_has(X86_FEATURE_LAM)) {
>+		report_skip("LAM is not supported, skip INVVPID with LAM");
>+		return;
>+	}
>+	write_cr4_safe(read_cr4() | X86_CR4_LAM_SUP);

why write_cr4_safe()?

This should succeed if LAM is supported. So it is better to use
write_cr4() because write_cr4() has an assertion which can catch
unexpected exceptions.

>+
>+	if (this_cpu_has(X86_FEATURE_LA57) && read_cr4() & X86_CR4_LA57)
>+		lam_mask = LAM57_MASK;
>+
>+	vaddr = alloc_vpage();
>+	install_page(current_page_table(), virt_to_phys(alloc_page()), vaddr);
>+	/*
>+	 * Since the stack memory address in KUT doesn't follow kernel address
>+	 * space partition rule, reuse the memory address for descriptor and
>+	 * the target address in the descriptor of invvpid.
>+	 */
>+	operand = (struct invvpid_operand *)vaddr;
>+	operand->vpid = 0xffff;
>+	operand->gla = (u64)vaddr;
>+	operand = (struct invvpid_operand *)set_la_non_canonical((u64)operand,
>+								 lam_mask);
>+	fault = test_for_exception(GP_VECTOR, ds_invvpid, operand);
>+	report(!fault, "INVVPID (LAM on): tagged operand");
>+
>+	/*
>+	 * LAM doesn't apply to the address inside the descriptor, expected
>+	 * failure and VMXERR_INVALID_OPERAND_TO_INVEPT_INVVPID set in
>+	 * VMX_INST_ERROR.
>+	 */
Maybe

	/*
	 * Verify that LAM doesn't apply to the address inside the descriptor
	 * even when LAM is enabled. i.e., the address in the descriptor should
	 * be canonical.
	 */
>+	try_invvpid(INVVPID_ADDR, 0xffff, NONCANONICAL);

shouldn't we use a kernel address here? e.g., vaddr. otherwise, we
cannot tell if there is an error in KVM's emulation because in this
test, LAM is enabled only for kernel address while INVVPID_ADDR is a
userspace address.

>+
>+	write_cr4_safe(read_cr4() & ~X86_CR4_LAM_SUP);

ditto.

With all above fixed:

Reviewed-by: Chao Gao <chao.gao@intel.com>


>+}
>+
> /*
>  * This does not test real-address mode, virtual-8086 mode, protected mode,
>  * or CPL > 0.
>@@ -3274,8 +3316,10 @@ static void invvpid_test(void)
> 	/*
> 	 * The gla operand is only validated for single-address INVVPID.
> 	 */
>-	if (types & (1u << INVVPID_ADDR))
>+	if (types & (1u << INVVPID_ADDR)) {
> 		try_invvpid(INVVPID_ADDR, 0xffff, NONCANONICAL);
>+		invvpid_test_lam();
>+	}
> 
> 	invvpid_test_gp();
> 	invvpid_test_ss();
>-- 
>2.25.1
>
Binbin Wu June 6, 2023, 5:47 a.m. UTC | #2 
On 6/5/2023 9:22 PM, Chao Gao wrote:
> On Tue, May 30, 2023 at 10:43:56AM +0800, Binbin Wu wrote:
>> LAM applies to the linear address of INVVPID operand, however,
>> it doesn't apply to the linear address in the INVVPID descriptor.
>>
>> The added cases use tagged operand or tagged target invalidation
>> address to make sure the behaviors are expected when LAM is on.
>>
>> Also, INVVPID case using tagged operand can be used as the common
>> test cases for VMX instruction VMExits.
>>
>> Signed-off-by: Binbin Wu <binbin.wu@linux.intel.com>
>> ---
>> x86/vmx_tests.c | 46 +++++++++++++++++++++++++++++++++++++++++++++-
>> 1 file changed, 45 insertions(+), 1 deletion(-)
>>
>> diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
>> index 217befe..3f3f203 100644
>> --- a/x86/vmx_tests.c
>> +++ b/x86/vmx_tests.c
>> @@ -3225,6 +3225,48 @@ static void invvpid_test_not_in_vmx_operation(void)
>> 	TEST_ASSERT(!vmx_on());
>> }
>>
>> +/* LAM applies to the target address inside the descriptor of invvpid */
> This isn't correct. LAM doesn't apply to that address. Right?
Oops, will fix it, thanks.

>
>> +static void invvpid_test_lam(void)
>> +{
>> +	void *vaddr;
>> +	struct invvpid_operand *operand;
>> +	u64 lam_mask = LAM48_MASK;
>> +	bool fault;
>> +
>> +	if (!this_cpu_has(X86_FEATURE_LAM)) {
>> +		report_skip("LAM is not supported, skip INVVPID with LAM");
>> +		return;
>> +	}
>> +	write_cr4_safe(read_cr4() | X86_CR4_LAM_SUP);
> why write_cr4_safe()?
>
> This should succeed if LAM is supported. So it is better to use
> write_cr4() because write_cr4() has an assertion which can catch
> unexpected exceptions.
OK.

>
>> +
>> +	if (this_cpu_has(X86_FEATURE_LA57) && read_cr4() & X86_CR4_LA57)
>> +		lam_mask = LAM57_MASK;
>> +
>> +	vaddr = alloc_vpage();
>> +	install_page(current_page_table(), virt_to_phys(alloc_page()), vaddr);
>> +	/*
>> +	 * Since the stack memory address in KUT doesn't follow kernel address
>> +	 * space partition rule, reuse the memory address for descriptor and
>> +	 * the target address in the descriptor of invvpid.
>> +	 */
>> +	operand = (struct invvpid_operand *)vaddr;
>> +	operand->vpid = 0xffff;
>> +	operand->gla = (u64)vaddr;
>> +	operand = (struct invvpid_operand *)set_la_non_canonical((u64)operand,
>> +								 lam_mask);
>> +	fault = test_for_exception(GP_VECTOR, ds_invvpid, operand);
>> +	report(!fault, "INVVPID (LAM on): tagged operand");
>> +
>> +	/*
>> +	 * LAM doesn't apply to the address inside the descriptor, expected
>> +	 * failure and VMXERR_INVALID_OPERAND_TO_INVEPT_INVVPID set in
>> +	 * VMX_INST_ERROR.
>> +	 */
> Maybe
>
> 	/*
> 	 * Verify that LAM doesn't apply to the address inside the descriptor
> 	 * even when LAM is enabled. i.e., the address in the descriptor should
> 	 * be canonical.
> 	 */
>> +	try_invvpid(INVVPID_ADDR, 0xffff, NONCANONICAL);
> shouldn't we use a kernel address here? e.g., vaddr. otherwise, we
> cannot tell if there is an error in KVM's emulation because in this
> test, LAM is enabled only for kernel address while INVVPID_ADDR is a
> userspace address.

INVVPID_ADDR is the invalidation type, not the address.
The address used  here is NONCANONICAL, which is 0xaaaaaaaaaaaaaaaaull and
is considered as kernel address.

>
>> +
>> +	write_cr4_safe(read_cr4() & ~X86_CR4_LAM_SUP);
> ditto.
>
> With all above fixed:
>
> Reviewed-by: Chao Gao <chao.gao@intel.com>
>
>
>> +}
>> +
>> /*
>>   * This does not test real-address mode, virtual-8086 mode, protected mode,
>>   * or CPL > 0.
>> @@ -3274,8 +3316,10 @@ static void invvpid_test(void)
>> 	/*
>> 	 * The gla operand is only validated for single-address INVVPID.
>> 	 */
>> -	if (types & (1u << INVVPID_ADDR))
>> +	if (types & (1u << INVVPID_ADDR)) {
>> 		try_invvpid(INVVPID_ADDR, 0xffff, NONCANONICAL);
>> +		invvpid_test_lam();
>> +	}
>>
>> 	invvpid_test_gp();
>> 	invvpid_test_ss();
>> -- 
>> 2.25.1
>>
Chao Gao June 6, 2023, 7:02 a.m. UTC | #3 
On Tue, Jun 06, 2023 at 01:47:07PM +0800, Binbin Wu wrote:
>> > +	try_invvpid(INVVPID_ADDR, 0xffff, NONCANONICAL);
>> shouldn't we use a kernel address here? e.g., vaddr. otherwise, we
>> cannot tell if there is an error in KVM's emulation because in this
>> test, LAM is enabled only for kernel address while INVVPID_ADDR is a
>> userspace address.
>
>INVVPID_ADDR is the invalidation type, not the address.
>The address used  here is NONCANONICAL, which is 0xaaaaaaaaaaaaaaaaull and
>is considered as kernel address.

Yes. Sorry about this misunderstanding.

Do you need the address to be canonical after masking metadata?
Binbin Wu June 6, 2023, 7:08 a.m. UTC | #4 
On 6/6/2023 3:02 PM, Chao Gao wrote:
> On Tue, Jun 06, 2023 at 01:47:07PM +0800, Binbin Wu wrote:
>>>> +	try_invvpid(INVVPID_ADDR, 0xffff, NONCANONICAL);
>>> shouldn't we use a kernel address here? e.g., vaddr. otherwise, we
>>> cannot tell if there is an error in KVM's emulation because in this
>>> test, LAM is enabled only for kernel address while INVVPID_ADDR is a
>>> userspace address.
>> INVVPID_ADDR is the invalidation type, not the address.
>> The address used  here is NONCANONICAL, which is 0xaaaaaaaaaaaaaaaaull and
>> is considered as kernel address.
> Yes. Sorry about this misunderstanding.
>
> Do you need the address to be canonical after masking metadata?

You are right, I will use set_la_non_canonical(), which is added in 2/4 
to pass a proper address for test.
Thanks.
diff mbox series

Patch

diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
index 217befe..3f3f203 100644
--- a/x86/vmx_tests.c
+++ b/x86/vmx_tests.c
@@ -3225,6 +3225,48 @@  static void invvpid_test_not_in_vmx_operation(void)
 	TEST_ASSERT(!vmx_on());
 }
 
+/* LAM applies to the target address inside the descriptor of invvpid */
+static void invvpid_test_lam(void)
+{
+	void *vaddr;
+	struct invvpid_operand *operand;
+	u64 lam_mask = LAM48_MASK;
+	bool fault;
+
+	if (!this_cpu_has(X86_FEATURE_LAM)) {
+		report_skip("LAM is not supported, skip INVVPID with LAM");
+		return;
+	}
+	write_cr4_safe(read_cr4() | X86_CR4_LAM_SUP);
+
+	if (this_cpu_has(X86_FEATURE_LA57) && read_cr4() & X86_CR4_LA57)
+		lam_mask = LAM57_MASK;
+
+	vaddr = alloc_vpage();
+	install_page(current_page_table(), virt_to_phys(alloc_page()), vaddr);
+	/*
+	 * Since the stack memory address in KUT doesn't follow kernel address
+	 * space partition rule, reuse the memory address for descriptor and
+	 * the target address in the descriptor of invvpid.
+	 */
+	operand = (struct invvpid_operand *)vaddr;
+	operand->vpid = 0xffff;
+	operand->gla = (u64)vaddr;
+	operand = (struct invvpid_operand *)set_la_non_canonical((u64)operand,
+								 lam_mask);
+	fault = test_for_exception(GP_VECTOR, ds_invvpid, operand);
+	report(!fault, "INVVPID (LAM on): tagged operand");
+
+	/*
+	 * LAM doesn't apply to the address inside the descriptor, expected
+	 * failure and VMXERR_INVALID_OPERAND_TO_INVEPT_INVVPID set in
+	 * VMX_INST_ERROR.
+	 */
+	try_invvpid(INVVPID_ADDR, 0xffff, NONCANONICAL);
+
+	write_cr4_safe(read_cr4() & ~X86_CR4_LAM_SUP);
+}
+
 /*
  * This does not test real-address mode, virtual-8086 mode, protected mode,
  * or CPL > 0.
@@ -3274,8 +3316,10 @@  static void invvpid_test(void)
 	/*
 	 * The gla operand is only validated for single-address INVVPID.
 	 */
-	if (types & (1u << INVVPID_ADDR))
+	if (types & (1u << INVVPID_ADDR)) {
 		try_invvpid(INVVPID_ADDR, 0xffff, NONCANONICAL);
+		invvpid_test_lam();
+	}
 
 	invvpid_test_gp();
 	invvpid_test_ss();