[PATCH 0/3] Set up KVM_EXIT_MEMORY_FAULTs when arm64/x86 stage-2 fault handlers fail

[PATCH 0/3] Set up KVM_EXIT_MEMORY_FAULTs when arm64/x86 stage-2 fault handlers fail

[Anish Moorthy]

Memory fault exits were originally conceived for the stage-2 fault
handlers in the first place: it's probably time they were actually added
there :)

Sean and Oliver: you guys were having a discussion on the arm64 patch
the last time I posted it: here's the link in case you need it.
https://lore.kernel.org/kvm/20240215235405.368539-9-amoorthy@google.com/

Anish Moorthy (3):
  KVM: x86: Do a KVM_MEMORY_FAULT EXIT when stage-2 fault handler
    EFAULTs
  KVM: arm64: Declare support for KVM_CAP_MEMORY_FAULT_INFO
  KVM: arm64: Do a KVM_EXIT_MEMORY_FAULT when stage-2 fault handler
    EFAULTs

 Documentation/virt/kvm/api.rst | 2 +-
 arch/arm64/kvm/arm.c           | 1 +
 arch/arm64/kvm/mmu.c           | 5 ++++-
 arch/x86/kvm/mmu/mmu.c         | 1 +
 4 files changed, 7 insertions(+), 2 deletions(-)


base-commit: 332d2c1d713e232e163386c35a3ba0c1b90df83f
-- 
2.46.0.rc2.264.g509ed76dc8-goog

[PATCH 1/3] KVM: x86: Do a KVM_MEMORY_FAULT EXIT when stage-2 fault handler EFAULTs

[Anish Moorthy]

Right now userspace just gets a bare EFAULT when the stage-2 fault
handler fails to fault in the relevant page. Set up a memory fault exit
when this happens, which at the very least eases debugging and might
also let userspace decide on/take some specific action other than
crashing the VM.

Signed-off-by: Anish Moorthy <amoorthy@google.com>
---
 arch/x86/kvm/mmu/mmu.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
index 901be9e420a4..c22c807696ae 100644
--- a/arch/x86/kvm/mmu/mmu.c
+++ b/arch/x86/kvm/mmu/mmu.c
@@ -3264,6 +3264,7 @@ static int kvm_handle_error_pfn(struct kvm_vcpu *vcpu, struct kvm_page_fault *fa
 		return RET_PF_RETRY;
 	}
 
+	kvm_mmu_prepare_memory_fault_exit(vcpu, fault);
 	return -EFAULT;
 }
 
-- 
2.46.0.rc2.264.g509ed76dc8-goog

[PATCH 2/3] KVM: arm64: Declare support for KVM_CAP_MEMORY_FAULT_INFO

[Anish Moorthy]

Although arm64 doesn't currently use memory fault exits anywhere,
it's still valid to advertise the capability: and a subsequent commit
will add KVM_EXIT_MEMORY_FAULTs to the stage-2 fault handler

Signed-off-by: Anish Moorthy <amoorthy@google.com>
---
 Documentation/virt/kvm/api.rst | 2 +-
 arch/arm64/kvm/arm.c           | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
index 8e5dad80b337..49c504b12688 100644
--- a/Documentation/virt/kvm/api.rst
+++ b/Documentation/virt/kvm/api.rst
@@ -8128,7 +8128,7 @@ unavailable to host or other VMs.
 7.34 KVM_CAP_MEMORY_FAULT_INFO
 ------------------------------
 
-:Architectures: x86
+:Architectures: x86, arm64
 :Returns: Informational only, -EINVAL on direct KVM_ENABLE_CAP.
 
 The presence of this capability indicates that KVM_RUN will fill
diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index a7ca776b51ec..4121b5a43b9c 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -335,6 +335,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
 	case KVM_CAP_ARM_SYSTEM_SUSPEND:
 	case KVM_CAP_IRQFD_RESAMPLE:
 	case KVM_CAP_COUNTER_OFFSET:
