[PATCH] KVM: Implement support for the RH bit

[PATCH] KVM: Implement support for the RH bit

[Sasha Levin]

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;
+
+		/* dest apic can't be 0xff if targeting specific processor */
+		BUG_ON(dest_mode == MSI_ADDR_DEST_MODE_PHYSICAL &&
+			(u8)dest == 0xff);
+	}
+
 	switch (short_hand) {
 	case APIC_DEST_NOSHORT:
-		if (dest_mode == 0)
+		if (dest_mode == MSI_ADDR_DEST_MODE_PHYSICAL)
 			/* 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);
 }
 
-- 
1.7.6.1

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

[Gleb Natapov]

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.

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

[Sasha Levin]

On Thu, 2011-09-01 at 20:45 +0300, Gleb Natapov wrote:
> 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.
> 

According to the spec:

"If RH is 0, then the DM bit is ignored and the message is sent ahead
independent of whether the physical or logical destination mode is used"

As far as I understand it, dest_mode parameter is invalid if the RH bit
is set.

> > +
> > +		/* 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?

I wrote this because of this line:

"When RH is 1 and the physical destination mode is used, the Destination
ID field must not be set to 0xFF"

I did get this check in the wrong location, it was supposed to be in the
'else' case.

> 
> > +	}
> > +
> >  	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?
> 

You're right - I'll fix that.

> >  			/* 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?

That was my plan initially, the problem I stumbled on was that quote
that I've pasted before:

"If RH is 0, then the DM bit is ignored and the message is sent ahead
independent of whether the physical or logical destination mode is used"

Which means that I can't use the DM bit from the address register, and
have to find the destination processor based on actual addressing model.

-- 

Sasha.

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

[Gleb Natapov]

On Thu, Sep 01, 2011 at 09:59:52PM +0300, Sasha Levin wrote:
> On Thu, 2011-09-01 at 20:45 +0300, Gleb Natapov wrote:
> > 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.
> > 
> 
> According to the spec:
> 
> "If RH is 0, then the DM bit is ignored and the message is sent ahead
> independent of whether the physical or logical destination mode is used"
So how ID field is interpreted?

> 
> As far as I understand it, dest_mode parameter is invalid if the RH bit
> is set.
> 
Intel spec says:
 When RH is 0, the interrupt is directed to the processor listed in the
 Destination ID field.
Looks like it says that Destination ID should be treated as physical.

But you do not just ignore dest_mode, you take it from icr_low of destination
cpu and this does not make sense to me.

> > > +
> > > +		/* 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?
> 
> I wrote this because of this line:
> 
> "When RH is 1 and the physical destination mode is used, the Destination
> ID field must not be set to 0xFF"
> 
> I did get this check in the wrong location, it was supposed to be in the
> 'else' case.
> 
You can kill a guest if it does something that is prohibited by the spec,
or you can stop delivering interrupts to the guest or change random
memory in the guest, but not Oops a host.

> > 
> > > +	}
> > > +
> > >  	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?
> > 
> 
> You're right - I'll fix that.
> 
> > >  			/* 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?
> 
> That was my plan initially, the problem I stumbled on was that quote
> that I've pasted before:
> 
> "If RH is 0, then the DM bit is ignored and the message is sent ahead
> independent of whether the physical or logical destination mode is used"
> 
> Which means that I can't use the DM bit from the address register, and
> have to find the destination processor based on actual addressing model.
> 
What is "actual addressing model"? Addressing is per message not some
global state.

--
			Gleb.

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

[Sasha Levin]

On Thu, 2011-09-01 at 22:31 +0300, Gleb Natapov wrote:
> On Thu, Sep 01, 2011 at 09:59:52PM +0300, Sasha Levin wrote:
> > On Thu, 2011-09-01 at 20:45 +0300, Gleb Natapov wrote:
> > > 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.
> > > 
> > 
> > According to the spec:
> > 
> > "If RH is 0, then the DM bit is ignored and the message is sent ahead
> > independent of whether the physical or logical destination mode is used"
> So how ID field is interpreted?
> 
> > 
> > As far as I understand it, dest_mode parameter is invalid if the RH bit
> > is set.
> > 
> Intel spec says:
>  When RH is 0, the interrupt is directed to the processor listed in the
>  Destination ID field.
> Looks like it says that Destination ID should be treated as physical.
> 
> But you do not just ignore dest_mode, you take it from icr_low of destination
> cpu and this does not make sense to me.
> 

dest_mode is picked up from the DM bit in the message address register,
the spec states that it's invalid - so I'm picking it from the ICR
instead.

It wasn't clear to me that it's simply physical, but I'm not too sure it
isn't either.

> > > > +
> > > > +		/* 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?
> > 
> > I wrote this because of this line:
> > 
> > "When RH is 1 and the physical destination mode is used, the Destination
> > ID field must not be set to 0xFF"
> > 
> > I did get this check in the wrong location, it was supposed to be in the
> > 'else' case.
> > 
> You can kill a guest if it does something that is prohibited by the spec,
> or you can stop delivering interrupts to the guest or change random
> memory in the guest, but not Oops a host.
> 
> > > 
> > > > +	}
> > > > +
> > > >  	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?
> > > 
> > 
> > You're right - I'll fix that.
> > 
> > > >  			/* 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?
> > 
> > That was my plan initially, the problem I stumbled on was that quote
> > that I've pasted before:
> > 
> > "If RH is 0, then the DM bit is ignored and the message is sent ahead
> > independent of whether the physical or logical destination mode is used"
> > 
> > Which means that I can't use the DM bit from the address register, and
> > have to find the destination processor based on actual addressing model.
> > 
> What is "actual addressing model"? Addressing is per message not some
> global state.
> 

By "actual addressing model" I meant checking the ICR of of each APIC to
see if we should address it logically or physically.

> --
> 			Gleb.

-- 

Sasha.

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

[Gleb Natapov]

On Thu, Sep 01, 2011 at 10:58:31PM +0300, Sasha Levin wrote:
> By "actual addressing model" I meant checking the ICR of of each APIC to
> see if we should address it logically or physically.
> 
This is definitely not what ICR of each APIC is about.

--
			Gleb.

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

[Sasha Levin]

On Thu, 2011-09-01 at 23:03 +0300, Gleb Natapov wrote:
> On Thu, Sep 01, 2011 at 10:58:31PM +0300, Sasha Levin wrote:
> > By "actual addressing model" I meant checking the ICR of of each APIC to
> > see if we should address it logically or physically.
> > 
> This is definitely not what ICR of each APIC is about.

Thats what I understood from section 10.6.2 in the spec.

-- 

Sasha.

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

[Gleb Natapov]

On Thu, Sep 01, 2011 at 11:05:00PM +0300, Sasha Levin wrote:
> On Thu, 2011-09-01 at 23:03 +0300, Gleb Natapov wrote:
> > On Thu, Sep 01, 2011 at 10:58:31PM +0300, Sasha Levin wrote:
> > > By "actual addressing model" I meant checking the ICR of of each APIC to
> > > see if we should address it logically or physically.
> > > 
> > This is definitely not what ICR of each APIC is about.
> 
> Thats what I understood from section 10.6.2 in the spec.
> 
Section 10.6.2 talks about ACPI messages generated by a CPU (IPIs) not
about addressing mode of the CPU itself (which is, again, per message).
 
--
			Gleb.

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

[Sasha Levin]

On Thu, 2011-09-01 at 23:19 +0300, Gleb Natapov wrote:
> On Thu, Sep 01, 2011 at 11:05:00PM +0300, Sasha Levin wrote:
> > On Thu, 2011-09-01 at 23:03 +0300, Gleb Natapov wrote:
> > > On Thu, Sep 01, 2011 at 10:58:31PM +0300, Sasha Levin wrote:
> > > > By "actual addressing model" I meant checking the ICR of of each APIC to
> > > > see if we should address it logically or physically.
> > > > 
> > > This is definitely not what ICR of each APIC is about.
> > 
> > Thats what I understood from section 10.6.2 in the spec.
> > 
> Section 10.6.2 talks about ACPI messages generated by a CPU (IPIs) not
> about addressing mode of the CPU itself (which is, again, per message).

Right, but it was referred to by section 10.11.1, that's why I assumed
so.

Anyway, I'll just replace the patch with what you've initially
suggested, so it would be contained within kvm_set_msi().

-- 

Sasha.