From: Alexander Graf <graf@amazon.com>
To: Christoffer Dall <christoffer.dall@arm.com>
Cc: "Daniel P. Berrangé" <berrange@redhat.com>,
"Marc Zyngier" <maz@kernel.org>,
"lkml - Kernel Mailing List" <linux-kernel@vger.kernel.org>,
"Stefan Hajnoczi" <stefanha@redhat.com>,
"Heinrich Schuchardt" <xypron.glpk@gmx.de>,
kvmarm@lists.cs.columbia.edu,
arm-mail-list <linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH 1/1] KVM: inject data abort if instruction cannot be decoded
Date: Fri, 6 Sep 2019 15:16:07 +0200 [thread overview]
Message-ID: <fbb6f068-a0eb-6ca8-d922-a7627243e230@amazon.com> (raw)
In-Reply-To: <20190906131252.GG4320@e113682-lin.lund.arm.com>
On 06.09.19 15:12, Christoffer Dall wrote:
> On Fri, Sep 06, 2019 at 02:08:15PM +0200, Alexander Graf wrote:
>>
>>
>> On 06.09.19 10:00, Christoffer Dall wrote:
>>> On Thu, Sep 05, 2019 at 02:09:18PM +0100, Marc Zyngier wrote:
>>>> On 05/09/2019 10:22, Christoffer Dall wrote:
>>>>> On Thu, Sep 05, 2019 at 09:56:44AM +0100, Peter Maydell wrote:
>>>>>> On Thu, 5 Sep 2019 at 09:52, Marc Zyngier <maz@kernel.org> wrote:
>>>>>>>
>>>>>>> On Thu, 05 Sep 2019 09:16:54 +0100,
>>>>>>> Peter Maydell <peter.maydell@linaro.org> wrote:
>>>>>>>> This is true, but the problem is that barfing out to userspace
>>>>>>>> makes it harder to debug the guest because it means that
>>>>>>>> the VM is immediately destroyed, whereas AIUI if we
>>>>>>>> inject some kind of exception then (assuming you're set up
>>>>>>>> to do kernel-debug via gdbstub) you can actually examine
>>>>>>>> the offending guest code with a debugger because at least
>>>>>>>> your VM is still around to inspect...
>>>>>>>
>>>>>>> To Christoffer's point, I find the benefit a bit dubious. Yes, you get
>>>>>>> an exception, but the instruction that caused it may be completely
>>>>>>> legal (store with post-increment, for example), leading to an even
>>>>>>> more puzzled developer (that exception should never have been
>>>>>>> delivered the first place).
>>>>>>
>>>>>> Right, but the combination of "host kernel prints a message
>>>>>> about an unsupported load/store insn" and "within-guest debug
>>>>>> dump/stack trace/etc" is much more useful than just having
>>>>>> "host kernel prints message" and "QEMU exits"; and it requires
>>>>>> about 3 lines of code change...
>>>>>>
>>>>>>> I'm far more in favour of dumping the state of the access in the run
>>>>>>> structure (much like we do for a MMIO access) and let userspace do
>>>>>>> something about it (such as dumping information on the console or
>>>>>>> breaking). It could even inject an exception *if* the user has asked
>>>>>>> for it.
>>>>>>
>>>>>> ...whereas this requires agreement on a kernel-userspace API,
>>>>>> larger changes in the kernel, somebody to implement the userspace
>>>>>> side of things, and the user to update both the kernel and QEMU.
>>>>>> It's hard for me to see that the benefit here over the 3-line
>>>>>> approach really outweighs the extra effort needed. In practice
>>>>>> saying "we should do this" is saying "we're going to do nothing",
>>>>>> based on the historical record.
>>>>>>
>>>>>
>>>>> How about something like the following (completely untested, liable for
>>>>> ABI discussions etc. etc., but for illustration purposes).
>>>>>
>>>>> I think it raises the question (and likely many other) of whether we can
>>>>> break the existing 'ABI' and change behavior for missing ISV
>>>>> retrospectively for legacy user space when the issue has occurred?
>>>>> Someone might have written code that reacts to the -ENOSYS, so I've
>>>>> taken the conservative approach for this for the time being.
>>>>>
>>>>>
>>>>> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
>>>>> index 8a37c8e89777..19a92c49039c 100644
>>>>> --- a/arch/arm/include/asm/kvm_host.h
>>>>> +++ b/arch/arm/include/asm/kvm_host.h
>>>>> @@ -76,6 +76,14 @@ struct kvm_arch {
>>>>> /* Mandated version of PSCI */
>>>>> u32 psci_version;
>>>>> +
>>>>> + /*
>>>>> + * If we encounter a data abort without valid instruction syndrome
>>>>> + * information, report this to user space. User space can (and
>>>>> + * should) opt in to this feature if KVM_CAP_ARM_NISV_TO_USER is
>>>>> + * supported.
>>>>> + */
>>>>> + bool return_nisv_io_abort_to_user;
>>>>> };
>>>>> #define KVM_NR_MEM_OBJS 40
>>>>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
>>>>> index f656169db8c3..019bc560edc1 100644
>>>>> --- a/arch/arm64/include/asm/kvm_host.h
>>>>> +++ b/arch/arm64/include/asm/kvm_host.h
>>>>> @@ -83,6 +83,14 @@ struct kvm_arch {
>>>>> /* Mandated version of PSCI */
>>>>> u32 psci_version;
>>>>> +
>>>>> + /*
>>>>> + * If we encounter a data abort without valid instruction syndrome
>>>>> + * information, report this to user space. User space can (and
>>>>> + * should) opt in to this feature if KVM_CAP_ARM_NISV_TO_USER is
>>>>> + * supported.
>>>>> + */
>>>>> + bool return_nisv_io_abort_to_user;
>>>>> };
>>>>> #define KVM_NR_MEM_OBJS 40
>>>>> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
>>>>> index 5e3f12d5359e..a4dd004d0db9 100644
>>>>> --- a/include/uapi/linux/kvm.h
>>>>> +++ b/include/uapi/linux/kvm.h
>>>>> @@ -235,6 +235,7 @@ struct kvm_hyperv_exit {
>>>>> #define KVM_EXIT_S390_STSI 25
>>>>> #define KVM_EXIT_IOAPIC_EOI 26
>>>>> #define KVM_EXIT_HYPERV 27
>>>>> +#define KVM_EXIT_ARM_NISV 28
>>>>> /* For KVM_EXIT_INTERNAL_ERROR */
>>>>> /* Emulate instruction failed. */
>>>>> @@ -996,6 +997,7 @@ struct kvm_ppc_resize_hpt {
>>>>> #define KVM_CAP_ARM_PTRAUTH_ADDRESS 171
>>>>> #define KVM_CAP_ARM_PTRAUTH_GENERIC 172
>>>>> #define KVM_CAP_PMU_EVENT_FILTER 173
>>>>> +#define KVM_CAP_ARM_NISV_TO_USER 174
>>>>> #ifdef KVM_CAP_IRQ_ROUTING
>>>>> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
>>>>> index 35a069815baf..2ce94bd9d4a9 100644
>>>>> --- a/virt/kvm/arm/arm.c
>>>>> +++ b/virt/kvm/arm/arm.c
>>>>> @@ -98,6 +98,26 @@ int kvm_arch_check_processor_compat(void)
>>>>> return 0;
>>>>> }
>>>>> +int kvm_vm_ioctl_enable_cap(struct kvm *kvm,
>>>>> + struct kvm_enable_cap *cap)
>>>>> +{
>>>>> + int r;
>>>>> +
>>>>> + if (cap->flags)
>>>>> + return -EINVAL;
>>>>> +
>>>>> + switch (cap->cap) {
>>>>> + case KVM_CAP_ARM_NISV_TO_USER:
>>>>> + r = 0;
>>>>> + kvm->arch.return_nisv_io_abort_to_user = true;
>>>>> + break;
>>>>> + default:
>>>>> + r = -EINVAL;
>>>>> + break;
>>>>> + }
>>>>> +
>>>>> + return r;
>>>>> +}
>>>>> /**
>>>>> * kvm_arch_init_vm - initializes a VM data structure
>>>>> @@ -196,6 +216,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
>>>>> case KVM_CAP_MP_STATE:
>>>>> case KVM_CAP_IMMEDIATE_EXIT:
>>>>> case KVM_CAP_VCPU_EVENTS:
>>>>> + case KVM_CAP_ARM_NISV_TO_USER:
>>>>> r = 1;
>>>>> break;
>>>>> case KVM_CAP_ARM_SET_DEVICE_ADDR:
>>>>> @@ -673,6 +694,8 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
>>>>> ret = kvm_handle_mmio_return(vcpu, vcpu->run);
>>>>> if (ret)
>>>>> return ret;
>>>>> + } else if (run->exit_reason == KVM_EXIT_ARM_NISV) {
>>>>> + kvm_inject_undefined(vcpu);
>>>>
>>>> Just to make sure I understand: Is the expectation here that userspace
>>>> could clear the exit reason if it managed to handle the exit? And
>>>> otherwise we'd inject an UNDEF on reentry?
>>>>
>>>
>>> Yes, but I think we should change that to an external abort. I'll test
>>> something and send a proper patch with more clear documentation.
>>
>> Why not leave the injection to user space in any case? API wise there is no
>> need to be backwards compatible, as we require the CAP to be enabled, right?
>>
>
> I'd prefer leaving it to userspace to worry about, but I thought Peter
> said that had been problematic historically, which I took at face value,
> but I could have misunderstood.
>
> If QEMU, kvmtool, and whatever the crazy^H cool kids are using in
> userspace these days are happy emulating the exception, then that's a
> viable approach. The main concern I have with that is whether they'll
> all get it right, and since we already have the code in the kernel to do
> this, it might make sense to re-use the kernel logic for it.
You could make the same argument about injecting an #SError on an out of
bounds access to MMIO.
If injecting a fault is too complicated, we should fix that rather than
create an unbalanced user space interface :).
> I'll leave it in for v1 of the patch, and if based on how that code and
> interface looks like, we agree it's better to leave it to userspace, I
> can remove it in v2.
Sure, works for me :). Please CC me on v1 so I can comment on it ;)
Alex
Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
next prev parent reply other threads:[~2019-09-06 13:16 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-09-04 18:07 [PATCH 1/1] KVM: inject data abort if instruction cannot be decoded Heinrich Schuchardt
2019-09-05 1:35 ` Guoheyi
2019-09-05 8:03 ` Marc Zyngier
2019-09-05 8:16 ` Peter Maydell
2019-09-05 8:25 ` Christoffer Dall
2019-09-05 8:32 ` Peter Maydell
2019-09-05 8:48 ` Heinrich Schuchardt
2019-09-05 8:52 ` Marc Zyngier
2019-09-05 8:56 ` Peter Maydell
2019-09-05 9:15 ` Marc Zyngier
2019-09-05 9:22 ` Christoffer Dall
2019-09-05 13:09 ` Marc Zyngier
2019-09-06 8:00 ` Christoffer Dall
2019-09-06 12:08 ` Alexander Graf
2019-09-06 12:34 ` Marc Zyngier
2019-09-06 13:02 ` [UNVERIFIED SENDER] " Alexander Graf
2019-09-06 13:12 ` Christoffer Dall
2019-09-06 13:16 ` Alexander Graf [this message]
2019-09-06 13:31 ` Peter Maydell
2019-09-06 13:41 ` Alexander Graf
2019-09-06 13:50 ` Peter Maydell
2019-09-06 14:12 ` Alexander Graf
2019-09-06 13:44 ` Christoffer Dall
2019-09-05 13:25 ` Heinrich Schuchardt
2019-09-06 7:58 ` Christoffer Dall
2019-09-05 8:28 ` Heinrich Schuchardt
2019-09-05 9:11 ` Marc Zyngier
2019-09-05 9:20 ` Stefan Hajnoczi
2019-09-05 9:23 ` Daniel P. Berrangé
2019-09-05 12:01 ` Heinrich Schuchardt
2019-09-05 12:16 ` Christoffer Dall
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=fbb6f068-a0eb-6ca8-d922-a7627243e230@amazon.com \
--to=graf@amazon.com \
--cc=berrange@redhat.com \
--cc=christoffer.dall@arm.com \
--cc=kvmarm@lists.cs.columbia.edu \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=maz@kernel.org \
--cc=stefanha@redhat.com \
--cc=xypron.glpk@gmx.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).