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

[Gleb Natapov <gleb@redhat.com>]

On Thu, Sep 01, 2011 at 07:05:35PM +0300, 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>
> ---
>  arch/x86/kvm/lapic.c      |   17 +++++++++++++++--
>  include/linux/kvm_types.h |    1 +
>  virt/kvm/ioapic.h         |    2 +-
>  virt/kvm/irq_comm.c       |   14 +++++++++++---
>  4 files changed, 28 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 57dcbd4..3347f9f 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -33,6 +33,7 @@
>  #include <asm/page.h>
>  #include <asm/current.h>
>  #include <asm/apicdef.h>
> +#include <asm/msidef.h>
>  #include <linux/atomic.h>
>  #include "kvm_cache_regs.h"
>  #include "irq.h"
> @@ -325,7 +326,7 @@ int kvm_apic_match_logical_addr(struct kvm_lapic *apic, u8 mda)
>  }
>  
>  int kvm_apic_match_dest(struct kvm_vcpu *vcpu, struct kvm_lapic *source,
> -			   int short_hand, int dest, int dest_mode)
> +			   int short_hand, int dest, int dest_mode, int rh)
>  {
>  	int result = 0;
>  	struct kvm_lapic *target = vcpu->arch.apic;
> @@ -335,9 +336,21 @@ int kvm_apic_match_dest(struct kvm_vcpu *vcpu, struct kvm_lapic *source,
>  		   target, source, dest, dest_mode, short_hand);
>  
>  	ASSERT(target);
> +
> +	/* If RH bit is not set, use addressing mode from ICR */
> +	if (rh == 0) {
> +		u32 icr_low = apic_get_reg(target, APIC_ICR);
> +
> +		dest_mode = icr_low & APIC_DEST_MASK;

Why is that? This function is called for messages generated by
ioapic/ipi/msi. In all those cases destination mode does not depend on
the content of icr_low register in the destination processor. This
function gets correct dest_mode as a parameter.

> +
> +		/* dest apic can't be 0xff if targeting specific processor */
> +		BUG_ON(dest_mode == MSI_ADDR_DEST_MODE_PHYSICAL &&
> +			(u8)dest == 0xff);
Can't this be triggered by a guest?

> +	}
> +
>  	switch (short_hand) {
>  	case APIC_DEST_NOSHORT:
> -		if (dest_mode == 0)
> +		if (dest_mode == MSI_ADDR_DEST_MODE_PHYSICAL)
This is apic code, why MSI defines?

>  			/* Physical mode. */
>  			result = kvm_apic_match_physical_addr(target, dest);
>  		else
> diff --git a/include/linux/kvm_types.h b/include/linux/kvm_types.h
> index fa7cc72..55d4d9a 100644
> --- a/include/linux/kvm_types.h
> +++ b/include/linux/kvm_types.h
> @@ -65,6 +65,7 @@ struct kvm_lapic_irq {
>  	u32 trig_mode;
>  	u32 shorthand;
>  	u32 dest_id;
> +	u32 redir_hint;
>  };
>  
>  struct gfn_to_hva_cache {
> diff --git a/virt/kvm/ioapic.h b/virt/kvm/ioapic.h
> index 0b190c3..fd3bb64 100644
> --- a/virt/kvm/ioapic.h
> +++ b/virt/kvm/ioapic.h
> @@ -68,7 +68,7 @@ static inline struct kvm_ioapic *ioapic_irqchip(struct kvm *kvm)
>  }
>  
>  int kvm_apic_match_dest(struct kvm_vcpu *vcpu, struct kvm_lapic *source,
> -		int short_hand, int dest, int dest_mode);
> +		   int short_hand, int dest, int dest_mode, int rh);
>  int kvm_apic_compare_prio(struct kvm_vcpu *vcpu1, struct kvm_vcpu *vcpu2);
>  void kvm_ioapic_update_eoi(struct kvm *kvm, int vector, int trigger_mode);
>  int kvm_ioapic_init(struct kvm *kvm);
> diff --git a/virt/kvm/irq_comm.c b/virt/kvm/irq_comm.c
> index 9f614b4..e53fc83 100644
> --- a/virt/kvm/irq_comm.c
> +++ b/virt/kvm/irq_comm.c
> @@ -68,6 +68,11 @@ static int kvm_set_ioapic_irq(struct kvm_kernel_irq_routing_entry *e,
>  	return kvm_ioapic_set_irq(ioapic, e->irqchip.pin, level);
>  }
>  
> +static inline bool kvm_is_rh_lowest_prio(struct kvm_lapic_irq *irq)
> +{
> +	return irq->redir_hint == MSI_ADDR_REDIRECTION_LOWPRI;
> +}
> +
>  inline static bool kvm_is_dm_lowest_prio(struct kvm_lapic_irq *irq)
>  {
>  #ifdef CONFIG_IA64
> @@ -93,10 +98,12 @@ int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
>  			continue;
>  
>  		if (!kvm_apic_match_dest(vcpu, src, irq->shorthand,
> -					irq->dest_id, irq->dest_mode))
> +					irq->dest_id, irq->dest_mode,
> +					irq->redir_hint))
>  			continue;
>  
> -		if (!kvm_is_dm_lowest_prio(irq)) {
> +		if (!kvm_is_dm_lowest_prio(irq) &&
> +		    !kvm_is_rh_lowest_prio(irq)) {
>  			if (r < 0)
>  				r = 0;
>  			r += kvm_apic_set_irq(vcpu, irq);
> @@ -128,13 +135,14 @@ 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.redir_hint = (1 << MSI_ADDR_REDIRECTION_SHIFT) & e->msi.address_lo;
>  	irq.dest_mode = (1 << MSI_ADDR_DEST_MODE_SHIFT) & e->msi.address_lo;
>  	irq.trig_mode = (1 << MSI_DATA_TRIGGER_SHIFT) & e->msi.data;
>  	irq.delivery_mode = e->msi.data & 0x700;
>  	irq.level = 1;
>  	irq.shorthand = 0;
>  
> -	/* TODO Deal with RH bit of MSI message address */
>  	return kvm_irq_delivery_to_apic(kvm, NULL, &irq);
>  }
>  
Reading the spec it looks like it will be enough to set irq.delivery_mode
to lowest priority if RH bit is set and destination mode is logical. The
change will be contained in kvm_set_msi() function. Do I miss something?
 
--
			Gleb.