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

[Jan Kiszka <jan.kiszka@siemens.com>]

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