On Fri, Aug 20, 2021, Brijesh Singh wrote:
> When SEV-SNP is enabled in the guest, the hardware places restrictions on
> all memory accesses based on the contents of the RMP table. When hardware
> encounters RMP check failure caused by the guest memory access it raises
> the #NPF. The error code contains additional information on the access
> type. See the APM volume 2 for additional information.
>
> Signed-off-by: Brijesh Singh <brijesh.singh@xxxxxxx>
> ---
> arch/x86/kvm/svm/sev.c | 76 ++++++++++++++++++++++++++++++++++++++++++
> arch/x86/kvm/svm/svm.c | 14 +++++---
> arch/x86/kvm/svm/svm.h | 1 +
> 3 files changed, 87 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 65b578463271..712e8907bc39 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -3651,3 +3651,79 @@ void sev_post_unmap_gfn(struct kvm *kvm, gfn_t gfn, kvm_pfn_t pfn, int token)
>
> srcu_read_unlock(&sev->psc_srcu, token);
> }
> +
> +void handle_rmp_page_fault(struct kvm_vcpu *vcpu, gpa_t gpa, u64 error_code)
> +{
> + int rmp_level, npt_level, rc, assigned;
Really silly nit: can you use 'r' or 'ret' instead of 'rc'? Outside of the
emulator, which should never be the gold standard for code ;-), 'rc' isn't used
in x86 KVM.
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 3784d389247b..3ba62f21b113 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -1933,15 +1933,21 @@ static int pf_interception(struct kvm_vcpu *vcpu)
> static int npf_interception(struct kvm_vcpu *vcpu)
> {
> struct vcpu_svm *svm = to_svm(vcpu);
> + int rc;
>
> u64 fault_address = svm->vmcb->control.exit_info_2;
> u64 error_code = svm->vmcb->control.exit_info_1;
>
> trace_kvm_page_fault(fault_address, error_code);
> - return kvm_mmu_page_fault(vcpu, fault_address, error_code,
> - static_cpu_has(X86_FEATURE_DECODEASSISTS) ?
> - svm->vmcb->control.insn_bytes : NULL,
> - svm->vmcb->control.insn_len);
> + rc = kvm_mmu_page_fault(vcpu, fault_address, error_code,
> + static_cpu_has(X86_FEATURE_DECODEASSISTS) ?
> + svm->vmcb->control.insn_bytes : NULL,
> + svm->vmcb->control.insn_len);
> +
> + if (error_code & PFERR_GUEST_RMP_MASK)
Don't think it will matter in the end, but shouldn't this consult 'rc' before
diving into RMP handling?
> + handle_rmp_page_fault(vcpu, fault_address, error_code);
Similar to my comments on the PSC patches, I think this is backwards. The MMU
should provide the control logic to convert between private and shared, and vendor
code should only get involved when there are side effects from the conversion.
Once I've made a dent in my review backlog, I'll fiddle with this and some of the
other MMU interactions, and hopefully come up with a workable sketch for the MMU
stuff. Yell at me if you haven't gotten an update by Wednesday or so.
