On 03/02/2016 17:23, Radim Krčmář wrote:
> Discard policy doesn't rely on information from notifiers, so we don't
> need to register notifiers unconditionally.
>
> Use of ps->lock doesn't make sense, but isn't any worse than before.
Oh, it's perfectly okay. Too fine-grained locks are bad, and lock
contention on ps->lock is a non-issue.
Can you however add a patch that says what fields of kvm_kpit_state are
protected by which locks? Then this patch will just add
/* Protected by kvm_kpit_state lock. */
above the reinject field.
Otherwise
Reviewed-by: Paolo Bonzini <[email protected]>
> Signed-off-by: Radim Krčmář <[email protected]>
> ---
> arch/x86/kvm/i8254.c | 44 ++++++++++++++++++++++++++++++--------------
> arch/x86/kvm/i8254.h | 1 +
> arch/x86/kvm/x86.c | 6 +++---
> 3 files changed, 34 insertions(+), 17 deletions(-)
>
> diff --git a/arch/x86/kvm/i8254.c b/arch/x86/kvm/i8254.c
> index fc554fbf71a7..f1dfc250bbe0 100644
> --- a/arch/x86/kvm/i8254.c
> +++ b/arch/x86/kvm/i8254.c
> @@ -237,9 +237,6 @@ static void kvm_pit_ack_irq(struct kvm_irq_ack_notifier
> *kian)
> struct kvm_kpit_state *ps = container_of(kian, struct kvm_kpit_state,
> irq_ack_notifier);
>
> - if (!ps->reinject)
> - return;
> -
> atomic_set(&ps->irq_ack, 1);
> if (atomic_add_unless(&ps->pending, -1, 0))
> /* in this case, we had multiple outstanding pit interrupts
> @@ -708,14 +705,13 @@ struct kvm_pit *kvm_create_pit(struct kvm *kvm, u32
> flags)
> hrtimer_init(&pit_state->timer, CLOCK_MONOTONIC, HRTIMER_MODE_ABS);
> pit_state->irq_ack_notifier.gsi = 0;
> pit_state->irq_ack_notifier.irq_acked = kvm_pit_ack_irq;
> - kvm_register_irq_ack_notifier(kvm, &pit_state->irq_ack_notifier);
> - pit_state->reinject = true;
> mutex_unlock(&pit->pit_state.lock);
>
> - kvm_pit_reset(pit);
> -
> pit->mask_notifier.func = pit_mask_notifer;
> - kvm_register_irq_mask_notifier(kvm, 0, &pit->mask_notifier);
> +
> + kvm_pit_set_reinject(pit, true);
> +
> + kvm_pit_reset(pit);
>
> kvm_iodevice_init(&pit->dev, &pit_dev_ops);
> ret = kvm_io_bus_register_dev(kvm, KVM_PIO_BUS, KVM_PIT_BASE_ADDRESS,
> @@ -738,14 +734,37 @@ fail_unregister:
> kvm_io_bus_unregister_dev(kvm, KVM_PIO_BUS, &pit->dev);
>
> fail:
> - kvm_unregister_irq_mask_notifier(kvm, 0, &pit->mask_notifier);
> - kvm_unregister_irq_ack_notifier(kvm, &pit_state->irq_ack_notifier);
> + kvm_pit_set_reinject(pit, false);
> kvm_free_irq_source_id(kvm, pit->irq_source_id);
> kthread_stop(pit->worker_task);
> kfree(pit);
> return NULL;
> }
>
> +void kvm_pit_set_reinject(struct kvm_pit *pit, bool reinject)
> +{
> + /* Implicit BUG on NULL dereference. */
> + struct kvm_kpit_state *ps = &pit->pit_state;
> + struct kvm *kvm = pit->kvm;
> +
> + mutex_lock(&ps->lock);
> +
> + if (ps->reinject == reinject)
> + goto out;
> +
> + ps->reinject = reinject;
> + if (reinject) {
> + kvm_pit_reset_reinject(pit);
> + kvm_register_irq_ack_notifier(kvm, &ps->irq_ack_notifier);
> + kvm_register_irq_mask_notifier(kvm, 0, &pit->mask_notifier);
> + } else {
> + kvm_unregister_irq_ack_notifier(kvm, &ps->irq_ack_notifier);
> + kvm_unregister_irq_mask_notifier(kvm, 0, &pit->mask_notifier);
> + }
> +out:
> + mutex_unlock(&ps->lock);
> +}
> +
> void kvm_free_pit(struct kvm *kvm)
> {
> struct hrtimer *timer;
> @@ -754,10 +773,7 @@ void kvm_free_pit(struct kvm *kvm)
> kvm_io_bus_unregister_dev(kvm, KVM_PIO_BUS,
> &kvm->arch.vpit->dev);
> kvm_io_bus_unregister_dev(kvm, KVM_PIO_BUS,
> &kvm->arch.vpit->speaker_dev);
> - kvm_unregister_irq_mask_notifier(kvm, 0,
> - &kvm->arch.vpit->mask_notifier);
> - kvm_unregister_irq_ack_notifier(kvm,
> - &kvm->arch.vpit->pit_state.irq_ack_notifier);
> + kvm_pit_set_reinject(kvm->arch.vpit, false);
> mutex_lock(&kvm->arch.vpit->pit_state.lock);
> timer = &kvm->arch.vpit->pit_state.timer;
> hrtimer_cancel(timer);
> diff --git a/arch/x86/kvm/i8254.h b/arch/x86/kvm/i8254.h
> index f8cf4b84f435..7ac94bc0799f 100644
> --- a/arch/x86/kvm/i8254.h
> +++ b/arch/x86/kvm/i8254.h
> @@ -60,5 +60,6 @@ void kvm_pit_load_count(struct kvm *kvm, int channel, u32
> val, int hpet_legacy_s
> struct kvm_pit *kvm_create_pit(struct kvm *kvm, u32 flags);
> void kvm_free_pit(struct kvm *kvm);
> void kvm_pit_reset(struct kvm_pit *pit);
> +void kvm_pit_set_reinject(struct kvm_pit *pit, bool reinject);
>
> #endif
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 5b937fdebc66..6bc03141e809 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -3652,9 +3652,9 @@ static int kvm_vm_ioctl_reinject(struct kvm *kvm,
> {
> if (!kvm->arch.vpit)
> return -ENXIO;
> - mutex_lock(&kvm->arch.vpit->pit_state.lock);
> - kvm->arch.vpit->pit_state.reinject = control->pit_reinject;
> - mutex_unlock(&kvm->arch.vpit->pit_state.lock);
> +
> + kvm_pit_set_reinject(kvm->arch.vpit, control->pit_reinject);
> +
> return 0;
> }
>
>
