Received-SPF: pass (google.com: domain of linux-kernel+bounces-149260-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240417153450.3608097-1-pbonzini@redhat.com> <20240417153450.3608097-6-pbonzini@redhat.com>
 <ZiA-DQi52hroCSZ8@google.com>
In-Reply-To: <ZiA-DQi52hroCSZ8@google.com>
From: Paolo Bonzini <pbonzini@redhat.com>
Date: Wed, 17 Apr 2024 23:31:49 +0200
Message-ID: <CABgObfaoVMzEhu6O5HPe=GXH-bCkpTwSy8Ji0a1=je6f3eSqRQ@mail.gmail.com>
Subject: Re: [PATCH 5/7] KVM: x86/mmu: Introduce kvm_tdp_map_page() to
 populate guest memory
To: Sean Christopherson <seanjc@google.com>
Cc: linux-kernel@vger.kernel.org, kvm@vger.kernel.org, 
	isaku.yamahata@intel.com, xiaoyao.li@intel.com, binbin.wu@linux.intel.com, 
	rick.p.edgecombe@intel.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Wed, Apr 17, 2024 at 11:24=E2=80=AFPM Sean Christopherson <seanjc@google=
com> wrote:
>
> On Wed, Apr 17, 2024, Paolo Bonzini wrote:
> > From: Isaku Yamahata <isaku.yamahata@intel.com>
> >
> > Introduce a helper function to call the KVM fault handler.  It allows a=
 new
> > ioctl to invoke the KVM fault handler to populate without seeing RET_PF=
_*
> > enums or other KVM MMU internal definitions because RET_PF_* are intern=
al
> > to x86 KVM MMU.  The implementation is restricted to two-dimensional pa=
ging
> > for simplicity.  The shadow paging uses GVA for faulting instead of L1 =
GPA.
> > It makes the API difficult to use.
> >
> > Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>
> > Message-ID: <9b866a0ae7147f96571c439e75429a03dcb659b6.1712785629.git.is=
aku.yamahata@intel.com>
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > ---
> >  arch/x86/kvm/mmu.h     |  3 +++
> >  arch/x86/kvm/mmu/mmu.c | 32 ++++++++++++++++++++++++++++++++
> >  2 files changed, 35 insertions(+)
> >
> > diff --git a/arch/x86/kvm/mmu.h b/arch/x86/kvm/mmu.h
> > index e8b620a85627..51ff4f67e115 100644
> > --- a/arch/x86/kvm/mmu.h
> > +++ b/arch/x86/kvm/mmu.h
> > @@ -183,6 +183,9 @@ static inline void kvm_mmu_refresh_passthrough_bits=
(struct kvm_vcpu *vcpu,
> >       __kvm_mmu_refresh_passthrough_bits(vcpu, mmu);
> >  }
> >
> > +int kvm_tdp_map_page(struct kvm_vcpu *vcpu, gpa_t gpa, u64 error_code,
> > +                  u8 *level);
> > +
> >  /*
> >   * Check if a given access (described through the I/D, W/R and U/S bit=
s of a
> >   * page fault error code pfec) causes a permission fault with the give=
n PTE
> > diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> > index 7fbcfc97edcc..fb2149d16f8d 100644
> > --- a/arch/x86/kvm/mmu/mmu.c
> > +++ b/arch/x86/kvm/mmu/mmu.c
> > @@ -4646,6 +4646,38 @@ int kvm_tdp_page_fault(struct kvm_vcpu *vcpu, st=
ruct kvm_page_fault *fault)
> >       return direct_page_fault(vcpu, fault);
> >  }
> >
> > +int kvm_tdp_map_page(struct kvm_vcpu *vcpu, gpa_t gpa, u64 error_code,
> > +                  u8 *level)
>
> If the return is an overloaded "long", then there's no need for @level, i=
e. do
> the level=3D>size conversion in this helper.
>
> > +{
> > +     int r;
> > +
> > +     /* Restrict to TDP page fault. */
>
> Do we want to restrict this to the TDP MMU?  Not for any particular reaso=
n, mostly
> just to keep moving towards officially deprecating/removing TDP support f=
rom the
> shadow MMU.

Heh, yet another thing I briefly thought about while reviewing Isaku's
work. In the end I decided that, with the implementation being just a
regular prefault, there's not much to save from keeping the shadow MMU
away from this.

The real ugly part is that if the memslots are zapped the
pre-population effect basically goes away (damn
kvm_arch_flush_shadow_memslot). This is the reason why I initially
thought of KVM_CHECK_EXTENSION for the VM file descriptor, to only
allow this for TDX VMs.

The real solution for this is to not "graduate" this ioctl too soon to
kvm/next. Let's keep it in kvm-coco-queue until TDX is ready and then
make a final decision.

Paolo

> > +     if (vcpu->arch.mmu->page_fault !=3D kvm_tdp_page_fault)
> > +             return -EOPNOTSUPP;
> > +
> > +     r =3D __kvm_mmu_do_page_fault(vcpu, gpa, error_code, true, NULL, =
level);
> > +     if (r < 0)
> > +             return r;
> > +
> > +     switch (r) {
> > +     case RET_PF_RETRY:
> > +             return -EAGAIN;
> > +
> > +     case RET_PF_FIXED:
> > +     case RET_PF_SPURIOUS:
> > +             return 0;
>
> Going with the "long" idea, this becomes:
>
>                 end =3D (gpa & KVM_HPAGE_MASK(level)) + KVM_HPAGE_SIZE(le=
vel);
>                 return min(size, end - gpa);
>
> though I would vote for a:
>
>                 break;
>
> so that the happy path is nicely isolated at the end of the function.
>
> > +
> > +     case RET_PF_EMULATE:
> > +             return -EINVAL;
> > +
> > +     case RET_PF_CONTINUE:
> > +     case RET_PF_INVALID:
> > +     default:
> > +             WARN_ON_ONCE(r);
> > +             return -EIO;
> > +     }
> > +}
> > +
> >  static void nonpaging_init_context(struct kvm_mmu *context)
> >  {
> >       context->page_fault =3D nonpaging_page_fault;
> > --
> > 2.43.0
> >
> >
>