On 28.03.2017 10:28, Paolo Bonzini wrote:
>
>
> On 23/03/2017 15:13, David Hildenbrand wrote:
>> I don't see any reason any more for this lock, seemed to be used to protect
>> removal of kvm->arch.vpic / kvm->arch.vioapic when already partially
>> inititalized, now access is properly protected using kvm->arch.irqchip_mode
>> and this shouldn't be necessary anymore.
>
> Do more readers of irqchip_mode need memory barriers now? Definitely we need an smp_wmb() after setting INIT_IN_PROGRESS.
>
My assumption was that if vpic/vioapic checks worked until now without
memory barriers, the same should be true when checking against
irqchip_mode. But as we are dropping some implicit barriers and
irqchip_mode is updated more frequently, you may be right.
Also, using rmb/wmb in a uniform fashion with irqchip_mode looks
cleaner, as we are removing all implicit "cannot happen because of XXX"
special cases.
> Something like this:
>
> diff --git a/arch/x86/kvm/ioapic.h b/arch/x86/kvm/ioapic.h
> index 3eb140ba57b6..b1df4d53fad1 100644
> --- a/arch/x86/kvm/ioapic.h
> +++ b/arch/x86/kvm/ioapic.h
> @@ -107,7 +107,9 @@ struct kvm_ioapic {
>
> static inline int ioapic_in_kernel(struct kvm *kvm)
> {
> - return kvm->arch.irqchip_mode == KVM_IRQCHIP_KERNEL;
> + int mode = kvm->arch.irqchip_mode;
> + smp_rmb();
> + return mode == KVM_IRQCHIP_KERNEL;
> }
>
> void kvm_rtc_eoi_tracking_restore_one(struct kvm_vcpu *vcpu);
> diff --git a/arch/x86/kvm/irq.h b/arch/x86/kvm/irq.h
> index 2455f16ee98a..6f7b3fb92dd4 100644
> --- a/arch/x86/kvm/irq.h
> +++ b/arch/x86/kvm/irq.h
> @@ -80,26 +80,30 @@ struct kvm_pic {
>
> static inline int pic_in_kernel(struct kvm *kvm)
> {
> - return kvm->arch.irqchip_mode == KVM_IRQCHIP_KERNEL;
> + int mode = kvm->arch.irqchip_mode;
> + smp_rmb();
> + return mode == KVM_IRQCHIP_KERNEL;
> }
>
> static inline int irqchip_split(struct kvm *kvm)
> {
> - return kvm->arch.irqchip_mode == KVM_IRQCHIP_SPLIT;
> + int mode = kvm->arch.irqchip_mode;
> + smp_rmb();
> + return mode == KVM_IRQCHIP_SPLIT;
> }
>
> static inline int irqchip_kernel(struct kvm *kvm)
> {
> - return kvm->arch.irqchip_mode == KVM_IRQCHIP_KERNEL;
> + int mode = kvm->arch.irqchip_mode;
> + smp_rmb();
> + return mode == KVM_IRQCHIP_KERNEL;
> }
>
> static inline int irqchip_in_kernel(struct kvm *kvm)
> {
> - bool ret = kvm->arch.irqchip_mode > KVM_IRQCHIP_INIT_IN_PROGRESS;
> -
> - /* Matches with wmb after initializing kvm->irq_routing. */
> + int mode = kvm->arch.irqchip_mode;
> smp_rmb();
> - return ret;
> + return mode > KVM_IRQCHIP_INIT_IN_PROGRESS;
> }
>
> void kvm_pic_reset(struct kvm_kpic_state *s);
> diff --git a/arch/x86/kvm/irq_comm.c b/arch/x86/kvm/irq_comm.c
> index 45ab43e5fbc5..e8306eeb5613 100644
> --- a/arch/x86/kvm/irq_comm.c
> +++ b/arch/x86/kvm/irq_comm.c
> @@ -286,6 +286,7 @@ int kvm_set_routing_entry(struct kvm *kvm,
> if (kvm->arch.irqchip_mode == KVM_IRQCHIP_NONE)
> goto out;
>
> + smp_rmb();
> switch (ue->type) {
> case KVM_IRQ_ROUTING_IRQCHIP:
> if (irqchip_split(kvm))
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 5dca5aed9c93..ed9143f305f9 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -3933,6 +3933,8 @@ static int kvm_vm_ioctl_enable_cap(struct kvm *kvm,
> if (kvm->created_vcpus)
> goto split_irqchip_unlock;
> kvm->arch.irqchip_mode = KVM_IRQCHIP_INIT_IN_PROGRESS;
> + /* Pairs with pic_in_kernel/ioapic_in_kernel/etc. */
> + smp_wmb();
> r = kvm_setup_empty_irq_routing(kvm);
> if (r) {
> kvm->arch.irqchip_mode = KVM_IRQCHIP_NONE;
> @@ -4026,6 +4028,8 @@ long kvm_arch_vm_ioctl(struct file *filp,
> }
>
> kvm->arch.irqchip_mode = KVM_IRQCHIP_INIT_IN_PROGRESS;
> + /* Pairs with pic_in_kernel/ioapic_in_kernel/etc. */
> + smp_wmb();
> r = kvm_setup_default_irq_routing(kvm);
> if (r) {
> kvm->arch.irqchip_mode = KVM_IRQCHIP_NONE;
>
> but with each part properly squashed into the right patch.
Sure, thanks!
>
> Paolo
--
Thanks,
David
