Re: [PATCH kvm-unit-tests] arm: gic: enable GIC MMIO tests for GICv3 as well
On Fri, 6 Sep 2019 08:48:37 +0200
Andrew Jones wrote:
Hi,
> On Thu, Sep 05, 2019 at 06:21:14PM +0100, Andre Przywara wrote:
> > So far the GIC MMIO tests were only enabled for a GICv2 guest. Modern
> > machines tend to have a GICv3-only GIC, so can't run those guests.
> > It turns out that most GIC distributor registers we test in the unit
> > tests are actually the same in GICv3, so we can just enable those tests
> > for GICv3 guests as well.
> > The only exception is the CPU number in the TYPER register, which we
> > just protect against running on a GICv3 guest.
> >
> > Signed-off-by: Andre Przywara
> > ---
> > arm/gic.c | 13 +++--
> > arm/unittests.cfg | 16 +++-
> > lib/arm/asm/gic.h | 2 ++
> > 3 files changed, 24 insertions(+), 7 deletions(-)
> >
> > diff --git a/arm/gic.c b/arm/gic.c
> > index ed5642e..bd3c027 100644
> > --- a/arm/gic.c
> > +++ b/arm/gic.c
> > @@ -6,6 +6,7 @@
> > * + MMIO access tests
> > * GICv3
> > * + test sending/receiving IPIs
> > + * + MMIO access tests
> > *
> > * Copyright (C) 2016, Red Hat Inc, Andrew Jones
> > *
> > @@ -483,7 +484,14 @@ static void gic_test_mmio(void)
> > idreg = gic_dist_base + GICD_ICPIDR2;
> > break;
> > case 0x3:
> > - report_abort("GICv3 MMIO tests NYI");
> > + /*
> > +* We only test generic registers or those affecting
> > +* SPIs, so don't need to consider the SGI base in
> > +* the redistributor here.
> > +*/
> > + gic_dist_base = gicv3_dist_base();
> > + idreg = gic_dist_base + GICD_PIDR2;
> > + break;
> > default:
> > report_abort("GIC version %d not supported", gic_version());
> > }
> > @@ -492,7 +500,8 @@ static void gic_test_mmio(void)
> > nr_irqs = GICD_TYPER_IRQS(reg);
> > report_info("number of implemented SPIs: %d", nr_irqs - GIC_FIRST_SPI);
> >
> > - test_typer_v2(reg);
> > + if (gic_version() == 0x2)
> > + test_typer_v2(reg);
> >
> > report_info("IIDR: 0x%08x", readl(gic_dist_base + GICD_IIDR));
> >
> > diff --git a/arm/unittests.cfg b/arm/unittests.cfg
> > index 6d3df92..3fd5b04 100644
> > --- a/arm/unittests.cfg
> > +++ b/arm/unittests.cfg
> > @@ -86,22 +86,28 @@ smp = $((($MAX_SMP < 8)?$MAX_SMP:8))
> > extra_params = -machine gic-version=2 -append 'ipi'
> > groups = gic
> >
> > -[gicv2-mmio]
> > +[gicv2-max-mmio]
> > file = gic.flat
> > smp = $((($MAX_SMP < 8)?$MAX_SMP:8))
> > extra_params = -machine gic-version=2 -append 'mmio'
> > groups = gic
> >
> > -[gicv2-mmio-up]
> > +[gicv3-max-mmio]
> > +file = gic.flat
> > +smp = $MAX_SMP
> > +extra_params = -machine gic-version=3 -append 'mmio'
> > +groups = gic
> > +
> > +[gic-mmio-up]
> > file = gic.flat
> > smp = 1
> > -extra_params = -machine gic-version=2 -append 'mmio'
> > +extra_params = -append 'mmio'
> > groups = gic
> >
> > -[gicv2-mmio-3p]
> > +[gic-mmio-3p]
> > file = gic.flat
> > smp = $((($MAX_SMP < 3)?$MAX_SMP:3))
> > -extra_params = -machine gic-version=2 -append 'mmio'
> > +extra_params = -append 'mmio'
> > groups = gic
> >
> > [gicv3-ipi]
> > diff --git a/lib/arm/asm/gic.h b/lib/arm/asm/gic.h
> > index f6dfb90..ffed025 100644
> > --- a/lib/arm/asm/gic.h
> > +++ b/lib/arm/asm/gic.h
> > @@ -23,6 +23,8 @@
> > #define GICD_ITARGETSR 0x0800
> > #define GICD_SGIR 0x0f00
> > #define GICD_ICPIDR2 0x0fe8
> > +/* only in GICv3 */
> > +#define GICD_PIDR2 0xffe8
>
> If this is gicv3-only, then shouldn't it go to lib/arm/asm/gic-v3.h ?
Ah, true, I missed that file.
Added it there, I will send a reworked version as part of now some bigger
series to test GICD_IROUTER and SPIs as well.
>
> >
> > #define GICD_TYPER_IRQS(typer) typer) & 0x1f) + 1) * 32)
> > #define GICD_INT_EN_SET_SGI0x
> > --
> > 2.17.1
> >
>
> Otherwise
>
> Reviewed-by: Andrew Jones
Thanks!
Cheers,
Andre.
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH 1/1] KVM: inject data abort if instruction cannot be decoded
On 06.09.19 15:50, Peter Maydell wrote: On Fri, 6 Sep 2019 at 14:41, Alexander Graf wrote: On 06.09.19 15:31, Peter Maydell wrote: (b) we try to reuse the code we already have that does TCG exception injection, which might or might not be a design mistake, and That's probably a design mistake, correct :) Well, conceptually it's not necessarily a bad idea, because in both cases what we're doing is "change the system register state (PC, ESR_EL1, ELR_EL1 etc) so that the CPU looks like it's just taken an exception"; but some of what the TCG code needs to do isn't necessary for KVM and all of it was not written with the idea of KVM in mind at all... Yes, and it probably makes sense to model the QEMU internal API similarly, so that exception generating code does not have to distinguish. However, it's much easier for KVM to ensure exception prioritization as well as internal state synchronization. Conceptually, as you've seen, it really doesn't make a lot of sense to pull register state from KVM, wiggle it and then push it down when KVM has awareness of the same transformation anyway. So I guess the path is clear: Create a generic interface for exception injection and a separate patch similar to what Christoffer already posted with the new CAP to route illegal MMIO traps into user space. Connecting the two dots in user space really should be trivial then. (famous last words) 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
Re: [PATCH 1/1] KVM: inject data abort if instruction cannot be decoded
On Fri, 6 Sep 2019 at 14:41, Alexander Graf wrote: > On 06.09.19 15:31, Peter Maydell wrote: > > (b) we try to reuse the code we already have that does TCG exception > > injection, which might or might not be a design mistake, and > > That's probably a design mistake, correct :) Well, conceptually it's not necessarily a bad idea, because in both cases what we're doing is "change the system register state (PC, ESR_EL1, ELR_EL1 etc) so that the CPU looks like it's just taken an exception"; but some of what the TCG code needs to do isn't necessary for KVM and all of it was not written with the idea of KVM in mind at all... thanks -- PMM ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH 1/1] KVM: inject data abort if instruction cannot be decoded
On Fri, Sep 06, 2019 at 02:31:42PM +0100, Peter Maydell wrote: > On Fri, 6 Sep 2019 at 14:13, Christoffer Dall > wrote: > > 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. > > Well, for QEMU we've had code that in theory might do this but > in practice it's never been tested. Essentially the problem is > that nobody ever wants to inject an exception from userspace > except in incredibly rare cases like "trying to use h/w breakpoints > simultaneously inside the VM and also to debug the VM from outside" > or "we're running on RAS hardware that just told us that the VM's > RAM was faulty". There's no even vaguely commonly-used usecase > for it today; and this ABI suggestion adds another "this is in > practice almost never going to happen" case to the pile. The > codepath is unreliable in QEMU because (a) it requires getting > syncing of sysreg values to and from the kernel right -- this is > about the only situation where userspace wants to modify sysregs > during execution of the VM, as opposed to just reading them -- and > (b) we try to reuse the code we already have that does TCG exception > injection, which might or might not be a design mistake, and > (c) as noted above it's a never-actually-used untested codepath... > > So I think if I were you I wouldn't commit to the kernel ABI until > somebody had at least written some RFC-quality patches for QEMU and > tested that they work and the ABI is OK in that sense. (For the > avoidance of doubt, I'm not volunteering to do that myself.) > I don't object to the idea in principle, though. > > PS: the other "injecting exceptions to the guest" oddity is that > if the kernel *does* find the ISV information and returns to userspace > for it to handle the MMIO, there's no way for userspace to say > "actually that address is supposed to generate a data abort". > That's a good point. A synchronous interface with a separate mechanism to ask the kernel to inject an exception might be a good solution, if there's an elegant way to do the latter. I'll have a look at that. Thanks, Christoffer
Re: [PATCH 1/1] KVM: inject data abort if instruction cannot be decoded
On Fri, 6 Sep 2019 at 14:13, Christoffer Dall wrote: > 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. Well, for QEMU we've had code that in theory might do this but in practice it's never been tested. Essentially the problem is that nobody ever wants to inject an exception from userspace except in incredibly rare cases like "trying to use h/w breakpoints simultaneously inside the VM and also to debug the VM from outside" or "we're running on RAS hardware that just told us that the VM's RAM was faulty". There's no even vaguely commonly-used usecase for it today; and this ABI suggestion adds another "this is in practice almost never going to happen" case to the pile. The codepath is unreliable in QEMU because (a) it requires getting syncing of sysreg values to and from the kernel right -- this is about the only situation where userspace wants to modify sysregs during execution of the VM, as opposed to just reading them -- and (b) we try to reuse the code we already have that does TCG exception injection, which might or might not be a design mistake, and (c) as noted above it's a never-actually-used untested codepath... So I think if I were you I wouldn't commit to the kernel ABI until somebody had at least written some RFC-quality patches for QEMU and tested that they work and the ABI is OK in that sense. (For the avoidance of doubt, I'm not volunteering to do that myself.) I don't object to the idea in principle, though. PS: the other "injecting exceptions to the guest" oddity is that if the kernel *does* find the ISV information and returns to userspace for it to handle the MMIO, there's no way for userspace to say "actually that address is supposed to generate a data abort". thanks -- PMM ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH 1/1] KVM: inject data abort if instruction cannot be decoded
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 wrote:
On Thu, 05 Sep 2019 09:16:54 +0100,
Peter Maydell 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_STSI25
#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) {
+
Re: [PATCH 1/1] KVM: inject data abort if instruction cannot be decoded
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 wrote:
> > > > > >
> > > > > > On Thu, 05 Sep 2019 09:16:54 +0100,
> > > > > > Peter Maydell 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_STSI25
> > > > #define KVM_EXI
Re: [UNVERIFIED SENDER] Re: [PATCH 1/1] KVM: inject data abort if instruction cannot be decoded
On 06.09.19 14:34, Marc Zyngier wrote:
On 06/09/2019 13:08, 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:
[...]
@@ -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?
IMHO it should be 100% a policy decision in user space whether to
emulate and what type of exception to inject, if anything.
The exception has to be something that the trapped instruction can
actually generate. An UNDEF is definitely wrong, as the guest would have
otherwise UNDEF'd at EL1, and KVM would have never seen it. You cannot
deviate from the rule of architecture, and userspace feels like the
wrong place to enforce it.
There are multiple viable options user space has:
1) Trigger an external abort
2) Emulate the instruction in user space
3) Inject a PV mechanism into the guest to emulate the insn inside
guest space
Why should we treat 1) any different from 2) or 3)? Why is there a "fast
path" for the external abort, on an exit that is not performance
critical or has any other reason to get special attention from kernel
space. All we're doing is add more code in a privileged layer for a case
that realistically should never occur in the first place.
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
Re: [PATCH 1/1] KVM: inject data abort if instruction cannot be decoded
On 06/09/2019 13:08, 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:
[...]
@@ -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?
>
> IMHO it should be 100% a policy decision in user space whether to
> emulate and what type of exception to inject, if anything.
The exception has to be something that the trapped instruction can
actually generate. An UNDEF is definitely wrong, as the guest would have
otherwise UNDEF'd at EL1, and KVM would have never seen it. You cannot
deviate from the rule of architecture, and userspace feels like the
wrong place to enforce it.
M.
--
Jazz is not dead, it just smells funny...
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH 1/1] KVM: inject data abort if instruction cannot be decoded
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 wrote:
On Thu, 05 Sep 2019 09:16:54 +0100,
Peter Maydell 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_STSI25
#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_abo
Re: [PATCH 1/1] KVM: inject data abort if instruction cannot be decoded
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 wrote:
> >>>
> >>> On Thu, 05 Sep 2019 09:16:54 +0100,
> >>> Peter Maydell 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_STSI25
> > #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 @@ in
Re: [PATCH 1/1] KVM: inject data abort if instruction cannot be decoded
On Thu, Sep 05, 2019 at 03:25:47PM +0200, Heinrich Schuchardt wrote:
> On 9/5/19 11:22 AM, 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 wrote:
> > > >
> > > > On Thu, 05 Sep 2019 09:16:54 +0100,
> > > > Peter Maydell 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;
>
> How about 32bit ARM?
>
What about it? Not sure I understand the comment.
> > };
> >
> > #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_STSI25
> > #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
