Re: [PATCH Part2 v6 39/49] KVM: SVM: Introduce ops for the post gfn map and unmap

[Kalra, Ashish]

On 11/17/2022 2:18 PM, Peter Gonda wrote:
> On Wed, Aug 17, 2022 at 9:47 PM Alper Gun <alpergun@xxxxxxxxxx> wrote:
> > On Mon, Jun 20, 2022 at 4:12 PM Ashish Kalra <Ashish.Kalra@xxxxxxx> wrote:
> > > From: Brijesh Singh <brijesh.singh@xxxxxxx>
> > >
> > > When SEV-SNP is enabled in the guest VM, the guest memory pages can
> > > either be a private or shared. A write from the hypervisor goes through
> > > the RMP checks. If hardware sees that hypervisor is attempting to write
> > > to a guest private page, then it triggers an RMP violation #PF.
> > >
> > > To avoid the RMP violation with GHCB pages, added new post_{map,unmap}_gfn
> > > functions to verify if its safe to map GHCB pages.  Uses a spinlock to
> > > protect against the page state change for existing mapped pages.
> > >
> > > Need to add generic post_{map,unmap}_gfn() ops that can be used to verify
> > > that its safe to map a given guest page in the hypervisor.
> > >
> > > This patch will need to be revisited later after consensus is reached on
> > > how to manage guest private memory as probably UPM private memslots will
> > > be able to handle this page state change more gracefully.
> > >
> > > Signed-off-by: Brijesh Singh <brijesh.singh@xxxxxxx>
> > > Signed-off by: Ashish Kalra <ashish.kalra@xxxxxxx>
> > > ---
> > >   arch/x86/include/asm/kvm-x86-ops.h |  1 +
> > >   arch/x86/include/asm/kvm_host.h    |  3 ++
> > >   arch/x86/kvm/svm/sev.c             | 48 ++++++++++++++++++++++++++++--
> > >   arch/x86/kvm/svm/svm.c             |  3 ++
> > >   arch/x86/kvm/svm/svm.h             | 11 +++++++
> > >   5 files changed, 64 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/arch/x86/include/asm/kvm-x86-ops.h b/arch/x86/include/asm/kvm-x86-ops.h
> > > index e0068e702692..2dd2bc0cf4c3 100644
> > > --- a/arch/x86/include/asm/kvm-x86-ops.h
> > > +++ b/arch/x86/include/asm/kvm-x86-ops.h
> > > @@ -130,6 +130,7 @@ KVM_X86_OP(vcpu_deliver_sipi_vector)
> > >   KVM_X86_OP_OPTIONAL_RET0(vcpu_get_apicv_inhibit_reasons);
> > >   KVM_X86_OP(alloc_apic_backing_page)
> > >   KVM_X86_OP_OPTIONAL(rmp_page_level_adjust)
> > > +KVM_X86_OP(update_protected_guest_state)
> > >
> > >   #undef KVM_X86_OP
> > >   #undef KVM_X86_OP_OPTIONAL
> > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> > > index 49b217dc8d7e..8abc0e724f5c 100644
> > > --- a/arch/x86/include/asm/kvm_host.h
> > > +++ b/arch/x86/include/asm/kvm_host.h
> > > @@ -1522,7 +1522,10 @@ struct kvm_x86_ops {
> > >          unsigned long (*vcpu_get_apicv_inhibit_reasons)(struct kvm_vcpu *vcpu);
> > >
> > >          void *(*alloc_apic_backing_page)(struct kvm_vcpu *vcpu);
> > > +
> > >          void (*rmp_page_level_adjust)(struct kvm *kvm, kvm_pfn_t pfn, int *level);
> > > +
> > > +       int (*update_protected_guest_state)(struct kvm_vcpu *vcpu);
> > >   };
> > >
> > >   struct kvm_x86_nested_ops {
> > > diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> > > index cb2d1bbb862b..4ed90331bca0 100644
> > > --- a/arch/x86/kvm/svm/sev.c
> > > +++ b/arch/x86/kvm/svm/sev.c
> > > @@ -341,6 +341,7 @@ static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp)
> > >                  if (ret)
> > >                          goto e_free;
> > >
> > > +               spin_lock_init(&sev->psc_lock);
> > >                  ret = sev_snp_init(&argp->error);
> > >          } else {
> > >                  ret = sev_platform_init(&argp->error);
> > > @@ -2828,19 +2829,28 @@ static inline int svm_map_ghcb(struct vcpu_svm *svm, struct kvm_host_map *map)
> > >   {
> > >          struct vmcb_control_area *control = &svm->vmcb->control;
> > >          u64 gfn = gpa_to_gfn(control->ghcb_gpa);
> > > +       struct kvm_vcpu *vcpu = &svm->vcpu;
> > >
> > > -       if (kvm_vcpu_map(&svm->vcpu, gfn, map)) {
> > > +       if (kvm_vcpu_map(vcpu, gfn, map)) {
> > >                  /* Unable to map GHCB from guest */
> > >                  pr_err("error mapping GHCB GFN [%#llx] from guest\n", gfn);
> > >                  return -EFAULT;
> > >          }
> > >
> > > +       if (sev_post_map_gfn(vcpu->kvm, map->gfn, map->pfn)) {
> > > +               kvm_vcpu_unmap(vcpu, map, false);
> > > +               return -EBUSY;
> > > +       }
> > > +
> > >          return 0;
> > >   }
> > >
> > >   static inline void svm_unmap_ghcb(struct vcpu_svm *svm, struct kvm_host_map *map)
> > >   {
> > > -       kvm_vcpu_unmap(&svm->vcpu, map, true);
> > > +       struct kvm_vcpu *vcpu = &svm->vcpu;
> > > +
> > > +       kvm_vcpu_unmap(vcpu, map, true);
> > > +       sev_post_unmap_gfn(vcpu->kvm, map->gfn, map->pfn);
> > >   }
> > >
> > >   static void dump_ghcb(struct vcpu_svm *svm)
> > > @@ -3383,6 +3393,8 @@ static int __snp_handle_page_state_change(struct kvm_vcpu *vcpu, enum psc_op op,
> > >                                  return PSC_UNDEF_ERR;
> > >                  }
> > >
> > > +               spin_lock(&sev->psc_lock);
> > > +
> > >                  write_lock(&kvm->mmu_lock);
> > >
> > >                  rc = kvm_mmu_get_tdp_walk(vcpu, gpa, &pfn, &npt_level);
> > > @@ -3417,6 +3429,8 @@ static int __snp_handle_page_state_change(struct kvm_vcpu *vcpu, enum psc_op op,
> > >
> > >                  write_unlock(&kvm->mmu_lock);
> > >
> > > +               spin_unlock(&sev->psc_lock);
> >
> > There is a corner case where the psc_lock is not released. If
> > kvm_mmu_get_tdp_walk fails, the lock will be kept and will cause soft
> > lockup.

This is also already fixed for v7.

Thanks,
Ashish