Re: [PATCH v7 09/45] kvm: arm64: Expose debug HW register numbers for Realm

[Steven Price <steven.price@arm.com>]

Hi Gavin,

On 03/03/2025 04:48, Gavin Shan wrote:
> On 2/14/25 2:13 AM, Steven Price wrote:
>> From: Suzuki K Poulose <suzuki.poulose@arm.com>
>>
>> Expose VM specific Debug HW register numbers.

Looking at this now, this patch description is garbage. Probably the
patch has changed over time - so I suspect it's my fault not Suzuki's.
We're not exposing anything new here. This is purely about telling the
VMM that a realm cannot (currently) be debugged. Something like the
below would be more accurate:

"""
kvm: arm64: Don't expose debug capabilities for realm guests

RMM v1.0 provides no mechanism for the host to perform debug operations
on the guest. So don't expose KVM_CAP_SET_GUEST_DEBUG and report 0
breakpoints and 0 watch points.
"""

>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>> Signed-off-by: Steven Price <steven.price@arm.com>
>> ---
>>   arch/arm64/kvm/arm.c | 24 +++++++++++++++++++++---
>>   1 file changed, 21 insertions(+), 3 deletions(-)
>>
> 
> Documentation/virt/kvm/api.rst needs to be updated accordingly.

I don't think (with the above clarification) there's anything to update
in the API documentation. There's nothing new being added, just
capabilities being hidden where the functionality isn't available.

And eventually we hope to add support for this (in a later RMM spec) - I
don't yet know exactly what form this will take, but I hope to keep the
interfaces as close as possible to what we already have so that existing
tooling can be used.

Thanks,
Steve

>> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
>> index b8fa82be251c..df6eb5e9ca96 100644
>> --- a/arch/arm64/kvm/arm.c
>> +++ b/arch/arm64/kvm/arm.c
>> @@ -78,6 +78,22 @@ bool is_kvm_arm_initialised(void)
>>       return kvm_arm_initialised;
>>   }
>>   +static u32 kvm_arm_get_num_brps(struct kvm *kvm)
>> +{
>> +    if (!kvm_is_realm(kvm))
>> +        return get_num_brps();
>> +    /* Realm guest is not debuggable. */
>> +    return 0;
>> +}
>> +
>> +static u32 kvm_arm_get_num_wrps(struct kvm *kvm)
>> +{
>> +    if (!kvm_is_realm(kvm))
>> +        return get_num_wrps();
>> +    /* Realm guest is not debuggable. */
>> +    return 0;
>> +}
>> +
> 
> The above two comments "Realm guest is not debuggable." can be dropped
> since
> the code is self-explanatory, and those two functions are unnecessary to be
> kept in that way, for example:
> 
>     case KVM_CAP_GUEST_DEBUG_HW_BPS:
>         return kvm_is_realm(kvm) ? 0 : get_num_brps();
>     case KVM_CAP_GUEST_DEBUG_HW_WRPS:
>         return kvm_is_realm(kvm) ? 0 : get_num_wrps();
> 
> 
>>   int kvm_arch_vcpu_should_kick(struct kvm_vcpu *vcpu)
>>   {
>>       return kvm_vcpu_exiting_guest_mode(vcpu) == IN_GUEST_MODE;
>> @@ -323,7 +339,6 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm,
>> long ext)
>>       case KVM_CAP_ARM_IRQ_LINE_LAYOUT_2:
>>       case KVM_CAP_ARM_NISV_TO_USER:
>>       case KVM_CAP_ARM_INJECT_EXT_DABT:
>> -    case KVM_CAP_SET_GUEST_DEBUG:
>>       case KVM_CAP_VCPU_ATTRIBUTES:
>>       case KVM_CAP_PTP_KVM:
>>       case KVM_CAP_ARM_SYSTEM_SUSPEND:
>> @@ -331,6 +346,9 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm,
>> long ext)
>>       case KVM_CAP_COUNTER_OFFSET:
>>           r = 1;
>>           break;
>> +    case KVM_CAP_SET_GUEST_DEBUG:
>> +        r = !kvm_is_realm(kvm);
>> +        break;
>>       case KVM_CAP_SET_GUEST_DEBUG2:
>>           return KVM_GUESTDBG_VALID_MASK;
>>       case KVM_CAP_ARM_SET_DEVICE_ADDR:
>> @@ -376,10 +394,10 @@ int kvm_vm_ioctl_check_extension(struct kvm
>> *kvm, long ext)
>>           r = cpus_have_final_cap(ARM64_HAS_32BIT_EL1);
>>           break;
>>       case KVM_CAP_GUEST_DEBUG_HW_BPS:
>> -        r = get_num_brps();
>> +        r = kvm_arm_get_num_brps(kvm);
>>           break;
>>       case KVM_CAP_GUEST_DEBUG_HW_WPS:
>> -        r = get_num_wrps();
>> +        r = kvm_arm_get_num_wrps(kvm);
>>           break;
>>       case KVM_CAP_ARM_PMU_V3:
>>           r = kvm_arm_support_pmu_v3();
> 
> Thanks,
> Gavin
>