diff mbox series

[v4,6/9] KVM: x86: When KVM judges CR3 valid or not, consider LAM bits

Message ID 20230209024022.3371768-7-robert.hu@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series Linear Address Masking (LAM) KVM Enabling | expand

Commit Message

Robert Hoo Feb. 9, 2023, 2:40 a.m. UTC 
Before apply to kvm_vcpu_is_illegal_gpa(), clear LAM bits if it's valid.

Signed-off-by: Robert Hoo <robert.hu@linux.intel.com>
Reviewed-by: Jingqi Liu <jingqi.liu@intel.com>
---
 arch/x86/kvm/x86.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

Comments

Chao Gao Feb. 13, 2023, 2:01 a.m. UTC | #1 
On Thu, Feb 09, 2023 at 10:40:19AM +0800, Robert Hoo wrote:
>Before apply to kvm_vcpu_is_illegal_gpa(), clear LAM bits if it's valid.

I prefer to squash this patch into patch 2 because it is also related to
CR3 LAM bits handling.

>
>Signed-off-by: Robert Hoo <robert.hu@linux.intel.com>
>Reviewed-by: Jingqi Liu <jingqi.liu@intel.com>
>---
> arch/x86/kvm/x86.c | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
>diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>index 1bdc8c0c80c0..3218f465ae71 100644
>--- a/arch/x86/kvm/x86.c
>+++ b/arch/x86/kvm/x86.c
>@@ -1231,6 +1231,14 @@ static void kvm_invalidate_pcid(struct kvm_vcpu *vcpu, unsigned long pcid)
> 	kvm_mmu_free_roots(vcpu->kvm, mmu, roots_to_free);
> }
> 
>+static bool kvm_is_valid_cr3(struct kvm_vcpu *vcpu, unsigned long cr3)

Since this function takes a "vcpu" argument, probably
kvm_vcpu_is_valid_cr3() is slightly better.

