Re: [PATCH v6 6/7] KVM: x86: Untag address when LAM applicable

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

On 3/20/2023 7:51 PM, Chao Gao wrote:
> On Sun, Mar 19, 2023 at 04:49:26PM +0800, Binbin Wu wrote:
>> Untag address for 64-bit memory/mmio operand in instruction emulations
>> and vmexit handlers when LAM is applicable.
>>
>> For instruction emulation, untag address in __linearize() before
>> canonical check. LAM doesn't apply to instruction fetch and invlpg,
>> use KVM_X86_UNTAG_ADDR_SKIP_LAM to skip LAM untag.
>>
>> For vmexit handlings related to 64-bit linear address:
>> - Cases need to untag address
>>   Operand(s) of VMX instructions and INVPCID
>>   Operand(s) of SGX ENCLS
>>   Linear address in INVVPID descriptor.
>> - Cases LAM doesn't apply to (no change needed)
>>   Operand of INVLPG
>>   Linear address in INVPCID descriptor
>>
>> Co-developed-by: Robert Hoo <robert.hu@linux.intel.com>
>> Signed-off-by: Robert Hoo <robert.hu@linux.intel.com>
>> Signed-off-by: Binbin Wu <binbin.wu@linux.intel.com>
>> ---
>> arch/x86/kvm/emulate.c    | 25 +++++++++++++++++--------
>> arch/x86/kvm/vmx/nested.c |  2 ++
>> arch/x86/kvm/vmx/sgx.c    |  1 +
>> arch/x86/kvm/x86.c        |  4 ++++
>> 4 files changed, 24 insertions(+), 8 deletions(-)
>>
>> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
>> index a630c5db971c..c46f0162498e 100644
>> --- a/arch/x86/kvm/emulate.c
>> +++ b/arch/x86/kvm/emulate.c
>> @@ -688,7 +688,8 @@ static __always_inline int __linearize(struct x86_emulate_ctxt *ctxt,
>> 				       struct segmented_address addr,
>> 				       unsigned *max_size, unsigned size,
>> 				       bool write, bool fetch,
>> -				       enum x86emul_mode mode, ulong *linear)
>> +				       enum x86emul_mode mode, ulong *linear,
>> +				       u64 untag_flags)
>> {
>> 	struct desc_struct desc;
>> 	bool usable;
>> @@ -701,9 +702,10 @@ static __always_inline int __linearize(struct x86_emulate_ctxt *ctxt,
>> 	*max_size = 0;
>> 	switch (mode) {
>> 	case X86EMUL_MODE_PROT64:
>> -		*linear = la;
>> +		*linear = static_call(kvm_x86_untag_addr)(ctxt->vcpu, la, untag_flags);
>> +
>> 		va_bits = ctxt_virt_addr_bits(ctxt);
>> -		if (!__is_canonical_address(la, va_bits))
>> +		if (!__is_canonical_address(*linear, va_bits))
>> 			goto bad;
>>
>> 		*max_size = min_t(u64, ~0u, (1ull << va_bits) - la);
>> @@ -757,8 +759,8 @@ static int linearize(struct x86_emulate_ctxt *ctxt,
>> 		     ulong *linear)
>> {
>> 	unsigned max_size;
>> -	return __linearize(ctxt, addr, &max_size, size, write, false,
>> -			   ctxt->mode, linear);
>> +	return __linearize(ctxt, addr, &max_size, size, false, false,
> 							^^^^^
> 							Should be "write".

Oops, thanks for catching it.


>
>> +			   ctxt->mode, linear, 0);
>> }
>>
>> static inline int assign_eip(struct x86_emulate_ctxt *ctxt, ulong dst)
>> @@ -771,7 +773,9 @@ static inline int assign_eip(struct x86_emulate_ctxt *ctxt, ulong dst)
>>
>> 	if (ctxt->op_bytes != sizeof(unsigned long))
>> 		addr.ea = dst & ((1UL << (ctxt->op_bytes << 3)) - 1);
>> -	rc = __linearize(ctxt, addr, &max_size, 1, false, true, ctxt->mode, &linear);
>> +	/* skip LAM untag for instruction */
> I think it would be more accurate to quote the spec:
>
> LAM does not apply to addresses used for instruction fetches or to those
> that specify the targets of jump and call instructions


OK.

>
>> +	rc = __linearize(ctxt, addr, &max_size, 1, false, true, ctxt->mode,
>> +		         &linear, KVM_X86_UNTAG_ADDR_SKIP_LAM);
>> 	if (rc == X86EMUL_CONTINUE)
>> 		ctxt->_eip = addr.ea;
>> 	return rc;
>> @@ -906,9 +910,11 @@ static int __do_insn_fetch_bytes(struct x86_emulate_ctxt *ctxt, int op_size)
>> 	 * __linearize is called with size 0 so that it does not do any
>> 	 * boundary check itself.  Instead, we use max_size to check
>> 	 * against op_size.
>> +	 *
>> +	 * skip LAM untag for instruction
> ditto