From: Paolo Bonzini <pbonzini@redhat.com> To: "Wu, Feng" <feng.wu@intel.com>, "alex.williamson@redhat.com" <alex.williamson@redhat.com>, "joro@8bytes.org" <joro@8bytes.org>, "mtosatti@redhat.com" <mtosatti@redhat.com> Cc: "eric.auger@linaro.org" <eric.auger@linaro.org>, "kvm@vger.kernel.org" <kvm@vger.kernel.org>, "iommu@lists.linux-foundation.org" <iommu@lists.linux-foundation.org>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, "Radim Krčmář" <rkrcmar@redhat.com> Subject: Re: [PATCH v8 03/13] KVM: Define a new interface kvm_intr_is_single_vcpu() Date: Thu, 17 Sep 2015 16:24:53 +0200 [thread overview] Message-ID: <55FACD35.1030602@redhat.com> (raw) In-Reply-To: <E959C4978C3B6342920538CF579893F002718968@SHSMSX104.ccr.corp.intel.com> >> On 17/09/2015 05:17, Wu, Feng wrote: >>>>>>> + if (irq->dest_mode == APIC_DEST_PHYSICAL) { >>>>>>> + if (irq->dest_id == 0xFF) >>>>>>> + goto out; >>>>>>> + >>>>>>> + if (irq->dest_id >= ARRAY_SIZE(map->phys_map)) { >>>>> >>>>> Warning here is wrong, the guest can trigger it. >>> Could you please share more information about how the guest >>> triggers these conditions (including the following two), Thanks >>> a lot! >> >> irq->dest_id is a 16-bit value, so it can be > 255. > > Yes, irq->dest_id is defined as u32, but by looking the current KVM > code, seems desst_id is used as an u8 variable, even in x2apic mode > the dest_id will not beyond 255 (except for broadcast dest in in x2apic > mode). Correct me if I am wrong. Thanks a lot! Actually you're right, the MSI destination is only 8 bits. I was confused because of #define MSI_ADDR_DEST_ID_MASK 0x00ffff0 in arch/x86/include/asm/msidef.h. But there may be a bug, see below... >>> + if (cid >= ARRAY_SIZE(map->logical_map)) { >>> + WARN_ON_ONCE(1); >> >> In x2apic mode irq->dest_id could have bits 12..15 set. > > cid is gotten from bit 16 ..31 of the ldr (in apic_logical_id()), and > in x2apic mode, ldr is constructed in kvm_apic_set_x2apic_id() as > below: > > u32 ldr = ((id >> 4) << 16) | (1 << (id & 0xf)); > > So in fact, cid is (id >> 4), I cannot think of why cid can beyond 15. I think kvm_apic_match_logical_addr for MSI and IOAPIC interrupts is buggy in x2apic mode. It does: if (apic_x2apic_mode(apic)) return ((logical_id >> 16) == (mda >> 16)) && (logical_id & mda & 0xffff) != 0; But mda is only 8-bits for MSI and IOAPIC interrupts. Radim, should kvm_apic_mda also handle the !ipi && x2apic_mda && dest_id != APIC_BROADCAST case? It never triggers with Linux because it uses only the physical mode (that's not super-easy to see; ioapic_set_affinity looks for the RTEs in irq_data->chip_data and that is allocated with kzalloc). > Do I miss something here? Thanks! No, you were right. But still I think the WARNs are unnecessary; it is conceivable that some future chipset adds support for more than 8-bits in the dest_id. Paolo > Thanks, > Feng > >> >> Paolo
WARNING: multiple messages have this Message-ID (diff)
From: Paolo Bonzini <pbonzini-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> To: "Wu, Feng" <feng.wu-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>, "alex.williamson-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org" <alex.williamson-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>, "joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org" <joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org>, "mtosatti-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org" <mtosatti-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> Cc: "iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org" <iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org>, "Radim Krčmář" <rkrcmar-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>, "linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" <linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>, "kvm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" <kvm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>, "eric.auger-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org" <eric.auger-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> Subject: Re: [PATCH v8 03/13] KVM: Define a new interface kvm_intr_is_single_vcpu() Date: Thu, 17 Sep 2015 16:24:53 +0200 [thread overview] Message-ID: <55FACD35.1030602@redhat.com> (raw) In-Reply-To: <E959C4978C3B6342920538CF579893F002718968-0J0gbvR4kTg/UvCtAeCM4rfspsVTdybXVpNB7YpNyf8@public.gmane.org> >> On 17/09/2015 05:17, Wu, Feng wrote: >>>>>>> + if (irq->dest_mode == APIC_DEST_PHYSICAL) { >>>>>>> + if (irq->dest_id == 0xFF) >>>>>>> + goto out; >>>>>>> + >>>>>>> + if (irq->dest_id >= ARRAY_SIZE(map->phys_map)) { >>>>> >>>>> Warning here is wrong, the guest can trigger it. >>> Could you please share more information about how the guest >>> triggers these conditions (including the following two), Thanks >>> a lot! >> >> irq->dest_id is a 16-bit value, so it can be > 255. > > Yes, irq->dest_id is defined as u32, but by looking the current KVM > code, seems desst_id is used as an u8 variable, even in x2apic mode > the dest_id will not beyond 255 (except for broadcast dest in in x2apic > mode). Correct me if I am wrong. Thanks a lot! Actually you're right, the MSI destination is only 8 bits. I was confused because of #define MSI_ADDR_DEST_ID_MASK 0x00ffff0 in arch/x86/include/asm/msidef.h. But there may be a bug, see below... >>> + if (cid >= ARRAY_SIZE(map->logical_map)) { >>> + WARN_ON_ONCE(1); >> >> In x2apic mode irq->dest_id could have bits 12..15 set. > > cid is gotten from bit 16 ..31 of the ldr (in apic_logical_id()), and > in x2apic mode, ldr is constructed in kvm_apic_set_x2apic_id() as > below: > > u32 ldr = ((id >> 4) << 16) | (1 << (id & 0xf)); > > So in fact, cid is (id >> 4), I cannot think of why cid can beyond 15. I think kvm_apic_match_logical_addr for MSI and IOAPIC interrupts is buggy in x2apic mode. It does: if (apic_x2apic_mode(apic)) return ((logical_id >> 16) == (mda >> 16)) && (logical_id & mda & 0xffff) != 0; But mda is only 8-bits for MSI and IOAPIC interrupts. Radim, should kvm_apic_mda also handle the !ipi && x2apic_mda && dest_id != APIC_BROADCAST case? It never triggers with Linux because it uses only the physical mode (that's not super-easy to see; ioapic_set_affinity looks for the RTEs in irq_data->chip_data and that is allocated with kzalloc). > Do I miss something here? Thanks! No, you were right. But still I think the WARNs are unnecessary; it is conceivable that some future chipset adds support for more than 8-bits in the dest_id. Paolo > Thanks, > Feng > >> >> Paolo
next prev parent reply other threads:[~2015-09-17 14:25 UTC|newest] Thread overview: 62+ messages / expand[flat|nested] mbox.gz Atom feed top 2015-09-16 8:49 [PATCH v8 00/13] Add VT-d Posted-Interrupts support Feng Wu 2015-09-16 8:49 ` Feng Wu 2015-09-16 8:49 ` [PATCH v8 01/13] KVM: Extend struct pi_desc for VT-d Posted-Interrupts Feng Wu 2015-09-16 8:49 ` Feng Wu 2015-09-16 8:49 ` [PATCH v8 02/13] KVM: Add some helper functions for Posted-Interrupts Feng Wu 2015-09-16 8:49 ` Feng Wu 2015-09-16 8:49 ` [PATCH v8 03/13] KVM: Define a new interface kvm_intr_is_single_vcpu() Feng Wu 2015-09-16 8:49 ` Feng Wu 2015-09-16 9:23 ` Paolo Bonzini 2015-09-16 9:23 ` Paolo Bonzini 2015-09-17 3:17 ` Wu, Feng 2015-09-17 3:17 ` Wu, Feng 2015-09-17 9:42 ` Paolo Bonzini 2015-09-17 13:36 ` Wu, Feng 2015-09-17 13:36 ` Wu, Feng 2015-09-17 14:24 ` Paolo Bonzini [this message] 2015-09-17 14:24 ` Paolo Bonzini 2015-09-17 15:58 ` Radim Krčmář 2015-09-17 16:00 ` Paolo Bonzini 2015-09-17 16:00 ` Paolo Bonzini 2015-09-17 23:18 ` Wu, Feng 2015-09-17 23:18 ` Wu, Feng 2015-09-18 16:16 ` Radim Krčmář 2015-09-18 16:16 ` Radim Krčmář 2015-09-18 16:17 ` Paolo Bonzini 2015-09-18 16:17 ` Paolo Bonzini 2015-09-17 23:15 ` Wu, Feng 2015-09-17 23:15 ` Wu, Feng 2015-09-16 8:50 ` [PATCH v8 04/13] KVM: Make struct kvm_irq_routing_table accessible Feng Wu 2015-09-16 8:50 ` Feng Wu 2015-09-16 8:50 ` [PATCH v8 05/13] KVM: make kvm_set_msi_irq() public Feng Wu 2015-09-16 8:50 ` Feng Wu 2015-09-16 8:50 ` [PATCH v8 06/13] vfio: Register/unregister irq_bypass_producer Feng Wu 2015-09-16 8:50 ` Feng Wu 2015-09-16 8:50 ` [PATCH v8 07/13] KVM: x86: Update IRTE for posted-interrupts Feng Wu 2015-09-16 8:50 ` Feng Wu 2015-09-16 8:50 ` [PATCH v8 08/13] KVM: Implement IRQ bypass consumer callbacks for x86 Feng Wu 2015-09-16 8:50 ` Feng Wu 2015-09-16 8:50 ` [PATCH v8 09/13] KVM: Add an arch specific hooks in 'struct kvm_kernel_irqfd' Feng Wu 2015-09-16 8:50 ` Feng Wu 2015-09-16 9:27 ` Paolo Bonzini 2015-09-16 9:27 ` Paolo Bonzini 2015-09-17 1:51 ` Wu, Feng 2015-09-17 1:51 ` Wu, Feng 2015-09-17 9:38 ` Paolo Bonzini 2015-09-17 9:38 ` Paolo Bonzini 2015-09-16 8:50 ` [PATCH v8 10/13] KVM: Update Posted-Interrupts Descriptor when vCPU is preempted Feng Wu 2015-09-16 8:50 ` Feng Wu 2015-09-16 9:29 ` Paolo Bonzini 2015-09-16 9:29 ` Paolo Bonzini 2015-09-16 8:50 ` [PATCH v8 11/13] KVM: Update Posted-Interrupts Descriptor when vCPU is blocked Feng Wu 2015-09-16 8:50 ` Feng Wu 2015-09-16 9:32 ` Paolo Bonzini 2015-09-16 9:32 ` Paolo Bonzini 2015-09-16 8:50 ` [PATCH v8 12/13] KVM: Warn if 'SN' is set during posting interrupts by software Feng Wu 2015-09-16 8:50 ` Feng Wu 2015-09-16 9:32 ` Paolo Bonzini 2015-09-16 9:32 ` Paolo Bonzini 2015-09-16 8:50 ` [PATCH v8 13/13] iommu/vt-d: Add a command line parameter for VT-d posted-interrupts Feng Wu 2015-09-16 8:50 ` Feng Wu 2015-09-16 9:34 ` [PATCH v8 00/13] Add VT-d Posted-Interrupts support Paolo Bonzini 2015-09-16 9:34 ` Paolo Bonzini
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=55FACD35.1030602@redhat.com \ --to=pbonzini@redhat.com \ --cc=alex.williamson@redhat.com \ --cc=eric.auger@linaro.org \ --cc=feng.wu@intel.com \ --cc=iommu@lists.linux-foundation.org \ --cc=joro@8bytes.org \ --cc=kvm@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=mtosatti@redhat.com \ --cc=rkrcmar@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: linkBe 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.