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

["Radim Krčmář" <rkrcmar@redhat.com>]

2015-03-12 21:08-0600, James Sullivan:
> ---
> 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
> 
> diff --git 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 may not correctly deliver MSIs "
> +			     "with RH set.\n");

Please begin the warning with "kvm: ".

It's better to have a line bit longer than 80 columns than to break a
string that we might want to `grep` for; reword if possible, or you
might prefer something like
   pr_warn_once(
   	"long");

(Documentation/CodingStyle, Chapter 2.  It's nitpicking, sorry, but we
 recently had a patch that fixed just that too ...
 http://permalink.gmane.org/gmane.comp.emulators.kvm.devel/133110)

Then,
Reviewed-by: Radim Krčmář <rkrcmar@redhat.com>

Thanks.


---
The warning message is very clever:
- it contains the magical "may" qualifier and being protected only by
  RH=1 creates weird-looking code structure, but it is technically right
  1) lowest-priority delivery may be set in msi.data, which avoids our
     otherwise incorrect behavior with RH=1/DM=1
  2) RH=1/DM=0 can't deliver to multiple APICs (broadcast is forbidden),
     but real hardware may overwrite delivery mode from msi.data
- being two lines apart adds to suspicion, yet it can be hint to those
  possible problems

I only fear it is too clever :)