Re: [Patch v5] x86: irq_comm: Add check for RH bit in kvm_set_msi_irq

[James Sullivan <sullivan.james.f@gmail.com>]

N.B. This patch has been re-submitted in a larger patch, see

<1426555822-3280-1-git-send-email-sullivan.james.f@gmail.com>

(The new patch relies on changes made in this patch, and as such
it makes more sense to bundle them)

On 03/13/2015 09:14 AM, James Sullivan wrote:
> This patch adds a check for RH=1 in kvm_set_msi_irq. Currently the
> DM bit is the only thing used to decide irq->dest_mode (logical when DM
> set, physical when unset). Documentation indicates that the DM bit will
> be 'ignored' when the RH bit is unset, and physical destination mode is
> used in this case.
> 
> Fixed this to set irq->dest_mode to APIC_DEST_LOGICAL just in case both
> RH=1/DM=1.
> 
> This patch doesn't completely handle RH=1; if RH=1 then the delivery will behave
> as in low priority mode (deliver the interrupt to only the lowest priority processor),
> but the delivery mode may still used to specify the semantics of the delivery beyond
> its destination.
> 
> I will be trying and comparing a few options to handle this fully (extension of
> struct kvm_lapic_irq, introduction of MSI specific delivery functions or helpers,
> etc) and hope to have some patches to show in the near future.
> 
> 
> Signed-off-by: James Sullivan <sullivan.james.f@gmail.com>
> ---
> Changes since v2:
>     * Added one time warning message when RH=1
>     * Documented conflict between RH=1 and delivery mode
>     * Tidied code to check RH=1/DM=1 (remove bool phys, use if/else)
> Changes since v3:
>     * Fixed logical error in RH=1/DM=1 check
>     * Aligned quotation blocks in multiline pr_warn_once argument
> Changes since v4:
>     * Put error message string on single line
> 
>  arch/x86/kvm/irq_comm.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/irq_comm.c b/arch/x86/kvm/irq_comm.c
> index 72298b3..f2887ea 100644
> --- a/arch/x86/kvm/irq_comm.c
> +++ b/arch/x86/kvm/irq_comm.c
> @@ -103,12 +103,24 @@ static inline void kvm_set_msi_irq(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;
> +	/*
> +	 * TODO Deal with RH bit of MSI message address
> +	 *  IF RH=1, then MSI delivers only to the processor with the
> +	 *  lowest interrupt priority among processors that can receive
> +	 *  the interrupt.
> +	 */
> +	if ((e->msi.address_lo & MSI_ADDR_REDIRECTION_LOWPRI) &&
> +			(e->msi.address_lo & MSI_ADDR_DEST_MODE_LOGICAL))
> +		irq->dest_mode = APIC_DEST_LOGICAL;
> +	else
> +		irq->dest_mode = APIC_DEST_PHYSICAL;
>  	irq->trig_mode = (1 << MSI_DATA_TRIGGER_SHIFT) & e->msi.data;
>  	irq->delivery_mode = e->msi.data & 0x700;
> +	if (e->msi.address_lo & MSI_ADDR_REDIRECTION_LOWPRI)
> +		pr_warn_once(
> +			"kvm: MSIs may not be correctly delivered with RH set.\n");
>  	irq->level = 1;
>  	irq->shorthand = 0;
> -	/* TODO Deal with RH bit of MSI message address */
>  }
> 
>  int kvm_set_msi(struct kvm_kernel_irq_routing_entry *e,
>