[PATCH v2] KVM: arm64: Initialize VCPU mdcr_el2 before loading it

[Alexandru Elisei]

Hi Marc,

On 3/30/21 6:13 PM, Alexandru Elisei wrote:
> [..]
>>> +}
>>> +
>>>  /**
>>>   * kvm_arm_reset_debug_ptr - reset the debug ptr to point to the vcpu state
>>>   */
>>> @@ -83,12 +137,7 @@ void kvm_arm_reset_debug_ptr(struct kvm_vcpu *vcpu)
>>>   * @vcpu:	the vcpu pointer
>>>   *
>>>   * This is called before each entry into the hypervisor to setup any
>>> - * debug related registers. Currently this just ensures we will trap
>>> - * access to:
>>> - *  - Performance monitors (MDCR_EL2_TPM/MDCR_EL2_TPMCR)
>>> - *  - Debug ROM Address (MDCR_EL2_TDRA)
>>> - *  - OS related registers (MDCR_EL2_TDOSA)
>>> - *  - Statistical profiler (MDCR_EL2_TPMS/MDCR_EL2_E2PB)
>>> + * debug related registers.
>>>   *
>>>   * Additionally, KVM only traps guest accesses to the debug registers if
>>>   * the guest is not actively using them (see the KVM_ARM64_DEBUG_DIRTY
>>> @@ -100,27 +149,14 @@ void kvm_arm_reset_debug_ptr(struct kvm_vcpu *vcpu)
>>>  
>>>  void kvm_arm_setup_debug(struct kvm_vcpu *vcpu)
>>>  {
>>> -	bool trap_debug = !(vcpu->arch.flags & KVM_ARM64_DEBUG_DIRTY);
>>>  	unsigned long mdscr, orig_mdcr_el2 = vcpu->arch.mdcr_el2;
>>>  
>>>  	trace_kvm_arm_setup_debug(vcpu, vcpu->guest_debug);
>>>  
>>> -	/*
>>> -	 * This also clears MDCR_EL2_E2PB_MASK to disable guest access
>>> -	 * to the profiling buffer.
>>> -	 */
>>> -	vcpu->arch.mdcr_el2 = __this_cpu_read(mdcr_el2) & MDCR_EL2_HPMN_MASK;
>>> -	vcpu->arch.mdcr_el2 |= (MDCR_EL2_TPM |
>>> -				MDCR_EL2_TPMS |
>>> -				MDCR_EL2_TPMCR |
>>> -				MDCR_EL2_TDRA |
>>> -				MDCR_EL2_TDOSA);
>>> +	kvm_arm_setup_mdcr_el2(vcpu, __this_cpu_read(mdcr_el2));
>>>  
>>>  	/* Is Guest debugging in effect? */
>>>  	if (vcpu->guest_debug) {
>>> -		/* Route all software debug exceptions to EL2 */
>>> -		vcpu->arch.mdcr_el2 |= MDCR_EL2_TDE;
>>> -
>>>  		/* Save guest debug state */
>>>  		save_guest_debug_regs(vcpu);
>>>  
>>> @@ -174,7 +210,6 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu)
>>>  
>>>  			vcpu->arch.debug_ptr = &vcpu->arch.external_debug_state;
>>>  			vcpu->arch.flags |= KVM_ARM64_DEBUG_DIRTY;
>>> -			trap_debug = true;
>> There is something that slightly worries me here: there is now a
>> disconnect between flagging debug as dirty and setting the
>> trapping. And actually, you now check for KVM_ARM64_DEBUG_DIRTY and
>> set the trap bits *before* setting the dirty bit itself.
>>
>> Here, I believe you end up with guest/host confusion of breakpoints,
>> which isn't great. Or did I miss something?
> I'm sorry, but I don't understand what you mean. This is my understanding of what
> is happening.
>
> Without this patch, trap_debug is set to true and the KVM_ARM64_DEBUG_DIRTY flag
> is set if vcpu->guest_debug & KVM_GUESTDBG_USE_HW. Further down, trap debug is
> only used when computing mdcr_el2.
>
> With this patch, trap_debug is set to true if vcpu->guest_debug &
> KVM_GUESTDBG_USE_HW and it's also used for computing mdcr_el2, but this happens in
> kvm_arm_setup_mdcr_el2(), which is called at the start of kvm_arm_setup_debug().
> The KVM_ARM_DEBUG_DIRTY flags is still set in kvm_arm_setup_debug() if
> vcpu->guest_debug & KVM_GUESTDBG_USE_HW, like before.
>
> The guest never runs with the value computed in kvm_vcpu_first_run_init() unless
> it's identical with the value recomputed in kvm_arm_setup_debug().
>
> The only difference I see is that mdcr_el2 is computed at the start of
> kvm_arm_setup_debug(). I get the feeling I'm also missing something.

I think I understand what you mean, you are worried that we won't set the bit in
mdcr_el2 to trap debug in the same place where we set the debug dirty flag. If
that's the case, then I can move kvm_arm_setup_mdcr_el2 right after the BUG_ON()
and remove the KVM_GUESTDBG_USE_HW check because the KVM_ARM_DEBUG_DIRTY would be
already set.

Question though, if mdcr_el2 is tied to the debug dirty flag, we ignore the flag
here (code without this patch):

    BUG_ON(!vcpu->guest_debug &&
        vcpu->arch.debug_ptr != &vcpu->arch.vcpu_debug_state);

    /* Trap debug register access */
    if (trap_debug)
        vcpu->arch.mdcr_el2 |= MDCR_EL2_TDA;

    /* If KDE or MDE are set, perform a full save/restore cycle. */
    if (vcpu_read_sys_reg(vcpu, MDSCR_EL1) & (DBG_MDSCR_KDE | DBG_MDSCR_MDE))
        vcpu->arch.flags |= KVM_ARM64_DEBUG_DIRTY;

I suppose there's something I don't understand yet about how this is supposed to work.

Thanks,

Alex

>
> Thanks,
>
> Alex
>
>>>  
>>>  			trace_kvm_arm_set_regset("BKPTS", get_num_brps(),
>>>  						&vcpu->arch.debug_ptr->dbg_bcr[0],
>>> @@ -189,10 +224,6 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu)
>>>  	BUG_ON(!vcpu->guest_debug &&
>>>  		vcpu->arch.debug_ptr != &vcpu->arch.vcpu_debug_state);
>>>  
>>> -	/* Trap debug register access */
>>> -	if (trap_debug)
>>> -		vcpu->arch.mdcr_el2 |= MDCR_EL2_TDA;
>>> -
>>>  	/* If KDE or MDE are set, perform a full save/restore cycle. */
>>>  	if (vcpu_read_sys_reg(vcpu, MDSCR_EL1) & (DBG_MDSCR_KDE | DBG_MDSCR_MDE))
>>>  		vcpu->arch.flags |= KVM_ARM64_DEBUG_DIRTY;
>>> @@ -201,7 +232,6 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu)
>>>  	if (has_vhe() && orig_mdcr_el2 != vcpu->arch.mdcr_el2)
>>>  		write_sysreg(vcpu->arch.mdcr_el2, mdcr_el2);
>>>  
>>> -	trace_kvm_arm_set_dreg32("MDCR_EL2", vcpu->arch.mdcr_el2);
>>>  	trace_kvm_arm_set_dreg32("MDSCR_EL1", vcpu_read_sys_reg(vcpu, MDSCR_EL1));
>>>  }
>> Thanks,
>>
>> 	M.
>>
> _______________________________________________
> kvmarm mailing list
> kvmarm at lists.cs.columbia.edu
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm