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

[Binbin Wu <binbin.wu@linux.intel.com>]

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
>>