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 16:00:51 +0200 Message-ID: <4E60E193.1050804@siemens.com> References: <1314949721-32761-1-git-send-email-levinsasha928@gmail.com> <4E60BDBA.5060806@siemens.com> <4E60BFD9.6090507@siemens.com> <4E60C7EB.9060401@siemens.com> <1314969237.31676.4.camel@lappy> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Cc: "kvm@vger.kernel.org" , Avi Kivity , Marcelo Tosatti , Gleb Natapov , "Tian, Kevin" To: Sasha Levin Return-path: Received: from goliath.siemens.de ([192.35.17.28]:34755 "EHLO goliath.siemens.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752344Ab1IBOA6 (ORCPT ); Fri, 2 Sep 2011 10:00:58 -0400 In-Reply-To: <1314969237.31676.4.camel@lappy> Sender: kvm-owner@vger.kernel.org List-ID: On 2011-09-02 15:13, Sasha Levin wrote: > On Fri, 2011-09-02 at 14:11 +0200, Jan Kiszka wrote: >> 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. > > Thing is, the spec specifically states that RH==1 should deliver to > lowest priority - even though it doesn't state whats the relationship > between delivery mode and RH bit. The spec says "When RH is 1 and the physical destination mode is used [DM=0], the Destination ID field must not be set to 0xFF; it must point to a processor that is present and enabled to receive the interrupt." As far as I understand, there is no "lowest prio" in the setup RH=1/DM=0. I've cc'ed Kevin who just worked on the APIC model. Maybe he can provide some authoritative answer or dig it up @Intel. Jan -- Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux