| Message ID | 20171006113921.24880-3-alex.bennee@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | KVM: arm64: single step emulation instructions | expand |
On 06/10/17 12:39, Alex Bennée wrote: > The system state of KVM when using userspace emulation is not complete > until we return into KVM_RUN. To handle mmio related updates we wait > until they have been committed and then schedule our KVM_EXIT_DEBUG. > > I've introduced a new function kvm_arm_maybe_return_debug() to wrap up > the differences between arm/arm64 which is currently null for arm. > > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > --- > arch/arm/include/asm/kvm_host.h | 2 ++ > arch/arm64/include/asm/kvm_host.h | 1 + > arch/arm64/kvm/debug.c | 21 +++++++++++++++++++++ > arch/arm64/kvm/handle_exit.c | 9 +++------ > virt/kvm/arm/arm.c | 2 +- > virt/kvm/arm/mmio.c | 3 ++- > 6 files changed, 30 insertions(+), 8 deletions(-) > > diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h > index 4a879f6ff13b..aec943f6d123 100644 > --- a/arch/arm/include/asm/kvm_host.h > +++ b/arch/arm/include/asm/kvm_host.h > @@ -285,6 +285,8 @@ static inline void kvm_arm_init_debug(void) {} > static inline void kvm_arm_setup_debug(struct kvm_vcpu *vcpu) {} > static inline void kvm_arm_clear_debug(struct kvm_vcpu *vcpu) {} > static inline void kvm_arm_reset_debug_ptr(struct kvm_vcpu *vcpu) {} > +static inline int kvm_arm_maybe_return_debug(struct kvm_vcpu *vcpu, > + struct kvm_run *run) {} > > int kvm_arm_vcpu_arch_set_attr(struct kvm_vcpu *vcpu, > struct kvm_device_attr *attr); > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > index e923b58606e2..fa67d21662f6 100644 > --- a/arch/arm64/include/asm/kvm_host.h > +++ b/arch/arm64/include/asm/kvm_host.h > @@ -369,6 +369,7 @@ void kvm_arm_init_debug(void); > void kvm_arm_setup_debug(struct kvm_vcpu *vcpu); > void kvm_arm_clear_debug(struct kvm_vcpu *vcpu); > void kvm_arm_reset_debug_ptr(struct kvm_vcpu *vcpu); > +int kvm_arm_maybe_return_debug(struct kvm_vcpu *vcpu, struct kvm_run *run); > int kvm_arm_vcpu_arch_set_attr(struct kvm_vcpu *vcpu, > struct kvm_device_attr *attr); > int kvm_arm_vcpu_arch_get_attr(struct kvm_vcpu *vcpu, > diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c > index dbadfaf850a7..a10a18c55c87 100644 > --- a/arch/arm64/kvm/debug.c > +++ b/arch/arm64/kvm/debug.c > @@ -221,3 +221,24 @@ void kvm_arm_clear_debug(struct kvm_vcpu *vcpu) > } > } > } > + > + > +/* > + * When KVM has successfully emulated the instruction we might want to > + * return we a KVM_EXIT_DEBUG. We can only do this once the emulation > + * is complete though so for userspace emulations we have to wait > + * until we have re-entered KVM. > + * > + * Return > 0 to return to guest, 0 (and set exit_reason) on proper > + * exit to userspace. > + */ > + > +int kvm_arm_maybe_return_debug(struct kvm_vcpu *vcpu, struct kvm_run *run) > +{ > + if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP) { > + run->exit_reason = KVM_EXIT_DEBUG; > + run->debug.arch.hsr = ESR_ELx_EC_SOFTSTP_LOW << ESR_ELx_EC_SHIFT; > + return 0; > + } > + return 1; > +} > diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c > index c918d291cb58..7b04f59217bf 100644 > --- a/arch/arm64/kvm/handle_exit.c > +++ b/arch/arm64/kvm/handle_exit.c > @@ -202,13 +202,10 @@ static int handle_trap_exceptions(struct kvm_vcpu *vcpu, struct kvm_run *run) > handled = exit_handler(vcpu, run); > } > > - if (handled && (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP)) { > - handled = 0; > - run->exit_reason = KVM_EXIT_DEBUG; > - run->debug.arch.hsr = ESR_ELx_EC_SOFTSTP_LOW << ESR_ELx_EC_SHIFT; > - } > + if (handled) > + return kvm_arm_maybe_return_debug(vcpu, run); > > - return handled; > + return 0; > } > > /* > diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c > index b9f68e4add71..3d28fe2daa26 100644 > --- a/virt/kvm/arm/arm.c > +++ b/virt/kvm/arm/arm.c > @@ -623,7 +623,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run) > > if (run->exit_reason == KVM_EXIT_MMIO) { > ret = kvm_handle_mmio_return(vcpu, vcpu->run); > - if (ret) > + if (ret < 1) > return ret; > } > > diff --git a/virt/kvm/arm/mmio.c b/virt/kvm/arm/mmio.c > index b6e715fd3c90..e43e3bd6222f 100644 > --- a/virt/kvm/arm/mmio.c > +++ b/virt/kvm/arm/mmio.c > @@ -117,7 +117,8 @@ int kvm_handle_mmio_return(struct kvm_vcpu *vcpu, struct kvm_run *run) > vcpu_set_reg(vcpu, vcpu->arch.mmio_decode.rt, data); > } > > - return 0; > + /* If debugging in effect we may need to return now */ > + return kvm_arm_maybe_return_debug(vcpu, run); Ah, that's how you do it. OK. Then the patch splitting is wrong, because everything is broken after patch #1. I'd prefer something like "kvm_handle_single_step" instead of this "kvm_arm_maybe_return_debug", but I'm very bad at naming things anyway. > } > > static int decode_hsr(struct kvm_vcpu *vcpu, bool *is_write, int *len) > Thanks, M.
On 06/10/17 13:37, Marc Zyngier wrote: > On 06/10/17 12:39, Alex Bennée wrote: >> The system state of KVM when using userspace emulation is not complete >> until we return into KVM_RUN. To handle mmio related updates we wait >> until they have been committed and then schedule our KVM_EXIT_DEBUG. >> >> I've introduced a new function kvm_arm_maybe_return_debug() to wrap up >> the differences between arm/arm64 which is currently null for arm. >> >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> >> --- >> arch/arm/include/asm/kvm_host.h | 2 ++ >> arch/arm64/include/asm/kvm_host.h | 1 + >> arch/arm64/kvm/debug.c | 21 +++++++++++++++++++++ >> arch/arm64/kvm/handle_exit.c | 9 +++------ >> virt/kvm/arm/arm.c | 2 +- >> virt/kvm/arm/mmio.c | 3 ++- >> 6 files changed, 30 insertions(+), 8 deletions(-) >> >> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h >> index 4a879f6ff13b..aec943f6d123 100644 >> --- a/arch/arm/include/asm/kvm_host.h >> +++ b/arch/arm/include/asm/kvm_host.h >> @@ -285,6 +285,8 @@ static inline void kvm_arm_init_debug(void) {} >> static inline void kvm_arm_setup_debug(struct kvm_vcpu *vcpu) {} >> static inline void kvm_arm_clear_debug(struct kvm_vcpu *vcpu) {} >> static inline void kvm_arm_reset_debug_ptr(struct kvm_vcpu *vcpu) {} >> +static inline int kvm_arm_maybe_return_debug(struct kvm_vcpu *vcpu, >> + struct kvm_run *run) {} >> >> int kvm_arm_vcpu_arch_set_attr(struct kvm_vcpu *vcpu, >> struct kvm_device_attr *attr); >> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h >> index e923b58606e2..fa67d21662f6 100644 >> --- a/arch/arm64/include/asm/kvm_host.h >> +++ b/arch/arm64/include/asm/kvm_host.h >> @@ -369,6 +369,7 @@ void kvm_arm_init_debug(void); >> void kvm_arm_setup_debug(struct kvm_vcpu *vcpu); >> void kvm_arm_clear_debug(struct kvm_vcpu *vcpu); >> void kvm_arm_reset_debug_ptr(struct kvm_vcpu *vcpu); >> +int kvm_arm_maybe_return_debug(struct kvm_vcpu *vcpu, struct kvm_run *run); >> int kvm_arm_vcpu_arch_set_attr(struct kvm_vcpu *vcpu, >> struct kvm_device_attr *attr); >> int kvm_arm_vcpu_arch_get_attr(struct kvm_vcpu *vcpu, >> diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c >> index dbadfaf850a7..a10a18c55c87 100644 >> --- a/arch/arm64/kvm/debug.c >> +++ b/arch/arm64/kvm/debug.c >> @@ -221,3 +221,24 @@ void kvm_arm_clear_debug(struct kvm_vcpu *vcpu) >> } >> } >> } >> + >> + >> +/* >> + * When KVM has successfully emulated the instruction we might want to >> + * return we a KVM_EXIT_DEBUG. We can only do this once the emulation >> + * is complete though so for userspace emulations we have to wait >> + * until we have re-entered KVM. >> + * >> + * Return > 0 to return to guest, 0 (and set exit_reason) on proper >> + * exit to userspace. >> + */ >> + >> +int kvm_arm_maybe_return_debug(struct kvm_vcpu *vcpu, struct kvm_run *run) >> +{ >> + if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP) { >> + run->exit_reason = KVM_EXIT_DEBUG; >> + run->debug.arch.hsr = ESR_ELx_EC_SOFTSTP_LOW << ESR_ELx_EC_SHIFT; >> + return 0; >> + } >> + return 1; >> +} >> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c >> index c918d291cb58..7b04f59217bf 100644 >> --- a/arch/arm64/kvm/handle_exit.c >> +++ b/arch/arm64/kvm/handle_exit.c >> @@ -202,13 +202,10 @@ static int handle_trap_exceptions(struct kvm_vcpu *vcpu, struct kvm_run *run) >> handled = exit_handler(vcpu, run); >> } >> >> - if (handled && (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP)) { >> - handled = 0; >> - run->exit_reason = KVM_EXIT_DEBUG; >> - run->debug.arch.hsr = ESR_ELx_EC_SOFTSTP_LOW << ESR_ELx_EC_SHIFT; >> - } >> + if (handled) >> + return kvm_arm_maybe_return_debug(vcpu, run); >> >> - return handled; >> + return 0; >> } >> >> /* >> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c >> index b9f68e4add71..3d28fe2daa26 100644 >> --- a/virt/kvm/arm/arm.c >> +++ b/virt/kvm/arm/arm.c >> @@ -623,7 +623,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run) >> >> if (run->exit_reason == KVM_EXIT_MMIO) { >> ret = kvm_handle_mmio_return(vcpu, vcpu->run); >> - if (ret) >> + if (ret < 1) >> return ret; >> } >> >> diff --git a/virt/kvm/arm/mmio.c b/virt/kvm/arm/mmio.c >> index b6e715fd3c90..e43e3bd6222f 100644 >> --- a/virt/kvm/arm/mmio.c >> +++ b/virt/kvm/arm/mmio.c >> @@ -117,7 +117,8 @@ int kvm_handle_mmio_return(struct kvm_vcpu *vcpu, struct kvm_run *run) >> vcpu_set_reg(vcpu, vcpu->arch.mmio_decode.rt, data); >> } >> >> - return 0; >> + /* If debugging in effect we may need to return now */ >> + return kvm_arm_maybe_return_debug(vcpu, run); > > Ah, that's how you do it. OK. Then the patch splitting is wrong, because > everything is broken after patch #1. Actually, it is not broken at all. I'm just confused by the very esoteric flow. Thanks. M.
On 06/10/17 12:39, Alex Bennée wrote: > The system state of KVM when using userspace emulation is not complete > until we return into KVM_RUN. To handle mmio related updates we wait > until they have been committed and then schedule our KVM_EXIT_DEBUG. > > I've introduced a new function kvm_arm_maybe_return_debug() to wrap up > the differences between arm/arm64 which is currently null for arm. > > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > --- > arch/arm/include/asm/kvm_host.h | 2 ++ > arch/arm64/include/asm/kvm_host.h | 1 + > arch/arm64/kvm/debug.c | 21 +++++++++++++++++++++ > arch/arm64/kvm/handle_exit.c | 9 +++------ > virt/kvm/arm/arm.c | 2 +- > virt/kvm/arm/mmio.c | 3 ++- > 6 files changed, 30 insertions(+), 8 deletions(-) > > diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h > index 4a879f6ff13b..aec943f6d123 100644 > --- a/arch/arm/include/asm/kvm_host.h > +++ b/arch/arm/include/asm/kvm_host.h > @@ -285,6 +285,8 @@ static inline void kvm_arm_init_debug(void) {} > static inline void kvm_arm_setup_debug(struct kvm_vcpu *vcpu) {} > static inline void kvm_arm_clear_debug(struct kvm_vcpu *vcpu) {} > static inline void kvm_arm_reset_debug_ptr(struct kvm_vcpu *vcpu) {} > +static inline int kvm_arm_maybe_return_debug(struct kvm_vcpu *vcpu, > + struct kvm_run *run) {} > This function should return 1. > int kvm_arm_vcpu_arch_set_attr(struct kvm_vcpu *vcpu, > struct kvm_device_attr *attr); > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > index e923b58606e2..fa67d21662f6 100644 > --- a/arch/arm64/include/asm/kvm_host.h > +++ b/arch/arm64/include/asm/kvm_host.h > @@ -369,6 +369,7 @@ void kvm_arm_init_debug(void); > void kvm_arm_setup_debug(struct kvm_vcpu *vcpu); > void kvm_arm_clear_debug(struct kvm_vcpu *vcpu); > void kvm_arm_reset_debug_ptr(struct kvm_vcpu *vcpu); > +int kvm_arm_maybe_return_debug(struct kvm_vcpu *vcpu, struct kvm_run *run); I feel the name could be a little bit more explicit: kvm_arm_trap_need_step_debug, kvm_arm_trap_step_return_debug, kvm_arm_trap_need_return_debug. At least, I think it would be nice that the name reflect that this check is meant for emulated instructions. Otherwise: Reviewed-by: Julien Thierry <julien.thierry@arm.com> Thanks,
Julien Thierry <julien.thierry@arm.com> writes: > On 06/10/17 12:39, Alex Bennée wrote: >> The system state of KVM when using userspace emulation is not complete >> until we return into KVM_RUN. To handle mmio related updates we wait >> until they have been committed and then schedule our KVM_EXIT_DEBUG. >> >> I've introduced a new function kvm_arm_maybe_return_debug() to wrap up >> the differences between arm/arm64 which is currently null for arm. >> >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> >> --- >> arch/arm/include/asm/kvm_host.h | 2 ++ >> arch/arm64/include/asm/kvm_host.h | 1 + >> arch/arm64/kvm/debug.c | 21 +++++++++++++++++++++ >> arch/arm64/kvm/handle_exit.c | 9 +++------ >> virt/kvm/arm/arm.c | 2 +- >> virt/kvm/arm/mmio.c | 3 ++- >> 6 files changed, 30 insertions(+), 8 deletions(-) >> >> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h >> index 4a879f6ff13b..aec943f6d123 100644 >> --- a/arch/arm/include/asm/kvm_host.h >> +++ b/arch/arm/include/asm/kvm_host.h >> @@ -285,6 +285,8 @@ static inline void kvm_arm_init_debug(void) {} >> static inline void kvm_arm_setup_debug(struct kvm_vcpu *vcpu) {} >> static inline void kvm_arm_clear_debug(struct kvm_vcpu *vcpu) {} >> static inline void kvm_arm_reset_debug_ptr(struct kvm_vcpu *vcpu) {} >> +static inline int kvm_arm_maybe_return_debug(struct kvm_vcpu *vcpu, >> + struct kvm_run *run) {} >> > > This function should return 1. So I did ponder making this a bool, returning true if we need to exit and testing in v/k/a/arm.c exit leg rather than in the mmio handler. At the moment it mirrors the existing exit logic which follows -1 err, 0 return, >0 handled. But as I mentioned in the cover letter this fell down a bit when dealing with the mmio case. > >> int kvm_arm_vcpu_arch_set_attr(struct kvm_vcpu *vcpu, >> struct kvm_device_attr *attr); >> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h >> index e923b58606e2..fa67d21662f6 100644 >> --- a/arch/arm64/include/asm/kvm_host.h >> +++ b/arch/arm64/include/asm/kvm_host.h >> @@ -369,6 +369,7 @@ void kvm_arm_init_debug(void); >> void kvm_arm_setup_debug(struct kvm_vcpu *vcpu); >> void kvm_arm_clear_debug(struct kvm_vcpu *vcpu); >> void kvm_arm_reset_debug_ptr(struct kvm_vcpu *vcpu); >> +int kvm_arm_maybe_return_debug(struct kvm_vcpu *vcpu, struct kvm_run *run); > > I feel the name could be a little bit more explicit: > > kvm_arm_trap_need_step_debug, kvm_arm_trap_step_return_debug, > kvm_arm_trap_need_return_debug. I wanted to keep the debug suffix so that's fine although I'm not so sure trap is correct because on the tail end of mmio emulation are we still trapping? Maybe kvm_arm_step_emulated_debug? > At least, I think it would be nice that the name reflect that this > check is meant for emulated instructions. > > Otherwise: > > Reviewed-by: Julien Thierry <julien.thierry@arm.com> > > Thanks, -- Alex Bennée
Marc Zyngier <marc.zyngier@arm.com> writes: > On 06/10/17 13:37, Marc Zyngier wrote: >> On 06/10/17 12:39, Alex Bennée wrote: >>> The system state of KVM when using userspace emulation is not complete >>> until we return into KVM_RUN. To handle mmio related updates we wait >>> until they have been committed and then schedule our KVM_EXIT_DEBUG. >>> >>> I've introduced a new function kvm_arm_maybe_return_debug() to wrap up >>> the differences between arm/arm64 which is currently null for arm. >>> >>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> >>> --- >>> arch/arm/include/asm/kvm_host.h | 2 ++ >>> arch/arm64/include/asm/kvm_host.h | 1 + >>> arch/arm64/kvm/debug.c | 21 +++++++++++++++++++++ >>> arch/arm64/kvm/handle_exit.c | 9 +++------ >>> virt/kvm/arm/arm.c | 2 +- >>> virt/kvm/arm/mmio.c | 3 ++- >>> 6 files changed, 30 insertions(+), 8 deletions(-) >>> >>> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h >>> index 4a879f6ff13b..aec943f6d123 100644 >>> --- a/arch/arm/include/asm/kvm_host.h >>> +++ b/arch/arm/include/asm/kvm_host.h >>> @@ -285,6 +285,8 @@ static inline void kvm_arm_init_debug(void) {} >>> static inline void kvm_arm_setup_debug(struct kvm_vcpu *vcpu) {} >>> static inline void kvm_arm_clear_debug(struct kvm_vcpu *vcpu) {} >>> static inline void kvm_arm_reset_debug_ptr(struct kvm_vcpu *vcpu) {} >>> +static inline int kvm_arm_maybe_return_debug(struct kvm_vcpu *vcpu, >>> + struct kvm_run *run) {} >>> >>> int kvm_arm_vcpu_arch_set_attr(struct kvm_vcpu *vcpu, >>> struct kvm_device_attr *attr); >>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h >>> index e923b58606e2..fa67d21662f6 100644 >>> --- a/arch/arm64/include/asm/kvm_host.h >>> +++ b/arch/arm64/include/asm/kvm_host.h >>> @@ -369,6 +369,7 @@ void kvm_arm_init_debug(void); >>> void kvm_arm_setup_debug(struct kvm_vcpu *vcpu); >>> void kvm_arm_clear_debug(struct kvm_vcpu *vcpu); >>> void kvm_arm_reset_debug_ptr(struct kvm_vcpu *vcpu); >>> +int kvm_arm_maybe_return_debug(struct kvm_vcpu *vcpu, struct kvm_run *run); >>> int kvm_arm_vcpu_arch_set_attr(struct kvm_vcpu *vcpu, >>> struct kvm_device_attr *attr); >>> int kvm_arm_vcpu_arch_get_attr(struct kvm_vcpu *vcpu, >>> diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c >>> index dbadfaf850a7..a10a18c55c87 100644 >>> --- a/arch/arm64/kvm/debug.c >>> +++ b/arch/arm64/kvm/debug.c >>> @@ -221,3 +221,24 @@ void kvm_arm_clear_debug(struct kvm_vcpu *vcpu) >>> } >>> } >>> } >>> + >>> + >>> +/* >>> + * When KVM has successfully emulated the instruction we might want to >>> + * return we a KVM_EXIT_DEBUG. We can only do this once the emulation >>> + * is complete though so for userspace emulations we have to wait >>> + * until we have re-entered KVM. >>> + * >>> + * Return > 0 to return to guest, 0 (and set exit_reason) on proper >>> + * exit to userspace. >>> + */ >>> + >>> +int kvm_arm_maybe_return_debug(struct kvm_vcpu *vcpu, struct kvm_run *run) >>> +{ >>> + if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP) { >>> + run->exit_reason = KVM_EXIT_DEBUG; >>> + run->debug.arch.hsr = ESR_ELx_EC_SOFTSTP_LOW << ESR_ELx_EC_SHIFT; >>> + return 0; >>> + } >>> + return 1; >>> +} >>> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c >>> index c918d291cb58..7b04f59217bf 100644 >>> --- a/arch/arm64/kvm/handle_exit.c >>> +++ b/arch/arm64/kvm/handle_exit.c >>> @@ -202,13 +202,10 @@ static int handle_trap_exceptions(struct kvm_vcpu *vcpu, struct kvm_run *run) >>> handled = exit_handler(vcpu, run); >>> } >>> >>> - if (handled && (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP)) { >>> - handled = 0; >>> - run->exit_reason = KVM_EXIT_DEBUG; >>> - run->debug.arch.hsr = ESR_ELx_EC_SOFTSTP_LOW << ESR_ELx_EC_SHIFT; >>> - } >>> + if (handled) >>> + return kvm_arm_maybe_return_debug(vcpu, run); >>> >>> - return handled; >>> + return 0; >>> } >>> >>> /* >>> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c >>> index b9f68e4add71..3d28fe2daa26 100644 >>> --- a/virt/kvm/arm/arm.c >>> +++ b/virt/kvm/arm/arm.c >>> @@ -623,7 +623,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run) >>> >>> if (run->exit_reason == KVM_EXIT_MMIO) { >>> ret = kvm_handle_mmio_return(vcpu, vcpu->run); >>> - if (ret) >>> + if (ret < 1) >>> return ret; >>> } >>> >>> diff --git a/virt/kvm/arm/mmio.c b/virt/kvm/arm/mmio.c >>> index b6e715fd3c90..e43e3bd6222f 100644 >>> --- a/virt/kvm/arm/mmio.c >>> +++ b/virt/kvm/arm/mmio.c >>> @@ -117,7 +117,8 @@ int kvm_handle_mmio_return(struct kvm_vcpu *vcpu, struct kvm_run *run) >>> vcpu_set_reg(vcpu, vcpu->arch.mmio_decode.rt, data); >>> } >>> >>> - return 0; >>> + /* If debugging in effect we may need to return now */ >>> + return kvm_arm_maybe_return_debug(vcpu, run); >> >> Ah, that's how you do it. OK. Then the patch splitting is wrong, because >> everything is broken after patch #1. > > Actually, it is not broken at all. I'm just confused by the very > esoteric flow. We could just merge the whole patch in one but I wanted to show the difference between in-kernel and out-of-kernel emulation. I could also move the step handling to the mmio leg in kvm_arch_vcpu_ioctl_run but you mentioned you use the mmio completion elsewhere anyway? -- Alex Bennée
On 06/10/17 14:47, Alex Bennée wrote: > > Marc Zyngier <marc.zyngier@arm.com> writes: > >> On 06/10/17 13:37, Marc Zyngier wrote: >>> On 06/10/17 12:39, Alex Bennée wrote: >>>> The system state of KVM when using userspace emulation is not complete >>>> until we return into KVM_RUN. To handle mmio related updates we wait >>>> until they have been committed and then schedule our KVM_EXIT_DEBUG. >>>> >>>> I've introduced a new function kvm_arm_maybe_return_debug() to wrap up >>>> the differences between arm/arm64 which is currently null for arm. >>>> >>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> >>>> --- >>>> arch/arm/include/asm/kvm_host.h | 2 ++ >>>> arch/arm64/include/asm/kvm_host.h | 1 + >>>> arch/arm64/kvm/debug.c | 21 +++++++++++++++++++++ >>>> arch/arm64/kvm/handle_exit.c | 9 +++------ >>>> virt/kvm/arm/arm.c | 2 +- >>>> virt/kvm/arm/mmio.c | 3 ++- >>>> 6 files changed, 30 insertions(+), 8 deletions(-) >>>> >>>> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h >>>> index 4a879f6ff13b..aec943f6d123 100644 >>>> --- a/arch/arm/include/asm/kvm_host.h >>>> +++ b/arch/arm/include/asm/kvm_host.h >>>> @@ -285,6 +285,8 @@ static inline void kvm_arm_init_debug(void) {} >>>> static inline void kvm_arm_setup_debug(struct kvm_vcpu *vcpu) {} >>>> static inline void kvm_arm_clear_debug(struct kvm_vcpu *vcpu) {} >>>> static inline void kvm_arm_reset_debug_ptr(struct kvm_vcpu *vcpu) {} >>>> +static inline int kvm_arm_maybe_return_debug(struct kvm_vcpu *vcpu, >>>> + struct kvm_run *run) {} >>>> >>>> int kvm_arm_vcpu_arch_set_attr(struct kvm_vcpu *vcpu, >>>> struct kvm_device_attr *attr); >>>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h >>>> index e923b58606e2..fa67d21662f6 100644 >>>> --- a/arch/arm64/include/asm/kvm_host.h >>>> +++ b/arch/arm64/include/asm/kvm_host.h >>>> @@ -369,6 +369,7 @@ void kvm_arm_init_debug(void); >>>> void kvm_arm_setup_debug(struct kvm_vcpu *vcpu); >>>> void kvm_arm_clear_debug(struct kvm_vcpu *vcpu); >>>> void kvm_arm_reset_debug_ptr(struct kvm_vcpu *vcpu); >>>> +int kvm_arm_maybe_return_debug(struct kvm_vcpu *vcpu, struct kvm_run *run); >>>> int kvm_arm_vcpu_arch_set_attr(struct kvm_vcpu *vcpu, >>>> struct kvm_device_attr *attr); >>>> int kvm_arm_vcpu_arch_get_attr(struct kvm_vcpu *vcpu, >>>> diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c >>>> index dbadfaf850a7..a10a18c55c87 100644 >>>> --- a/arch/arm64/kvm/debug.c >>>> +++ b/arch/arm64/kvm/debug.c >>>> @@ -221,3 +221,24 @@ void kvm_arm_clear_debug(struct kvm_vcpu *vcpu) >>>> } >>>> } >>>> } >>>> + >>>> + >>>> +/* >>>> + * When KVM has successfully emulated the instruction we might want to >>>> + * return we a KVM_EXIT_DEBUG. We can only do this once the emulation >>>> + * is complete though so for userspace emulations we have to wait >>>> + * until we have re-entered KVM. >>>> + * >>>> + * Return > 0 to return to guest, 0 (and set exit_reason) on proper >>>> + * exit to userspace. >>>> + */ >>>> + >>>> +int kvm_arm_maybe_return_debug(struct kvm_vcpu *vcpu, struct kvm_run *run) >>>> +{ >>>> + if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP) { >>>> + run->exit_reason = KVM_EXIT_DEBUG; >>>> + run->debug.arch.hsr = ESR_ELx_EC_SOFTSTP_LOW << ESR_ELx_EC_SHIFT; >>>> + return 0; >>>> + } >>>> + return 1; >>>> +} >>>> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c >>>> index c918d291cb58..7b04f59217bf 100644 >>>> --- a/arch/arm64/kvm/handle_exit.c >>>> +++ b/arch/arm64/kvm/handle_exit.c >>>> @@ -202,13 +202,10 @@ static int handle_trap_exceptions(struct kvm_vcpu *vcpu, struct kvm_run *run) >>>> handled = exit_handler(vcpu, run); >>>> } >>>> >>>> - if (handled && (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP)) { >>>> - handled = 0; >>>> - run->exit_reason = KVM_EXIT_DEBUG; >>>> - run->debug.arch.hsr = ESR_ELx_EC_SOFTSTP_LOW << ESR_ELx_EC_SHIFT; >>>> - } >>>> + if (handled) >>>> + return kvm_arm_maybe_return_debug(vcpu, run); >>>> >>>> - return handled; >>>> + return 0; >>>> } >>>> >>>> /* >>>> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c >>>> index b9f68e4add71..3d28fe2daa26 100644 >>>> --- a/virt/kvm/arm/arm.c >>>> +++ b/virt/kvm/arm/arm.c >>>> @@ -623,7 +623,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run) >>>> >>>> if (run->exit_reason == KVM_EXIT_MMIO) { >>>> ret = kvm_handle_mmio_return(vcpu, vcpu->run); >>>> - if (ret) >>>> + if (ret < 1) >>>> return ret; >>>> } >>>> >>>> diff --git a/virt/kvm/arm/mmio.c b/virt/kvm/arm/mmio.c >>>> index b6e715fd3c90..e43e3bd6222f 100644 >>>> --- a/virt/kvm/arm/mmio.c >>>> +++ b/virt/kvm/arm/mmio.c >>>> @@ -117,7 +117,8 @@ int kvm_handle_mmio_return(struct kvm_vcpu *vcpu, struct kvm_run *run) >>>> vcpu_set_reg(vcpu, vcpu->arch.mmio_decode.rt, data); >>>> } >>>> >>>> - return 0; >>>> + /* If debugging in effect we may need to return now */ >>>> + return kvm_arm_maybe_return_debug(vcpu, run); >>> >>> Ah, that's how you do it. OK. Then the patch splitting is wrong, because >>> everything is broken after patch #1. >> >> Actually, it is not broken at all. I'm just confused by the very >> esoteric flow. > > We could just merge the whole patch in one but I wanted to show the > difference between in-kernel and out-of-kernel emulation. > > I could also move the step handling to the mmio leg in > kvm_arch_vcpu_ioctl_run but you mentioned you use the mmio completion > elsewhere anyway? Yes, look at the end of io_mem_abort(). This is used by the vgic to complete a read emulation in the kernel. And actually, this means that we shouldn't have to mess with handle_exit. Just check for the return value of kvm_handle_mmio_return in the call sites (including the one in io_mem_abort), and exit if we need to single-step... Thoughts? M.
On 06/10/17 15:00, Marc Zyngier wrote: > On 06/10/17 14:47, Alex Bennée wrote: >> >> Marc Zyngier <marc.zyngier@arm.com> writes: >> >>> On 06/10/17 13:37, Marc Zyngier wrote: >>>> On 06/10/17 12:39, Alex Bennée wrote: >>>>> The system state of KVM when using userspace emulation is not complete >>>>> until we return into KVM_RUN. To handle mmio related updates we wait >>>>> until they have been committed and then schedule our KVM_EXIT_DEBUG. >>>>> >>>>> I've introduced a new function kvm_arm_maybe_return_debug() to wrap up >>>>> the differences between arm/arm64 which is currently null for arm. >>>>> >>>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> >>>>> --- >>>>> arch/arm/include/asm/kvm_host.h | 2 ++ >>>>> arch/arm64/include/asm/kvm_host.h | 1 + >>>>> arch/arm64/kvm/debug.c | 21 +++++++++++++++++++++ >>>>> arch/arm64/kvm/handle_exit.c | 9 +++------ >>>>> virt/kvm/arm/arm.c | 2 +- >>>>> virt/kvm/arm/mmio.c | 3 ++- >>>>> 6 files changed, 30 insertions(+), 8 deletions(-) >>>>> >>>>> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h >>>>> index 4a879f6ff13b..aec943f6d123 100644 >>>>> --- a/arch/arm/include/asm/kvm_host.h >>>>> +++ b/arch/arm/include/asm/kvm_host.h >>>>> @@ -285,6 +285,8 @@ static inline void kvm_arm_init_debug(void) {} >>>>> static inline void kvm_arm_setup_debug(struct kvm_vcpu *vcpu) {} >>>>> static inline void kvm_arm_clear_debug(struct kvm_vcpu *vcpu) {} >>>>> static inline void kvm_arm_reset_debug_ptr(struct kvm_vcpu *vcpu) {} >>>>> +static inline int kvm_arm_maybe_return_debug(struct kvm_vcpu *vcpu, >>>>> + struct kvm_run *run) {} >>>>> >>>>> int kvm_arm_vcpu_arch_set_attr(struct kvm_vcpu *vcpu, >>>>> struct kvm_device_attr *attr); >>>>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h >>>>> index e923b58606e2..fa67d21662f6 100644 >>>>> --- a/arch/arm64/include/asm/kvm_host.h >>>>> +++ b/arch/arm64/include/asm/kvm_host.h >>>>> @@ -369,6 +369,7 @@ void kvm_arm_init_debug(void); >>>>> void kvm_arm_setup_debug(struct kvm_vcpu *vcpu); >>>>> void kvm_arm_clear_debug(struct kvm_vcpu *vcpu); >>>>> void kvm_arm_reset_debug_ptr(struct kvm_vcpu *vcpu); >>>>> +int kvm_arm_maybe_return_debug(struct kvm_vcpu *vcpu, struct kvm_run *run); >>>>> int kvm_arm_vcpu_arch_set_attr(struct kvm_vcpu *vcpu, >>>>> struct kvm_device_attr *attr); >>>>> int kvm_arm_vcpu_arch_get_attr(struct kvm_vcpu *vcpu, >>>>> diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c >>>>> index dbadfaf850a7..a10a18c55c87 100644 >>>>> --- a/arch/arm64/kvm/debug.c >>>>> +++ b/arch/arm64/kvm/debug.c >>>>> @@ -221,3 +221,24 @@ void kvm_arm_clear_debug(struct kvm_vcpu *vcpu) >>>>> } >>>>> } >>>>> } >>>>> + >>>>> + >>>>> +/* >>>>> + * When KVM has successfully emulated the instruction we might want to >>>>> + * return we a KVM_EXIT_DEBUG. We can only do this once the emulation >>>>> + * is complete though so for userspace emulations we have to wait >>>>> + * until we have re-entered KVM. >>>>> + * >>>>> + * Return > 0 to return to guest, 0 (and set exit_reason) on proper >>>>> + * exit to userspace. >>>>> + */ >>>>> + >>>>> +int kvm_arm_maybe_return_debug(struct kvm_vcpu *vcpu, struct kvm_run *run) >>>>> +{ >>>>> + if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP) { >>>>> + run->exit_reason = KVM_EXIT_DEBUG; >>>>> + run->debug.arch.hsr = ESR_ELx_EC_SOFTSTP_LOW << ESR_ELx_EC_SHIFT; >>>>> + return 0; >>>>> + } >>>>> + return 1; >>>>> +} >>>>> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c >>>>> index c918d291cb58..7b04f59217bf 100644 >>>>> --- a/arch/arm64/kvm/handle_exit.c >>>>> +++ b/arch/arm64/kvm/handle_exit.c >>>>> @@ -202,13 +202,10 @@ static int handle_trap_exceptions(struct kvm_vcpu *vcpu, struct kvm_run *run) >>>>> handled = exit_handler(vcpu, run); >>>>> } >>>>> >>>>> - if (handled && (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP)) { >>>>> - handled = 0; >>>>> - run->exit_reason = KVM_EXIT_DEBUG; >>>>> - run->debug.arch.hsr = ESR_ELx_EC_SOFTSTP_LOW << ESR_ELx_EC_SHIFT; >>>>> - } >>>>> + if (handled) >>>>> + return kvm_arm_maybe_return_debug(vcpu, run); >>>>> >>>>> - return handled; >>>>> + return 0; >>>>> } >>>>> >>>>> /* >>>>> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c >>>>> index b9f68e4add71..3d28fe2daa26 100644 >>>>> --- a/virt/kvm/arm/arm.c >>>>> +++ b/virt/kvm/arm/arm.c >>>>> @@ -623,7 +623,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run) >>>>> >>>>> if (run->exit_reason == KVM_EXIT_MMIO) { >>>>> ret = kvm_handle_mmio_return(vcpu, vcpu->run); >>>>> - if (ret) >>>>> + if (ret < 1) >>>>> return ret; >>>>> } >>>>> >>>>> diff --git a/virt/kvm/arm/mmio.c b/virt/kvm/arm/mmio.c >>>>> index b6e715fd3c90..e43e3bd6222f 100644 >>>>> --- a/virt/kvm/arm/mmio.c >>>>> +++ b/virt/kvm/arm/mmio.c >>>>> @@ -117,7 +117,8 @@ int kvm_handle_mmio_return(struct kvm_vcpu *vcpu, struct kvm_run *run) >>>>> vcpu_set_reg(vcpu, vcpu->arch.mmio_decode.rt, data); >>>>> } >>>>> >>>>> - return 0; >>>>> + /* If debugging in effect we may need to return now */ >>>>> + return kvm_arm_maybe_return_debug(vcpu, run); >>>> >>>> Ah, that's how you do it. OK. Then the patch splitting is wrong, because >>>> everything is broken after patch #1. >>> >>> Actually, it is not broken at all. I'm just confused by the very >>> esoteric flow. >> >> We could just merge the whole patch in one but I wanted to show the >> difference between in-kernel and out-of-kernel emulation. >> >> I could also move the step handling to the mmio leg in >> kvm_arch_vcpu_ioctl_run but you mentioned you use the mmio completion >> elsewhere anyway? > Yes, look at the end of io_mem_abort(). This is used by the vgic to > complete a read emulation in the kernel. > > And actually, this means that we shouldn't have to mess with > handle_exit. Just check for the return value of kvm_handle_mmio_return > in the call sites (including the one in io_mem_abort), and exit if we > need to single-step... > I think we need to mess with handle_exit (or at least something else than kvm_handle_mmio call sites) because the patches don't only fix MMIO single stepping, but also other emulated stuff (system register accesses, ...). But with your suggestion maybe we can at least handle both MMIO cases in a similar manner. I think we still need the code in handle_exit, or add more code to deal case by case with other emulated instructions. Thanks,
On 06/10/17 14:45, Alex Bennée wrote: > > Julien Thierry <julien.thierry@arm.com> writes: > >> On 06/10/17 12:39, Alex Bennée wrote: >>> The system state of KVM when using userspace emulation is not complete >>> until we return into KVM_RUN. To handle mmio related updates we wait >>> until they have been committed and then schedule our KVM_EXIT_DEBUG. >>> >>> I've introduced a new function kvm_arm_maybe_return_debug() to wrap up >>> the differences between arm/arm64 which is currently null for arm. >>> >>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> >>> --- >>> arch/arm/include/asm/kvm_host.h | 2 ++ >>> arch/arm64/include/asm/kvm_host.h | 1 + >>> arch/arm64/kvm/debug.c | 21 +++++++++++++++++++++ >>> arch/arm64/kvm/handle_exit.c | 9 +++------ >>> virt/kvm/arm/arm.c | 2 +- >>> virt/kvm/arm/mmio.c | 3 ++- >>> 6 files changed, 30 insertions(+), 8 deletions(-) >>> >>> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h >>> index 4a879f6ff13b..aec943f6d123 100644 >>> --- a/arch/arm/include/asm/kvm_host.h >>> +++ b/arch/arm/include/asm/kvm_host.h >>> @@ -285,6 +285,8 @@ static inline void kvm_arm_init_debug(void) {} >>> static inline void kvm_arm_setup_debug(struct kvm_vcpu *vcpu) {} >>> static inline void kvm_arm_clear_debug(struct kvm_vcpu *vcpu) {} >>> static inline void kvm_arm_reset_debug_ptr(struct kvm_vcpu *vcpu) {} >>> +static inline int kvm_arm_maybe_return_debug(struct kvm_vcpu *vcpu, >>> + struct kvm_run *run) {} >>> >> >> This function should return 1. > > So I did ponder making this a bool, returning true if we need to exit > and testing in v/k/a/arm.c exit leg rather than in the mmio handler. > > At the moment it mirrors the existing exit logic which follows -1 err, 0 > return, >0 handled. But as I mentioned in the cover letter this fell > down a bit when dealing with the mmio case. > Hmmm, my main issue is that this version doesn't have a return statement, which probably triggers a gcc warning with ARCH=arm and also might cause arm (32bit) kvm to exit upon handling mmio return when we don't want to. Otherwise, I also wondered about using a bool here. But following the pre-existing logic makes sense to me (but I have no strong feeling about it). >> >>> int kvm_arm_vcpu_arch_set_attr(struct kvm_vcpu *vcpu, >>> struct kvm_device_attr *attr); >>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h >>> index e923b58606e2..fa67d21662f6 100644 >>> --- a/arch/arm64/include/asm/kvm_host.h >>> +++ b/arch/arm64/include/asm/kvm_host.h >>> @@ -369,6 +369,7 @@ void kvm_arm_init_debug(void); >>> void kvm_arm_setup_debug(struct kvm_vcpu *vcpu); >>> void kvm_arm_clear_debug(struct kvm_vcpu *vcpu); >>> void kvm_arm_reset_debug_ptr(struct kvm_vcpu *vcpu); >>> +int kvm_arm_maybe_return_debug(struct kvm_vcpu *vcpu, struct kvm_run *run); >> >> I feel the name could be a little bit more explicit: >> >> kvm_arm_trap_need_step_debug, kvm_arm_trap_step_return_debug, >> kvm_arm_trap_need_return_debug. > > I wanted to keep the debug suffix so that's fine although I'm not so > sure trap is correct because on the tail end of mmio emulation are we > still trapping? > > Maybe kvm_arm_step_emulated_debug? > Yes, sounds good. Thanks, >> At least, I think it would be nice that the name reflect that this >> check is meant for emulated instructions. >> >> Otherwise: >> >> Reviewed-by: Julien Thierry <julien.thierry@arm.com> >> >> Thanks, > > > -- > Alex Bennée >
On 06/10/17 15:26, Julien Thierry wrote: > > > On 06/10/17 15:00, Marc Zyngier wrote: >> On 06/10/17 14:47, Alex Bennée wrote: >>> >>> Marc Zyngier <marc.zyngier@arm.com> writes: >>> >>>> On 06/10/17 13:37, Marc Zyngier wrote: >>>>> On 06/10/17 12:39, Alex Bennée wrote: >>>>>> The system state of KVM when using userspace emulation is not complete >>>>>> until we return into KVM_RUN. To handle mmio related updates we wait >>>>>> until they have been committed and then schedule our KVM_EXIT_DEBUG. >>>>>> >>>>>> I've introduced a new function kvm_arm_maybe_return_debug() to wrap up >>>>>> the differences between arm/arm64 which is currently null for arm. >>>>>> >>>>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> >>>>>> --- >>>>>> arch/arm/include/asm/kvm_host.h | 2 ++ >>>>>> arch/arm64/include/asm/kvm_host.h | 1 + >>>>>> arch/arm64/kvm/debug.c | 21 +++++++++++++++++++++ >>>>>> arch/arm64/kvm/handle_exit.c | 9 +++------ >>>>>> virt/kvm/arm/arm.c | 2 +- >>>>>> virt/kvm/arm/mmio.c | 3 ++- >>>>>> 6 files changed, 30 insertions(+), 8 deletions(-) >>>>>> >>>>>> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h >>>>>> index 4a879f6ff13b..aec943f6d123 100644 >>>>>> --- a/arch/arm/include/asm/kvm_host.h >>>>>> +++ b/arch/arm/include/asm/kvm_host.h >>>>>> @@ -285,6 +285,8 @@ static inline void kvm_arm_init_debug(void) {} >>>>>> static inline void kvm_arm_setup_debug(struct kvm_vcpu *vcpu) {} >>>>>> static inline void kvm_arm_clear_debug(struct kvm_vcpu *vcpu) {} >>>>>> static inline void kvm_arm_reset_debug_ptr(struct kvm_vcpu *vcpu) {} >>>>>> +static inline int kvm_arm_maybe_return_debug(struct kvm_vcpu *vcpu, >>>>>> + struct kvm_run *run) {} >>>>>> >>>>>> int kvm_arm_vcpu_arch_set_attr(struct kvm_vcpu *vcpu, >>>>>> struct kvm_device_attr *attr); >>>>>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h >>>>>> index e923b58606e2..fa67d21662f6 100644 >>>>>> --- a/arch/arm64/include/asm/kvm_host.h >>>>>> +++ b/arch/arm64/include/asm/kvm_host.h >>>>>> @@ -369,6 +369,7 @@ void kvm_arm_init_debug(void); >>>>>> void kvm_arm_setup_debug(struct kvm_vcpu *vcpu); >>>>>> void kvm_arm_clear_debug(struct kvm_vcpu *vcpu); >>>>>> void kvm_arm_reset_debug_ptr(struct kvm_vcpu *vcpu); >>>>>> +int kvm_arm_maybe_return_debug(struct kvm_vcpu *vcpu, struct kvm_run *run); >>>>>> int kvm_arm_vcpu_arch_set_attr(struct kvm_vcpu *vcpu, >>>>>> struct kvm_device_attr *attr); >>>>>> int kvm_arm_vcpu_arch_get_attr(struct kvm_vcpu *vcpu, >>>>>> diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c >>>>>> index dbadfaf850a7..a10a18c55c87 100644 >>>>>> --- a/arch/arm64/kvm/debug.c >>>>>> +++ b/arch/arm64/kvm/debug.c >>>>>> @@ -221,3 +221,24 @@ void kvm_arm_clear_debug(struct kvm_vcpu *vcpu) >>>>>> } >>>>>> } >>>>>> } >>>>>> + >>>>>> + >>>>>> +/* >>>>>> + * When KVM has successfully emulated the instruction we might want to >>>>>> + * return we a KVM_EXIT_DEBUG. We can only do this once the emulation >>>>>> + * is complete though so for userspace emulations we have to wait >>>>>> + * until we have re-entered KVM. >>>>>> + * >>>>>> + * Return > 0 to return to guest, 0 (and set exit_reason) on proper >>>>>> + * exit to userspace. >>>>>> + */ >>>>>> + >>>>>> +int kvm_arm_maybe_return_debug(struct kvm_vcpu *vcpu, struct kvm_run *run) >>>>>> +{ >>>>>> + if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP) { >>>>>> + run->exit_reason = KVM_EXIT_DEBUG; >>>>>> + run->debug.arch.hsr = ESR_ELx_EC_SOFTSTP_LOW << ESR_ELx_EC_SHIFT; >>>>>> + return 0; >>>>>> + } >>>>>> + return 1; >>>>>> +} >>>>>> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c >>>>>> index c918d291cb58..7b04f59217bf 100644 >>>>>> --- a/arch/arm64/kvm/handle_exit.c >>>>>> +++ b/arch/arm64/kvm/handle_exit.c >>>>>> @@ -202,13 +202,10 @@ static int handle_trap_exceptions(struct kvm_vcpu *vcpu, struct kvm_run *run) >>>>>> handled = exit_handler(vcpu, run); >>>>>> } >>>>>> >>>>>> - if (handled && (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP)) { >>>>>> - handled = 0; >>>>>> - run->exit_reason = KVM_EXIT_DEBUG; >>>>>> - run->debug.arch.hsr = ESR_ELx_EC_SOFTSTP_LOW << ESR_ELx_EC_SHIFT; >>>>>> - } >>>>>> + if (handled) >>>>>> + return kvm_arm_maybe_return_debug(vcpu, run); >>>>>> >>>>>> - return handled; >>>>>> + return 0; >>>>>> } >>>>>> >>>>>> /* >>>>>> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c >>>>>> index b9f68e4add71..3d28fe2daa26 100644 >>>>>> --- a/virt/kvm/arm/arm.c >>>>>> +++ b/virt/kvm/arm/arm.c >>>>>> @@ -623,7 +623,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run) >>>>>> >>>>>> if (run->exit_reason == KVM_EXIT_MMIO) { >>>>>> ret = kvm_handle_mmio_return(vcpu, vcpu->run); >>>>>> - if (ret) >>>>>> + if (ret < 1) >>>>>> return ret; >>>>>> } >>>>>> >>>>>> diff --git a/virt/kvm/arm/mmio.c b/virt/kvm/arm/mmio.c >>>>>> index b6e715fd3c90..e43e3bd6222f 100644 >>>>>> --- a/virt/kvm/arm/mmio.c >>>>>> +++ b/virt/kvm/arm/mmio.c >>>>>> @@ -117,7 +117,8 @@ int kvm_handle_mmio_return(struct kvm_vcpu *vcpu, struct kvm_run *run) >>>>>> vcpu_set_reg(vcpu, vcpu->arch.mmio_decode.rt, data); >>>>>> } >>>>>> >>>>>> - return 0; >>>>>> + /* If debugging in effect we may need to return now */ >>>>>> + return kvm_arm_maybe_return_debug(vcpu, run); >>>>> >>>>> Ah, that's how you do it. OK. Then the patch splitting is wrong, because >>>>> everything is broken after patch #1. >>>> >>>> Actually, it is not broken at all. I'm just confused by the very >>>> esoteric flow. >>> >>> We could just merge the whole patch in one but I wanted to show the >>> difference between in-kernel and out-of-kernel emulation. >>> >>> I could also move the step handling to the mmio leg in >>> kvm_arch_vcpu_ioctl_run but you mentioned you use the mmio completion >>> elsewhere anyway? >> Yes, look at the end of io_mem_abort(). This is used by the vgic to >> complete a read emulation in the kernel. >> >> And actually, this means that we shouldn't have to mess with >> handle_exit. Just check for the return value of kvm_handle_mmio_return >> in the call sites (including the one in io_mem_abort), and exit if we >> need to single-step... >> > > I think we need to mess with handle_exit (or at least something else > than kvm_handle_mmio call sites) because the patches don't only fix MMIO > single stepping, but also other emulated stuff (system register > accesses, ...). Ah, true. I was too focussed on the the MMIO problem. > But with your suggestion maybe we can at least handle both MMIO cases in > a similar manner. I think we still need the code in handle_exit, or add > more code to deal case by case with other emulated instructions. Fair enough. M.
On Fri, Oct 06, 2017 at 12:39:21PM +0100, Alex Bennée wrote: > The system state of KVM when using userspace emulation is not complete > until we return into KVM_RUN. To handle mmio related updates we wait > until they have been committed and then schedule our KVM_EXIT_DEBUG. > > I've introduced a new function kvm_arm_maybe_return_debug() to wrap up > the differences between arm/arm64 which is currently null for arm. > > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > --- > arch/arm/include/asm/kvm_host.h | 2 ++ > arch/arm64/include/asm/kvm_host.h | 1 + > arch/arm64/kvm/debug.c | 21 +++++++++++++++++++++ > arch/arm64/kvm/handle_exit.c | 9 +++------ > virt/kvm/arm/arm.c | 2 +- > virt/kvm/arm/mmio.c | 3 ++- > 6 files changed, 30 insertions(+), 8 deletions(-) > > diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h > index 4a879f6ff13b..aec943f6d123 100644 > --- a/arch/arm/include/asm/kvm_host.h > +++ b/arch/arm/include/asm/kvm_host.h > @@ -285,6 +285,8 @@ static inline void kvm_arm_init_debug(void) {} > static inline void kvm_arm_setup_debug(struct kvm_vcpu *vcpu) {} > static inline void kvm_arm_clear_debug(struct kvm_vcpu *vcpu) {} > static inline void kvm_arm_reset_debug_ptr(struct kvm_vcpu *vcpu) {} > +static inline int kvm_arm_maybe_return_debug(struct kvm_vcpu *vcpu, > + struct kvm_run *run) {} > > int kvm_arm_vcpu_arch_set_attr(struct kvm_vcpu *vcpu, > struct kvm_device_attr *attr); > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > index e923b58606e2..fa67d21662f6 100644 > --- a/arch/arm64/include/asm/kvm_host.h > +++ b/arch/arm64/include/asm/kvm_host.h > @@ -369,6 +369,7 @@ void kvm_arm_init_debug(void); > void kvm_arm_setup_debug(struct kvm_vcpu *vcpu); > void kvm_arm_clear_debug(struct kvm_vcpu *vcpu); > void kvm_arm_reset_debug_ptr(struct kvm_vcpu *vcpu); > +int kvm_arm_maybe_return_debug(struct kvm_vcpu *vcpu, struct kvm_run *run); > int kvm_arm_vcpu_arch_set_attr(struct kvm_vcpu *vcpu, > struct kvm_device_attr *attr); > int kvm_arm_vcpu_arch_get_attr(struct kvm_vcpu *vcpu, > diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c > index dbadfaf850a7..a10a18c55c87 100644 > --- a/arch/arm64/kvm/debug.c > +++ b/arch/arm64/kvm/debug.c > @@ -221,3 +221,24 @@ void kvm_arm_clear_debug(struct kvm_vcpu *vcpu) > } > } > } > + > + > +/* > + * When KVM has successfully emulated the instruction we might want to > + * return we a KVM_EXIT_DEBUG. We can only do this once the emulation > + * is complete though so for userspace emulations we have to wait > + * until we have re-entered KVM. > + * > + * Return > 0 to return to guest, 0 (and set exit_reason) on proper > + * exit to userspace. > + */ > + > +int kvm_arm_maybe_return_debug(struct kvm_vcpu *vcpu, struct kvm_run *run) > +{ > + if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP) { > + run->exit_reason = KVM_EXIT_DEBUG; > + run->debug.arch.hsr = ESR_ELx_EC_SOFTSTP_LOW << ESR_ELx_EC_SHIFT; > + return 0; > + } > + return 1; > +} > diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c > index c918d291cb58..7b04f59217bf 100644 > --- a/arch/arm64/kvm/handle_exit.c > +++ b/arch/arm64/kvm/handle_exit.c > @@ -202,13 +202,10 @@ static int handle_trap_exceptions(struct kvm_vcpu *vcpu, struct kvm_run *run) > handled = exit_handler(vcpu, run); > } > > - if (handled && (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP)) { > - handled = 0; > - run->exit_reason = KVM_EXIT_DEBUG; > - run->debug.arch.hsr = ESR_ELx_EC_SOFTSTP_LOW << ESR_ELx_EC_SHIFT; > - } > + if (handled) > + return kvm_arm_maybe_return_debug(vcpu, run); Again, this seems to override the return value of exit_handler, which may be something negative. Just so I'm clear: There's no intended functionality change of this particular hunk, it's just to share the logic in kvm_arm_maybe_return_debug, right? > > - return handled; > + return 0; > } > > /* > diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c > index b9f68e4add71..3d28fe2daa26 100644 > --- a/virt/kvm/arm/arm.c > +++ b/virt/kvm/arm/arm.c > @@ -623,7 +623,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run) > > if (run->exit_reason == KVM_EXIT_MMIO) { > ret = kvm_handle_mmio_return(vcpu, vcpu->run); > - if (ret) > + if (ret < 1) > return ret; > } > > diff --git a/virt/kvm/arm/mmio.c b/virt/kvm/arm/mmio.c > index b6e715fd3c90..e43e3bd6222f 100644 > --- a/virt/kvm/arm/mmio.c > +++ b/virt/kvm/arm/mmio.c > @@ -117,7 +117,8 @@ int kvm_handle_mmio_return(struct kvm_vcpu *vcpu, struct kvm_run *run) > vcpu_set_reg(vcpu, vcpu->arch.mmio_decode.rt, data); > } > > - return 0; > + /* If debugging in effect we may need to return now */ Will this ever be about other types of debugging (watchpoint on a MMIO access?) or should we limit the text and description to single-stepping? > + return kvm_arm_maybe_return_debug(vcpu, run); > } > > static int decode_hsr(struct kvm_vcpu *vcpu, bool *is_write, int *len) > -- > 2.14.1 > Thanks, -Christoffer
On Fri, Oct 06, 2017 at 02:45:35PM +0100, Alex Bennée wrote: > > Julien Thierry <julien.thierry@arm.com> writes: > > > On 06/10/17 12:39, Alex Bennée wrote: > >> The system state of KVM when using userspace emulation is not complete > >> until we return into KVM_RUN. To handle mmio related updates we wait > >> until they have been committed and then schedule our KVM_EXIT_DEBUG. > >> > >> I've introduced a new function kvm_arm_maybe_return_debug() to wrap up > >> the differences between arm/arm64 which is currently null for arm. > >> > >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > >> --- > >> arch/arm/include/asm/kvm_host.h | 2 ++ > >> arch/arm64/include/asm/kvm_host.h | 1 + > >> arch/arm64/kvm/debug.c | 21 +++++++++++++++++++++ > >> arch/arm64/kvm/handle_exit.c | 9 +++------ > >> virt/kvm/arm/arm.c | 2 +- > >> virt/kvm/arm/mmio.c | 3 ++- > >> 6 files changed, 30 insertions(+), 8 deletions(-) > >> > >> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h > >> index 4a879f6ff13b..aec943f6d123 100644 > >> --- a/arch/arm/include/asm/kvm_host.h > >> +++ b/arch/arm/include/asm/kvm_host.h > >> @@ -285,6 +285,8 @@ static inline void kvm_arm_init_debug(void) {} > >> static inline void kvm_arm_setup_debug(struct kvm_vcpu *vcpu) {} > >> static inline void kvm_arm_clear_debug(struct kvm_vcpu *vcpu) {} > >> static inline void kvm_arm_reset_debug_ptr(struct kvm_vcpu *vcpu) {} > >> +static inline int kvm_arm_maybe_return_debug(struct kvm_vcpu *vcpu, > >> + struct kvm_run *run) {} > >> > > > > This function should return 1. > > So I did ponder making this a bool, returning true if we need to exit > and testing in v/k/a/arm.c exit leg rather than in the mmio handler. > > At the moment it mirrors the existing exit logic which follows -1 err, 0 > return, >0 handled. But as I mentioned in the cover letter this fell > down a bit when dealing with the mmio case. > > > > >> int kvm_arm_vcpu_arch_set_attr(struct kvm_vcpu *vcpu, > >> struct kvm_device_attr *attr); > >> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > >> index e923b58606e2..fa67d21662f6 100644 > >> --- a/arch/arm64/include/asm/kvm_host.h > >> +++ b/arch/arm64/include/asm/kvm_host.h > >> @@ -369,6 +369,7 @@ void kvm_arm_init_debug(void); > >> void kvm_arm_setup_debug(struct kvm_vcpu *vcpu); > >> void kvm_arm_clear_debug(struct kvm_vcpu *vcpu); > >> void kvm_arm_reset_debug_ptr(struct kvm_vcpu *vcpu); > >> +int kvm_arm_maybe_return_debug(struct kvm_vcpu *vcpu, struct kvm_run *run); > > > > I feel the name could be a little bit more explicit: > > > > kvm_arm_trap_need_step_debug, kvm_arm_trap_step_return_debug, > > kvm_arm_trap_need_return_debug. > > I wanted to keep the debug suffix so that's fine although I'm not so > sure trap is correct because on the tail end of mmio emulation are we > still trapping? > > Maybe kvm_arm_step_emulated_debug? I think you should name it: kvm_arm_should_complete_emulated_instr_debug() - or something better - and call it directly from kvm_arch_vcpu_ioctl_run, so that it becomes: ret = kvm_handle_mmio_return(vcpu, vcpu->run); if (ret) return ret; ret = kvm_arm_should_complete_emulated_instr_debug(vcpu); if (ret) return ret; > > > At least, I think it would be nice that the name reflect that this > > check is meant for emulated instructions. > > > > Otherwise: > > > > Reviewed-by: Julien Thierry <julien.thierry@arm.com> > > > > Thanks, > > Thanks, -Christoffer
Christoffer Dall <cdall@linaro.org> writes: > On Fri, Oct 06, 2017 at 02:45:35PM +0100, Alex Bennée wrote: >> >> Julien Thierry <julien.thierry@arm.com> writes: >> >> > On 06/10/17 12:39, Alex Bennée wrote: >> >> The system state of KVM when using userspace emulation is not complete >> >> until we return into KVM_RUN. To handle mmio related updates we wait >> >> until they have been committed and then schedule our KVM_EXIT_DEBUG. >> >> >> >> I've introduced a new function kvm_arm_maybe_return_debug() to wrap up >> >> the differences between arm/arm64 which is currently null for arm. >> >> >> >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> >> >> --- >> >> arch/arm/include/asm/kvm_host.h | 2 ++ >> >> arch/arm64/include/asm/kvm_host.h | 1 + >> >> arch/arm64/kvm/debug.c | 21 +++++++++++++++++++++ >> >> arch/arm64/kvm/handle_exit.c | 9 +++------ >> >> virt/kvm/arm/arm.c | 2 +- >> >> virt/kvm/arm/mmio.c | 3 ++- >> >> 6 files changed, 30 insertions(+), 8 deletions(-) >> >> >> >> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h >> >> index 4a879f6ff13b..aec943f6d123 100644 >> >> --- a/arch/arm/include/asm/kvm_host.h >> >> +++ b/arch/arm/include/asm/kvm_host.h >> >> @@ -285,6 +285,8 @@ static inline void kvm_arm_init_debug(void) {} >> >> static inline void kvm_arm_setup_debug(struct kvm_vcpu *vcpu) {} >> >> static inline void kvm_arm_clear_debug(struct kvm_vcpu *vcpu) {} >> >> static inline void kvm_arm_reset_debug_ptr(struct kvm_vcpu *vcpu) {} >> >> +static inline int kvm_arm_maybe_return_debug(struct kvm_vcpu *vcpu, >> >> + struct kvm_run *run) {} >> >> >> > >> > This function should return 1. >> >> So I did ponder making this a bool, returning true if we need to exit >> and testing in v/k/a/arm.c exit leg rather than in the mmio handler. >> >> At the moment it mirrors the existing exit logic which follows -1 err, 0 >> return, >0 handled. But as I mentioned in the cover letter this fell >> down a bit when dealing with the mmio case. >> >> > >> >> int kvm_arm_vcpu_arch_set_attr(struct kvm_vcpu *vcpu, >> >> struct kvm_device_attr *attr); >> >> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h >> >> index e923b58606e2..fa67d21662f6 100644 >> >> --- a/arch/arm64/include/asm/kvm_host.h >> >> +++ b/arch/arm64/include/asm/kvm_host.h >> >> @@ -369,6 +369,7 @@ void kvm_arm_init_debug(void); >> >> void kvm_arm_setup_debug(struct kvm_vcpu *vcpu); >> >> void kvm_arm_clear_debug(struct kvm_vcpu *vcpu); >> >> void kvm_arm_reset_debug_ptr(struct kvm_vcpu *vcpu); >> >> +int kvm_arm_maybe_return_debug(struct kvm_vcpu *vcpu, struct kvm_run *run); >> > >> > I feel the name could be a little bit more explicit: >> > >> > kvm_arm_trap_need_step_debug, kvm_arm_trap_step_return_debug, >> > kvm_arm_trap_need_return_debug. >> >> I wanted to keep the debug suffix so that's fine although I'm not so >> sure trap is correct because on the tail end of mmio emulation are we >> still trapping? >> >> Maybe kvm_arm_step_emulated_debug? > > I think you should name it: > > kvm_arm_should_complete_emulated_instr_debug() - or something better - Naming is hard :-/ > and call it directly from kvm_arch_vcpu_ioctl_run, so that it becomes: > > ret = kvm_handle_mmio_return(vcpu, vcpu->run); > if (ret) > return ret; > ret = kvm_arm_should_complete_emulated_instr_debug(vcpu); > if (ret) > return ret; This runs into the problem of slightly different ret semantics for here and in handle_exit. Maybe just having a bool response and: if (kvm_arm_should_complete_emulated_instr_debug(vcpu)) return 0; And then in handle_exit: if (handled == 1 && kvm_arm_should_complete_emulated_instr_debug(vcpu)) return 0; else return handled; ? > >> >> > At least, I think it would be nice that the name reflect that this >> > check is meant for emulated instructions. >> > >> > Otherwise: >> > >> > Reviewed-by: Julien Thierry <julien.thierry@arm.com> >> > >> > Thanks, >> >> > Thanks, > -Christoffer -- Alex Bennée
Christoffer Dall <cdall@linaro.org> writes: > On Fri, Oct 06, 2017 at 12:39:21PM +0100, Alex Bennée wrote: >> The system state of KVM when using userspace emulation is not complete >> until we return into KVM_RUN. To handle mmio related updates we wait >> until they have been committed and then schedule our KVM_EXIT_DEBUG. >> >> I've introduced a new function kvm_arm_maybe_return_debug() to wrap up >> the differences between arm/arm64 which is currently null for arm. >> >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> >> --- >> arch/arm/include/asm/kvm_host.h | 2 ++ >> arch/arm64/include/asm/kvm_host.h | 1 + >> arch/arm64/kvm/debug.c | 21 +++++++++++++++++++++ >> arch/arm64/kvm/handle_exit.c | 9 +++------ >> virt/kvm/arm/arm.c | 2 +- >> virt/kvm/arm/mmio.c | 3 ++- >> 6 files changed, 30 insertions(+), 8 deletions(-) >> >> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h >> index 4a879f6ff13b..aec943f6d123 100644 >> --- a/arch/arm/include/asm/kvm_host.h >> +++ b/arch/arm/include/asm/kvm_host.h >> @@ -285,6 +285,8 @@ static inline void kvm_arm_init_debug(void) {} >> static inline void kvm_arm_setup_debug(struct kvm_vcpu *vcpu) {} >> static inline void kvm_arm_clear_debug(struct kvm_vcpu *vcpu) {} >> static inline void kvm_arm_reset_debug_ptr(struct kvm_vcpu *vcpu) {} >> +static inline int kvm_arm_maybe_return_debug(struct kvm_vcpu *vcpu, >> + struct kvm_run *run) {} >> >> int kvm_arm_vcpu_arch_set_attr(struct kvm_vcpu *vcpu, >> struct kvm_device_attr *attr); >> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h >> index e923b58606e2..fa67d21662f6 100644 >> --- a/arch/arm64/include/asm/kvm_host.h >> +++ b/arch/arm64/include/asm/kvm_host.h >> @@ -369,6 +369,7 @@ void kvm_arm_init_debug(void); >> void kvm_arm_setup_debug(struct kvm_vcpu *vcpu); >> void kvm_arm_clear_debug(struct kvm_vcpu *vcpu); >> void kvm_arm_reset_debug_ptr(struct kvm_vcpu *vcpu); >> +int kvm_arm_maybe_return_debug(struct kvm_vcpu *vcpu, struct kvm_run *run); >> int kvm_arm_vcpu_arch_set_attr(struct kvm_vcpu *vcpu, >> struct kvm_device_attr *attr); >> int kvm_arm_vcpu_arch_get_attr(struct kvm_vcpu *vcpu, >> diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c >> index dbadfaf850a7..a10a18c55c87 100644 >> --- a/arch/arm64/kvm/debug.c >> +++ b/arch/arm64/kvm/debug.c >> @@ -221,3 +221,24 @@ void kvm_arm_clear_debug(struct kvm_vcpu *vcpu) >> } >> } >> } >> + >> + >> +/* >> + * When KVM has successfully emulated the instruction we might want to >> + * return we a KVM_EXIT_DEBUG. We can only do this once the emulation >> + * is complete though so for userspace emulations we have to wait >> + * until we have re-entered KVM. >> + * >> + * Return > 0 to return to guest, 0 (and set exit_reason) on proper >> + * exit to userspace. >> + */ >> + >> +int kvm_arm_maybe_return_debug(struct kvm_vcpu *vcpu, struct kvm_run *run) >> +{ >> + if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP) { >> + run->exit_reason = KVM_EXIT_DEBUG; >> + run->debug.arch.hsr = ESR_ELx_EC_SOFTSTP_LOW << ESR_ELx_EC_SHIFT; >> + return 0; >> + } >> + return 1; >> +} >> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c >> index c918d291cb58..7b04f59217bf 100644 >> --- a/arch/arm64/kvm/handle_exit.c >> +++ b/arch/arm64/kvm/handle_exit.c >> @@ -202,13 +202,10 @@ static int handle_trap_exceptions(struct kvm_vcpu *vcpu, struct kvm_run *run) >> handled = exit_handler(vcpu, run); >> } >> >> - if (handled && (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP)) { >> - handled = 0; >> - run->exit_reason = KVM_EXIT_DEBUG; >> - run->debug.arch.hsr = ESR_ELx_EC_SOFTSTP_LOW << ESR_ELx_EC_SHIFT; >> - } >> + if (handled) >> + return kvm_arm_maybe_return_debug(vcpu, run); > > Again, this seems to override the return value of exit_handler, which > may be something negative. > > Just so I'm clear: There's no intended functionality change of this > particular hunk, it's just to share the logic in > kvm_arm_maybe_return_debug, right? Yes, modulo the annoying semantics in the two places of the vcpu run ioctl loop. > >> >> - return handled; >> + return 0; >> } >> >> /* >> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c >> index b9f68e4add71..3d28fe2daa26 100644 >> --- a/virt/kvm/arm/arm.c >> +++ b/virt/kvm/arm/arm.c >> @@ -623,7 +623,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run) >> >> if (run->exit_reason == KVM_EXIT_MMIO) { >> ret = kvm_handle_mmio_return(vcpu, vcpu->run); >> - if (ret) >> + if (ret < 1) >> return ret; >> } >> >> diff --git a/virt/kvm/arm/mmio.c b/virt/kvm/arm/mmio.c >> index b6e715fd3c90..e43e3bd6222f 100644 >> --- a/virt/kvm/arm/mmio.c >> +++ b/virt/kvm/arm/mmio.c >> @@ -117,7 +117,8 @@ int kvm_handle_mmio_return(struct kvm_vcpu *vcpu, struct kvm_run *run) >> vcpu_set_reg(vcpu, vcpu->arch.mmio_decode.rt, data); >> } >> >> - return 0; >> + /* If debugging in effect we may need to return now */ > > Will this ever be about other types of debugging (watchpoint on a MMIO > access?) or should we limit the text and description to > single-stepping? Hmm I don't think so. A hbreak should hit (via normal exception path) before we attempt any emulation. I suspect watchpoints wouldn't hit for emulation though - that would be trickier to do nicely though as it would need to be checked for in both kernel and userspace emulation. > >> + return kvm_arm_maybe_return_debug(vcpu, run); >> } >> >> static int decode_hsr(struct kvm_vcpu *vcpu, bool *is_write, int *len) >> -- >> 2.14.1 >> > > Thanks, > -Christoffer -- Alex Bennée
On Fri, Oct 13, 2017 at 10:23:21AM +0100, Alex Bennée wrote: > > Christoffer Dall <cdall@linaro.org> writes: > > > On Fri, Oct 06, 2017 at 02:45:35PM +0100, Alex Bennée wrote: > >> > >> Julien Thierry <julien.thierry@arm.com> writes: > >> > >> > On 06/10/17 12:39, Alex Bennée wrote: > >> >> The system state of KVM when using userspace emulation is not complete > >> >> until we return into KVM_RUN. To handle mmio related updates we wait > >> >> until they have been committed and then schedule our KVM_EXIT_DEBUG. > >> >> > >> >> I've introduced a new function kvm_arm_maybe_return_debug() to wrap up > >> >> the differences between arm/arm64 which is currently null for arm. > >> >> > >> >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > >> >> --- > >> >> arch/arm/include/asm/kvm_host.h | 2 ++ > >> >> arch/arm64/include/asm/kvm_host.h | 1 + > >> >> arch/arm64/kvm/debug.c | 21 +++++++++++++++++++++ > >> >> arch/arm64/kvm/handle_exit.c | 9 +++------ > >> >> virt/kvm/arm/arm.c | 2 +- > >> >> virt/kvm/arm/mmio.c | 3 ++- > >> >> 6 files changed, 30 insertions(+), 8 deletions(-) > >> >> > >> >> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h > >> >> index 4a879f6ff13b..aec943f6d123 100644 > >> >> --- a/arch/arm/include/asm/kvm_host.h > >> >> +++ b/arch/arm/include/asm/kvm_host.h > >> >> @@ -285,6 +285,8 @@ static inline void kvm_arm_init_debug(void) {} > >> >> static inline void kvm_arm_setup_debug(struct kvm_vcpu *vcpu) {} > >> >> static inline void kvm_arm_clear_debug(struct kvm_vcpu *vcpu) {} > >> >> static inline void kvm_arm_reset_debug_ptr(struct kvm_vcpu *vcpu) {} > >> >> +static inline int kvm_arm_maybe_return_debug(struct kvm_vcpu *vcpu, > >> >> + struct kvm_run *run) {} > >> >> > >> > > >> > This function should return 1. > >> > >> So I did ponder making this a bool, returning true if we need to exit > >> and testing in v/k/a/arm.c exit leg rather than in the mmio handler. > >> > >> At the moment it mirrors the existing exit logic which follows -1 err, 0 > >> return, >0 handled. But as I mentioned in the cover letter this fell > >> down a bit when dealing with the mmio case. > >> > >> > > >> >> int kvm_arm_vcpu_arch_set_attr(struct kvm_vcpu *vcpu, > >> >> struct kvm_device_attr *attr); > >> >> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > >> >> index e923b58606e2..fa67d21662f6 100644 > >> >> --- a/arch/arm64/include/asm/kvm_host.h > >> >> +++ b/arch/arm64/include/asm/kvm_host.h > >> >> @@ -369,6 +369,7 @@ void kvm_arm_init_debug(void); > >> >> void kvm_arm_setup_debug(struct kvm_vcpu *vcpu); > >> >> void kvm_arm_clear_debug(struct kvm_vcpu *vcpu); > >> >> void kvm_arm_reset_debug_ptr(struct kvm_vcpu *vcpu); > >> >> +int kvm_arm_maybe_return_debug(struct kvm_vcpu *vcpu, struct kvm_run *run); > >> > > >> > I feel the name could be a little bit more explicit: > >> > > >> > kvm_arm_trap_need_step_debug, kvm_arm_trap_step_return_debug, > >> > kvm_arm_trap_need_return_debug. > >> > >> I wanted to keep the debug suffix so that's fine although I'm not so > >> sure trap is correct because on the tail end of mmio emulation are we > >> still trapping? > >> > >> Maybe kvm_arm_step_emulated_debug? > > > > I think you should name it: > > > > kvm_arm_should_complete_emulated_instr_debug() - or something better - > > Naming is hard :-/ > Yeah, my suggestion was half-way a joke, but as Julien said, it should reflect what it's trying to tell you as concretely as possible. > > and call it directly from kvm_arch_vcpu_ioctl_run, so that it becomes: > > > > ret = kvm_handle_mmio_return(vcpu, vcpu->run); > > if (ret) > > return ret; > > ret = kvm_arm_should_complete_emulated_instr_debug(vcpu); > > if (ret) > > return ret; > > This runs into the problem of slightly different ret semantics for here > and in handle_exit. Maybe just having a bool response and: > > if (kvm_arm_should_complete_emulated_instr_debug(vcpu)) > return 0; Ah yeah, my example was broken. > > And then in handle_exit: > > if (handled == 1 && kvm_arm_should_complete_emulated_instr_debug(vcpu)) > return 0; > else > return handled; > Yes, looks good, assuming we can find a better name. Thanks, -Christoffer
On Fri, Oct 13, 2017 at 10:27:36AM +0100, Alex Bennée wrote: > > Christoffer Dall <cdall@linaro.org> writes: > > > On Fri, Oct 06, 2017 at 12:39:21PM +0100, Alex Bennée wrote: > >> The system state of KVM when using userspace emulation is not complete > >> until we return into KVM_RUN. To handle mmio related updates we wait > >> until they have been committed and then schedule our KVM_EXIT_DEBUG. > >> > >> I've introduced a new function kvm_arm_maybe_return_debug() to wrap up > >> the differences between arm/arm64 which is currently null for arm. > >> > >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > >> --- > >> arch/arm/include/asm/kvm_host.h | 2 ++ > >> arch/arm64/include/asm/kvm_host.h | 1 + > >> arch/arm64/kvm/debug.c | 21 +++++++++++++++++++++ > >> arch/arm64/kvm/handle_exit.c | 9 +++------ > >> virt/kvm/arm/arm.c | 2 +- > >> virt/kvm/arm/mmio.c | 3 ++- > >> 6 files changed, 30 insertions(+), 8 deletions(-) > >> > >> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h > >> index 4a879f6ff13b..aec943f6d123 100644 > >> --- a/arch/arm/include/asm/kvm_host.h > >> +++ b/arch/arm/include/asm/kvm_host.h > >> @@ -285,6 +285,8 @@ static inline void kvm_arm_init_debug(void) {} > >> static inline void kvm_arm_setup_debug(struct kvm_vcpu *vcpu) {} > >> static inline void kvm_arm_clear_debug(struct kvm_vcpu *vcpu) {} > >> static inline void kvm_arm_reset_debug_ptr(struct kvm_vcpu *vcpu) {} > >> +static inline int kvm_arm_maybe_return_debug(struct kvm_vcpu *vcpu, > >> + struct kvm_run *run) {} > >> > >> int kvm_arm_vcpu_arch_set_attr(struct kvm_vcpu *vcpu, > >> struct kvm_device_attr *attr); > >> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > >> index e923b58606e2..fa67d21662f6 100644 > >> --- a/arch/arm64/include/asm/kvm_host.h > >> +++ b/arch/arm64/include/asm/kvm_host.h > >> @@ -369,6 +369,7 @@ void kvm_arm_init_debug(void); > >> void kvm_arm_setup_debug(struct kvm_vcpu *vcpu); > >> void kvm_arm_clear_debug(struct kvm_vcpu *vcpu); > >> void kvm_arm_reset_debug_ptr(struct kvm_vcpu *vcpu); > >> +int kvm_arm_maybe_return_debug(struct kvm_vcpu *vcpu, struct kvm_run *run); > >> int kvm_arm_vcpu_arch_set_attr(struct kvm_vcpu *vcpu, > >> struct kvm_device_attr *attr); > >> int kvm_arm_vcpu_arch_get_attr(struct kvm_vcpu *vcpu, > >> diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c > >> index dbadfaf850a7..a10a18c55c87 100644 > >> --- a/arch/arm64/kvm/debug.c > >> +++ b/arch/arm64/kvm/debug.c > >> @@ -221,3 +221,24 @@ void kvm_arm_clear_debug(struct kvm_vcpu *vcpu) > >> } > >> } > >> } > >> + > >> + > >> +/* > >> + * When KVM has successfully emulated the instruction we might want to > >> + * return we a KVM_EXIT_DEBUG. We can only do this once the emulation > >> + * is complete though so for userspace emulations we have to wait > >> + * until we have re-entered KVM. > >> + * > >> + * Return > 0 to return to guest, 0 (and set exit_reason) on proper > >> + * exit to userspace. > >> + */ > >> + > >> +int kvm_arm_maybe_return_debug(struct kvm_vcpu *vcpu, struct kvm_run *run) > >> +{ > >> + if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP) { > >> + run->exit_reason = KVM_EXIT_DEBUG; > >> + run->debug.arch.hsr = ESR_ELx_EC_SOFTSTP_LOW << ESR_ELx_EC_SHIFT; > >> + return 0; > >> + } > >> + return 1; > >> +} > >> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c > >> index c918d291cb58..7b04f59217bf 100644 > >> --- a/arch/arm64/kvm/handle_exit.c > >> +++ b/arch/arm64/kvm/handle_exit.c > >> @@ -202,13 +202,10 @@ static int handle_trap_exceptions(struct kvm_vcpu *vcpu, struct kvm_run *run) > >> handled = exit_handler(vcpu, run); > >> } > >> > >> - if (handled && (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP)) { > >> - handled = 0; > >> - run->exit_reason = KVM_EXIT_DEBUG; > >> - run->debug.arch.hsr = ESR_ELx_EC_SOFTSTP_LOW << ESR_ELx_EC_SHIFT; > >> - } > >> + if (handled) > >> + return kvm_arm_maybe_return_debug(vcpu, run); > > > > Again, this seems to override the return value of exit_handler, which > > may be something negative. > > > > Just so I'm clear: There's no intended functionality change of this > > particular hunk, it's just to share the logic in > > kvm_arm_maybe_return_debug, right? > > Yes, modulo the annoying semantics in the two places of the vcpu run > ioctl loop. > > > > >> > >> - return handled; > >> + return 0; > >> } > >> > >> /* > >> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c > >> index b9f68e4add71..3d28fe2daa26 100644 > >> --- a/virt/kvm/arm/arm.c > >> +++ b/virt/kvm/arm/arm.c > >> @@ -623,7 +623,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run) > >> > >> if (run->exit_reason == KVM_EXIT_MMIO) { > >> ret = kvm_handle_mmio_return(vcpu, vcpu->run); > >> - if (ret) > >> + if (ret < 1) > >> return ret; > >> } > >> > >> diff --git a/virt/kvm/arm/mmio.c b/virt/kvm/arm/mmio.c > >> index b6e715fd3c90..e43e3bd6222f 100644 > >> --- a/virt/kvm/arm/mmio.c > >> +++ b/virt/kvm/arm/mmio.c > >> @@ -117,7 +117,8 @@ int kvm_handle_mmio_return(struct kvm_vcpu *vcpu, struct kvm_run *run) > >> vcpu_set_reg(vcpu, vcpu->arch.mmio_decode.rt, data); > >> } > >> > >> - return 0; > >> + /* If debugging in effect we may need to return now */ > > > > Will this ever be about other types of debugging (watchpoint on a MMIO > > access?) or should we limit the text and description to > > single-stepping? > > Hmm I don't think so. A hbreak should hit (via normal exception path) > before we attempt any emulation. I suspect watchpoints wouldn't hit for > emulation though - that would be trickier to do nicely though as it > would need to be checked for in both kernel and userspace emulation. > > > Then I think we should be specific in function naming and comments and refer to single-stepping as opposed to something more generic, because single-stepping seems to be the case we care about. Thanks, -Christoffer
diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h index 4a879f6ff13b..aec943f6d123 100644 --- a/arch/arm/include/asm/kvm_host.h +++ b/arch/arm/include/asm/kvm_host.h @@ -285,6 +285,8 @@ static inline void kvm_arm_init_debug(void) {} static inline void kvm_arm_setup_debug(struct kvm_vcpu *vcpu) {} static inline void kvm_arm_clear_debug(struct kvm_vcpu *vcpu) {} static inline void kvm_arm_reset_debug_ptr(struct kvm_vcpu *vcpu) {} +static inline int kvm_arm_maybe_return_debug(struct kvm_vcpu *vcpu, + struct kvm_run *run) {} int kvm_arm_vcpu_arch_set_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr); diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h index e923b58606e2..fa67d21662f6 100644 --- a/arch/arm64/include/asm/kvm_host.h +++ b/arch/arm64/include/asm/kvm_host.h @@ -369,6 +369,7 @@ void kvm_arm_init_debug(void); void kvm_arm_setup_debug(struct kvm_vcpu *vcpu); void kvm_arm_clear_debug(struct kvm_vcpu *vcpu); void kvm_arm_reset_debug_ptr(struct kvm_vcpu *vcpu); +int kvm_arm_maybe_return_debug(struct kvm_vcpu *vcpu, struct kvm_run *run); int kvm_arm_vcpu_arch_set_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr); int kvm_arm_vcpu_arch_get_attr(struct kvm_vcpu *vcpu, diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c index dbadfaf850a7..a10a18c55c87 100644 --- a/arch/arm64/kvm/debug.c +++ b/arch/arm64/kvm/debug.c @@ -221,3 +221,24 @@ void kvm_arm_clear_debug(struct kvm_vcpu *vcpu) } } } + + +/* + * When KVM has successfully emulated the instruction we might want to + * return we a KVM_EXIT_DEBUG. We can only do this once the emulation + * is complete though so for userspace emulations we have to wait + * until we have re-entered KVM. + * + * Return > 0 to return to guest, 0 (and set exit_reason) on proper + * exit to userspace. + */ + +int kvm_arm_maybe_return_debug(struct kvm_vcpu *vcpu, struct kvm_run *run) +{ + if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP) { + run->exit_reason = KVM_EXIT_DEBUG; + run->debug.arch.hsr = ESR_ELx_EC_SOFTSTP_LOW << ESR_ELx_EC_SHIFT; + return 0; + } + return 1; +} diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c index c918d291cb58..7b04f59217bf 100644 --- a/arch/arm64/kvm/handle_exit.c +++ b/arch/arm64/kvm/handle_exit.c @@ -202,13 +202,10 @@ static int handle_trap_exceptions(struct kvm_vcpu *vcpu, struct kvm_run *run) handled = exit_handler(vcpu, run); } - if (handled && (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP)) { - handled = 0; - run->exit_reason = KVM_EXIT_DEBUG; - run->debug.arch.hsr = ESR_ELx_EC_SOFTSTP_LOW << ESR_ELx_EC_SHIFT; - } + if (handled) + return kvm_arm_maybe_return_debug(vcpu, run); - return handled; + return 0; } /* diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c index b9f68e4add71..3d28fe2daa26 100644 --- a/virt/kvm/arm/arm.c +++ b/virt/kvm/arm/arm.c @@ -623,7 +623,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run) if (run->exit_reason == KVM_EXIT_MMIO) { ret = kvm_handle_mmio_return(vcpu, vcpu->run); - if (ret) + if (ret < 1) return ret; } diff --git a/virt/kvm/arm/mmio.c b/virt/kvm/arm/mmio.c index b6e715fd3c90..e43e3bd6222f 100644 --- a/virt/kvm/arm/mmio.c +++ b/virt/kvm/arm/mmio.c @@ -117,7 +117,8 @@ int kvm_handle_mmio_return(struct kvm_vcpu *vcpu, struct kvm_run *run) vcpu_set_reg(vcpu, vcpu->arch.mmio_decode.rt, data); } - return 0; + /* If debugging in effect we may need to return now */ + return kvm_arm_maybe_return_debug(vcpu, run); } static int decode_hsr(struct kvm_vcpu *vcpu, bool *is_write, int *len)
The system state of KVM when using userspace emulation is not complete until we return into KVM_RUN. To handle mmio related updates we wait until they have been committed and then schedule our KVM_EXIT_DEBUG. I've introduced a new function kvm_arm_maybe_return_debug() to wrap up the differences between arm/arm64 which is currently null for arm. Signed-off-by: Alex Bennée <alex.bennee@linaro.org> --- arch/arm/include/asm/kvm_host.h | 2 ++ arch/arm64/include/asm/kvm_host.h | 1 + arch/arm64/kvm/debug.c | 21 +++++++++++++++++++++ arch/arm64/kvm/handle_exit.c | 9 +++------ virt/kvm/arm/arm.c | 2 +- virt/kvm/arm/mmio.c | 3 ++- 6 files changed, 30 insertions(+), 8 deletions(-)