Received-SPF: pass (google.com: domain of linux-kernel+bounces-85077-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Precedence: bulk
MIME-Version: 1.0
References: <20240227232100.478238-1-pbonzini@redhat.com> <20240227232100.478238-15-pbonzini@redhat.com>
 <Zd6UVfhzdMp8z2O2@google.com>
In-Reply-To: <Zd6UVfhzdMp8z2O2@google.com>
From: Paolo Bonzini <pbonzini@redhat.com>
Date: Wed, 28 Feb 2024 14:13:25 +0100
Message-ID: <CABgObfb=5L7p7RDJMtxaGpCVgNfogWuOFCN7W5DBFdP+LAUhFA@mail.gmail.com>
Subject: Re: [PATCH 14/21] KVM: x86/mmu: pass error code back to MMU when
 async pf is ready
To: Sean Christopherson <seanjc@google.com>
Cc: linux-kernel@vger.kernel.org, kvm@vger.kernel.org, michael.roth@amd.com, 
	isaku.yamahata@intel.com, thomas.lendacky@amd.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Wed, Feb 28, 2024 at 3:03=E2=80=AFAM Sean Christopherson <seanjc@google.=
com> wrote:
>
> On Tue, Feb 27, 2024, Paolo Bonzini wrote:
> > Right now the error code is not used when an async page fault is comple=
ted.
> > This is not a problem in the current code, but it is untidy.  For prote=
cted
> > VMs we need to check that the page attributes match the current state o=
f the
> > page.  Async page faults can only occur on shared pages (because
> > private pages go through kvm_faultin_pfn_private() instead of
> > __gfn_to_pfn_memslot()), but it is risky to rely on the polarity of
> > PFERR_GUEST_ENC_MASK and the high 32 bits of the error code being zero.
> > So, for clarity and future-proofing of the code, pipe the error code
> > from kvm_arch_setup_async_pf() to kvm_arch_async_page_ready() via the
> > architecture-specific async page fault data.
> >
> > Extracted from a patch by Isaku Yamahata.
> >
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > ---
> >  arch/x86/include/asm/kvm_host.h |  1 +
> >  arch/x86/kvm/mmu/mmu.c          | 14 +++++++-------
> >  2 files changed, 8 insertions(+), 7 deletions(-)
> >
> > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm=
_host.h
> > index a4514c2ef0ec..24e30ca2ca8f 100644
> > --- a/arch/x86/include/asm/kvm_host.h
> > +++ b/arch/x86/include/asm/kvm_host.h
> > @@ -1839,6 +1839,7 @@ struct kvm_arch_async_pf {
> >       gfn_t gfn;
> >       unsigned long cr3;
> >       bool direct_map;
> > +     u64 error_code;
> >  };
> >
> >  extern u32 __read_mostly kvm_nr_uret_msrs;
> > diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> > index f58ca6cb789a..c9890e5b6e4c 100644
> > --- a/arch/x86/kvm/mmu/mmu.c
> > +++ b/arch/x86/kvm/mmu/mmu.c
> > @@ -4260,18 +4260,18 @@ static u32 alloc_apf_token(struct kvm_vcpu *vcp=
u)
> >       return (vcpu->arch.apf.id++ << 12) | vcpu->vcpu_id;
> >  }
> >
> > -static bool kvm_arch_setup_async_pf(struct kvm_vcpu *vcpu, gpa_t cr2_o=
r_gpa,
> > -                                 gfn_t gfn)
> > +static bool kvm_arch_setup_async_pf(struct kvm_vcpu *vcpu,
> > +                                 struct kvm_page_fault *fault)
> >  {
> >       struct kvm_arch_async_pf arch;
> >
> >       arch.token =3D alloc_apf_token(vcpu);
> > -     arch.gfn =3D gfn;
> > +     arch.gfn =3D fault->gfn;
> >       arch.direct_map =3D vcpu->arch.mmu->root_role.direct;
> >       arch.cr3 =3D kvm_mmu_get_guest_pgd(vcpu, vcpu->arch.mmu);
> >
> > -     return kvm_setup_async_pf(vcpu, cr2_or_gpa,
> > -                               kvm_vcpu_gfn_to_hva(vcpu, gfn), &arch);
> > +     return kvm_setup_async_pf(vcpu, fault->addr,
> > +                               kvm_vcpu_gfn_to_hva(vcpu, fault->gfn), =
&arch);
> >  }
> >
> >  void kvm_arch_async_page_ready(struct kvm_vcpu *vcpu, struct kvm_async=
_pf *work)
> > @@ -4290,7 +4290,7 @@ void kvm_arch_async_page_ready(struct kvm_vcpu *v=
cpu, struct kvm_async_pf *work)
> >             work->arch.cr3 !=3D kvm_mmu_get_guest_pgd(vcpu, vcpu->arch.=
mmu))
> >               return;
> >
> > -     kvm_mmu_do_page_fault(vcpu, work->cr2_or_gpa, 0, true, NULL);
> > +     kvm_mmu_do_page_fault(vcpu, work->cr2_or_gpa, work->arch.error_co=
de, true, NULL);
>
> This is silly.  If we're going to bother plumbing in the error code, then=
 we
> should use it to do sanity checks.  Things have gone off the rails if end=
 up with
> an async #PF on private memory.

Sure, I split this part out not just because it makes sense to do so,
but also because it's not strictly necessary. I'll add the check and
tweak the changelog.

Paolo

>
> >  }
> >
> >  static inline u8 kvm_max_level_for_order(int order)
> > @@ -4395,7 +4395,7 @@ static int __kvm_faultin_pfn(struct kvm_vcpu *vcp=
u, struct kvm_page_fault *fault
> >                       trace_kvm_async_pf_repeated_fault(fault->addr, fa=
ult->gfn);
> >                       kvm_make_request(KVM_REQ_APF_HALT, vcpu);
> >                       return RET_PF_RETRY;
> > -             } else if (kvm_arch_setup_async_pf(vcpu, fault->addr, fau=
lt->gfn)) {
> > +             } else if (kvm_arch_setup_async_pf(vcpu, fault)) {
> >                       return RET_PF_RETRY;
> >               }
> >       }
> > --
> > 2.39.0
> >
> >
>