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

[Sasha Levin <levinsasha928@gmail.com>]

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.

Maybe we should set irq.delivery_mode only if RH==1?

> 
> BTW, irq_comm.c is surely the wrong place for all this IA32-specific
> interpretation of MSI address and data. And we have yet another
> guest-triggerable printk in kvm_irq_delivery_to_apic (messages to
> physical ID 0xff).
> 
> Jan
> 

-- 

Sasha.