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

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

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