diff mbox series

[v10,7/9] KVM: VMX: Implement and wire get_untagged_addr() for LAM

Message ID 20230719144131.29052-8-binbin.wu@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series Linear Address Masking (LAM) KVM Enabling | expand

Commit Message

Binbin Wu July 19, 2023, 2:41 p.m. UTC 
Implement LAM version of get_untagged_addr() in VMX.

Skip address untag for instruction fetch, branch target and operand of INVLPG,
which LAM doesn't apply to.  Skip address untag for implicit system accesses
since LAM doesn't apply to the loading of base addresses of memory management
registers and segment registers, their values still need to be canonical (for
now, get_untagged_addr() interface is not called for implicit system accesses,
just for future proof).

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>
Reviewed-by: Chao Gao <chao.gao@intel.com>
---
 arch/x86/kvm/vmx/vmx.c | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

Comments

Sean Christopherson Aug. 16, 2023, 10:01 p.m. UTC | #1 
On Wed, Jul 19, 2023, Binbin Wu wrote:
> +	return (sign_extend64(gva, lam_bit) & ~BIT_ULL(63)) | (gva & BIT_ULL(63));

Almost forgot.  Please add a comment explaning how LAM untags the address,
specifically the whole bit 63 preservation.  The logic is actually straightforward,
but the above looks way more complex than it actually is.  This?

	/*
	 * Untag the address by sign-extending the LAM bit, but NOT to bit 63.
	 * Bit 63 is retained from the raw virtual address so that untagging
	 * doesn't change a user access to a supervisor access, and vice versa.
	 */
Binbin Wu Aug. 17, 2023, 9:51 a.m. UTC | #2 
On 8/17/2023 6:01 AM, Sean Christopherson wrote:
> On Wed, Jul 19, 2023, Binbin Wu wrote:
>> +	return (sign_extend64(gva, lam_bit) & ~BIT_ULL(63)) | (gva & BIT_ULL(63));
> Almost forgot.  Please add a comment explaning how LAM untags the address,
> specifically the whole bit 63 preservation.  The logic is actually straightforward,
> but the above looks way more complex than it actually is.  This?
>
> 	/*
> 	 * Untag the address by sign-extending the LAM bit, but NOT to bit 63.
> 	 * Bit 63 is retained from the raw virtual address so that untagging
> 	 * doesn't change a user access to a supervisor access, and vice versa.
> 	 */
OK.

Besides it, I find I forgot adding the comments for the function. I will 
add it back if you don't object.

