[RFC PATCH v3 01/16] KVM: arm64: Initialize VCPU mdcr_el2 before loading it

[Alexandru Elisei]

Hi James,

Many thanks for having a look at the series!

On 11/19/20 4:58 PM, James Morse wrote:
> Hi Alex,
>
> On 27/10/2020 17:26, Alexandru Elisei wrote:
>> When a VCPU is created, the kvm_vcpu struct is initialized to zero in
>> kvm_vm_ioctl_create_vcpu(). On VHE systems, the first time
>> vcpu.arch.mdcr_el2 is loaded on hardware is in vcpu_load(), before it is
>> set to a sensible value in kvm_arm_setup_debug() later in the run loop. The
>> result is that KVM executes for a short time with MDCR_EL2 set to zero.
>>
>> This is mostly harmless as we don't need to trap debug and SPE register
>> accesses from EL1 (we're still running in the host at EL2), but we do set
>> MDCR_EL2.HPMN to 0 which is constrained unpredictable according to ARM DDI
>> 0487F.b, page D13-3620; the required behavior from the hardware in this
>> case is to reserve an unkown number of registers for EL2 and EL3 exclusive
>> use.
>>
>> Initialize mdcr_el2 in kvm_vcpu_vcpu_first_run_init(), so we can avoid the
>> constrained unpredictable behavior and to ensure that the MDCR_EL2 register
>> has the same value after each vcpu_load(), including the first time the
>> VCPU is run.
>
>> diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
>> index 7a7e425616b5..22ee448aee2b 100644
>> --- a/arch/arm64/kvm/debug.c
>> +++ b/arch/arm64/kvm/debug.c
>> @@ -68,6 +68,59 @@ void kvm_arm_init_debug(void)
>> +static void kvm_arm_setup_mdcr_el2(struct kvm_vcpu *vcpu, u32 host_mdcr)
>> +{
>> +	bool trap_debug = !(vcpu->arch.flags & KVM_ARM64_DEBUG_DIRTY);
>> +
>> +	/*
>> +	 * This also clears MDCR_EL2_E2PB_MASK to disable guest access
>> +	 * to the profiling buffer.
>> +	 */
>> +	vcpu->arch.mdcr_el2 = host_mdcr & MDCR_EL2_HPMN_MASK;
>> +	vcpu->arch.mdcr_el2 |= (MDCR_EL2_TPM |
>> +				MDCR_EL2_TPMS |
>> +				MDCR_EL2_TPMCR |
>> +				MDCR_EL2_TDRA |
>> +				MDCR_EL2_TDOSA);
>> +	if (vcpu->guest_debug) {
>> +		/* Route all software debug exceptions to EL2 */
>> +		vcpu->arch.mdcr_el2 |= MDCR_EL2_TDE;
>> +		if (vcpu->guest_debug & KVM_GUESTDBG_USE_HW)
>> +			trap_debug = true;
>> +	}
> This had me confused for a while... could you hint that this is when the guest is being
> 'external' debugged by the VMM? (its clear-er before this change)

I can put a comment above the if statement similar to the one from
kvm_arm_setup_debug(), where this code is lifted from:

        /* Is the VCPU being debugged by userspace? */

What do you think?

Thanks,
Alex