+	case KVM_CAP_MEMORY_FAULT_INFO:
 		r = 1;
 		break;
 	case KVM_CAP_SET_GUEST_DEBUG2:
-- 
2.46.0.rc2.264.g509ed76dc8-goog

Re: [PATCH 2/3] KVM: arm64: Declare support for KVM_CAP_MEMORY_FAULT_INFO

[Oliver Upton]

On Fri, Aug 02, 2024 at 10:40:30PM +0000, Anish Moorthy wrote:
> Although arm64 doesn't currently use memory fault exits anywhere,
> it's still valid to advertise the capability: and a subsequent commit
> will add KVM_EXIT_MEMORY_FAULTs to the stage-2 fault handler
> 
> Signed-off-by: Anish Moorthy <amoorthy@google.com>
> ---
>  Documentation/virt/kvm/api.rst | 2 +-
>  arch/arm64/kvm/arm.c           | 1 +
>  2 files changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
> index 8e5dad80b337..49c504b12688 100644
> --- a/Documentation/virt/kvm/api.rst
> +++ b/Documentation/virt/kvm/api.rst
> @@ -8128,7 +8128,7 @@ unavailable to host or other VMs.
>  7.34 KVM_CAP_MEMORY_FAULT_INFO
>  ------------------------------

<nitpick>

The wording of the cap documentation isn't as relaxed as I'd
anticipated. Perhaps:

  The presence of this capability indicates that KVM_RUN *may* fill
  kvm_run.memory_fault if ...

IOW, userspace is not guaranteed that the structure is filled for every
'memory fault'.

> -:Architectures: x86
> +:Architectures: x86, arm64

nitpick: alphabetize

>  :Returns: Informational only, -EINVAL on direct KVM_ENABLE_CAP.
>  
>  The presence of this capability indicates that KVM_RUN will fill
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index a7ca776b51ec..4121b5a43b9c 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -335,6 +335,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
>  	case KVM_CAP_ARM_SYSTEM_SUSPEND:
>  	case KVM_CAP_IRQFD_RESAMPLE:
>  	case KVM_CAP_COUNTER_OFFSET:
> +	case KVM_CAP_MEMORY_FAULT_INFO:
>  		r = 1;
>  		break;

Please just squash this into the following patch. Introducing the
capability without the implied functionality doesn't make a lot of
sense.

-- 
Thanks,
Oliver

Re: [PATCH 2/3] KVM: arm64: Declare support for KVM_CAP_MEMORY_FAULT_INFO

[Anish Moorthy]

On Mon, Aug 5, 2024 at 3:51 PM Oliver Upton <oliver.upton@linux.dev> wrote:
>
> The wording of the cap documentation isn't as relaxed as I'd
> anticipated. Perhaps:
>
>   The presence of this capability indicates that KVM_RUN *may* fill
>   kvm_run.memory_fault if ...
>
> IOW, userspace is not guaranteed that the structure is filled for every
> 'memory fault'.

Agreed, I can add a patch to update the docs

While we're at it, what do we think of removing this disclaimer?

>Note: Userspaces which attempt to resolve memory faults so that they can retry
> KVM_RUN are encouraged to guard against repeatedly receiving the same
> error/annotated fault.

I originally added this bit due to my concerns with the idea of
filling kvm_run.memory_fault even for EFAULTs that weren't guaranteed
to be returned by KVM_RUN [1]. However if I'm interpreting Sean's
response to [2] correctly, I think we're now committed to only
KVM_EXIT_MEMORY_FAULTing for EFAULTs/EHWPOISONs which return from
KVM_RUN. At the very least, that seems to be true of current usages.

[1] https://lore.kernel.org/kvm/CAF7b7mrDt6sPQiTenSiqTOHORo1TSPhjSC-tt8fJtuq55B86kg@mail.gmail.com/
[2] https://lore.kernel.org/kvm/CAF7b7mqYr0J-J2oaU=c-dzLys-m6Ttp7ZOb3Em7n1wUj3rhh+A@mail.gmail.com/#t

