diff mbox series

[1/3] KVM: x86/mmu: Fix return value in tdp_mmu_map_handle_target_level()

Message ID 00875eb37d6b5cc9d19bb19e31db3130ac1d8730.1620200410.git.kai.huang@intel.com (mailing list archive)
State New, archived
Headers show
Series TDP MMU: several minor fixes or improvements | expand

Commit Message

Huang, Kai May 5, 2021, 9:37 a.m. UTC 
Currently tdp_mmu_map_handle_target_level() returns 0, which is
RET_PF_RETRY, when page fault is actually fixed.  This makes
kvm_tdp_mmu_map() also return RET_PF_RETRY in this case, instead of
RET_PF_FIXED.  Fix by initializing ret to RET_PF_FIXED.

Signed-off-by: Kai Huang <kai.huang@intel.com>
---
 arch/x86/kvm/mmu/tdp_mmu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Sean Christopherson May 5, 2021, 4 p.m. UTC | #1 
On Wed, May 05, 2021, Kai Huang wrote:
> Currently tdp_mmu_map_handle_target_level() returns 0, which is
> RET_PF_RETRY, when page fault is actually fixed.  This makes
> kvm_tdp_mmu_map() also return RET_PF_RETRY in this case, instead of
> RET_PF_FIXED.  Fix by initializing ret to RET_PF_FIXED.

Probably worth adding a blurb to call out that the bad return value is benign
since kvm_mmu_page_fault() resumes the guest on RET_PF_RETRY or RET_PF_FIXED.
And for good measure, a Fixes without stable@.

  Fixes: bb18842e2111 ("kvm: x86/mmu: Add TDP MMU PF handler")

Reviewed-by: Sean Christopherson <seanjc@google.com> 


Side topic...

Ben, does the TDP MMU intentionally not prefetch pages?  If so, why not?

> Signed-off-by: Kai Huang <kai.huang@intel.com>
> ---
>  arch/x86/kvm/mmu/tdp_mmu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
> index ff0ae030004d..1cad4c9f7c34 100644
> --- a/arch/x86/kvm/mmu/tdp_mmu.c
> +++ b/arch/x86/kvm/mmu/tdp_mmu.c
> @@ -905,7 +905,7 @@ static int tdp_mmu_map_handle_target_level(struct kvm_vcpu *vcpu, int write,
>  					  kvm_pfn_t pfn, bool prefault)
>  {
>  	u64 new_spte;
> -	int ret = 0;
> +	int ret = RET_PF_FIXED;
>  	int make_spte_ret = 0;
>  
>  	if (unlikely(is_noslot_pfn(pfn)))
> -- 
> 2.31.1
>
Ben Gardon May 5, 2021, 4:04 p.m. UTC | #2 
On Wed, May 5, 2021 at 9:00 AM Sean Christopherson <seanjc@google.com> wrote:
>
> On Wed, May 05, 2021, Kai Huang wrote:
> > Currently tdp_mmu_map_handle_target_level() returns 0, which is
> > RET_PF_RETRY, when page fault is actually fixed.  This makes
> > kvm_tdp_mmu_map() also return RET_PF_RETRY in this case, instead of
> > RET_PF_FIXED.  Fix by initializing ret to RET_PF_FIXED.
>
> Probably worth adding a blurb to call out that the bad return value is benign
> since kvm_mmu_page_fault() resumes the guest on RET_PF_RETRY or RET_PF_FIXED.
> And for good measure, a Fixes without stable@.
>
>   Fixes: bb18842e2111 ("kvm: x86/mmu: Add TDP MMU PF handler")
>
> Reviewed-by: Sean Christopherson <seanjc@google.com>

Haha I was just about to add the same two comments. Besides those,
this patch looks good to me as well.

Reviewed-by: Ben Gardon <bgardon@google.com>

>
>
> Side topic...
>
> Ben, does the TDP MMU intentionally not prefetch pages?  If so, why not?

It does not currently prefetch pages, but there's no reason it
shouldn't. It simply hasn't been implemented yet.

>
> > Signed-off-by: Kai Huang <kai.huang@intel.com>
> > ---
> >  arch/x86/kvm/mmu/tdp_mmu.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
> > index ff0ae030004d..1cad4c9f7c34 100644
> > --- a/arch/x86/kvm/mmu/tdp_mmu.c
> > +++ b/arch/x86/kvm/mmu/tdp_mmu.c
> > @@ -905,7 +905,7 @@ static int tdp_mmu_map_handle_target_level(struct kvm_vcpu *vcpu, int write,
> >                                         kvm_pfn_t pfn, bool prefault)
> >  {
> >       u64 new_spte;
> > -     int ret = 0;
> > +     int ret = RET_PF_FIXED;
> >       int make_spte_ret = 0;
> >
> >       if (unlikely(is_noslot_pfn(pfn)))
> > --
> > 2.31.1
> >
Huang, Kai May 6, 2021, 1:56 a.m. UTC | #3 
On Wed, 2021-05-05 at 09:04 -0700, Ben Gardon wrote:
> On Wed, May 5, 2021 at 9:00 AM Sean Christopherson <seanjc@google.com> wrote:
> > 
> > On Wed, May 05, 2021, Kai Huang wrote:
> > > Currently tdp_mmu_map_handle_target_level() returns 0, which is
> > > RET_PF_RETRY, when page fault is actually fixed.  This makes
> > > kvm_tdp_mmu_map() also return RET_PF_RETRY in this case, instead of
> > > RET_PF_FIXED.  Fix by initializing ret to RET_PF_FIXED.
> > 
> > Probably worth adding a blurb to call out that the bad return value is benign
> > since kvm_mmu_page_fault() resumes the guest on RET_PF_RETRY or RET_PF_FIXED.
> > And for good measure, a Fixes without stable@.
> > 
> >   Fixes: bb18842e2111 ("kvm: x86/mmu: Add TDP MMU PF handler")
> > 
> > Reviewed-by: Sean Christopherson <seanjc@google.com>
> 
> Haha I was just about to add the same two comments. Besides those,
> this patch looks good to me as well.
> 
> Reviewed-by: Ben Gardon <bgardon@google.com>
> 
> 

Thanks Sean and Ben. I'll add Sean's suggestion to commit message, and add a Fixes:...
diff mbox series

Patch

diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
index ff0ae030004d..1cad4c9f7c34 100644
--- a/arch/x86/kvm/mmu/tdp_mmu.c
+++ b/arch/x86/kvm/mmu/tdp_mmu.c
@@ -905,7 +905,7 @@  static int tdp_mmu_map_handle_target_level(struct kvm_vcpu *vcpu, int write,
 					  kvm_pfn_t pfn, bool prefault)
 {
 	u64 new_spte;
-	int ret = 0;
+	int ret = RET_PF_FIXED;
 	int make_spte_ret = 0;
 
 	if (unlikely(is_noslot_pfn(pfn)))