On Wed, 31 Mar 2021 16:25:46 +0100,
Alexandru Elisei <alexandru.elisei@xxxxxxx> 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.
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