> > -:Architectures: x86
> > +:Architectures: x86, arm64
>
> nitpick: alphabetize
>
> >  :Returns: Informational only, -EINVAL on direct KVM_ENABLE_CAP.
> >
> >  The presence of this capability indicates that KVM_RUN will fill
> > diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> > index a7ca776b51ec..4121b5a43b9c 100644
> > --- a/arch/arm64/kvm/arm.c
> > +++ b/arch/arm64/kvm/arm.c
> > @@ -335,6 +335,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
> >       case KVM_CAP_ARM_SYSTEM_SUSPEND:
> >       case KVM_CAP_IRQFD_RESAMPLE:
> >       case KVM_CAP_COUNTER_OFFSET:
> > +     case KVM_CAP_MEMORY_FAULT_INFO:
> >               r = 1;
> >               break;
>
> Please just squash this into the following patch. Introducing the
> capability without the implied functionality doesn't make a lot of
> sense.
>
> --
> Thanks,
> Oliver

Re: [PATCH 2/3] KVM: arm64: Declare support for KVM_CAP_MEMORY_FAULT_INFO

[Oliver Upton]

On Tue, Aug 06, 2024 at 11:14:15AM -0700, Anish Moorthy wrote:
> On Mon, Aug 5, 2024 at 3:51 PM Oliver Upton <oliver.upton@linux.dev> wrote:
> >
> > The wording of the cap documentation isn't as relaxed as I'd
> > anticipated. Perhaps:
> >
> >   The presence of this capability indicates that KVM_RUN *may* fill
> >   kvm_run.memory_fault if ...
> >
> > IOW, userspace is not guaranteed that the structure is filled for every
> > 'memory fault'.
> 
> Agreed, I can add a patch to update the docs
> 
> While we're at it, what do we think of removing this disclaimer?
> 
> >Note: Userspaces which attempt to resolve memory faults so that they can retry
> > KVM_RUN are encouraged to guard against repeatedly receiving the same
> > error/annotated fault.
> 
> I originally added this bit due to my concerns with the idea of
> filling kvm_run.memory_fault even for EFAULTs that weren't guaranteed
> to be returned by KVM_RUN [1].

This sort of language generally isn't necessary in UAPI descriptions. We
cannot exhaustively describe the ways userspace might misuse an
interface.

> However if I'm interpreting Sean's
> response to [2] correctly, I think we're now committed to only
> KVM_EXIT_MEMORY_FAULTing for EFAULTs/EHWPOISONs which return from
> KVM_RUN. At the very least, that seems to be true of current usages.

Yeah, I'd have a similar expectation. No point in a half-attempt at
getting out to userspace and subsequently stomping the context.

-- 
Thanks,
Oliver

Re: [PATCH 2/3] KVM: arm64: Declare support for KVM_CAP_MEMORY_FAULT_INFO

[Sean Christopherson]

On Tue, Aug 06, 2024, Oliver Upton wrote:
> On Tue, Aug 06, 2024 at 11:14:15AM -0700, Anish Moorthy wrote:
> > On Mon, Aug 5, 2024 at 3:51 PM Oliver Upton <oliver.upton@linux.dev> wrote:
> > >
> > > The wording of the cap documentation isn't as relaxed as I'd
> > > anticipated. Perhaps:
> > >
> > >   The presence of this capability indicates that KVM_RUN *may* fill
> > >   kvm_run.memory_fault if ...
> > >
> > > IOW, userspace is not guaranteed that the structure is filled for every
> > > 'memory fault'.
> > 
> > Agreed, I can add a patch to update the docs
> > 
> > While we're at it, what do we think of removing this disclaimer?
> > 
> > >Note: Userspaces which attempt to resolve memory faults so that they can retry
> > > KVM_RUN are encouraged to guard against repeatedly receiving the same
> > > error/annotated fault.
> > 
> > I originally added this bit due to my concerns with the idea of
> > filling kvm_run.memory_fault even for EFAULTs that weren't guaranteed
> > to be returned by KVM_RUN [1].
> 
> This sort of language generally isn't necessary in UAPI descriptions. We
> cannot exhaustively describe the ways userspace might misuse an
> interface.

