Re: [PATCH v2 7/9] KVM: SEV: Forcefully invalidate SNP VMSA if its backing gmem page is zapped

[Ackerley Tng]

Sean Christopherson <seanjc@xxxxxxxxxx> writes:

>
> [...snip...]
>
> diff --git a/arch/x86/include/asm/kvm-x86-ops.h b/arch/x86/include/asm/kvm-x86-ops.h
> index e36eba952705..69ca2a848ad6 100644
> --- a/arch/x86/include/asm/kvm-x86-ops.h
> +++ b/arch/x86/include/asm/kvm-x86-ops.h
> @@ -134,6 +134,7 @@ KVM_X86_OP_OPTIONAL(mem_enc_unregister_region)
>  KVM_X86_OP_OPTIONAL(vm_copy_enc_context_from)
>  KVM_X86_OP_OPTIONAL(vm_move_enc_context_from)
>  KVM_X86_OP_OPTIONAL(guest_memory_reclaimed)
> +KVM_X86_OP_OPTIONAL(reload_vmsa)

Regarding naming, this seems to be the most platform-specific KVM_X86_OP
there is (VMSA is a SEV-ES/SNP only thing), though I can't think of a
way to make this seem more general, to make it stick out less among the
x86 ops. Maybe it's not necessary to make it blend in anyway.

>  KVM_X86_OP(get_feature_msr)
>  KVM_X86_OP(check_emulate_instruction)
>  KVM_X86_OP(apic_init_signal_blocked)
> @@ -148,6 +149,7 @@ KVM_X86_OP_OPTIONAL(alloc_apic_backing_page)
>  KVM_X86_OP_OPTIONAL_RET0(gmem_prepare)
>  KVM_X86_OP_OPTIONAL_RET0(gmem_max_mapping_level)
>  #ifdef CONFIG_HAVE_KVM_ARCH_GMEM_INVALIDATE
> +KVM_X86_OP_OPTIONAL(gmem_invalidate_range)

Clarification: .gmem_invalidate_range() should be thought of as "this is
the hook called when some guest_memfd memory is invalidated" (as in,
this is a point in the lifecycle of guest_memfd memory) and not "this
hook invalidates guest memory in this range", right?

>  KVM_X86_OP_OPTIONAL(gmem_free_folio)
>  #endif
>  #endif
>
> [...snip...]
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index adc1e1b244c7..9df6acf9a982 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -8167,6 +8167,8 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
>  				goto out;
>  			}
>  		}
> +		if (kvm_check_request(KVM_REQ_VMSA_PAGE_RELOAD, vcpu))
> +			kvm_x86_call(reload_vmsa)(vcpu);

Should this be wrapped with an #ifdef to skip the check entirely for
non-SNP platforms?

>  	}
>
>  	if (kvm_check_request(KVM_REQ_EVENT, vcpu) || req_int_win ||
> @@ -10592,6 +10594,10 @@ int kvm_arch_gmem_prepare(struct kvm *kvm, gfn_t gfn, kvm_pfn_t pfn, int max_ord
>  #endif
>
>
> [...snip...]
>
> diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
> index 1618acc3ca64..8ec5041934db 100644
> --- a/virt/kvm/guest_memfd.c
> +++ b/virt/kvm/guest_memfd.c
> @@ -185,6 +185,10 @@ static void __kvm_gmem_invalidate_start(struct gmem_file *f, pgoff_t start,
>  		}
>
>  		flush |= kvm_mmu_unmap_gfn_range(kvm, &gfn_range);
> +
> +#ifdef CONFIG_HAVE_KVM_ARCH_GMEM_INVALIDATE
> +		kvm_arch_gmem_invalidate_range(kvm, &gfn_range);
> +#endif

In general, how do you think of using stubs vs @ifdefs?

Would it be better to define a stub for kvm_arch_gmem_invalidate_range()
that does nothing #ifdef CONFIG_HAVE_KVM_ARCH_GMEM_INVALIDATE?

>  	}
>
>  	if (flush)
> --
> 2.55.0.rc0.799.gd6f94ed593-goog