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

[Marc Zyngier]

On Wed, 31 Mar 2021 16:25:46 +0100,
Alexandru Elisei <alexandru.elisei at arm.com> wrote:
> 
> Hi Marc,
> 
> On 3/30/21 9:07 PM, Marc Zyngier wrote:

[...]

> > I think it would be absolutely fine to make the slow path of
> > kvm_vcpu_first_run_init() run with preempt disabled. This happens so
> > rarely that that it isn't worth thinking about it.
> 
> It looks to me like it's a bit too heavy-handed to run the entire
> function kvm_vcpu_first_run_init() with preemption disabled just for
> __this_cpu_read() in kvm_arm_setup_mdcr_el2(). Not because of the
> performance cost (it's negligible, as it's called exactly once in
> the VCPU lifetime), but because it's not obvious why it is needed.
> 
> I tried this:
> 
> @@ -580,7 +580,9 @@ static int kvm_vcpu_first_run_init(struct kvm_vcpu *vcpu)
>  
>         vcpu->arch.has_run_once = true;
>  
> -       kvm_arm_vcpu_init_debug(vcpu);
> +       preempt_disable();
> +       kvm_arm_setup_mdcr_el2(vcpu);
> +       preempt_enable();
>  
>         if (likely(irqchip_in_kernel(kvm))) {
>                 /*
> 
> and it still looks a bit off to me because preemption needs to be
> disabled because of an implementation detail in
> kvm_arm_setup_mdcr_el2(), as the function operates on the VCPU
> struct and preemption can be enabled for that.
> 
> I was thinking something like this:
> 
> @@ -119,7 +119,9 @@ static void kvm_arm_setup_mdcr_el2(struct kvm_vcpu *vcpu, u32
> host_mdcr)
>   */
>  void kvm_arm_vcpu_init_debug(struct kvm_vcpu *vcpu)
>  {
> -       kvm_arm_setup_mdcr_el2(vcpu, this_cpu_read(mdcr_el2));
> +       preempt_disable();
> +       kvm_arm_setup_mdcr_el2(vcpu);
> +       preempt_enable();
>  }
>  
>  /**
> 
> What do you think?

I'm fine with it either way, so pick you favourite!

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.