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

[Gleb Natapov <gleb@redhat.com>]

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.