From: Marcelo Tosatti <mtosatti@redhat.com>
To: Sheng Yang <sheng@linux.intel.com>
Cc: Avi Kivity <avi@redhat.com>,
kvm@vger.kernel.org, Gleb Natapov <gleb@redhat.com>
Subject: Re: [PATCH] KVM: Merge kvm_ioapic_get_delivery_bitmask into kvm_get_intr_delivery_bitmask
Date: Tue, 3 Mar 2009 08:20:37 -0300 [thread overview]
Message-ID: <20090303112037.GA13997@amt.cnet> (raw)
In-Reply-To: <1235981973-6740-1-git-send-email-sheng@linux.intel.com>
On Mon, Mar 02, 2009 at 04:19:33PM +0800, Sheng Yang wrote:
> Gleb fixed bitmap ops usage in kvm_ioapic_get_delivery_bitmask.
>
> Sheng merged two functions, as well as fix several issues in
> kvm_get_intr_delivery_bitmask:
> 1. deliver_bitmask is a bitmap rather than a unsigned long intereger.
> 2. Lowest priority target bitmap wrong calculated by mistake.
> 3. Prevent potential NULL reference.
> 4. Declaration in include/kvm_host.h caused powerpc compilation warning.
>
> Signed-off-by: Gleb Natapov <gleb@redhat.com>
> Signed-off-by: Sheng Yang <sheng@linux.intel.com>
> ---
> include/linux/kvm_host.h | 3 ---
> virt/kvm/ioapic.c | 39 ---------------------------------------
> virt/kvm/ioapic.h | 5 +++--
> virt/kvm/irq_comm.c | 42 ++++++++++++++++++++++++++++++++++++++----
> 4 files changed, 41 insertions(+), 48 deletions(-)
>
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 3832243..fb60f31 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -363,9 +363,6 @@ void kvm_unregister_irq_mask_notifier(struct kvm *kvm, int irq,
> struct kvm_irq_mask_notifier *kimn);
> void kvm_fire_mask_notifiers(struct kvm *kvm, int irq, bool mask);
>
> -void kvm_get_intr_delivery_bitmask(struct kvm_ioapic *ioapic,
> - union kvm_ioapic_redirect_entry *entry,
> - unsigned long *deliver_bitmask);
> int kvm_set_irq(struct kvm *kvm, int irq_source_id, int irq, int level);
> void kvm_notify_acked_irq(struct kvm *kvm, unsigned irqchip, unsigned pin);
> void kvm_register_irq_ack_notifier(struct kvm *kvm,
> diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c
> index 7c2cb2b..2c40ff8 100644
> --- a/virt/kvm/ioapic.c
> +++ b/virt/kvm/ioapic.c
> @@ -161,45 +161,6 @@ static void ioapic_inj_nmi(struct kvm_vcpu *vcpu)
> kvm_vcpu_kick(vcpu);
> }
>
> -void kvm_ioapic_get_delivery_bitmask(struct kvm_ioapic *ioapic, u8 dest,
> - u8 dest_mode, unsigned long *mask)
> -{
> - int i;
> - struct kvm *kvm = ioapic->kvm;
> - struct kvm_vcpu *vcpu;
> -
> - ioapic_debug("dest %d dest_mode %d\n", dest, dest_mode);
> -
> - *mask = 0;
> - if (dest_mode == 0) { /* Physical mode. */
> - if (dest == 0xFF) { /* Broadcast. */
> - for (i = 0; i < KVM_MAX_VCPUS; ++i)
> - if (kvm->vcpus[i] && kvm->vcpus[i]->arch.apic)
> - *mask |= 1 << i;
> - return;
> - }
> - for (i = 0; i < KVM_MAX_VCPUS; ++i) {
> - vcpu = kvm->vcpus[i];
> - if (!vcpu)
> - continue;
> - if (kvm_apic_match_physical_addr(vcpu->arch.apic, dest)) {
> - if (vcpu->arch.apic)
> - *mask = 1 << i;
> - break;
> - }
> - }
> - } else if (dest != 0) /* Logical mode, MDA non-zero. */
> - for (i = 0; i < KVM_MAX_VCPUS; ++i) {
> - vcpu = kvm->vcpus[i];
> - if (!vcpu)
> - continue;
> - if (vcpu->arch.apic &&
> - kvm_apic_match_logical_addr(vcpu->arch.apic, dest))
> - *mask |= 1 << vcpu->vcpu_id;
> - }
> - ioapic_debug("mask %x\n", *mask);
> -}
> -
> static int ioapic_deliver(struct kvm_ioapic *ioapic, int irq)
> {
> union kvm_ioapic_redirect_entry entry = ioapic->redirtbl[irq];
> diff --git a/virt/kvm/ioapic.h b/virt/kvm/ioapic.h
> index 7275f87..c8032ab 100644
> --- a/virt/kvm/ioapic.h
> +++ b/virt/kvm/ioapic.h
> @@ -70,7 +70,8 @@ void kvm_ioapic_update_eoi(struct kvm *kvm, int vector, int trigger_mode);
> int kvm_ioapic_init(struct kvm *kvm);
> int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int level);
> void kvm_ioapic_reset(struct kvm_ioapic *ioapic);
> -void kvm_ioapic_get_delivery_bitmask(struct kvm_ioapic *ioapic, u8 dest,
> - u8 dest_mode, unsigned long *mask);
> +void kvm_get_intr_delivery_bitmask(struct kvm_ioapic *ioapic,
> + union kvm_ioapic_redirect_entry *entry,
> + unsigned long *deliver_bitmask);
>
> #endif
> diff --git a/virt/kvm/irq_comm.c b/virt/kvm/irq_comm.c
> index d165e05..bb6c5d0 100644
> --- a/virt/kvm/irq_comm.c
> +++ b/virt/kvm/irq_comm.c
> @@ -47,15 +47,49 @@ void kvm_get_intr_delivery_bitmask(struct kvm_ioapic *ioapic,
> union kvm_ioapic_redirect_entry *entry,
> unsigned long *deliver_bitmask)
> {
> + int i;
> + struct kvm *kvm = ioapic->kvm;
> struct kvm_vcpu *vcpu;
>
> - kvm_ioapic_get_delivery_bitmask(ioapic, entry->fields.dest_id,
> - entry->fields.dest_mode,
> - deliver_bitmask);
> + bitmap_zero(deliver_bitmask, KVM_MAX_VCPUS);
I think you can drop the zeroing from the callers?
> +
> + if (entry->fields.dest_mode == 0) { /* Physical mode. */
> + if (entry->fields.dest_id == 0xFF) { /* Broadcast. */
> + for (i = 0; i < KVM_MAX_VCPUS; ++i)
> + if (kvm->vcpus[i] && kvm->vcpus[i]->arch.apic)
> + __set_bit(i, deliver_bitmask);
> + return;
> + }
The spec says broadcast is not supported with lowest priority delivery mode,
and that "must not be configured by software". I wonder what happens in
HW if you do that.
> + for (i = 0; i < KVM_MAX_VCPUS; ++i) {
> + vcpu = kvm->vcpus[i];
> + if (!vcpu)
> + continue;
> + if (kvm_apic_match_physical_addr(vcpu->arch.apic,
> + entry->fields.dest_id)) {
> + if (vcpu->arch.apic)
> + __set_bit(i, deliver_bitmask);
> + break;
> + }
> + }
> + } else if (entry->fields.dest_id != 0) /* Logical mode, MDA non-zero. */
> + for (i = 0; i < KVM_MAX_VCPUS; ++i) {
> + vcpu = kvm->vcpus[i];
> + if (!vcpu)
> + continue;
> + if (vcpu->arch.apic &&
> + kvm_apic_match_logical_addr(vcpu->arch.apic,
> + entry->fields.dest_id))
> + __set_bit(i, deliver_bitmask);
> + }
> +
> switch (entry->fields.delivery_mode) {
> case IOAPIC_LOWEST_PRIORITY:
> + /* Select one in deliver_bitmask */
> vcpu = kvm_get_lowest_prio_vcpu(ioapic->kvm,
> entry->fields.vector, deliver_bitmask);
> + bitmap_zero(deliver_bitmask, KVM_MAX_VCPUS);
> + if (!vcpu)
> + return;
> __set_bit(vcpu->vcpu_id, deliver_bitmask);
> break;
> case IOAPIC_FIXED:
> @@ -65,7 +99,7 @@ void kvm_get_intr_delivery_bitmask(struct kvm_ioapic *ioapic,
> if (printk_ratelimit())
> printk(KERN_INFO "kvm: unsupported delivery mode %d\n",
> entry->fields.delivery_mode);
> - *deliver_bitmask = 0;
> + bitmap_zero(deliver_bitmask, KVM_MAX_VCPUS);
> }
> }
Unrelated question, what was the issue (in detail) which caused
this change again:
commit d602a5d8d74980a3c0b097c35c04036a04286018
Author: Avi Kivity <avi@qumranet.com>
Date: Mon Feb 25 10:28:31 2008 +0200
KVM: Route irq 0 to vcpu 0 exclusively
Some Linux versions allow the timer interrupt to be processed by more than
one cpu, leading to hangs due to tsc instability. Work around the issue
by only disaptching the interrupt to vcpu 0.
Problem analyzed (and patch tested) by Sheng Yang.
Signed-off-by: Avi Kivity <avi@qumranet.com>
Thanks
next prev parent reply other threads:[~2009-03-03 11:21 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2009-03-02 8:19 [PATCH] KVM: Merge kvm_ioapic_get_delivery_bitmask into kvm_get_intr_delivery_bitmask Sheng Yang
2009-03-03 11:20 ` Marcelo Tosatti [this message]
2009-03-04 2:41 ` Sheng Yang
2009-03-04 2:55 ` Marcelo Tosatti
2009-03-04 2:59 ` Sheng Yang
2009-03-04 5:33 ` [PATCH 1/1] " Sheng Yang
2009-03-04 18:30 ` Marcelo Tosatti
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20090303112037.GA13997@amt.cnet \
--to=mtosatti@redhat.com \
--cc=avi@redhat.com \
--cc=gleb@redhat.com \
--cc=kvm@vger.kernel.org \
--cc=sheng@linux.intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.