diff mbox series

[WIP,v2,12/14] KVM: arm64: Implement KVM_CAP_MEMORY_FAULT_NOWAIT

Message ID 20230315021738.1151386-13-amoorthy@google.com (mailing list archive)
State New, archived
Headers show
Series Avoiding slow get-user-pages via memory fault exit | expand

Commit Message

Anish Moorthy March 15, 2023, 2:17 a.m. UTC 
When a memslot has the KVM_MEM_MEMORY_FAULT_EXIT flag set, exit to
userspace upon encountering a page fault for which the userspace
page tables do not contain a present mapping.

Signed-off-by: Anish Moorthy <amoorthy@google.com>
Acked-by: James Houghton <jthoughton@google.com>
---
 arch/arm64/kvm/arm.c |  1 +
 arch/arm64/kvm/mmu.c | 14 ++++++++++++--
 2 files changed, 13 insertions(+), 2 deletions(-)

Comments

Oliver Upton March 17, 2023, 6:27 p.m. UTC | #1 
On Wed, Mar 15, 2023 at 02:17:36AM +0000, Anish Moorthy wrote:
> When a memslot has the KVM_MEM_MEMORY_FAULT_EXIT flag set, exit to
> userspace upon encountering a page fault for which the userspace
> page tables do not contain a present mapping.
> 
> Signed-off-by: Anish Moorthy <amoorthy@google.com>
> Acked-by: James Houghton <jthoughton@google.com>
> ---
>  arch/arm64/kvm/arm.c |  1 +
>  arch/arm64/kvm/mmu.c | 14 ++++++++++++--
>  2 files changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index 3bd732eaf0872..f8337e757c777 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -220,6 +220,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
>  	case KVM_CAP_VCPU_ATTRIBUTES:
>  	case KVM_CAP_PTP_KVM:
>  	case KVM_CAP_ARM_SYSTEM_SUSPEND:
> +	case KVM_CAP_MEMORY_FAULT_NOWAIT:
>  		r = 1;
>  		break;
>  	case KVM_CAP_SET_GUEST_DEBUG2:
> diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
> index 735044859eb25..0d04ffc81f783 100644
> --- a/arch/arm64/kvm/mmu.c
> +++ b/arch/arm64/kvm/mmu.c
> @@ -1206,6 +1206,7 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
>  	unsigned long vma_pagesize, fault_granule;
>  	enum kvm_pgtable_prot prot = KVM_PGTABLE_PROT_R;
>  	struct kvm_pgtable *pgt;
> +	bool exit_on_memory_fault = kvm_slot_fault_on_absent_mapping(memslot);
>  
>  	fault_granule = 1UL << ARM64_HW_PGTABLE_LEVEL_SHIFT(fault_level);
>  	write_fault = kvm_is_write_fault(vcpu);
> @@ -1303,8 +1304,17 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
>  	 */
>  	smp_rmb();
>  
> -	pfn = __gfn_to_pfn_memslot(memslot, gfn, false, false, NULL,
> -				   write_fault, &writable, NULL);
> +	pfn = __gfn_to_pfn_memslot(
> +		memslot, gfn, exit_on_memory_fault, false, NULL,
> +		write_fault, &writable, NULL);

As stated before [*], this google3-esque style does not match the kernel style
guide. You may want to check if your work machine is setting up a G3-specific
editor configuration behind your back.

[*] https://lore.kernel.org/kvm/Y+0QRsZ4yWyUdpnc@google.com/

