| Message ID | 1c510b24fc1d7cbae8aa4b69c0799ebd32e65b82.1628739116.git.houwenlong93@linux.alibaba.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/2] KVM: Refactor kvm_arch_vcpu_fault() to return a struct page pointer | expand |
On 12.08.21 06:02, Hou Wenlong wrote: > From: Sean Christopherson <seanjc@google.com> > > 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 <houwenlong93@linux.alibaba.com> > Signed-off-by: Sean Christopherson <seanjc@google.com> > Signed-off-by: Hou Wenlong <houwenlong93@linux.alibaba.com> > --- > 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 <david@redhat.com> 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.
On 12/08/21 11:04, David Hildenbrand wrote: > > Reviewed-by: David Hildenbrand <david@redhat.com> > > 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. I have no preference either way. It should definitely have selftests, but in x86 land there are some features that are not covered by QEMU and were nevertheless accepted upstream with selftests. Paolo
On 12.08.21 11:04, David Hildenbrand wrote: > On 12.08.21 06:02, Hou Wenlong wrote: >> From: Sean Christopherson <seanjc@google.com> >> >> 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 <houwenlong93@linux.alibaba.com> >> Signed-off-by: Sean Christopherson <seanjc@google.com> >> Signed-off-by: Hou Wenlong <houwenlong93@linux.alibaba.com> >> --- >> 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 <david@redhat.com> > > 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. I recently talked to the ucontrol users and they will look into selftests.
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;