| Message ID | 20190529222219.27994-1-farosas@linux.ibm.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v2] KVM: PPC: Report single stepping capability | expand |
On Wed, May 29, 2019 at 07:22:19PM -0300, Fabiano Rosas wrote: > When calling the KVM_SET_GUEST_DEBUG ioctl, userspace might request > the next instruction to be single stepped via the > KVM_GUESTDBG_SINGLESTEP control bit of the kvm_guest_debug structure. > > We currently don't have support for guest single stepping implemented > in Book3S HV. > > This patch adds the KVM_CAP_PPC_GUEST_DEBUG_SSTEP capability in order > to inform userspace about the state of single stepping support. Comment/question below: > --- a/arch/powerpc/kvm/powerpc.c > +++ b/arch/powerpc/kvm/powerpc.c > @@ -538,6 +538,11 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext) > case KVM_CAP_IMMEDIATE_EXIT: > r = 1; > break; > + case KVM_CAP_PPC_GUEST_DEBUG_SSTEP: > +#ifdef CONFIG_BOOKE > + r = 1; > + break; > +#endif In the !CONFIG_BOOKE case, this will fall through to code which will return 0 for HV KVM or 1 for PR KVM. Is that what was intended? If so, then why do we need the CONFIG_BOOKE case? Isn't hv_enabled always 0 on Book E? In any case, I think this needs at least a /* fall through */ comment in the code, and something explicit in the patch description to say that we intend to return 1 on PR KVM. Paul.
Paul Mackerras <paulus@ozlabs.org> writes: > On Wed, May 29, 2019 at 07:22:19PM -0300, Fabiano Rosas wrote: >> When calling the KVM_SET_GUEST_DEBUG ioctl, userspace might request >> the next instruction to be single stepped via the >> KVM_GUESTDBG_SINGLESTEP control bit of the kvm_guest_debug structure. >> >> We currently don't have support for guest single stepping implemented >> in Book3S HV. >> >> This patch adds the KVM_CAP_PPC_GUEST_DEBUG_SSTEP capability in order >> to inform userspace about the state of single stepping support. > > Comment/question below: > >> --- a/arch/powerpc/kvm/powerpc.c >> +++ b/arch/powerpc/kvm/powerpc.c >> @@ -538,6 +538,11 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext) >> case KVM_CAP_IMMEDIATE_EXIT: >> r = 1; >> break; >> + case KVM_CAP_PPC_GUEST_DEBUG_SSTEP: >> +#ifdef CONFIG_BOOKE >> + r = 1; >> + break; >> +#endif > > In the !CONFIG_BOOKE case, this will fall through to code which will > return 0 for HV KVM or 1 for PR KVM. Is that what was intended? Yes. The intention is to return 0 for HV and 1 for everything else. > If so, then why do we need the CONFIG_BOOKE case? Isn't hv_enabled > always 0 on Book E? Good point. I made a mistake there indeed. > In any case, I think this needs at least a /* fall through */ comment > in the code, and something explicit in the patch description to say > that we intend to return 1 on PR KVM. I'll send another version. Thanks
diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt index ba6c42c576dd..a77643bfa917 100644 --- a/Documentation/virtual/kvm/api.txt +++ b/Documentation/virtual/kvm/api.txt @@ -2969,6 +2969,9 @@ can be determined by querying the KVM_CAP_GUEST_DEBUG_HW_BPS and KVM_CAP_GUEST_DEBUG_HW_WPS capabilities which return a positive number indicating the number of supported registers. +For ppc, the KVM_CAP_PPC_GUEST_DEBUG_SSTEP capability indicates whether +the single-step debug event (KVM_GUESTDBG_SINGLESTEP) is supported. + When debug events exit the main run loop with the reason KVM_EXIT_DEBUG with the kvm_debug_exit_arch part of the kvm_run structure containing architecture specific debug information. diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c index 3393b166817a..fd7e7d55637e 100644 --- a/arch/powerpc/kvm/powerpc.c +++ b/arch/powerpc/kvm/powerpc.c @@ -538,6 +538,11 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext) case KVM_CAP_IMMEDIATE_EXIT: r = 1; break; + case KVM_CAP_PPC_GUEST_DEBUG_SSTEP: +#ifdef CONFIG_BOOKE + r = 1; + break; +#endif case KVM_CAP_PPC_PAIRED_SINGLES: case KVM_CAP_PPC_OSI: case KVM_CAP_PPC_GET_PVINFO: diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h index 2fe12b40d503..cad9fcd90f39 100644 --- a/include/uapi/linux/kvm.h +++ b/include/uapi/linux/kvm.h @@ -993,6 +993,7 @@ struct kvm_ppc_resize_hpt { #define KVM_CAP_ARM_SVE 170 #define KVM_CAP_ARM_PTRAUTH_ADDRESS 171 #define KVM_CAP_ARM_PTRAUTH_GENERIC 172 +#define KVM_CAP_PPC_GUEST_DEBUG_SSTEP 173 #ifdef KVM_CAP_IRQ_ROUTING
When calling the KVM_SET_GUEST_DEBUG ioctl, userspace might request the next instruction to be single stepped via the KVM_GUESTDBG_SINGLESTEP control bit of the kvm_guest_debug structure. We currently don't have support for guest single stepping implemented in Book3S HV. This patch adds the KVM_CAP_PPC_GUEST_DEBUG_SSTEP capability in order to inform userspace about the state of single stepping support. Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com> --- v1 -> v2: - add capability description to Documentation/virtual/kvm/api.txt Documentation/virtual/kvm/api.txt | 3 +++ arch/powerpc/kvm/powerpc.c | 5 +++++ include/uapi/linux/kvm.h | 1 + 3 files changed, 9 insertions(+)