> +	if (exit_on_memory_fault && pfn == KVM_PFN_ERR_FAULT) {

nit: I don't think the local is explicitly necessary. I still find this
readable:

	if (pfn == KVM_PFN_ERR_FAULT && kvm_slot_fault_on_absent_mapping(memslot))

> +		vcpu->run->exit_reason = KVM_EXIT_MEMORY_FAULT;
> +		vcpu->run->memory_fault.flags = 0;
> +		vcpu->run->memory_fault.gpa = gfn << PAGE_SHIFT;
> +		vcpu->run->memory_fault.len = vma_pagesize;
> +		return 0;
> +	}
>  	if (pfn == KVM_PFN_ERR_HWPOISON) {
>  		kvm_send_hwpoison_signal(hva, vma_shift);
>  		return 1;
> -- 
> 2.40.0.rc1.284.g88254d51c5-goog
> 
>
Anish Moorthy March 17, 2023, 7 p.m. UTC | #2 
On Fri, Mar 17, 2023 at 11:27 AM Oliver Upton <oliver.upton@linux.dev> wrote:

> > +     pfn = __gfn_to_pfn_memslot(
> > +             memslot, gfn, exit_on_memory_fault, false, NULL,
> > +             write_fault, &writable, NULL);
>
> As stated before [*], this google3-esque style does not match the kernel style
> guide. You may want to check if your work machine is setting up a G3-specific
> editor configuration behind your back.
>
> [*] https://lore.kernel.org/kvm/Y+0QRsZ4yWyUdpnc@google.com/

If you're referring to the indentation, then that was definitely me.
I'll give the style guide another readthrough before I submit the next
version then, since checkpatch.pl doesn't seem to complain here.

> > +     if (exit_on_memory_fault && pfn == KVM_PFN_ERR_FAULT) {
>
> nit: I don't think the local is explicitly necessary. I still find this
> readable:

The local was for keeping a consistent value between the two blocks of code here

    pfn = __gfn_to_pfn_memslot(
        memslot, gfn, exit_on_memory_fault, false, NULL,
        write_fault, &writable, NULL);

    if (exit_on_memory_fault && pfn == KVM_PFN_ERR_FAULT) {
        // Set up vCPU exit and return 0
    }

I wanted to avoid the possibility of causing an early
__gfn_to_pfn_memslot exit but then not populating the vCPU exit.
Oliver Upton March 17, 2023, 7:03 p.m. UTC | #3 
On Fri, Mar 17, 2023 at 12:00:30PM -0700, Anish Moorthy wrote:
> On Fri, Mar 17, 2023 at 11:27 AM Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> > > +     pfn = __gfn_to_pfn_memslot(
> > > +             memslot, gfn, exit_on_memory_fault, false, NULL,
> > > +             write_fault, &writable, NULL);
> >
> > As stated before [*], this google3-esque style does not match the kernel style
> > guide. You may want to check if your work machine is setting up a G3-specific
> > editor configuration behind your back.
> >
> > [*] https://lore.kernel.org/kvm/Y+0QRsZ4yWyUdpnc@google.com/
> 
> If you're referring to the indentation, then that was definitely me.
> I'll give the style guide another readthrough before I submit the next
> version then, since checkpatch.pl doesn't seem to complain here.
> 
> > > +     if (exit_on_memory_fault && pfn == KVM_PFN_ERR_FAULT) {
> >
> > nit: I don't think the local is explicitly necessary. I still find this
> > readable:
> 
> The local was for keeping a consistent value between the two blocks of code here
> 
>     pfn = __gfn_to_pfn_memslot(
>         memslot, gfn, exit_on_memory_fault, false, NULL,
>         write_fault, &writable, NULL);
> 
>     if (exit_on_memory_fault && pfn == KVM_PFN_ERR_FAULT) {
>         // Set up vCPU exit and return 0
>     }
> 
> I wanted to avoid the possibility of causing an early
> __gfn_to_pfn_memslot exit but then not populating the vCPU exit.

Ignore me, I didn't see the other use of the local.
Sean Christopherson March 17, 2023, 7:24 p.m. UTC | #4 
On Fri, Mar 17, 2023, Anish Moorthy wrote:
> On Fri, Mar 17, 2023 at 11:27 AM Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> > > +     pfn = __gfn_to_pfn_memslot(
> > > +             memslot, gfn, exit_on_memory_fault, false, NULL,
> > > +             write_fault, &writable, NULL);
> >
> > As stated before [*], this google3-esque style does not match the kernel style
> > guide. You may want to check if your work machine is setting up a G3-specific
> > editor configuration behind your back.
> >
> > [*] https://lore.kernel.org/kvm/Y+0QRsZ4yWyUdpnc@google.com/
> 
> If you're referring to the indentation, then that was definitely me.

The two issues are (1) don't put newlines immediately after an opening '(', and
(2) align indentation relative to the direct parent '(' that encapsulates the code.

Concretely, the above should be:

	pfn = __gfn_to_pfn_memslot(memslot, gfn, exit_on_memory_fault, false,
				   NULL, write_fault, &writable, NULL);

> I'll give the style guide another readthrough before I submit the next
> version then, since checkpatch.pl doesn't seem to complain here.

I don't think checkpatch looks for these particular style issues.  FWIW, you
really shouldn't need to read through the formal documentation for these "basic"
rules, just spend time poking around the code base.  If your code looks different
than everything else, then you're likely doing it wrong.
diff mbox series

Patch

diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index 3bd732eaf0872..f8337e757c777 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -220,6 +220,7 @@  int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
 	case KVM_CAP_VCPU_ATTRIBUTES:
 	case KVM_CAP_PTP_KVM:
 	case KVM_CAP_ARM_SYSTEM_SUSPEND:
+	case KVM_CAP_MEMORY_FAULT_NOWAIT:
 		r = 1;
 		break;
 	case KVM_CAP_SET_GUEST_DEBUG2:
diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
index 735044859eb25..0d04ffc81f783 100644
--- a/arch/arm64/kvm/mmu.c
+++ b/arch/arm64/kvm/mmu.c
@@ -1206,6 +1206,7 @@  static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
 	unsigned long vma_pagesize, fault_granule;
 	enum kvm_pgtable_prot prot = KVM_PGTABLE_PROT_R;
 	struct kvm_pgtable *pgt;
+	bool exit_on_memory_fault = kvm_slot_fault_on_absent_mapping(memslot);
 
 	fault_granule = 1UL << ARM64_HW_PGTABLE_LEVEL_SHIFT(fault_level);
 	write_fault = kvm_is_write_fault(vcpu);
@@ -1303,8 +1304,17 @@  static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
 	 */
 	smp_rmb();
 
-	pfn = __gfn_to_pfn_memslot(memslot, gfn, false, false, NULL,
-				   write_fault, &writable, NULL);
+	pfn = __gfn_to_pfn_memslot(
+		memslot, gfn, exit_on_memory_fault, false, NULL,
+		write_fault, &writable, NULL);
+
+	if (exit_on_memory_fault && pfn == KVM_PFN_ERR_FAULT) {
+		vcpu->run->exit_reason = KVM_EXIT_MEMORY_FAULT;
+		vcpu->run->memory_fault.flags = 0;
+		vcpu->run->memory_fault.gpa = gfn << PAGE_SHIFT;
+		vcpu->run->memory_fault.len = vma_pagesize;
+		return 0;
+	}
 	if (pfn == KVM_PFN_ERR_HWPOISON) {
 		kvm_send_hwpoison_signal(hva, vma_shift);
 		return 1;