* [PATCH v2] KVM: Implement support for the RH bit @ 2011-09-02 7:48 Sasha Levin 2011-09-02 11:27 ` Jan Kiszka 0 siblings, 1 reply; 16+ messages in thread From: Sasha Levin @ 2011-09-02 7:48 UTC (permalink / raw) To: kvm; +Cc: Sasha Levin, Avi Kivity, Marcelo Tosatti 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 <avi@redhat.com> Cc: Marcelo Tosatti <mtosatti@redhat.com> Signed-off-by: Sasha Levin <levinsasha928@gmail.com> --- 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); } -- 1.7.6.1 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v2] KVM: Implement support for the RH bit 2011-09-02 7:48 [PATCH v2] KVM: Implement support for the RH bit Sasha Levin @ 2011-09-02 11:27 ` Jan Kiszka 2011-09-02 11:36 ` Jan Kiszka 0 siblings, 1 reply; 16+ messages in thread From: Jan Kiszka @ 2011-09-02 11:27 UTC (permalink / raw) To: Sasha Levin; +Cc: kvm, Avi Kivity, Marcelo Tosatti 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 <avi@redhat.com> > Cc: Marcelo Tosatti <mtosatti@redhat.com> > Signed-off-by: Sasha Levin <levinsasha928@gmail.com> > --- > 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. Jan -- Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] KVM: Implement support for the RH bit 2011-09-02 11:27 ` Jan Kiszka @ 2011-09-02 11:36 ` Jan Kiszka 2011-09-02 12:11 ` Jan Kiszka 2011-09-02 12:25 ` Gleb Natapov 0 siblings, 2 replies; 16+ messages in thread From: Jan Kiszka @ 2011-09-02 11:36 UTC (permalink / raw) To: Sasha Levin; +Cc: kvm, Avi Kivity, Marcelo Tosatti 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 <avi@redhat.com> >> Cc: Marcelo Tosatti <mtosatti@redhat.com> >> Signed-off-by: Sasha Levin <levinsasha928@gmail.com> >> --- >> 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. Jan -- Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] KVM: Implement support for the RH bit 2011-09-02 11:36 ` Jan Kiszka @ 2011-09-02 12:11 ` Jan Kiszka 2011-09-02 13:13 ` Sasha Levin 2011-09-02 12:25 ` Gleb Natapov 1 sibling, 1 reply; 16+ messages in thread From: Jan Kiszka @ 2011-09-02 12:11 UTC (permalink / raw) To: Sasha Levin; +Cc: kvm, Avi Kivity, Marcelo Tosatti, Gleb Natapov 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 <avi@redhat.com> >>> Cc: Marcelo Tosatti <mtosatti@redhat.com> >>> Signed-off-by: Sasha Levin <levinsasha928@gmail.com> >>> --- >>> 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 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v2] KVM: Implement support for the RH bit 2011-09-02 12:11 ` Jan Kiszka @ 2011-09-02 13:13 ` Sasha Levin 2011-09-02 14:00 ` Jan Kiszka 0 siblings, 1 reply; 16+ messages in thread From: Sasha Levin @ 2011-09-02 13:13 UTC (permalink / raw) To: Jan Kiszka; +Cc: kvm, Avi Kivity, Marcelo Tosatti, Gleb Natapov 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 <avi@redhat.com> > >>> Cc: Marcelo Tosatti <mtosatti@redhat.com> > >>> Signed-off-by: Sasha Levin <levinsasha928@gmail.com> > >>> --- > >>> 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. Maybe we should set irq.delivery_mode only if RH==1? > > 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 > -- Sasha. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] KVM: Implement support for the RH bit 2011-09-02 13:13 ` Sasha Levin @ 2011-09-02 14:00 ` Jan Kiszka 2011-09-02 14:11 ` Sasha Levin 0 siblings, 1 reply; 16+ messages in thread From: Jan Kiszka @ 2011-09-02 14:00 UTC (permalink / raw) To: Sasha Levin; +Cc: kvm, Avi Kivity, Marcelo Tosatti, Gleb Natapov, Tian, Kevin 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 <avi@redhat.com> >>>>> Cc: Marcelo Tosatti <mtosatti@redhat.com> >>>>> Signed-off-by: Sasha Levin <levinsasha928@gmail.com> >>>>> --- >>>>> 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] KVM: Implement support for the RH bit 2011-09-02 14:00 ` Jan Kiszka @ 2011-09-02 14:11 ` Sasha Levin 2011-09-02 14:25 ` Jan Kiszka 0 siblings, 1 reply; 16+ messages in thread From: Sasha Levin @ 2011-09-02 14:11 UTC (permalink / raw) To: Jan Kiszka; +Cc: kvm, Avi Kivity, Marcelo Tosatti, Gleb Natapov, Tian, Kevin On Fri, 2011-09-02 at 16:00 +0200, Jan Kiszka wrote: > 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 <avi@redhat.com> > >>>>> Cc: Marcelo Tosatti <mtosatti@redhat.com> > >>>>> Signed-off-by: Sasha Levin <levinsasha928@gmail.com> > >>>>> --- > >>>>> 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." > When RH=1 and DM=0 yes, but what happens when RH=1 and DM=1? >From what I understand we need to select the processor running at the lowest priority between the processors that are identified by the destination ID: "If RH is 1 and DM is 1, the Destination ID Field is interpreted as in logical destination mode and the redirection is limited to only those processors that are part of the logical group of processors based on the processor's logical APIC ID and the Destination ID field in the message." > 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 > -- Sasha. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] KVM: Implement support for the RH bit 2011-09-02 14:11 ` Sasha Levin @ 2011-09-02 14:25 ` Jan Kiszka 2011-09-02 14:30 ` Sasha Levin 0 siblings, 1 reply; 16+ messages in thread From: Jan Kiszka @ 2011-09-02 14:25 UTC (permalink / raw) To: Sasha Levin; +Cc: kvm, Avi Kivity, Marcelo Tosatti, Gleb Natapov, Tian, Kevin On 2011-09-02 16:11, Sasha Levin wrote: > On Fri, 2011-09-02 at 16:00 +0200, Jan Kiszka wrote: >> 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 <avi@redhat.com> >>>>>>> Cc: Marcelo Tosatti <mtosatti@redhat.com> >>>>>>> Signed-off-by: Sasha Levin <levinsasha928@gmail.com> >>>>>>> --- >>>>>>> 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." >> > > When RH=1 and DM=0 yes, but what happens when RH=1 and DM=1? irq.dest_mode becomes non-zero, and kvm_apic_match_dest uses kvm_apic_match_logical_addr for filtering out possible target CPUs. Mmh, a remaining question is if kvm_irq_delivery_to_apic is then already doing the right thing, even for delivery_mode != APIC_DM_LOWEST. Again my question to you: Did you observe unexpected behaviour with some real guests, or is this just based on code and spec study so far? If we had a test case, that could also provide valuable hints. Jan -- Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] KVM: Implement support for the RH bit 2011-09-02 14:25 ` Jan Kiszka @ 2011-09-02 14:30 ` Sasha Levin 2011-09-02 14:36 ` Jan Kiszka 0 siblings, 1 reply; 16+ messages in thread From: Sasha Levin @ 2011-09-02 14:30 UTC (permalink / raw) To: Jan Kiszka; +Cc: kvm, Avi Kivity, Marcelo Tosatti, Gleb Natapov, Tian, Kevin On Fri, 2011-09-02 at 16:25 +0200, Jan Kiszka wrote: > On 2011-09-02 16:11, Sasha Levin wrote: > > On Fri, 2011-09-02 at 16:00 +0200, Jan Kiszka wrote: > >> 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 <avi@redhat.com> > >>>>>>> Cc: Marcelo Tosatti <mtosatti@redhat.com> > >>>>>>> Signed-off-by: Sasha Levin <levinsasha928@gmail.com> > >>>>>>> --- > >>>>>>> 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." > >> > > > > When RH=1 and DM=0 yes, but what happens when RH=1 and DM=1? > > irq.dest_mode becomes non-zero, and kvm_apic_match_dest uses > kvm_apic_match_logical_addr for filtering out possible target CPUs. > > Mmh, a remaining question is if kvm_irq_delivery_to_apic is then already > doing the right thing, even for delivery_mode != APIC_DM_LOWEST. > The missing part is that when RH=1 we must look for the lowest priority: "Redirection hint indication (RH) - This bit indicates whether the message should be directed to the processor with the lowest interrupt priority among processors that can receive the interrupt." So it's not enough to set dest_mode, we must also make sure that delivery_mode is set to low prio when RH=1. > Again my question to you: Did you observe unexpected behaviour with some > real guests, or is this just based on code and spec study so far? If we > had a test case, that could also provide valuable hints. Sorry, no test case. I've stumbled on the 'TODO' comment when I was digging into the MSI implementation in KVM and decided to implement it based on specs. > > Jan > -- Sasha. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] KVM: Implement support for the RH bit 2011-09-02 14:30 ` Sasha Levin @ 2011-09-02 14:36 ` Jan Kiszka 2011-09-02 14:44 ` Gleb Natapov 0 siblings, 1 reply; 16+ messages in thread From: Jan Kiszka @ 2011-09-02 14:36 UTC (permalink / raw) To: Sasha Levin; +Cc: kvm, Avi Kivity, Marcelo Tosatti, Gleb Natapov, Tian, Kevin On 2011-09-02 16:30, Sasha Levin wrote: > On Fri, 2011-09-02 at 16:25 +0200, Jan Kiszka wrote: >> On 2011-09-02 16:11, Sasha Levin wrote: >>> On Fri, 2011-09-02 at 16:00 +0200, Jan Kiszka wrote: >>>> 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 <avi@redhat.com> >>>>>>>>> Cc: Marcelo Tosatti <mtosatti@redhat.com> >>>>>>>>> Signed-off-by: Sasha Levin <levinsasha928@gmail.com> >>>>>>>>> --- >>>>>>>>> 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." >>>> >>> >>> When RH=1 and DM=0 yes, but what happens when RH=1 and DM=1? >> >> irq.dest_mode becomes non-zero, and kvm_apic_match_dest uses >> kvm_apic_match_logical_addr for filtering out possible target CPUs. >> >> Mmh, a remaining question is if kvm_irq_delivery_to_apic is then already >> doing the right thing, even for delivery_mode != APIC_DM_LOWEST. >> > > The missing part is that when RH=1 we must look for the lowest priority: > > "Redirection hint indication (RH) - This bit indicates whether the > message should be directed to the processor with the lowest interrupt > priority among processors that can receive the interrupt." > > So it's not enough to set dest_mode, we must also make sure that > delivery_mode is set to low prio when RH=1. That's debatable. delivery_mode == APIC_DM_LOWEST includes this target selection, but also more. I have a bad feeling when we just overwrite delivery_mode as defined by the MSI data field instead of only patching kvm_irq_delivery_to_apic or kvm_is_dm_lowest_prio - if required. > >> Again my question to you: Did you observe unexpected behaviour with some >> real guests, or is this just based on code and spec study so far? If we >> had a test case, that could also provide valuable hints. > > Sorry, no test case. > > I've stumbled on the 'TODO' comment when I was digging into the MSI > implementation in KVM and decided to implement it based on specs. Then we definitely need some blessing by Intel to avoid subtle regressions. Jan -- Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] KVM: Implement support for the RH bit 2011-09-02 14:36 ` Jan Kiszka @ 2011-09-02 14:44 ` Gleb Natapov 2011-09-02 14:52 ` Jan Kiszka 0 siblings, 1 reply; 16+ messages in thread From: Gleb Natapov @ 2011-09-02 14:44 UTC (permalink / raw) To: Jan Kiszka; +Cc: Sasha Levin, kvm, Avi Kivity, Marcelo Tosatti, Tian, Kevin On Fri, Sep 02, 2011 at 04:36:46PM +0200, Jan Kiszka wrote: > On 2011-09-02 16:30, Sasha Levin wrote: > > On Fri, 2011-09-02 at 16:25 +0200, Jan Kiszka wrote: > >> On 2011-09-02 16:11, Sasha Levin wrote: > >>> On Fri, 2011-09-02 at 16:00 +0200, Jan Kiszka wrote: > >>>> 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 <avi@redhat.com> > >>>>>>>>> Cc: Marcelo Tosatti <mtosatti@redhat.com> > >>>>>>>>> Signed-off-by: Sasha Levin <levinsasha928@gmail.com> > >>>>>>>>> --- > >>>>>>>>> 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." > >>>> > >>> > >>> When RH=1 and DM=0 yes, but what happens when RH=1 and DM=1? > >> > >> irq.dest_mode becomes non-zero, and kvm_apic_match_dest uses > >> kvm_apic_match_logical_addr for filtering out possible target CPUs. > >> > >> Mmh, a remaining question is if kvm_irq_delivery_to_apic is then already > >> doing the right thing, even for delivery_mode != APIC_DM_LOWEST. > >> > > > > The missing part is that when RH=1 we must look for the lowest priority: > > > > "Redirection hint indication (RH) - This bit indicates whether the > > message should be directed to the processor with the lowest interrupt > > priority among processors that can receive the interrupt." > > > > So it's not enough to set dest_mode, we must also make sure that > > delivery_mode is set to low prio when RH=1. > > That's debatable. delivery_mode == APIC_DM_LOWEST includes this target > selection, but also more. I have a bad feeling when we just overwrite > delivery_mode as defined by the MSI data field instead of only patching > kvm_irq_delivery_to_apic or kvm_is_dm_lowest_prio - if required. > Patching them how? To behave exactly like delivery_mode == APIC_DM_LOWEST in case RH bit is set? Then setting delivery_mode to APIC_DM_LOWEST will achieve the same goal. > > > >> Again my question to you: Did you observe unexpected behaviour with some > >> real guests, or is this just based on code and spec study so far? If we > >> had a test case, that could also provide valuable hints. > > > > Sorry, no test case. > > > > I've stumbled on the 'TODO' comment when I was digging into the MSI > > implementation in KVM and decided to implement it based on specs. > > Then we definitely need some blessing by Intel to avoid subtle regressions. > Yes, if we are going to pursue that we need Intel to clarify what SDM means. -- Gleb. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] KVM: Implement support for the RH bit 2011-09-02 14:44 ` Gleb Natapov @ 2011-09-02 14:52 ` Jan Kiszka 2011-09-02 15:03 ` Gleb Natapov 0 siblings, 1 reply; 16+ messages in thread From: Jan Kiszka @ 2011-09-02 14:52 UTC (permalink / raw) To: Gleb Natapov; +Cc: Sasha Levin, kvm, Avi Kivity, Marcelo Tosatti, Tian, Kevin On 2011-09-02 16:44, Gleb Natapov wrote: > On Fri, Sep 02, 2011 at 04:36:46PM +0200, Jan Kiszka wrote: >> On 2011-09-02 16:30, Sasha Levin wrote: >>> On Fri, 2011-09-02 at 16:25 +0200, Jan Kiszka wrote: >>>> On 2011-09-02 16:11, Sasha Levin wrote: >>>>> On Fri, 2011-09-02 at 16:00 +0200, Jan Kiszka wrote: >>>>>> 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 <avi@redhat.com> >>>>>>>>>>> Cc: Marcelo Tosatti <mtosatti@redhat.com> >>>>>>>>>>> Signed-off-by: Sasha Levin <levinsasha928@gmail.com> >>>>>>>>>>> --- >>>>>>>>>>> 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." >>>>>> >>>>> >>>>> When RH=1 and DM=0 yes, but what happens when RH=1 and DM=1? >>>> >>>> irq.dest_mode becomes non-zero, and kvm_apic_match_dest uses >>>> kvm_apic_match_logical_addr for filtering out possible target CPUs. >>>> >>>> Mmh, a remaining question is if kvm_irq_delivery_to_apic is then already >>>> doing the right thing, even for delivery_mode != APIC_DM_LOWEST. >>>> >>> >>> The missing part is that when RH=1 we must look for the lowest priority: >>> >>> "Redirection hint indication (RH) - This bit indicates whether the >>> message should be directed to the processor with the lowest interrupt >>> priority among processors that can receive the interrupt." >>> >>> So it's not enough to set dest_mode, we must also make sure that >>> delivery_mode is set to low prio when RH=1. >> >> That's debatable. delivery_mode == APIC_DM_LOWEST includes this target >> selection, but also more. I have a bad feeling when we just overwrite >> delivery_mode as defined by the MSI data field instead of only patching >> kvm_irq_delivery_to_apic or kvm_is_dm_lowest_prio - if required. >> > Patching them how? To behave exactly like delivery_mode == APIC_DM_LOWEST in > case RH bit is set? Then setting delivery_mode to APIC_DM_LOWEST will > achieve the same goal. /Wrt selecting the target CPU, but not regarding the vector type delivered to that CPU (think of obscure things like RH=1, delivery_mode=APIC_DM_NMI). If RH=1 only meant hard-wiring delivery_mode to single value, then this would be redundant encoding. And that's always suspicious (unless there is legacy involved). Jan -- Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] KVM: Implement support for the RH bit 2011-09-02 14:52 ` Jan Kiszka @ 2011-09-02 15:03 ` Gleb Natapov 0 siblings, 0 replies; 16+ messages in thread From: Gleb Natapov @ 2011-09-02 15:03 UTC (permalink / raw) To: Jan Kiszka; +Cc: Sasha Levin, kvm, Avi Kivity, Marcelo Tosatti, Tian, Kevin On Fri, Sep 02, 2011 at 04:52:51PM +0200, Jan Kiszka wrote: > >> That's debatable. delivery_mode == APIC_DM_LOWEST includes this target > >> selection, but also more. I have a bad feeling when we just overwrite > >> delivery_mode as defined by the MSI data field instead of only patching > >> kvm_irq_delivery_to_apic or kvm_is_dm_lowest_prio - if required. > >> > > Patching them how? To behave exactly like delivery_mode == APIC_DM_LOWEST in > > case RH bit is set? Then setting delivery_mode to APIC_DM_LOWEST will > > achieve the same goal. > > /Wrt selecting the target CPU, but not regarding the vector type > delivered to that CPU (think of obscure things like RH=1, > delivery_mode=APIC_DM_NMI). If RH=1 only meant hard-wiring delivery_mode > to single value, then this would be redundant encoding. And that's > always suspicious (unless there is legacy involved). > Hmm, good point. -- Gleb. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] KVM: Implement support for the RH bit 2011-09-02 11:36 ` Jan Kiszka 2011-09-02 12:11 ` Jan Kiszka @ 2011-09-02 12:25 ` Gleb Natapov 2011-09-02 13:00 ` Jan Kiszka 1 sibling, 1 reply; 16+ messages in thread From: Gleb Natapov @ 2011-09-02 12:25 UTC (permalink / raw) To: Jan Kiszka; +Cc: Sasha Levin, kvm, Avi Kivity, Marcelo Tosatti On Fri, Sep 02, 2011 at 01:36:57PM +0200, 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 <avi@redhat.com> > >> Cc: Marcelo Tosatti <mtosatti@redhat.com> > >> Signed-off-by: Sasha Levin <levinsasha928@gmail.com> > >> --- > >> 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. > Yes, looks like delivery_mode assignment in the else clause is not needed. This RH bit is strange. How is it different from setting delivery mode to lowest priority in the data register? What practical problem Sasha tries to fix here? -- Gleb. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] KVM: Implement support for the RH bit 2011-09-02 12:25 ` Gleb Natapov @ 2011-09-02 13:00 ` Jan Kiszka 2011-09-02 14:22 ` Gleb Natapov 0 siblings, 1 reply; 16+ messages in thread From: Jan Kiszka @ 2011-09-02 13:00 UTC (permalink / raw) To: Gleb Natapov; +Cc: Sasha Levin, kvm, Avi Kivity, Marcelo Tosatti On 2011-09-02 14:25, Gleb Natapov wrote: > On Fri, Sep 02, 2011 at 01:36:57PM +0200, 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 <avi@redhat.com> >>>> Cc: Marcelo Tosatti <mtosatti@redhat.com> >>>> Signed-off-by: Sasha Levin <levinsasha928@gmail.com> >>>> --- >>>> 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. >> > Yes, looks like delivery_mode assignment in the else clause is not > needed. This RH bit is strange. How is it different from setting > delivery mode to lowest priority in the data register? What practical > problem Sasha tries to fix here? Logical addressing should not be available without RH==1. It was so far nevertheless because we evaluated DM even if RH was 0. Jan -- Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] KVM: Implement support for the RH bit 2011-09-02 13:00 ` Jan Kiszka @ 2011-09-02 14:22 ` Gleb Natapov 0 siblings, 0 replies; 16+ messages in thread From: Gleb Natapov @ 2011-09-02 14:22 UTC (permalink / raw) To: Jan Kiszka; +Cc: Sasha Levin, kvm, Avi Kivity, Marcelo Tosatti On Fri, Sep 02, 2011 at 03:00:05PM +0200, Jan Kiszka wrote: > On 2011-09-02 14:25, Gleb Natapov wrote: > > On Fri, Sep 02, 2011 at 01:36:57PM +0200, 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 <avi@redhat.com> > >>>> Cc: Marcelo Tosatti <mtosatti@redhat.com> > >>>> Signed-off-by: Sasha Levin <levinsasha928@gmail.com> > >>>> --- > >>>> 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. > >> > > Yes, looks like delivery_mode assignment in the else clause is not > > needed. This RH bit is strange. How is it different from setting > > delivery mode to lowest priority in the data register? What practical > > problem Sasha tries to fix here? > > Logical addressing should not be available without RH==1. It was so far > nevertheless because we evaluated DM even if RH was 0. > Spec says "If RH is 0, then the DM bit is ignored". I don't know how switch (which DM bit really is) can be ignored though. It either logical of physical, there is not "ignore" state. You are saying that this mysterious phrase from the spec should be interpreted as "If RH is 0, then the DM bit is always treated as if it is 0" and you may be right, but clarification from Intel would be nice. -- Gleb. ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2011-09-02 15:03 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2011-09-02 7:48 [PATCH v2] KVM: Implement support for the RH bit Sasha Levin 2011-09-02 11:27 ` Jan Kiszka 2011-09-02 11:36 ` Jan Kiszka 2011-09-02 12:11 ` Jan Kiszka 2011-09-02 13:13 ` Sasha Levin 2011-09-02 14:00 ` Jan Kiszka 2011-09-02 14:11 ` Sasha Levin 2011-09-02 14:25 ` Jan Kiszka 2011-09-02 14:30 ` Sasha Levin 2011-09-02 14:36 ` Jan Kiszka 2011-09-02 14:44 ` Gleb Natapov 2011-09-02 14:52 ` Jan Kiszka 2011-09-02 15:03 ` Gleb Natapov 2011-09-02 12:25 ` Gleb Natapov 2011-09-02 13:00 ` Jan Kiszka 2011-09-02 14:22 ` Gleb Natapov
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.