[v6,25/29] KVM: Call kvm_arch_flush_shadow_memslot() on the old slot in kvm_invalidate_memslot()

Commit Message

Maciej S. Szmigiero Nov. 30, 2021, 9:41 p.m. UTC

From: "Maciej S. Szmigiero" <[e-mail]>

kvm_invalidate_memslot() calls kvm_arch_flush_shadow_memslot() on the
active, but KVM_MEMSLOT_INVALID slot.
Do it on the inactive (but valid) old slot instead since arch code really
should not get passed such invalid slot.

Suggested-by: Sean Christopherson <[e-mail]>
Signed-off-by: Maciej S. Szmigiero <[e-mail]>
---
 virt/kvm/kvm_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Sean Christopherson Dec. 1, 2021, 3:27 a.m. UTC | #1

On Tue, Nov 30, 2021, Maciej S. Szmigiero wrote:
> From: "Maciej S. Szmigiero" <[e-mail]>
> 
> kvm_invalidate_memslot() calls kvm_arch_flush_shadow_memslot() on the
> active, but KVM_MEMSLOT_INVALID slot.
> Do it on the inactive (but valid) old slot instead since arch code really
> should not get passed such invalid slot.

One other thing that's worth noting in the changelog is that "old->arch" may have
stale data.  IMO that's perfectly ok, but it's definitely a quirk.  Ideally KVM
would disallow touching "arch" for an INVALID slot, but that would require another
arch hook if kvm_prepare_memory_region() failed to refresh old->arch if necessary
before restoring it. :-/

Paolo, thoughts on this goofy case?  I don't love it, but I dislike having

	kvm_arch_flush_shadow_memslot(kvm, invalid_slot);

in the final code even more.

Reviewed-by: Sean Christopherson <[e-mail]> 

> Suggested-by: Sean Christopherson <[e-mail]>
> Signed-off-by: Maciej S. Szmigiero <[e-mail]>
> ---
>  virt/kvm/kvm_main.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index c57748ee41e8..086f18969bc3 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -1632,7 +1632,7 @@ static void kvm_invalidate_memslot(struct kvm *kvm,
>  	 *	- gfn_to_hva (kvm_read_guest, gfn_to_pfn)
>  	 *	- kvm_is_visible_gfn (mmu_check_root)
>  	 */
> -	kvm_arch_flush_shadow_memslot(kvm, working_slot);
> +	kvm_arch_flush_shadow_memslot(kvm, old);
>  
>  	/* Was released by kvm_swap_active_memslots, reacquire. */
>  	mutex_lock(&kvm->slots_arch_lock);

Maciej S. Szmigiero Dec. 1, 2021, 3:45 p.m. UTC | #2

On 01.12.2021 04:27, Sean Christopherson wrote:
> On Tue, Nov 30, 2021, Maciej S. Szmigiero wrote:
>> From: "Maciej S. Szmigiero" <[e-mail]>
>>
>> kvm_invalidate_memslot() calls kvm_arch_flush_shadow_memslot() on the
>> active, but KVM_MEMSLOT_INVALID slot.
>> Do it on the inactive (but valid) old slot instead since arch code really
>> should not get passed such invalid slot.
> 
> One other thing that's worth noting in the changelog is that "old->arch" may have
> stale data.  IMO that's perfectly ok, but it's definitely a quirk. 

Will add this caveat to this commit message.

> Ideally KVM
> would disallow touching "arch" for an INVALID slot, but that would require another
> arch hook if kvm_prepare_memory_region() failed to refresh old->arch if necessary
> before restoring it. :-/

It looks to me that only PPC book3s_hv actually uses the "arch" field in
its kvm_arch_flush_shadow_memslot() implementation.

But at the same time it does not seem to do lazy allocation for rmaps or
similar stuff.

However, the code there is complex enough for this change to be of
higher-than-usual risk.
That's why I have split it out from the main memslots sets rework.

Thanks,
Maciej

Patch

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index c57748ee41e8..086f18969bc3 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -1632,7 +1632,7 @@  static void kvm_invalidate_memslot(struct kvm *kvm,
 	 *	- gfn_to_hva (kvm_read_guest, gfn_to_pfn)
 	 *	- kvm_is_visible_gfn (mmu_check_root)
 	 */
-	kvm_arch_flush_shadow_memslot(kvm, working_slot);
+	kvm_arch_flush_shadow_memslot(kvm, old);
 
 	/* Was released by kvm_swap_active_memslots, reacquire. */
 	mutex_lock(&kvm->slots_arch_lock);