[PATCH v2] KVM: PPC: Report single stepping capability

[PATCH v2] KVM: PPC: Report single stepping capability

[Fabiano Rosas]

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(+)

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
 
-- 
2.20.1

Re: [PATCH v2] KVM: PPC: Report single stepping capability

[Paul Mackerras]

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.

Re: [PATCH v2] KVM: PPC: Report single stepping capability

[Fabiano Rosas]

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