From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jan Kiszka Subject: Re: [PATCH v2] KVM: Implement support for the RH bit Date: Fri, 02 Sep 2011 14:11:23 +0200 Message-ID: <4E60C7EB.9060401@siemens.com> References: <1314949721-32761-1-git-send-email-levinsasha928@gmail.com> <4E60BDBA.5060806@siemens.com> <4E60BFD9.6090507@siemens.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit Cc: kvm@vger.kernel.org, Avi Kivity , Marcelo Tosatti , Gleb Natapov To: Sasha Levin Return-path: Received: from david.siemens.de ([192.35.17.14]:24671 "EHLO david.siemens.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751564Ab1IBML2 (ORCPT ); Fri, 2 Sep 2011 08:11:28 -0400 In-Reply-To: <4E60BFD9.6090507@siemens.com> Sender: kvm-owner@vger.kernel.org List-ID: On 2011-09-02 13:36, Jan Kiszka wrote: > On 2011-09-02 13:27, Jan Kiszka wrote: >> On 2011-09-02 09:48, Sasha Levin wrote: >>> The RH bit exists in the message address register (lower 32 bits of >>> the address). >>> >>> The bit indicates whether the message should go to the processor which was >>> indicated in the destination ID bits, or whether it should go to the >>> processor running at the lowest priority. >>> >>> Cc: Avi Kivity >>> Cc: Marcelo Tosatti >>> Signed-off-by: Sasha Levin >>> --- >>> virt/kvm/irq_comm.c | 17 ++++++++++++++++- >>> 1 files changed, 16 insertions(+), 1 deletions(-) >>> >>> diff --git a/virt/kvm/irq_comm.c b/virt/kvm/irq_comm.c >>> index 9f614b4..0ba3a3d 100644 >>> --- a/virt/kvm/irq_comm.c >>> +++ b/virt/kvm/irq_comm.c >>> @@ -134,7 +134,22 @@ int kvm_set_msi(struct kvm_kernel_irq_routing_entry *e, >>> irq.level = 1; >>> irq.shorthand = 0; >>> >>> - /* TODO Deal with RH bit of MSI message address */ >>> + /* >>> + * If the RH bit is set, we'll deliver to the processor running >>> + * at the lowest priority. >>> + */ >>> + if (e->msi.address_lo & MSI_ADDR_REDIRECTION_LOWPRI) { >>> + irq.delivery_mode = MSI_DATA_DELIVERY_LOWPRI; >>> + } else { >>> + /* >>> + * If the RH bit is not set, we'll deliver to the specific >>> + * processor mentioned in destination ID, and ignore the DM >>> + * bit. >>> + */ >>> + irq.dest_mode = MSI_ADDR_DEST_MODE_PHYSICAL; >>> + irq.delivery_mode = MSI_DATA_DELIVERY_FIXED; >>> + } >>> + >>> return kvm_irq_delivery_to_apic(kvm, NULL, &irq); >>> } >>> >> >> Do you happen have a kvm unit test for this? Or how did you validate the >> change? It doesn't look incorrect to me, I'd just like to check it QEMU >> as well which apparently already has the logic above but also some >> contradictory comment. > > Err, no, QEMU does not have this logic, it also ignores RH. > > But the above bits make "irq.delivery_mode = e->msi.data & 0x700" > pointless. And that strongly suggests something is still wrong. I tend to believe that this is what the spec tries to tell us: diff --git a/virt/kvm/irq_comm.c b/virt/kvm/irq_comm.c index 9f614b4..b72f77a 100644 --- a/virt/kvm/irq_comm.c +++ b/virt/kvm/irq_comm.c @@ -128,7 +128,8 @@ int kvm_set_msi(struct kvm_kernel_irq_routing_entry *e, MSI_ADDR_DEST_ID_MASK) >> MSI_ADDR_DEST_ID_SHIFT; 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 = ((e->msi.address_lo & MSI_ADDR_DEST_MODE_LOGICAL) && + (e->msi.address_lo & MSI_ADDR_REDIRECTION_LOWPRI)); irq.trig_mode = (1 << MSI_DATA_TRIGGER_SHIFT) & e->msi.data; irq.delivery_mode = e->msi.data & 0x700; irq.level = 1; ie. the DM flag is only relevant if RH is set, and RH==0 is equivalent to RH==1 && DH==0. BTW, irq_comm.c is surely the wrong place for all this IA32-specific interpretation of MSI address and data. And we have yet another guest-triggerable printk in kvm_irq_delivery_to_apic (messages to physical ID 0xff). Jan -- Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux