Hi,
> +
> +inline void kvm_populate_efault_info(struct kvm_vcpu *vcpu,
> + uint64_t gpa, uint64_t len, uint64_t flags)
> +{
> + if (WARN_ON_ONCE(!vcpu))
> + return;
> +
> + preempt_disable();
> + /*
> + * Ensure the this vCPU isn't modifying another vCPU's run struct, which
> + * would open the door for races between concurrent calls to this
> + * function.
> + */
> + if (WARN_ON_ONCE(vcpu != __this_cpu_read(kvm_running_vcpu)))
> + goto out;
Sorry, I also wrote to the v3 discussion for this patch.
Re-iterating what I said there:
Why use WARN_ON_ONCE when there is a clear possiblity of preemption
kicking in (with the possibility of vcpu_load/vcpu_put being called
in the new task) before preempt_disable() is called in this function ?
I think you should use WARN_ON_ONCE only where there is some impossible
or unhandled situation happening, not when there is a possibility of that
situation clearly happening as per the kernel code. I think that this WARN_ON_ONCE
could make sense if kvm_populate_efault_info() is called from atomic context,
but not when you are disabling preemption from this function itself.
Basically I don't think there is any way we can guarantee that
preemption DOESN'T kick in before the preempt_disable() such that
this if-check is actually something that deserves to have a kernel
WARN_ON_ONCE() warning.
Can we get rid of this WARN_ON_ONCE and straightaway jump to the
out label if "(vcpu != __this_cpu_read(kvm_running_vcpu))" is true, or
please do correct me if I am wrong about something ?
> + /*
> + * Try not to overwrite an already-populated run struct.
> + * This isn't a perfect solution, as there's no guarantee that the exit
> + * reason is set before the run struct is populated, but it should prevent
> + * at least some bugs.
> + */
> + else if (WARN_ON_ONCE(vcpu->run->exit_reason != KVM_EXIT_UNKNOWN))
> + goto out;
> +
> + vcpu->run->exit_reason = KVM_EXIT_MEMORY_FAULT;
> + vcpu->run->memory_fault.gpa = gpa;
> + vcpu->run->memory_fault.len = len;
> + vcpu->run->memory_fault.flags = flags;
> +
> +out:
> + preempt_enable();
> +}
> --
> 2.41.0.rc0.172.g3f132b7071-goog
>
