From mboxrd@z Thu Jan 1 00:00:00 1970 From: David Hildenbrand Date: Thu, 12 Aug 2021 09:04:58 +0000 Subject: Re: [PATCH v2 1/2] KVM: Refactor kvm_arch_vcpu_fault() to return a struct page pointer Message-Id: <98adbd3c-ec6f-5689-1686-2a8a7909951a@redhat.com> List-Id: References: <1c510b24fc1d7cbae8aa4b69c0799ebd32e65b82.1628739116.git.houwenlong93@linux.alibaba.com> In-Reply-To: <1c510b24fc1d7cbae8aa4b69c0799ebd32e65b82.1628739116.git.houwenlong93@linux.alibaba.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Hou Wenlong , kvm@vger.kernel.org Cc: Sean Christopherson , Marc Zyngier , James Morse , Alexandru Elisei , Suzuki K Poulose , Catalin Marinas , Will Deacon , Huacai Chen , Aleksandar Markovic , Thomas Bogendoerfer , Paul Mackerras , Michael Ellerman , Benjamin Herrenschmidt , Christian Borntraeger , Janosch Frank , Cornelia Huck , Claudio Imbrenda , Heiko Carstens , Vasily Gorbik , Paolo Bonzini , Vitaly Kuznetsov , Wanpeng Li , Jim Mattson , Joerg Roedel , Thomas Gleixner , Ingo Molnar , Borislav Petkov , x86@kernel.org, "H. Peter Anvin" , linux-arm-kernel@lists.infradead.org, kvmarm@lists.cs.columbia.edu, linux-kernel@vger.kernel.org, linux-mips@vger.kernel.org, kvm-ppc@vger.kernel.org, linuxppc-dev@lists.ozlabs.org, linux-s390@vger.kernel.org On 12.08.21 06:02, Hou Wenlong wrote: > From: Sean Christopherson > > Refactor kvm_arch_vcpu_fault() to return 'struct page *' instead of > 'vm_fault_t' to simplify architecture specific implementations that do > more than return SIGBUS. Currently this only applies to s390, but a > future patch will move x86's pio_data handling into x86 where it belongs. > > No functional changed intended. > > Cc: Hou Wenlong > Signed-off-by: Sean Christopherson > Signed-off-by: Hou Wenlong > --- > arch/arm64/kvm/arm.c | 4 ++-- > arch/mips/kvm/mips.c | 4 ++-- > arch/powerpc/kvm/powerpc.c | 4 ++-- > arch/s390/kvm/kvm-s390.c | 12 ++++-------- > arch/x86/kvm/x86.c | 4 ++-- > include/linux/kvm_host.h | 2 +- > virt/kvm/kvm_main.c | 5 ++++- > 7 files changed, 17 insertions(+), 18 deletions(-) > > diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c > index e9a2b8f27792..83f4ffe3e4f2 100644 > --- a/arch/arm64/kvm/arm.c > +++ b/arch/arm64/kvm/arm.c > @@ -161,9 +161,9 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type) > return ret; > } > > -vm_fault_t kvm_arch_vcpu_fault(struct kvm_vcpu *vcpu, struct vm_fault *vmf) > +struct page *kvm_arch_vcpu_fault(struct kvm_vcpu *vcpu, struct vm_fault *vmf) > { > - return VM_FAULT_SIGBUS; > + return NULL; > } > > > diff --git a/arch/mips/kvm/mips.c b/arch/mips/kvm/mips.c > index af9dd029a4e1..ae79874e6fd2 100644 > --- a/arch/mips/kvm/mips.c > +++ b/arch/mips/kvm/mips.c > @@ -1053,9 +1053,9 @@ int kvm_arch_vcpu_ioctl_set_fpu(struct kvm_vcpu *vcpu, struct kvm_fpu *fpu) > return -ENOIOCTLCMD; > } > > -vm_fault_t kvm_arch_vcpu_fault(struct kvm_vcpu *vcpu, struct vm_fault *vmf) > +struct page *kvm_arch_vcpu_fault(struct kvm_vcpu *vcpu, struct vm_fault *vmf) > { > - return VM_FAULT_SIGBUS; > + return NULL; > } > > int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext) > diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c > index be33b5321a76..b9c21f9ab784 100644 > --- a/arch/powerpc/kvm/powerpc.c > +++ b/arch/powerpc/kvm/powerpc.c > @@ -2090,9 +2090,9 @@ long kvm_arch_vcpu_ioctl(struct file *filp, > return r; > } > > -vm_fault_t kvm_arch_vcpu_fault(struct kvm_vcpu *vcpu, struct vm_fault *vmf) > +struct page *kvm_arch_vcpu_fault(struct kvm_vcpu *vcpu, struct vm_fault *vmf) > { > - return VM_FAULT_SIGBUS; > + return NULL; > } > > static int kvm_vm_ioctl_get_pvinfo(struct kvm_ppc_pvinfo *pvinfo) > diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c > index 02574d7b3612..e1b69833e228 100644 > --- a/arch/s390/kvm/kvm-s390.c > +++ b/arch/s390/kvm/kvm-s390.c > @@ -4979,17 +4979,13 @@ long kvm_arch_vcpu_ioctl(struct file *filp, > return r; > } > > -vm_fault_t kvm_arch_vcpu_fault(struct kvm_vcpu *vcpu, struct vm_fault *vmf) > +struct page *kvm_arch_vcpu_fault(struct kvm_vcpu *vcpu, struct vm_fault *vmf) > { > #ifdef CONFIG_KVM_S390_UCONTROL > - if ((vmf->pgoff = KVM_S390_SIE_PAGE_OFFSET) > - && (kvm_is_ucontrol(vcpu->kvm))) { > - vmf->page = virt_to_page(vcpu->arch.sie_block); > - get_page(vmf->page); > - return 0; > - } > + if (vmf->pgoff = KVM_S390_SIE_PAGE_OFFSET && kvm_is_ucontrol(vcpu->kvm)) > + return virt_to_page(vcpu->arch.sie_block); > #endif > - return VM_FAULT_SIGBUS; > + return NULL; > } > > /* Section: memory related */ > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 3cedc7cc132a..1e3bbe5cd33a 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -5347,9 +5347,9 @@ long kvm_arch_vcpu_ioctl(struct file *filp, > return r; > } > > -vm_fault_t kvm_arch_vcpu_fault(struct kvm_vcpu *vcpu, struct vm_fault *vmf) > +struct page *kvm_arch_vcpu_fault(struct kvm_vcpu *vcpu, struct vm_fault *vmf) > { > - return VM_FAULT_SIGBUS; > + return NULL; > } > > static int kvm_vm_ioctl_set_tss_addr(struct kvm *kvm, unsigned long addr) > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h > index 492d183dd7d0..a949de534722 100644 > --- a/include/linux/kvm_host.h > +++ b/include/linux/kvm_host.h > @@ -995,7 +995,7 @@ long kvm_arch_dev_ioctl(struct file *filp, > unsigned int ioctl, unsigned long arg); > long kvm_arch_vcpu_ioctl(struct file *filp, > unsigned int ioctl, unsigned long arg); > -vm_fault_t kvm_arch_vcpu_fault(struct kvm_vcpu *vcpu, struct vm_fault *vmf); > +struct page *kvm_arch_vcpu_fault(struct kvm_vcpu *vcpu, struct vm_fault *vmf); > > int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext); > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index 30d322519253..f7d21418971b 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -3448,7 +3448,10 @@ static vm_fault_t kvm_vcpu_fault(struct vm_fault *vmf) > &vcpu->dirty_ring, > vmf->pgoff - KVM_DIRTY_LOG_PAGE_OFFSET); > else > - return kvm_arch_vcpu_fault(vcpu, vmf); > + page = kvm_arch_vcpu_fault(vcpu, vmf); > + if (!page) > + return VM_FAULT_SIGBUS; > + > get_page(page); > vmf->page = page; > return 0; > Reviewed-by: David Hildenbrand But at the same time I wonder if we should just get rid of CONFIG_KVM_S390_UCONTROL and consequently kvm_arch_vcpu_fault(). In practice CONFIG_KVM_S390_UCONTROL, is never enabled in any reasonable kernel build and consequently it's never tested; further, exposing the sie_block to user space allows user space to generate random SIE validity intercepts. CONFIG_KVM_S390_UCONTROL feels like something that should just be maintained out of tree by someone who really needs to hack deep into hw virtualization for testing purposes etc. -- Thanks, David / dhildenb