[PATCH MANUALSEL 5.14 1/5] KVM: X86: fix lazy allocation of rmaps

[PATCH MANUALSEL 5.14 1/5] KVM: X86: fix lazy allocation of rmaps

[Sasha Levin]

From: Paolo Bonzini <pbonzini@redhat.com>

[ Upstream commit fa13843d1565d4c5b3aeb9be3343b313416bef46 ]

If allocation of rmaps fails, but some of the pointers have already been written,
those pointers can be cleaned up when the memslot is freed, or even reused later
for another attempt at allocating the rmaps.  Therefore there is no need to
WARN, as done for example in memslot_rmap_alloc, but the allocation *must* be
skipped lest KVM will overwrite the previous pointer and will indeed leak memory.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 arch/x86/kvm/x86.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 4b0e866e9f08..60d9aa0ab389 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -11341,7 +11341,8 @@ static int memslot_rmap_alloc(struct kvm_memory_slot *slot,
 		int lpages = gfn_to_index(slot->base_gfn + npages - 1,
 					  slot->base_gfn, level) + 1;
 
-		WARN_ON(slot->arch.rmap[i]);
+		if (slot->arch.rmap[i])
+			continue;
 
 		slot->arch.rmap[i] = kvcalloc(lpages, sz, GFP_KERNEL_ACCOUNT);
 		if (!slot->arch.rmap[i]) {
-- 
2.33.0

[PATCH MANUALSEL 5.14 2/5] KVM: x86: WARN if APIC HW/SW disable static keys are non-zero on unload

[Sasha Levin]

From: Sean Christopherson <seanjc@google.com>

[ Upstream commit 9139a7a64581c80d157027ae20e86f2f24d4292c ]

WARN if the static keys used to track if any vCPU has disabled its APIC
are left elevated at module exit.  Unlike the underflow case, nothing in
the static key infrastructure will complain if a key is left elevated,
and because an elevated key only affects performance, nothing in KVM will
fail if either key is improperly incremented.

Signed-off-by: Sean Christopherson <seanjc@google.com>
Message-Id: <20211013003554.47705-3-seanjc@google.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 arch/x86/kvm/lapic.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index ba5a27879f1d..18cb699b0a14 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -2942,5 +2942,7 @@ int kvm_apic_accept_events(struct kvm_vcpu *vcpu)
 void kvm_lapic_exit(void)
 {
 	static_key_deferred_flush(&apic_hw_disabled);
+	WARN_ON(static_branch_unlikely(&apic_hw_disabled.key));
 	static_key_deferred_flush(&apic_sw_disabled);
+	WARN_ON(static_branch_unlikely(&apic_sw_disabled.key));
 }
-- 
2.33.0

[PATCH MANUALSEL 5.14 3/5] KVM: VMX: Remove redundant handling of bus lock vmexit

[Sasha Levin]

From: Hao Xiang <hao.xiang@linux.alibaba.com>

[ Upstream commit d61863c66f9b443192997613cd6aeca3f65cc313 ]

Hardware may or may not set exit_reason.bus_lock_detected on BUS_LOCK
VM-Exits. Dealing with KVM_RUN_X86_BUS_LOCK in handle_bus_lock_vmexit
could be redundant when exit_reason.basic is EXIT_REASON_BUS_LOCK.

We can remove redundant handling of bus lock vmexit. Unconditionally Set
exit_reason.bus_lock_detected in handle_bus_lock_vmexit(), and deal with
KVM_RUN_X86_BUS_LOCK only in vmx_handle_exit().

Signed-off-by: Hao Xiang <hao.xiang@linux.alibaba.com>
Message-Id: <1634299161-30101-1-git-send-email-hao.xiang@linux.alibaba.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 arch/x86/kvm/vmx/vmx.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 55de1eb135f9..87150b3c9c5f 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -5551,9 +5551,13 @@ static int handle_encls(struct kvm_vcpu *vcpu)
 
 static int handle_bus_lock_vmexit(struct kvm_vcpu *vcpu)
 {
-	vcpu->run->exit_reason = KVM_EXIT_X86_BUS_LOCK;
-	vcpu->run->flags |= KVM_RUN_X86_BUS_LOCK;
-	return 0;
+	/*
+	 * Hardware may or may not set the BUS_LOCK_DETECTED flag on BUS_LOCK
+	 * VM-Exits. Unconditionally set the flag here and leave the handling to
+	 * vmx_handle_exit().
+	 */
+	to_vmx(vcpu)->exit_reason.bus_lock_detected = true;
+	return 1;
 }
 
 /*
@@ -6039,9 +6043,8 @@ static int vmx_handle_exit(struct kvm_vcpu *vcpu, fastpath_t exit_fastpath)
 	int ret = __vmx_handle_exit(vcpu, exit_fastpath);
 
 	/*
-	 * Even when current exit reason is handled by KVM internally, we
-	 * still need to exit to user space when bus lock detected to inform
-	 * that there is a bus lock in guest.
+	 * Exit to user space when bus lock detected to inform that there is
+	 * a bus lock in guest.
 	 */
 	if (to_vmx(vcpu)->exit_reason.bus_lock_detected) {
 		if (ret > 0)
-- 
2.33.0

[PATCH MANUALSEL 5.14 4/5] KVM: SEV-ES: Set guest_state_protected after VMSA update

[Sasha Levin]

From: Peter Gonda <pgonda@google.com>

[ Upstream commit baa1e5ca172ce7bf9554070139482dd7ea919528 ]

The refactoring in commit bb18a6777465 ("KVM: SEV: Acquire
vcpu mutex when updating VMSA") left behind the assignment to
svm->vcpu.arch.guest_state_protected; add it back.

Signed-off-by: Peter Gonda <pgonda@google.com>
[Delta between v2 and v3 of Peter's patch, which had already been
 committed; the commit message is my own. - Paolo]
Fixes: bb18a6777465 ("KVM: SEV: Acquire vcpu mutex when updating VMSA")
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 arch/x86/kvm/svm/sev.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index cb166bde449b..50dabccd664e 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -619,7 +619,12 @@ static int __sev_launch_update_vmsa(struct kvm *kvm, struct kvm_vcpu *vcpu,
 	vmsa.handle = to_kvm_svm(kvm)->sev_info.handle;
 	vmsa.address = __sme_pa(svm->vmsa);
 	vmsa.len = PAGE_SIZE;
-	return sev_issue_cmd(kvm, SEV_CMD_LAUNCH_UPDATE_VMSA, &vmsa, error);
+	ret = sev_issue_cmd(kvm, SEV_CMD_LAUNCH_UPDATE_VMSA, &vmsa, error);
+	if (ret)
+	  return ret;
+
+	vcpu->arch.guest_state_protected = true;
+	return 0;
 }
 
 static int sev_launch_update_vmsa(struct kvm *kvm, struct kvm_sev_cmd *argp)
-- 
2.33.0

[PATCH MANUALSEL 5.14 5/5] KVM: MMU: Reset mmu->pkru_mask to avoid stale data

[Sasha Levin]

From: Chenyi Qiang <chenyi.qiang@intel.com>

[ Upstream commit a3ca5281bb771d8103ea16f0a6a8a5df9a7fb4f3 ]

When updating mmu->pkru_mask, the value can only be added but it isn't
reset in advance. This will make mmu->pkru_mask keep the stale data.
Fix this issue.

Fixes: 2d344105f57c ("KVM, pkeys: introduce pkru_mask to cache conditions")
Signed-off-by: Chenyi Qiang <chenyi.qiang@intel.com>
Message-Id: <20211021071022.1140-1-chenyi.qiang@intel.com>
Reviewed-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 arch/x86/kvm/mmu/mmu.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
index c268fb59f779..6719a8041f59 100644
--- a/arch/x86/kvm/mmu/mmu.c
+++ b/arch/x86/kvm/mmu/mmu.c
@@ -4465,10 +4465,10 @@ static void update_pkru_bitmask(struct kvm_mmu *mmu)
 	unsigned bit;
 	bool wp;
 
-	if (!is_cr4_pke(mmu)) {
-		mmu->pkru_mask = 0;
+	mmu->pkru_mask = 0;
+
+	if (!is_cr4_pke(mmu))
 		return;
-	}
 
 	wp = is_cr0_wp(mmu);
 
-- 
2.33.0

Re: [PATCH MANUALSEL 5.14 1/5] KVM: X86: fix lazy allocation of rmaps

[Paolo Bonzini]

On 25/10/21 22:38, Sasha Levin wrote:
> From: Paolo Bonzini <pbonzini@redhat.com>
> 
> [ Upstream commit fa13843d1565d4c5b3aeb9be3343b313416bef46 ]
> 
> If allocation of rmaps fails, but some of the pointers have already been written,
> those pointers can be cleaned up when the memslot is freed, or even reused later
> for another attempt at allocating the rmaps.  Therefore there is no need to
> WARN, as done for example in memslot_rmap_alloc, but the allocation *must* be
> skipped lest KVM will overwrite the previous pointer and will indeed leak memory.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Sasha Levin <sashal@kernel.org>
> ---
>   arch/x86/kvm/x86.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 4b0e866e9f08..60d9aa0ab389 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -11341,7 +11341,8 @@ static int memslot_rmap_alloc(struct kvm_memory_slot *slot,
>   		int lpages = gfn_to_index(slot->base_gfn + npages - 1,
>   					  slot->base_gfn, level) + 1;
>   
> -		WARN_ON(slot->arch.rmap[i]);
> +		if (slot->arch.rmap[i])
> +			continue;
>   
>   		slot->arch.rmap[i] = kvcalloc(lpages, sz, GFP_KERNEL_ACCOUNT);
>   		if (!slot->arch.rmap[i]) {
> 

NACK

There is no lazy allocation of rmaps in 5.14, and any failure to 
allocate goes straight to memslot_rmap_free followed by return -ENOMEM. 
  So the WARN_ON is justified there.

Paolo

Re: [PATCH MANUALSEL 5.14 2/5] KVM: x86: WARN if APIC HW/SW disable static keys are non-zero on unload

[Paolo Bonzini]

On 25/10/21 22:38, Sasha Levin wrote:
> From: Sean Christopherson <seanjc@google.com>
> 
> [ Upstream commit 9139a7a64581c80d157027ae20e86f2f24d4292c ]
> 
> WARN if the static keys used to track if any vCPU has disabled its APIC
> are left elevated at module exit.  Unlike the underflow case, nothing in
> the static key infrastructure will complain if a key is left elevated,
> and because an elevated key only affects performance, nothing in KVM will
> fail if either key is improperly incremented.
> 
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> Message-Id: <20211013003554.47705-3-seanjc@google.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Sasha Levin <sashal@kernel.org>
> ---
>   arch/x86/kvm/lapic.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index ba5a27879f1d..18cb699b0a14 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -2942,5 +2942,7 @@ int kvm_apic_accept_events(struct kvm_vcpu *vcpu)
>   void kvm_lapic_exit(void)
>   {
>   	static_key_deferred_flush(&apic_hw_disabled);
> +	WARN_ON(static_branch_unlikely(&apic_hw_disabled.key));
>   	static_key_deferred_flush(&apic_sw_disabled);
> +	WARN_ON(static_branch_unlikely(&apic_sw_disabled.key));
>   }
> 

NACK

Paolo

Re: [PATCH MANUALSEL 5.14 3/5] KVM: VMX: Remove redundant handling of bus lock vmexit

[Paolo Bonzini]

On 25/10/21 22:38, Sasha Levin wrote:
> From: Hao Xiang <hao.xiang@linux.alibaba.com>
> 
> [ Upstream commit d61863c66f9b443192997613cd6aeca3f65cc313 ]
> 
> Hardware may or may not set exit_reason.bus_lock_detected on BUS_LOCK
> VM-Exits. Dealing with KVM_RUN_X86_BUS_LOCK in handle_bus_lock_vmexit
> could be redundant when exit_reason.basic is EXIT_REASON_BUS_LOCK.
> 
> We can remove redundant handling of bus lock vmexit. Unconditionally Set
> exit_reason.bus_lock_detected in handle_bus_lock_vmexit(), and deal with
> KVM_RUN_X86_BUS_LOCK only in vmx_handle_exit().
> 
> Signed-off-by: Hao Xiang <hao.xiang@linux.alibaba.com>
> Message-Id: <1634299161-30101-1-git-send-email-hao.xiang@linux.alibaba.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Sasha Levin <sashal@kernel.org>
> ---
>   arch/x86/kvm/vmx/vmx.c | 15 +++++++++------
>   1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 55de1eb135f9..87150b3c9c5f 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -5551,9 +5551,13 @@ static int handle_encls(struct kvm_vcpu *vcpu)
>   
>   static int handle_bus_lock_vmexit(struct kvm_vcpu *vcpu)
>   {
> -	vcpu->run->exit_reason = KVM_EXIT_X86_BUS_LOCK;
> -	vcpu->run->flags |= KVM_RUN_X86_BUS_LOCK;
> -	return 0;
> +	/*
> +	 * Hardware may or may not set the BUS_LOCK_DETECTED flag on BUS_LOCK
> +	 * VM-Exits. Unconditionally set the flag here and leave the handling to
> +	 * vmx_handle_exit().
> +	 */
> +	to_vmx(vcpu)->exit_reason.bus_lock_detected = true;
> +	return 1;
>   }
>   
>   /*
> @@ -6039,9 +6043,8 @@ static int vmx_handle_exit(struct kvm_vcpu *vcpu, fastpath_t exit_fastpath)
>   	int ret = __vmx_handle_exit(vcpu, exit_fastpath);
>   
>   	/*
> -	 * Even when current exit reason is handled by KVM internally, we
> -	 * still need to exit to user space when bus lock detected to inform
> -	 * that there is a bus lock in guest.
> +	 * Exit to user space when bus lock detected to inform that there is
> +	 * a bus lock in guest.
>   	 */
>   	if (to_vmx(vcpu)->exit_reason.bus_lock_detected) {
>   		if (ret > 0)
> 

NACK

No functional change; this time the bot is not doing a good job. :)

Paolo

Re: [PATCH MANUALSEL 5.14 4/5] KVM: SEV-ES: Set guest_state_protected after VMSA update

[Paolo Bonzini]

On 25/10/21 22:38, Sasha Levin wrote:
> From: Peter Gonda <pgonda@google.com>
> 
> [ Upstream commit baa1e5ca172ce7bf9554070139482dd7ea919528 ]
> 
> The refactoring in commit bb18a6777465 ("KVM: SEV: Acquire
> vcpu mutex when updating VMSA") left behind the assignment to
> svm->vcpu.arch.guest_state_protected; add it back.
> 
> Signed-off-by: Peter Gonda <pgonda@google.com>
> [Delta between v2 and v3 of Peter's patch, which had already been
>   committed; the commit message is my own. - Paolo]
> Fixes: bb18a6777465 ("KVM: SEV: Acquire vcpu mutex when updating VMSA")
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Sasha Levin <sashal@kernel.org>
> ---
>   arch/x86/kvm/svm/sev.c | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index cb166bde449b..50dabccd664e 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -619,7 +619,12 @@ static int __sev_launch_update_vmsa(struct kvm *kvm, struct kvm_vcpu *vcpu,
>   	vmsa.handle = to_kvm_svm(kvm)->sev_info.handle;
>   	vmsa.address = __sme_pa(svm->vmsa);
>   	vmsa.len = PAGE_SIZE;
> -	return sev_issue_cmd(kvm, SEV_CMD_LAUNCH_UPDATE_VMSA, &vmsa, error);
> +	ret = sev_issue_cmd(kvm, SEV_CMD_LAUNCH_UPDATE_VMSA, &vmsa, error);
> +	if (ret)
> +	  return ret;
> +
> +	vcpu->arch.guest_state_protected = true;
> +	return 0;
>   }
>   
>   static int sev_launch_update_vmsa(struct kvm *kvm, struct kvm_sev_cmd *argp)
> 

Acked-by: Paolo Bonzini <pbonzini@redhat.com>

I missed that bb18a677746543e7f5eeb478129c92cedb0f9658 was Cc'd to 
stable.  Good bot. :)

Paolo

Re: [PATCH MANUALSEL 5.14 5/5] KVM: MMU: Reset mmu->pkru_mask to avoid stale data

[Paolo Bonzini]

On 25/10/21 22:38, Sasha Levin wrote:
> From: Chenyi Qiang <chenyi.qiang@intel.com>
> 
> [ Upstream commit a3ca5281bb771d8103ea16f0a6a8a5df9a7fb4f3 ]
> 
> When updating mmu->pkru_mask, the value can only be added but it isn't
> reset in advance. This will make mmu->pkru_mask keep the stale data.
> Fix this issue.
> 
> Fixes: 2d344105f57c ("KVM, pkeys: introduce pkru_mask to cache conditions")
> Signed-off-by: Chenyi Qiang <chenyi.qiang@intel.com>
> Message-Id: <20211021071022.1140-1-chenyi.qiang@intel.com>
> Reviewed-by: Sean Christopherson <seanjc@google.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Sasha Levin <sashal@kernel.org>
> ---
>   arch/x86/kvm/mmu/mmu.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> index c268fb59f779..6719a8041f59 100644
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c
> @@ -4465,10 +4465,10 @@ static void update_pkru_bitmask(struct kvm_mmu *mmu)
>   	unsigned bit;
>   	bool wp;
>   
> -	if (!is_cr4_pke(mmu)) {
> -		mmu->pkru_mask = 0;
> +	mmu->pkru_mask = 0;
> +
> +	if (!is_cr4_pke(mmu))
>   		return;
> -	}
>   
>   	wp = is_cr0_wp(mmu);
>   
> 

Acked-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [PATCH MANUALSEL 5.14 1/5] KVM: X86: fix lazy allocation of rmaps

[Sasha Levin]

On Tue, Oct 26, 2021 at 06:14:34PM +0200, Paolo Bonzini wrote:
>On 25/10/21 22:38, Sasha Levin wrote:
>>From: Paolo Bonzini <pbonzini@redhat.com>
>>
>>[ Upstream commit fa13843d1565d4c5b3aeb9be3343b313416bef46 ]
>>
>>If allocation of rmaps fails, but some of the pointers have already been written,
>>those pointers can be cleaned up when the memslot is freed, or even reused later
>>for another attempt at allocating the rmaps.  Therefore there is no need to
>>WARN, as done for example in memslot_rmap_alloc, but the allocation *must* be
>>skipped lest KVM will overwrite the previous pointer and will indeed leak memory.
>>
>>Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>>Signed-off-by: Sasha Levin <sashal@kernel.org>
>>---
>>  arch/x86/kvm/x86.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>>diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>>index 4b0e866e9f08..60d9aa0ab389 100644
>>--- a/arch/x86/kvm/x86.c
>>+++ b/arch/x86/kvm/x86.c
>>@@ -11341,7 +11341,8 @@ static int memslot_rmap_alloc(struct kvm_memory_slot *slot,
>>  		int lpages = gfn_to_index(slot->base_gfn + npages - 1,
>>  					  slot->base_gfn, level) + 1;
>>-		WARN_ON(slot->arch.rmap[i]);
>>+		if (slot->arch.rmap[i])
>>+			continue;
>>  		slot->arch.rmap[i] = kvcalloc(lpages, sz, GFP_KERNEL_ACCOUNT);
>>  		if (!slot->arch.rmap[i]) {
>>
>
>NACK
>
>There is no lazy allocation of rmaps in 5.14, and any failure to 
>allocate goes straight to memslot_rmap_free followed by return 
>-ENOMEM.  So the WARN_ON is justified there.

I'll queue up the ones you've acked, thanks :)

-- 
Thanks,
Sasha