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ář <[email protected]>
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 :)
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
