Re: [PATCH v2] x86: irq_comm: Add check for RH bit in kvm_set_msi_irq

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

2015-03-11 09:05-0600, James Sullivan:
> This is a revised patch to the previous submission, adding a check for RH=1/DM=1
> in kvm_set_msi_irq.

(Btw. you can put it below the /^---$/ line, where it gets automatically
 trimmed -- information on changes since the last version is very
 important in review, but of little use later.)

> 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 is still used to specify the semantics of the delivery beyond
> its destination. We can't just set irq->delivery_mode to APIC_DM_LOWPRI if RH=1 since
> this squashes the other delivery semantics. I've documented this in the patch.

It's possible that hardware designers didn't want to waste silicon
either, thus overwriting delivery mode would be the correct, and easy,
thing to do.  (Or have I missed its definition?)

> A future fix may be to implement a separate interrupt delivery function for MSI
> interrupts in kvm_set_msi, which is a bit hacky but probably the only way to do this
> without modifying the kvm_lapic_irq struct. I'll write this up and see how it looks,
> unless there are major objections.

Thank you;  no objections without patches :)

I think that exploring other options makes sense here.
We would probably re-implement most of the code, so adding extra
parameter to interrupt delivery functions would be easier to maintain.
Extending 'struct kvm_lapic_irq' seems acceptable too: it's an internal
structure and we already have kvm_is_dm_lowest_prio() helper.

> diff --git a/arch/x86/kvm/irq_comm.c b/arch/x86/kvm/irq_comm.c
> @@ -97,18 +97,34 @@ int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
>  static inline void kvm_set_msi_irq(struct kvm_kernel_irq_routing_entry *e,
>  				   struct kvm_lapic_irq *irq)
>  {
> +	bool phys;
>  	trace_kvm_msi_set_irq(e->msi.address_lo, e->msi.data);

(We usually have have an empty line after declarations.
 I've seen patches that fixed just this, because some tool complains.)

> +	/*
> +	 * Set dest_mode to logical just in case both RH=1/DH=1,
> +	 * otherwise default to physical
> +	 */

This comment contains the same algorithm written in other (less defined)
language, right next to a similarly complex implementation ...

> +	phys = ((e->msi.address_lo & (MSI_ADDR_REDIRECTION_LOWPRI |
> +			MSI_ADDR_DEST_MODE_LOGICAL)) !=
> +		(MSI_ADDR_REDIRECTION_LOWPRI |
> +			MSI_ADDR_DEST_MODE_LOGICAL));
> +	irq->dest_mode = phys ? APIC_DEST_PHYSICAL : APIC_DEST_LOGICAL;

Wouldn't changing white spacing, using 'if', defining a union mask,
abstracting the all-bits-in-mask-are-set operation, or something help to
make this code clear without a comment?

>  	irq->delivery_mode = e->msi.data & 0x700;

A system that configured DM and RH might not handle delivery to multiple
APICs.  Please add a warning (once), to ease debugging if/when this
feature gets its first user.

Thanks.

> +·    /*
> +·     * 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. However other delivery semantics are still
> +·     *  specified by the delivery mode, so we can't default to
> +·     *  APIC_DM_LOWPRI if RH=1.
> +·     */

(We would, if there was some benefit in erring that way.)