I don't disagree in general, but I think this one is worth calling out because
it's easy to screw up and arguably the most likely "failure" scenario.  E.g. KVM
has had multiple bugs (I can think of four off the top of my head) where a vCPU
gets stuck because KVM doesn't resolve a fault.  It's not hard to imagine userspace
doing the same.

Re: [PATCH 2/3] KVM: arm64: Declare support for KVM_CAP_MEMORY_FAULT_INFO

[Oliver Upton]

On Wed, Aug 07, 2024 at 07:15:16AM -0700, Sean Christopherson wrote:
> On Tue, Aug 06, 2024, Oliver Upton wrote:
> > This sort of language generally isn't necessary in UAPI descriptions. We
> > cannot exhaustively describe the ways userspace might misuse an
> > interface.
> 
> I don't disagree in general, but I think this one is worth calling out because
> it's easy to screw up and arguably the most likely "failure" scenario.  E.g. KVM
> has had multiple bugs (I can think of four off the top of my head) where a vCPU
> gets stuck because KVM doesn't resolve a fault.  It's not hard to imagine userspace
> doing the same.

Yeah, I don't see any reason to go and rip it out, just a suggestion for
the next time around.

-- 
Thanks,
Oliver

[PATCH 3/3] KVM: arm64: Do a KVM_EXIT_MEMORY_FAULT when stage-2 fault handler EFAULTs

[Anish Moorthy]

Right now userspace just gets a bare EFAULT when the stage-2 fault
handler fails to fault in the relevant page. Set up a memory fault exit
when this happens, which at the very least eases debugging and might
also let userspace decide on/take some specific action other than
crashing the VM.

Signed-off-by: Anish Moorthy <amoorthy@google.com>
---
 arch/arm64/kvm/mmu.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
index 6981b1bc0946..52b4f8e648fb 100644
--- a/arch/arm64/kvm/mmu.c
+++ b/arch/arm64/kvm/mmu.c
@@ -1568,8 +1568,11 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
 		kvm_send_hwpoison_signal(hva, vma_shift);
 		return 0;
 	}
