Re: [PATCH v2] KVM: Implement support for the RH bit

[Sasha Levin <levinsasha928@gmail.com>]

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.