| Message ID | 20170407085041.5257-3-david@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
2017-04-07 10:50+0200, David Hildenbrand: > Let's add a new mode and set it while we create the irqchip via > KVM_CREATE_IRQCHIP and KVM_CAP_SPLIT_IRQCHIP. > > This mode will be used later to test if adding routes > (in kvm_set_routing_entry()) is already allowed. > > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > @@ -3934,9 +3934,14 @@ static int kvm_vm_ioctl_enable_cap(struct kvm *kvm, > goto split_irqchip_unlock; > if (kvm->created_vcpus) > goto split_irqchip_unlock; > + kvm->arch.irqchip_mode = KVM_IRQCHIP_INIT_IN_PROGRESS; > r = kvm_setup_empty_irq_routing(kvm); > - if (r) > + if (r) { > + kvm->arch.irqchip_mode = KVM_IRQCHIP_NONE; > + /* Pairs with smp_rmb() when reading irqchip_mode */ > + smp_wmb(); > goto split_irqchip_unlock; > + } > /* Pairs with irqchip_in_kernel. */ > smp_wmb(); > kvm->arch.irqchip_mode = KVM_IRQCHIP_SPLIT; > @@ -4024,8 +4029,12 @@ long kvm_arch_vm_ioctl(struct file *filp, > goto create_irqchip_unlock; > } > > + kvm->arch.irqchip_mode = KVM_IRQCHIP_INIT_IN_PROGRESS; > r = kvm_setup_default_irq_routing(kvm); > if (r) { > + kvm->arch.irqchip_mode = KVM_IRQCHIP_NONE; > + /* Pairs with smp_rmb() when reading irqchip_mode */ > + smp_wmb(); I think these two barriers are pointless. KVM_IRQCHIP_INIT_IN_PROGRESS and KVM_IRQCHIP_NONE have the same result for readers of irqchip mode and we don't even have a barrier after setting KVM_IRQCHIP_INIT_IN_PROGRESS mode, so it looks weird. I can remove them if you agree. Thanks.
On 12.04.2017 20:26, Radim Krčmář wrote: > 2017-04-07 10:50+0200, David Hildenbrand: >> Let's add a new mode and set it while we create the irqchip via >> KVM_CREATE_IRQCHIP and KVM_CAP_SPLIT_IRQCHIP. >> >> This mode will be used later to test if adding routes >> (in kvm_set_routing_entry()) is already allowed. >> >> Signed-off-by: David Hildenbrand <david@redhat.com> >> --- >> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c >> @@ -3934,9 +3934,14 @@ static int kvm_vm_ioctl_enable_cap(struct kvm *kvm, >> goto split_irqchip_unlock; >> if (kvm->created_vcpus) >> goto split_irqchip_unlock; >> + kvm->arch.irqchip_mode = KVM_IRQCHIP_INIT_IN_PROGRESS; >> r = kvm_setup_empty_irq_routing(kvm); >> - if (r) >> + if (r) { >> + kvm->arch.irqchip_mode = KVM_IRQCHIP_NONE; >> + /* Pairs with smp_rmb() when reading irqchip_mode */ >> + smp_wmb(); >> goto split_irqchip_unlock; >> + } >> /* Pairs with irqchip_in_kernel. */ >> smp_wmb(); >> kvm->arch.irqchip_mode = KVM_IRQCHIP_SPLIT; >> @@ -4024,8 +4029,12 @@ long kvm_arch_vm_ioctl(struct file *filp, >> goto create_irqchip_unlock; >> } >> >> + kvm->arch.irqchip_mode = KVM_IRQCHIP_INIT_IN_PROGRESS; >> r = kvm_setup_default_irq_routing(kvm); >> if (r) { >> + kvm->arch.irqchip_mode = KVM_IRQCHIP_NONE; >> + /* Pairs with smp_rmb() when reading irqchip_mode */ >> + smp_wmb(); > > I think these two barriers are pointless. > > KVM_IRQCHIP_INIT_IN_PROGRESS and KVM_IRQCHIP_NONE have the same result > for readers of irqchip mode and we don't even have a barrier after > setting KVM_IRQCHIP_INIT_IN_PROGRESS mode, so it looks weird. > > I can remove them if you agree. Paolo requested this, and I agreed to rather have a unified handling for memory barriers with kvm->arch.irqchip_mode, then trying to optimize for special cases. (implicit knowledge here is e.g. that this is protected by kvm->lock) If you think we can drop this, feel free to drop. Thanks! > > Thanks. >
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h index 74ef58c..164e544 100644 --- a/arch/x86/include/asm/kvm_host.h +++ b/arch/x86/include/asm/kvm_host.h @@ -727,6 +727,7 @@ struct kvm_hv { enum kvm_irqchip_mode { KVM_IRQCHIP_NONE, + KVM_IRQCHIP_INIT_IN_PROGRESS, /* temporarily set during creation */ KVM_IRQCHIP_KERNEL, /* created with KVM_CREATE_IRQCHIP */ KVM_IRQCHIP_SPLIT, /* created with KVM_CAP_SPLIT_IRQCHIP */ }; diff --git a/arch/x86/kvm/irq.h b/arch/x86/kvm/irq.h index 40d5b2c..59e05fe 100644 --- a/arch/x86/kvm/irq.h +++ b/arch/x86/kvm/irq.h @@ -93,21 +93,29 @@ static inline int pic_in_kernel(struct kvm *kvm) static inline int irqchip_split(struct kvm *kvm) { - return kvm->arch.irqchip_mode == KVM_IRQCHIP_SPLIT; + int mode = kvm->arch.irqchip_mode; + + /* Matches smp_wmb() when setting 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; + + /* Matches smp_wmb() when setting 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_NONE; + int mode = kvm->arch.irqchip_mode; - /* Matches with wmb after initializing kvm->irq_routing. */ + /* Matches smp_wmb() when setting 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/x86.c b/arch/x86/kvm/x86.c index ccbd45e..8f776f0 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -3934,9 +3934,14 @@ static int kvm_vm_ioctl_enable_cap(struct kvm *kvm, goto split_irqchip_unlock; if (kvm->created_vcpus) goto split_irqchip_unlock; + kvm->arch.irqchip_mode = KVM_IRQCHIP_INIT_IN_PROGRESS; r = kvm_setup_empty_irq_routing(kvm); - if (r) + if (r) { + kvm->arch.irqchip_mode = KVM_IRQCHIP_NONE; + /* Pairs with smp_rmb() when reading irqchip_mode */ + smp_wmb(); goto split_irqchip_unlock; + } /* Pairs with irqchip_in_kernel. */ smp_wmb(); kvm->arch.irqchip_mode = KVM_IRQCHIP_SPLIT; @@ -4024,8 +4029,12 @@ long kvm_arch_vm_ioctl(struct file *filp, goto create_irqchip_unlock; } + kvm->arch.irqchip_mode = KVM_IRQCHIP_INIT_IN_PROGRESS; r = kvm_setup_default_irq_routing(kvm); if (r) { + kvm->arch.irqchip_mode = KVM_IRQCHIP_NONE; + /* Pairs with smp_rmb() when reading irqchip_mode */ + smp_wmb(); mutex_lock(&kvm->slots_lock); mutex_lock(&kvm->irq_lock); kvm_ioapic_destroy(kvm);
Let's add a new mode and set it while we create the irqchip via KVM_CREATE_IRQCHIP and KVM_CAP_SPLIT_IRQCHIP. This mode will be used later to test if adding routes (in kvm_set_routing_entry()) is already allowed. Signed-off-by: David Hildenbrand <david@redhat.com> --- arch/x86/include/asm/kvm_host.h | 1 + arch/x86/kvm/irq.h | 18 +++++++++++++----- arch/x86/kvm/x86.c | 11 ++++++++++- 3 files changed, 24 insertions(+), 6 deletions(-)