-	if (is_error_noslot_pfn(pfn))
+	if (is_error_noslot_pfn(pfn)) {
+		kvm_prepare_memory_fault_exit(vcpu, fault_ipa, vma_pagesize,
+					      write_fault, exec_fault, false);
 		return -EFAULT;
+	}
 
 	if (kvm_is_device_pfn(pfn)) {
 		/*
-- 
2.46.0.rc2.264.g509ed76dc8-goog

Re: [PATCH 3/3] KVM: arm64: Do a KVM_EXIT_MEMORY_FAULT when stage-2 fault handler EFAULTs

[Oliver Upton]

On Fri, Aug 02, 2024 at 10:40:31PM +0000, Anish Moorthy wrote:
> Right now userspace just gets a bare EFAULT when the stage-2 fault
> handler fails to fault in the relevant page. Set up a memory fault exit
> when this happens, which at the very least eases debugging and might
> also let userspace decide on/take some specific action other than
> crashing the VM.

There are several other 'bare' EFAULTs remaining (unexpected fault
context, failed vma_lookup(), nested PTW), so the patch doesn't exactly
match the shortlog.

Is there a reason why those are unaddressed? In any case, it doesn't
hurt to be unambiguous in the shortlog if we're only focused on this single
error condition, e.g.

  KVM: arm64: Do a memory fault exit if __gfn_to_pfn_memslot() fails

> Signed-off-by: Anish Moorthy <amoorthy@google.com>
> ---
>  arch/arm64/kvm/mmu.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
> index 6981b1bc0946..52b4f8e648fb 100644
> --- a/arch/arm64/kvm/mmu.c
> +++ b/arch/arm64/kvm/mmu.c
> @@ -1568,8 +1568,11 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
>  		kvm_send_hwpoison_signal(hva, vma_shift);
>  		return 0;
>  	}
> -	if (is_error_noslot_pfn(pfn))
> +	if (is_error_noslot_pfn(pfn)) {
> +		kvm_prepare_memory_fault_exit(vcpu, fault_ipa, vma_pagesize,
> +					      write_fault, exec_fault, false);
>  		return -EFAULT;
> +	}
>  
>  	if (kvm_is_device_pfn(pfn)) {
>  		/*
> -- 
> 2.46.0.rc2.264.g509ed76dc8-goog
> 

-- 
Thanks,
Oliver

Re: [PATCH 3/3] KVM: arm64: Do a KVM_EXIT_MEMORY_FAULT when stage-2 fault handler EFAULTs

[Anish Moorthy]

On Mon, Aug 5, 2024 at 4:02 PM Oliver Upton <oliver.upton@linux.dev> wrote:
>
> On Fri, Aug 02, 2024 at 10:40:31PM +0000, Anish Moorthy wrote:
> > Right now userspace just gets a bare EFAULT when the stage-2 fault
> > handler fails to fault in the relevant page. Set up a memory fault exit
> > when this happens, which at the very least eases debugging and might
> > also let userspace decide on/take some specific action other than
> > crashing the VM.
>
> There are several other 'bare' EFAULTs remaining (unexpected fault
> context, failed vma_lookup(), nested PTW), so the patch doesn't exactly
> match the shortlog.
>
> Is there a reason why those are unaddressed? In any case, it doesn't
> hurt to be unambiguous in the shortlog if we're only focused on this single
> error condition, e.g.
>
>   KVM: arm64: Do a memory fault exit if __gfn_to_pfn_memslot() fails

Ah, right- forgot to address this before I sent it out.

Basically: those cases you mention (besides MTE, where it seems simple
enough to add an annotation) happen before vma_pagesize is calculated,
and it doesn't look trivial to just move logic around to do that
calculation up at the top. Lmk if there's a good solution here, but in
the meantime I'll just take your suggested shortlog

[1] https://github.com/torvalds/linux/blob/master/arch/arm64/kvm/mmu.c#L1479-L1514

Re: [PATCH 3/3] KVM: arm64: Do a KVM_EXIT_MEMORY_FAULT when stage-2 fault handler EFAULTs

[Oliver Upton]

On Tue, Aug 06, 2024 at 11:44:39AM -0700, Anish Moorthy wrote:
> On Mon, Aug 5, 2024 at 4:02 PM Oliver Upton <oliver.upton@linux.dev> wrote:
> >
> > On Fri, Aug 02, 2024 at 10:40:31PM +0000, Anish Moorthy wrote:
> > > Right now userspace just gets a bare EFAULT when the stage-2 fault
> > > handler fails to fault in the relevant page. Set up a memory fault exit
> > > when this happens, which at the very least eases debugging and might
> > > also let userspace decide on/take some specific action other than
> > > crashing the VM.
> >
> > There are several other 'bare' EFAULTs remaining (unexpected fault
> > context, failed vma_lookup(), nested PTW), so the patch doesn't exactly
> > match the shortlog.
> >
> > Is there a reason why those are unaddressed? In any case, it doesn't
> > hurt to be unambiguous in the shortlog if we're only focused on this single
> > error condition, e.g.
> >
> >   KVM: arm64: Do a memory fault exit if __gfn_to_pfn_memslot() fails
> 
> Ah, right- forgot to address this before I sent it out.
> 
> Basically: those cases you mention (besides MTE, where it seems simple
> enough to add an annotation) happen before vma_pagesize is calculated,

If the motivation is to add additional information for debugging
unexpected KVM/VMM behavior then this really ought to be addressed. You
could fall back to PAGE_SIZE, or better yet just don't report a size
whatsoever (size = 0) if it cannot be reliably determined.

Userspace probably only cares about logging @flags and @gpa before
killing the VM.

-- 
Thanks,
Oliver