>+{
>+	if (guest_cpuid_has(vcpu, X86_FEATURE_LAM))

check if the vcpu is in the 64 bit long mode?

>+		cr3 &= ~(X86_CR3_LAM_U48 | X86_CR3_LAM_U57);
>+
>+	return kvm_vcpu_is_legal_gpa(vcpu, cr3);
>+}
>+
> int kvm_set_cr3(struct kvm_vcpu *vcpu, unsigned long cr3)
> {
> 	bool skip_tlb_flush = false;
>@@ -1254,7 +1262,7 @@ int kvm_set_cr3(struct kvm_vcpu *vcpu, unsigned long cr3)
> 	 * stuff CR3, e.g. for RSM emulation, and there is no guarantee that
> 	 * the current vCPU mode is accurate.
> 	 */
>-	if (kvm_vcpu_is_illegal_gpa(vcpu, cr3))
>+	if (!kvm_is_valid_cr3(vcpu, cr3))

There are other call sites of kvm_vcpu_is_illegal_gpa() to validate cr3.
Do you need to modify them?

> 		return 1;
> 
> 	if (is_pae_paging(vcpu) && !load_pdptrs(vcpu, cr3))
>-- 
>2.31.1
>
Robert Hoo Feb. 13, 2023, 1:25 p.m. UTC | #2 
On Mon, 2023-02-13 at 10:01 +0800, Chao Gao wrote:
> On Thu, Feb 09, 2023 at 10:40:19AM +0800, Robert Hoo wrote:
> > Before apply to kvm_vcpu_is_illegal_gpa(), clear LAM bits if it's
> > valid.
> 
> I prefer to squash this patch into patch 2 because it is also related
> to
> CR3 LAM bits handling.
> 
Though all surround CR3, I would prefer split into pieces, so that
easier for review and accept. I can change their order to group
together. Is is all right for you?
> > 
> > 
> > +static bool kvm_is_valid_cr3(struct kvm_vcpu *vcpu, unsigned long
> > cr3)
> 
> Since this function takes a "vcpu" argument, probably
> kvm_vcpu_is_valid_cr3() is slightly better.

OK, to align with kvm_vcpu_is_legal_gpa().
> 
> > +{
> > +	if (guest_cpuid_has(vcpu, X86_FEATURE_LAM))
> 
> check if the vcpu is in the 64 bit long mode?

Emm, looks necessary. 
	(guest_cpuid_has(vcpu, X86_FEATURE_LAM) && is_long_mode(vcpu))
Let me ponder more, e.g. when guest has LAM feature but not in long
mode...
> 
> > +		cr3 &= ~(X86_CR3_LAM_U48 | X86_CR3_LAM_U57);
> > +
> > +	return kvm_vcpu_is_legal_gpa(vcpu, cr3);
> > +}
> > +
> > int kvm_set_cr3(struct kvm_vcpu *vcpu, unsigned long cr3)
> > {
> > 	bool skip_tlb_flush = false;
> > @@ -1254,7 +1262,7 @@ int kvm_set_cr3(struct kvm_vcpu *vcpu,
> > unsigned long cr3)
> > 	 * stuff CR3, e.g. for RSM emulation, and there is no guarantee
> > that
> > 	 * the current vCPU mode is accurate.
> > 	 */
> > -	if (kvm_vcpu_is_illegal_gpa(vcpu, cr3))
> > +	if (!kvm_is_valid_cr3(vcpu, cr3))
> 
> There are other call sites of kvm_vcpu_is_illegal_gpa() to validate
> cr3.
> Do you need to modify them?

I don't think so. Others are for gpa validation, no need to change.
Here is for CR3.
> 
> > 		return 1;
> > 
> > 	if (is_pae_paging(vcpu) && !load_pdptrs(vcpu, cr3))
> > -- 
> > 2.31.1
> >
Chao Gao Feb. 14, 2023, 6:18 a.m. UTC | #3 
On Mon, Feb 13, 2023 at 09:25:50PM +0800, Robert Hoo wrote:
>On Mon, 2023-02-13 at 10:01 +0800, Chao Gao wrote:
>> On Thu, Feb 09, 2023 at 10:40:19AM +0800, Robert Hoo wrote:
>> > Before apply to kvm_vcpu_is_illegal_gpa(), clear LAM bits if it's
>> > valid.
>> 
>> I prefer to squash this patch into patch 2 because it is also related
>> to
>> CR3 LAM bits handling.
>> 
>Though all surround CR3, I would prefer split into pieces, so that
>easier for review and accept. I can change their order to group
>together. Is is all right for you?

No. To me, it doesn't make review easier at all.

This patch itself is incomplete and lacks proper justification. Merging
related patches together can show the whole picture and it is easier
to write some justification.

There are two ways that make sense to me:
option 1:

patch 1: virtualize CR4.LAM_SUP
patch 2: virtualize CR3.LAM_U48/U57
patch 3: virtualize LAM masking on applicable data accesses
patch 4: expose LAM CPUID bit to user sapce VMM

option 2:
given the series has only 100 LoC, you can merge all patches into a
single patch.


>> > {
>> > 	bool skip_tlb_flush = false;
>> > @@ -1254,7 +1262,7 @@ int kvm_set_cr3(struct kvm_vcpu *vcpu,
>> > unsigned long cr3)
>> > 	 * stuff CR3, e.g. for RSM emulation, and there is no guarantee
>> > that
>> > 	 * the current vCPU mode is accurate.
>> > 	 */
>> > -	if (kvm_vcpu_is_illegal_gpa(vcpu, cr3))
>> > +	if (!kvm_is_valid_cr3(vcpu, cr3))
>> 
>> There are other call sites of kvm_vcpu_is_illegal_gpa() to validate
>> cr3.
>> Do you need to modify them?
>
>I don't think so. Others are for gpa validation, no need to change.
>Here is for CR3.

how about the call in kvm_is_valid_sregs()? if you don't change it, when
user space VMM tries to set a CR3 with any LAM bits, KVM thinks the CR3
is illegal and returns an error. To me it means live migration probably
is broken.

And the call in nested_vmx_check_host_state()? L1 VMM should be allowed to
program a CR3 with LAM bits set to VMCS's HOST_CR3 field. Actually, it
is exactly what this patch 6 is doing.

Please inspect other call sites carefully.

>> 
>> > 		return 1;
>> > 
>> > 	if (is_pae_paging(vcpu) && !load_pdptrs(vcpu, cr3))
>> > -- 
>> > 2.31.1
>> > 
>
Chao Gao Feb. 14, 2023, 7 a.m. UTC | #4 
On Tue, Feb 14, 2023 at 02:18:59PM +0800, Chao Gao wrote:
>>> > 	bool skip_tlb_flush = false;
>>> > @@ -1254,7 +1262,7 @@ int kvm_set_cr3(struct kvm_vcpu *vcpu,
>>> > unsigned long cr3)
>>> > 	 * stuff CR3, e.g. for RSM emulation, and there is no guarantee
>>> > that
>>> > 	 * the current vCPU mode is accurate.
>>> > 	 */
>>> > -	if (kvm_vcpu_is_illegal_gpa(vcpu, cr3))
>>> > +	if (!kvm_is_valid_cr3(vcpu, cr3))
>>> 
>>> There are other call sites of kvm_vcpu_is_illegal_gpa() to validate
>>> cr3.
>>> Do you need to modify them?
>>
>>I don't think so. Others are for gpa validation, no need to change.
>>Here is for CR3.
>
>how about the call in kvm_is_valid_sregs()? if you don't change it, when
>user space VMM tries to set a CR3 with any LAM bits, KVM thinks the CR3
>is illegal and returns an error. To me it means live migration probably
>is broken.
>
>And the call in nested_vmx_check_host_state()? L1 VMM should be allowed to
>program a CR3 with LAM bits set to VMCS's HOST_CR3 field. Actually, it
>is exactly what this patch 6 is doing.

Please disregard "Actually, it is exactly what this patch 6 is doing".
My brain just disconnected.
Robert Hoo Feb. 18, 2023, 5:44 a.m. UTC | #5 
On Tue, 2023-02-14 at 14:18 +0800, Chao Gao wrote:
> There are two ways that make sense to me:
> option 1:
> 
> patch 1: virtualize CR4.LAM_SUP
> patch 2: virtualize CR3.LAM_U48/U57
> patch 3: virtualize LAM masking on applicable data accesses
And patch 4: KVM emulation: Apply LAM when emulating data access
> patch 4: expose LAM CPUID bit to user sapce VMM

> > > > 	 * the current vCPU mode is accurate.
> > > > 	 */
> > > > -	if (kvm_vcpu_is_illegal_gpa(vcpu, cr3))
> > > > +	if (!kvm_is_valid_cr3(vcpu, cr3))
> > > 
> > > There are other call sites of kvm_vcpu_is_illegal_gpa() to
> > > validate
> > > cr3.
> > > Do you need to modify them?
> > 
> > I don't think so. Others are for gpa validation, no need to change.
> > Here is for CR3.
> 
> how about the call in kvm_is_valid_sregs()? if you don't change it,
> when
> user space VMM tries to set a CR3 with any LAM bits, KVM thinks the
> CR3
> is illegal and returns an error. To me it means live migration
> probably
> is broken.

Agree. Will add this check in v5.
> 
> And the call in nested_vmx_check_host_state()? L1 VMM should be
> allowed to
> program a CR3 with LAM bits set to VMCS's HOST_CR3 field. 

Right, per spec, it should be allowed.
diff mbox series

Patch

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 1bdc8c0c80c0..3218f465ae71 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1231,6 +1231,14 @@  static void kvm_invalidate_pcid(struct kvm_vcpu *vcpu, unsigned long pcid)
 	kvm_mmu_free_roots(vcpu->kvm, mmu, roots_to_free);
 }
 
+static bool kvm_is_valid_cr3(struct kvm_vcpu *vcpu, unsigned long cr3)
+{
+	if (guest_cpuid_has(vcpu, X86_FEATURE_LAM))
+		cr3 &= ~(X86_CR3_LAM_U48 | X86_CR3_LAM_U57);
+
+	return kvm_vcpu_is_legal_gpa(vcpu, cr3);
+}
+
 int kvm_set_cr3(struct kvm_vcpu *vcpu, unsigned long cr3)
 {
 	bool skip_tlb_flush = false;
@@ -1254,7 +1262,7 @@  int kvm_set_cr3(struct kvm_vcpu *vcpu, unsigned long cr3)
 	 * stuff CR3, e.g. for RSM emulation, and there is no guarantee that
 	 * the current vCPU mode is accurate.
 	 */
-	if (kvm_vcpu_is_illegal_gpa(vcpu, cr3))
+	if (!kvm_is_valid_cr3(vcpu, cr3))
 		return 1;
 
 	if (is_pae_paging(vcpu) && !load_pdptrs(vcpu, cr3))