+/*
+ * Only called in 64-bit mode.
+ *
+ * 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
+ *
+ * Note that KVM masks the metadata in addresses, performs the (original)
+ * canonicality checking and then walks page table. This is slightly
+ * different from hardware behavior but achieves the same effect.
+ * Specifically, if LAM is enabled, the processor performs a modified
+ * canonicality checking where the metadata are ignored instead of
+ * masked. After the modified canonicality checking, the processor masks
+ * the metadata before passing addresses for paging translation.
+ */
Sean Christopherson Aug. 17, 2023, 2:44 p.m. UTC | #3 
On Thu, Aug 17, 2023, Binbin Wu wrote:
> 
> 
> On 8/17/2023 6:01 AM, Sean Christopherson wrote:
> > On Wed, Jul 19, 2023, Binbin Wu wrote:
> > > +	return (sign_extend64(gva, lam_bit) & ~BIT_ULL(63)) | (gva & BIT_ULL(63));
> > Almost forgot.  Please add a comment explaning how LAM untags the address,
> > specifically the whole bit 63 preservation.  The logic is actually straightforward,
> > but the above looks way more complex than it actually is.  This?
> > 
> > 	/*
> > 	 * Untag the address by sign-extending the LAM bit, but NOT to bit 63.
> > 	 * Bit 63 is retained from the raw virtual address so that untagging
> > 	 * doesn't change a user access to a supervisor access, and vice versa.
> > 	 */
> OK.
> 
> Besides it, I find I forgot adding the comments for the function. I will add
> it back if you don't object.
> 
> +/*
> + * Only called in 64-bit mode.

This is no longer true.

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

I vote to not include the table, IMO it does more harm than good, e.g. I only
understood what the last U57+4-lvl entry is conveying after reading the same
figure in the ISE.  Again, the concept of masking bits 62:{56,47} is quite
straightforward, and that's what this function handles.  The gory details of
userspace not 


> + * Note that KVM masks the metadata in addresses, performs the (original)
> + * canonicality checking and then walks page table. This is slightly
> + * different from hardware behavior but achieves the same effect.
> + * Specifically, if LAM is enabled, the processor performs a modified
> + * canonicality checking where the metadata are ignored instead of
> + * masked. After the modified canonicality checking, the processor masks
> + * the metadata before passing addresses for paging translation.

Please drop this.  I don't think we can extrapolate exact hardware behavior from
the ISE blurbs that say the masking is applied after the modified canonicality
check.  Hardware/ucode could very well take the exact same approach as KVM, all
that matters is that the behavior is architecturally correct.

If we're concerned about the blurbs saying the masking is performed *after* the
canonicality checks, e.g. this

  After this modified canonicality check is performed, bits 62:48 are masked by
  sign-extending the value of bit 47 (1)

then the comment should focus on whether or not KVM adheres to the architecture
(SDM), e.g.

/*
 * Note, the SDM states that the linear address is masked *after* the modified
 * canonicality check, whereas KVM masks (untags) the address and then performs
 * a "normal" canonicality check.  Functionally, the two methods are identical,
 * and when the masking occurs relative to the canonicality check isn't visible
 * to software, i.e. KVM's behavior doesn't violate the SDM.
 */
diff mbox series

Patch

diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index bcee5dc3dd0b..abf6d42672cd 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -8177,6 +8177,39 @@  static void vmx_vm_destroy(struct kvm *kvm)
 	free_pages((unsigned long)kvm_vmx->pid_table, vmx_get_pid_table_order(kvm));
 }
 
+static gva_t vmx_get_untagged_addr(struct kvm_vcpu *vcpu, gva_t gva,
+			    unsigned int flags)
+{
+	unsigned long cr3_bits;
+	int lam_bit;
+
+	if (flags & (X86EMUL_F_FETCH | X86EMUL_F_BRANCH | X86EMUL_F_IMPLICIT |
+	             X86EMUL_F_INVTLB))
+		return gva;
+
+	if (!is_64_bit_mode(vcpu))
+		return gva;
+
+	/*
+	 * Bit 63 determines if the address should be treated as user address
+	 * or a supervisor address.
+	 */
+	if (!(gva & BIT_ULL(63))) {
+		cr3_bits = kvm_get_active_cr3_lam_bits(vcpu);
+		if (!(cr3_bits & (X86_CR3_LAM_U57 | X86_CR3_LAM_U48)))
+			return gva;
+
+		/* LAM_U48 is ignored if LAM_U57 is set. */
+		lam_bit = cr3_bits & X86_CR3_LAM_U57 ? 56 : 47;
+	} else {
+		if (!kvm_is_cr4_bit_set(vcpu, X86_CR4_LAM_SUP))
+			return gva;
+
+		lam_bit = kvm_is_cr4_bit_set(vcpu, X86_CR4_LA57) ? 56 : 47;
+	}
+	return (sign_extend64(gva, lam_bit) & ~BIT_ULL(63)) | (gva & BIT_ULL(63));
+}
+
 static struct kvm_x86_ops vmx_x86_ops __initdata = {
 	.name = KBUILD_MODNAME,
 
@@ -8316,6 +8349,8 @@  static struct kvm_x86_ops vmx_x86_ops __initdata = {
 	.complete_emulated_msr = kvm_complete_insn_gp,
 
 	.vcpu_deliver_sipi_vector = kvm_vcpu_deliver_sipi_vector,
+
+	.get_untagged_addr = vmx_get_untagged_addr,
 };
 
 static unsigned int vmx_handle_intel_pt_intr(void)