From: Sean Christopherson <sean.j.christopherson@intel.com>
To: Peter Xu <peterx@redhat.com>
Cc: linux-kernel@vger.kernel.org, kvm@vger.kernel.org,
Nitesh Narayan Lal <nitesh@redhat.com>,
Paolo Bonzini <pbonzini@redhat.com>,
Vitaly Kuznetsov <vkuznets@redhat.com>
Subject: Re: [PATCH v4 3/6] KVM: X86: Use APIC_DEST_* macros properly in kvm_lapic_irq.dest_mode
Date: Tue, 3 Dec 2019 14:07:52 -0800 [thread overview]
Message-ID: <20191203220752.GJ19877@linux.intel.com> (raw)
In-Reply-To: <20191203165903.22917-4-peterx@redhat.com>
On Tue, Dec 03, 2019 at 11:59:00AM -0500, Peter Xu wrote:
> We were using either APIC_DEST_PHYSICAL|APIC_DEST_LOGICAL or 0|1 to
> fill in kvm_lapic_irq.dest_mode. It's fine only because in most cases
> when we check against dest_mode it's against APIC_DEST_PHYSICAL (which
> equals to 0). However, that's not consistent. We'll have problem
> when we want to start checking against APIC_DEST_LOGICAL, which does
> not equals to 1.
>
> This patch firstly introduces kvm_lapic_irq_dest_mode() helper to take
> any boolean of destination mode and return the APIC_DEST_* macro.
> Then, it replaces the 0|1 settings of irq.dest_mode with the helper.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
> arch/x86/include/asm/kvm_host.h | 5 +++++
> arch/x86/kvm/ioapic.c | 9 ++++++---
> arch/x86/kvm/irq_comm.c | 7 ++++---
> arch/x86/kvm/x86.c | 2 +-
> 4 files changed, 16 insertions(+), 7 deletions(-)
>
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index b79cd6aa4075..f815c97b1b57 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -1022,6 +1022,11 @@ struct kvm_lapic_irq {
> bool msi_redir_hint;
> };
>
> +static inline u16 kvm_lapic_irq_dest_mode(bool dest_mode)
> +{
> + return dest_mode ? APIC_DEST_LOGICAL : APIC_DEST_PHYSICAL;
IMO this belongs in ioapic.c as it's specifically provided for converting
an I/O APIC redirection entry into a local APIC destination mode. Without
the I/O APIC context, %true==APIC_DEST_LOGICAL looks like a completely
arbitrary decision. And if it's in ioapic.c, it can take the union
of a bool, which avoids the casting and shortens the callers. E.g.:
static u64 ioapic_to_lapic_dest_mode(union kvm_ioapic_redirect_entry *e)
{
return e->fields.dest_mode ? APIC_DEST_LOGICAL : APIC_DEST_PHYSICAL;
}
The other option would be to use the same approach as delivery_mode and
open code the shift.
> +}
> +
> struct kvm_x86_ops {
> int (*cpu_has_kvm_support)(void); /* __init */
> int (*disabled_by_bios)(void); /* __init */
> diff --git a/arch/x86/kvm/ioapic.c b/arch/x86/kvm/ioapic.c
> index 9fd2dd89a1c5..e623a4f8d27e 100644
> --- a/arch/x86/kvm/ioapic.c
> +++ b/arch/x86/kvm/ioapic.c
> @@ -331,7 +331,8 @@ static void ioapic_write_indirect(struct kvm_ioapic *ioapic, u32 val)
> irq.vector = e->fields.vector;
> irq.delivery_mode = e->fields.delivery_mode << 8;
> irq.dest_id = e->fields.dest_id;
> - irq.dest_mode = e->fields.dest_mode;
> + irq.dest_mode =
> + kvm_lapic_irq_dest_mode(!!e->fields.dest_mode);
> bitmap_zero(&vcpu_bitmap, 16);
> kvm_bitmap_or_dest_vcpus(ioapic->kvm, &irq,
> &vcpu_bitmap);
> @@ -343,7 +344,9 @@ static void ioapic_write_indirect(struct kvm_ioapic *ioapic, u32 val)
> * keep ioapic_handled_vectors synchronized.
> */
> irq.dest_id = old_dest_id;
> - irq.dest_mode = old_dest_mode;
> + irq.dest_mode =
> + kvm_lapic_irq_dest_mode(
> + !!e->fields.dest_mode);
> kvm_bitmap_or_dest_vcpus(ioapic->kvm, &irq,
> &vcpu_bitmap);
> }
> @@ -369,7 +372,7 @@ static int ioapic_service(struct kvm_ioapic *ioapic, int irq, bool line_status)
>
> irqe.dest_id = entry->fields.dest_id;
> irqe.vector = entry->fields.vector;
> - irqe.dest_mode = entry->fields.dest_mode;
> + irqe.dest_mode = kvm_lapic_irq_dest_mode(!!entry->fields.dest_mode);
> irqe.trig_mode = entry->fields.trig_mode;
> irqe.delivery_mode = entry->fields.delivery_mode << 8;
> irqe.level = 1;
> diff --git a/arch/x86/kvm/irq_comm.c b/arch/x86/kvm/irq_comm.c
> index 8ecd48d31800..22108ed66a76 100644
> --- a/arch/x86/kvm/irq_comm.c
> +++ b/arch/x86/kvm/irq_comm.c
> @@ -52,8 +52,8 @@ int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
> unsigned long dest_vcpu_bitmap[BITS_TO_LONGS(KVM_MAX_VCPUS)];
> unsigned int dest_vcpus = 0;
>
> - if (irq->dest_mode == 0 && irq->dest_id == 0xff &&
> - kvm_lowest_prio_delivery(irq)) {
> + if (irq->dest_mode == APIC_DEST_PHYSICAL &&
> + irq->dest_id == 0xff && kvm_lowest_prio_delivery(irq)) {
> printk(KERN_INFO "kvm: apic: phys broadcast and lowest prio\n");
> irq->delivery_mode = APIC_DM_FIXED;
> }
> @@ -114,7 +114,8 @@ void kvm_set_msi_irq(struct kvm *kvm, struct kvm_kernel_irq_routing_entry *e,
> irq->dest_id |= MSI_ADDR_EXT_DEST_ID(e->msi.address_hi);
> irq->vector = (e->msi.data &
> MSI_DATA_VECTOR_MASK) >> MSI_DATA_VECTOR_SHIFT;
> - irq->dest_mode = (1 << MSI_ADDR_DEST_MODE_SHIFT) & e->msi.address_lo;
> + irq->dest_mode = kvm_lapic_irq_dest_mode(
> + !!((1 << MSI_ADDR_DEST_MODE_SHIFT) & e->msi.address_lo));
> irq->trig_mode = (1 << MSI_DATA_TRIGGER_SHIFT) & e->msi.data;
> irq->delivery_mode = e->msi.data & 0x700;
> irq->msi_redir_hint = ((e->msi.address_lo
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 3ed167e039e5..3b00d662dc14 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -7356,7 +7356,7 @@ static void kvm_pv_kick_cpu_op(struct kvm *kvm, unsigned long flags, int apicid)
> struct kvm_lapic_irq lapic_irq;
>
> lapic_irq.shorthand = 0;
> - lapic_irq.dest_mode = 0;
> + lapic_irq.dest_mode = APIC_DEST_PHYSICAL;
> lapic_irq.level = 0;
> lapic_irq.dest_id = apicid;
> lapic_irq.msi_redir_hint = false;
> --
> 2.21.0
>
next prev parent reply other threads:[~2019-12-03 22:07 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-12-03 16:58 [PATCH v4 0/6] KVM: X86: Cleanups on dest_mode and headers Peter Xu
2019-12-03 16:58 ` [PATCH v4 1/6] KVM: X86: Fix kvm_bitmap_or_dest_vcpus() to use irq shorthand Peter Xu
2019-12-03 16:58 ` [PATCH v4 2/6] KVM: X86: Move irrelevant declarations out of ioapic.h Peter Xu
2019-12-03 16:59 ` [PATCH v4 3/6] KVM: X86: Use APIC_DEST_* macros properly in kvm_lapic_irq.dest_mode Peter Xu
2019-12-03 22:07 ` Sean Christopherson [this message]
2019-12-03 22:15 ` Peter Xu
2019-12-03 22:20 ` Sean Christopherson
2019-12-04 18:48 ` Peter Xu
2019-12-03 16:59 ` [PATCH v4 4/6] KVM: X86: Drop KVM_APIC_SHORT_MASK and KVM_APIC_DEST_MASK Peter Xu
2019-12-03 16:59 ` [PATCH v4 5/6] KVM: X86: Fix callers of kvm_apic_match_dest() to use correct macros Peter Xu
2019-12-03 16:59 ` [PATCH v4 6/6] KVM: X86: Conert the last users of "shorthand = 0" to use macros Peter Xu
2019-12-04 16:12 ` Vitaly Kuznetsov
2019-12-04 18:53 ` Peter Xu
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=20191203220752.GJ19877@linux.intel.com \
--to=sean.j.christopherson@intel.com \
--cc=kvm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=nitesh@redhat.com \
--cc=pbonzini@redhat.com \
--cc=peterx@redhat.com \
--cc=vkuznets@redhat.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.