Re: [PATCH v6 5/7] KVM: x86: Introduce untag_addr() in kvm_x86_ops

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

On 3/20/2023 8:23 PM, Binbin Wu wrote:
>
> On 3/20/2023 8:07 PM, Chao Gao wrote:
>> On Sun, Mar 19, 2023 at 04:49:25PM +0800, Binbin Wu wrote:
>>> Introduce a new interface untag_addr() to kvm_x86_ops to untag the 
>>> metadata
>> >from linear address. Implement LAM version in VMX and dummy version 
>> in SVM.
>>> When enabled feature like Intel Linear Address Masking or AMD Upper
>>> Address Ignore, linear address may be tagged with metadata. Linear
>>> address should be checked for modified canonicality and untagged in
>>> instrution emulations or vmexit handlings if LAM or UAI is applicable.
>>>
>>> Introduce untag_addr() to kvm_x86_ops to hide the code related to 
>>> vendor
>>> specific details.
>>> - For VMX, LAM version is implemented.
>>>   LAM has a modified canonical check when applicable:
>>>   * LAM_S48                : [ 1 ][ metadata ][ 1 ]
>>>                                63               47
>>>   * LAM_U48                : [ 0 ][ metadata ][ 0 ]
>>>                                63               47
>>>   * LAM_S57                : [ 1 ][ metadata ][ 1 ]
>>>                                63               56
>>>   * LAM_U57 + 5-lvl paging : [ 0 ][ metadata ][ 0 ]
>>>                                63               56
>>>   * LAM_U57 + 4-lvl paging : [ 0 ][ metadata ][ 0...0 ]
>>>                                63               56..47
>>>   If LAM is applicable to certain address, untag the metadata bits and
>>>   replace them with the value of bit 47 (LAM48) or bit 56 (LAM57). 
>>> Later
>>>   the untagged address will do legacy canonical check. So that LAM 
>>> canonical
>>>   check and mask can be covered by "untag + legacy canonical check".
>>>
>>>   For cases LAM is not applicable, 'flags' is passed to the interface
>>>   to skip untag.
>>>
>>> - For SVM, add a dummy version to do nothing, but return the original
>>>   address.
>>>
>>> Signed-off-by: Binbin Wu <binbin.wu@linux.intel.com>
>>> ---
>>> arch/x86/include/asm/kvm-x86-ops.h |  1 +
>>> arch/x86/include/asm/kvm_host.h    |  5 +++
>>> arch/x86/kvm/svm/svm.c             |  7 ++++
>>> arch/x86/kvm/vmx/vmx.c             | 60 ++++++++++++++++++++++++++++++
>>> arch/x86/kvm/vmx/vmx.h             |  2 +
>>> 5 files changed, 75 insertions(+)
>>>
>>> diff --git a/arch/x86/include/asm/kvm-x86-ops.h 
>>> b/arch/x86/include/asm/kvm-x86-ops.h
>>> index 8dc345cc6318..7d63d1b942ac 100644
>>> --- a/arch/x86/include/asm/kvm-x86-ops.h
>>> +++ b/arch/x86/include/asm/kvm-x86-ops.h
>>> @@ -52,6 +52,7 @@ KVM_X86_OP(cache_reg)
>>> KVM_X86_OP(get_rflags)
>>> KVM_X86_OP(set_rflags)
>>> KVM_X86_OP(get_if_flag)
>>> +KVM_X86_OP(untag_addr)
>> Suppose AMD doesn't/won't use CR4.LAM_SUP and CR3.LAM_U48/U57 for other
>> purposes, it is fine to use a common x86 function to perform LAM masking
>> for pointers. It won't do anything harmful on AMD parts because those
>> enabling bits shouldn't be set and then no bits will be masked out by
>> the common x86 function.
>>
>> Probably we can defer the introduction of the hook to when the
>> assumption becomes wrong.
>
> Another reason I introduced the hook is I noticed the AMD Upper 
> Address Ignore using [63:57] as metadata.
> So the untag implementaion will be differnet. But indeed, it also will 
> be a future issue.
>
> Let's hear more opinions from others, if more guys think the hook is 
> unnecessary for now, I can switch back to
> a common x86 function.

Hi Sean,

What's your opinion? Do you think it is too early to introduce the hook?



>
>