| Message ID | 20240802224031.154064-3-amoorthy@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Set up KVM_EXIT_MEMORY_FAULTs when arm64/x86 stage-2 fault handlers fail | expand |
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.
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
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.
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.
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.
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:
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(-)