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

[Fabiano Rosas <farosas@linux.ibm.com>]

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