[PATCH v2 0/3] KVM: X86: Cleanups on dest_mode and headers

[PATCH v2 0/3] KVM: X86: Cleanups on dest_mode and headers

[Peter Xu]

Each patch explains itself.

Please have a look, thanks.

Peter Xu (3):
  KVM: X86: Some cleanups in ioapic.h/lapic.h
  KVM: X86: Use APIC_DEST_* macros properly
  KVM: X86: Fixup kvm_apic_match_dest() dest_mode parameter

 arch/x86/kvm/ioapic.c   | 9 ++++++---
 arch/x86/kvm/ioapic.h   | 6 ------
 arch/x86/kvm/irq_comm.c | 7 ++++---
 arch/x86/kvm/lapic.c    | 5 +++--
 arch/x86/kvm/lapic.h    | 7 +++++--
 arch/x86/kvm/x86.c      | 2 +-
 6 files changed, 19 insertions(+), 17 deletions(-)

-- 
2.21.0

[PATCH v2 1/3] KVM: X86: Some cleanups in ioapic.h/lapic.h

[Peter Xu]

Both kvm_apic_match_dest() and kvm_irq_delivery_to_apic() should
probably suite more to lapic.h comparing to ioapic.h, moving.

kvm_apic_match_dest() is defined twice, once in each of the header.
Removing the one defined in ioapic.h.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 arch/x86/kvm/ioapic.h | 6 ------
 arch/x86/kvm/lapic.h  | 5 ++++-
 2 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kvm/ioapic.h b/arch/x86/kvm/ioapic.h
index ea1a4e0297da..2fb2e3c80724 100644
--- a/arch/x86/kvm/ioapic.h
+++ b/arch/x86/kvm/ioapic.h
@@ -116,9 +116,6 @@ static inline int ioapic_in_kernel(struct kvm *kvm)
 }
 
 void kvm_rtc_eoi_tracking_restore_one(struct kvm_vcpu *vcpu);
-bool kvm_apic_match_dest(struct kvm_vcpu *vcpu, struct kvm_lapic *source,
-		int short_hand, unsigned int dest, int dest_mode);
-int kvm_apic_compare_prio(struct kvm_vcpu *vcpu1, struct kvm_vcpu *vcpu2);
 void kvm_ioapic_update_eoi(struct kvm_vcpu *vcpu, int vector,
 			int trigger_mode);
 int kvm_ioapic_init(struct kvm *kvm);
@@ -126,9 +123,6 @@ void kvm_ioapic_destroy(struct kvm *kvm);
 int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int irq_source_id,
 		       int level, bool line_status);
 void kvm_ioapic_clear_all(struct kvm_ioapic *ioapic, int irq_source_id);
-int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
-			     struct kvm_lapic_irq *irq,
-			     struct dest_map *dest_map);
 void kvm_get_ioapic(struct kvm *kvm, struct kvm_ioapic_state *state);
 void kvm_set_ioapic(struct kvm *kvm, struct kvm_ioapic_state *state);
 void kvm_ioapic_scan_entry(struct kvm_vcpu *vcpu,
diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
index 39925afdfcdc..19b36196e2ff 100644
--- a/arch/x86/kvm/lapic.h
+++ b/arch/x86/kvm/lapic.h
@@ -83,7 +83,10 @@ int kvm_lapic_reg_read(struct kvm_lapic *apic, u32 offset, int len,
 		       void *data);
 bool kvm_apic_match_dest(struct kvm_vcpu *vcpu, struct kvm_lapic *source,
 			   int short_hand, unsigned int dest, int dest_mode);
-
+int kvm_apic_compare_prio(struct kvm_vcpu *vcpu1, struct kvm_vcpu *vcpu2);
+int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
+			     struct kvm_lapic_irq *irq,
+			     struct dest_map *dest_map);
 bool __kvm_apic_update_irr(u32 *pir, void *regs, int *max_irr);
 bool kvm_apic_update_irr(struct kvm_vcpu *vcpu, u32 *pir, int *max_irr);
 void kvm_apic_update_ppr(struct kvm_vcpu *vcpu);
-- 
2.21.0

Re: [PATCH v2 1/3] KVM: X86: Some cleanups in ioapic.h/lapic.h

[Vitaly Kuznetsov]

Peter Xu <peterx@redhat.com> writes:

> Both kvm_apic_match_dest() and kvm_irq_delivery_to_apic() should
> probably suite more to lapic.h comparing to ioapic.h, moving.
>
> kvm_apic_match_dest() is defined twice, once in each of the header.
> Removing the one defined in ioapic.h.
>

kvm_apic_match_dest()'s implementation lives in lapic.c so moving the
declaration to lapic.h makes perfect sense. kvm_irq_delivery_to_apic()'s
body is, however, in irq_comm.c and declarations for it are usually
found in asm/kvm_host.h. I'm not sure but maybe it would make sense to
move kvm_irq_delivery_to_apic()'s body to lapic.c too.

(Personally, I'd also greatly appreciate if functions working with lapic
exclusively would have 'lapic' instead of 'apic' in their names. But
this is unrelated to the patch.)

> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  arch/x86/kvm/ioapic.h | 6 ------
>  arch/x86/kvm/lapic.h  | 5 ++++-
>  2 files changed, 4 insertions(+), 7 deletions(-)
>
> diff --git a/arch/x86/kvm/ioapic.h b/arch/x86/kvm/ioapic.h
> index ea1a4e0297da..2fb2e3c80724 100644
> --- a/arch/x86/kvm/ioapic.h
> +++ b/arch/x86/kvm/ioapic.h
> @@ -116,9 +116,6 @@ static inline int ioapic_in_kernel(struct kvm *kvm)
>  }
>  
>  void kvm_rtc_eoi_tracking_restore_one(struct kvm_vcpu *vcpu);
> -bool kvm_apic_match_dest(struct kvm_vcpu *vcpu, struct kvm_lapic *source,
> -		int short_hand, unsigned int dest, int dest_mode);
> -int kvm_apic_compare_prio(struct kvm_vcpu *vcpu1, struct kvm_vcpu *vcpu2);
>  void kvm_ioapic_update_eoi(struct kvm_vcpu *vcpu, int vector,
>  			int trigger_mode);
>  int kvm_ioapic_init(struct kvm *kvm);
> @@ -126,9 +123,6 @@ void kvm_ioapic_destroy(struct kvm *kvm);
>  int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int irq_source_id,
>  		       int level, bool line_status);
>  void kvm_ioapic_clear_all(struct kvm_ioapic *ioapic, int irq_source_id);
> -int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
> -			     struct kvm_lapic_irq *irq,
> -			     struct dest_map *dest_map);
>  void kvm_get_ioapic(struct kvm *kvm, struct kvm_ioapic_state *state);
>  void kvm_set_ioapic(struct kvm *kvm, struct kvm_ioapic_state *state);
>  void kvm_ioapic_scan_entry(struct kvm_vcpu *vcpu,
> diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
> index 39925afdfcdc..19b36196e2ff 100644
> --- a/arch/x86/kvm/lapic.h
> +++ b/arch/x86/kvm/lapic.h
> @@ -83,7 +83,10 @@ int kvm_lapic_reg_read(struct kvm_lapic *apic, u32 offset, int len,
>  		       void *data);
>  bool kvm_apic_match_dest(struct kvm_vcpu *vcpu, struct kvm_lapic *source,
>  			   int short_hand, unsigned int dest, int dest_mode);
> -
> +int kvm_apic_compare_prio(struct kvm_vcpu *vcpu1, struct kvm_vcpu *vcpu2);
> +int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
> +			     struct kvm_lapic_irq *irq,
> +			     struct dest_map *dest_map);
>  bool __kvm_apic_update_irr(u32 *pir, void *regs, int *max_irr);
>  bool kvm_apic_update_irr(struct kvm_vcpu *vcpu, u32 *pir, int *max_irr);
>  void kvm_apic_update_ppr(struct kvm_vcpu *vcpu);

-- 
Vitaly

Re: [PATCH v2 1/3] KVM: X86: Some cleanups in ioapic.h/lapic.h

[Sean Christopherson]

On Mon, Dec 02, 2019 at 10:27:51AM +0100, Vitaly Kuznetsov wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > Both kvm_apic_match_dest() and kvm_irq_delivery_to_apic() should
> > probably suite more to lapic.h comparing to ioapic.h, moving.

Please use imperative mode, state *why* the prototypes belong in lapic.h,
and don't hedge, e.g.:

  kvm_apic_match_dest() is defined in lapic.c, move its declaration from
  ioapic.h to lapic.h.

Ditto for the subject:

  KVM: x86: Clean up function declarations in ioapic.h

> > kvm_apic_match_dest() is defined twice, once in each of the header.

s/defined/declared

> > Removing the one defined in ioapic.h.

Again:

  Remove a redundant declaration of kvm_apic_match_dest() from ioapic.h,
  it is declared and defined in lapic.{h,c}.

> kvm_apic_match_dest()'s implementation lives in lapic.c so moving the
> declaration to lapic.h makes perfect sense. kvm_irq_delivery_to_apic()'s
> body is, however, in irq_comm.c and declarations for it are usually
> found in asm/kvm_host.h. I'm not sure but maybe it would make sense to
> move kvm_irq_delivery_to_apic()'s body to lapic.c too.

asm/kvm_host.h is generally used only for exported functions, internal-only
functions for irq_comm.c are declared in arch/x86/kvm/irq.h.

> (Personally, I'd also greatly appreciate if functions working with lapic
> exclusively would have 'lapic' instead of 'apic' in their names. But
> this is unrelated to the patch.)
> 
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  arch/x86/kvm/ioapic.h | 6 ------
> >  arch/x86/kvm/lapic.h  | 5 ++++-
> >  2 files changed, 4 insertions(+), 7 deletions(-)
> >
> > diff --git a/arch/x86/kvm/ioapic.h b/arch/x86/kvm/ioapic.h
> > index ea1a4e0297da..2fb2e3c80724 100644
> > --- a/arch/x86/kvm/ioapic.h
> > +++ b/arch/x86/kvm/ioapic.h
> > @@ -116,9 +116,6 @@ static inline int ioapic_in_kernel(struct kvm *kvm)
> >  }
> >  
> >  void kvm_rtc_eoi_tracking_restore_one(struct kvm_vcpu *vcpu);
> > -bool kvm_apic_match_dest(struct kvm_vcpu *vcpu, struct kvm_lapic *source,
> > -		int short_hand, unsigned int dest, int dest_mode);
> > -int kvm_apic_compare_prio(struct kvm_vcpu *vcpu1, struct kvm_vcpu *vcpu2);
> >  void kvm_ioapic_update_eoi(struct kvm_vcpu *vcpu, int vector,
> >  			int trigger_mode);
> >  int kvm_ioapic_init(struct kvm *kvm);
> > @@ -126,9 +123,6 @@ void kvm_ioapic_destroy(struct kvm *kvm);
> >  int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int irq_source_id,
> >  		       int level, bool line_status);
> >  void kvm_ioapic_clear_all(struct kvm_ioapic *ioapic, int irq_source_id);
> > -int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
> > -			     struct kvm_lapic_irq *irq,
> > -			     struct dest_map *dest_map);
> >  void kvm_get_ioapic(struct kvm *kvm, struct kvm_ioapic_state *state);
> >  void kvm_set_ioapic(struct kvm *kvm, struct kvm_ioapic_state *state);
> >  void kvm_ioapic_scan_entry(struct kvm_vcpu *vcpu,
> > diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
> > index 39925afdfcdc..19b36196e2ff 100644
> > --- a/arch/x86/kvm/lapic.h
> > +++ b/arch/x86/kvm/lapic.h
> > @@ -83,7 +83,10 @@ int kvm_lapic_reg_read(struct kvm_lapic *apic, u32 offset, int len,
> >  		       void *data);
> >  bool kvm_apic_match_dest(struct kvm_vcpu *vcpu, struct kvm_lapic *source,
> >  			   int short_hand, unsigned int dest, int dest_mode);
> > -
> > +int kvm_apic_compare_prio(struct kvm_vcpu *vcpu1, struct kvm_vcpu *vcpu2);
> > +int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
> > +			     struct kvm_lapic_irq *irq,
> > +			     struct dest_map *dest_map);
> >  bool __kvm_apic_update_irr(u32 *pir, void *regs, int *max_irr);
> >  bool kvm_apic_update_irr(struct kvm_vcpu *vcpu, u32 *pir, int *max_irr);
> >  void kvm_apic_update_ppr(struct kvm_vcpu *vcpu);
> 
> -- 
> Vitaly
> 

Re: [PATCH v2 1/3] KVM: X86: Some cleanups in ioapic.h/lapic.h

[Peter Xu]

On Mon, Dec 02, 2019 at 09:47:41AM -0800, Sean Christopherson wrote:
> > kvm_apic_match_dest()'s implementation lives in lapic.c so moving the
> > declaration to lapic.h makes perfect sense. kvm_irq_delivery_to_apic()'s
> > body is, however, in irq_comm.c and declarations for it are usually
> > found in asm/kvm_host.h. I'm not sure but maybe it would make sense to
> > move kvm_irq_delivery_to_apic()'s body to lapic.c too.
> 
> asm/kvm_host.h is generally used only for exported functions, internal-only
> functions for irq_comm.c are declared in arch/x86/kvm/irq.h.

I think I'm fine with either lapic.h or irq.h.  Let me do with irq.h.

I'll try to avoid moving the body of kvm_irq_delivery_to_apic() if you
don't disagree, I think it's ok to be in irq.c, while I'll normally
avoid doing those churns because I wanted to keep the git history
clean so it's easier to read the history per-line of that function. :)

Thanks,

-- 
Peter Xu

[PATCH v2 2/3] KVM: X86: Use APIC_DEST_* macros properly

[Peter Xu]

Previously we were using either APIC_DEST_PHYSICAL|APIC_DEST_LOGICAL
or 0|1 to fill in kvm_lapic_irq.dest_mode, and it's done in an adhoc
way.  It's fine imho only because in most cases when we check against
dest_mode it's against APIC_DEST_PHYSICAL (which equals to 0).
However, that's not consistent, majorly because APIC_DEST_LOGICAL does
not equals to 1, so if one day we check irq.dest_mode against
APIC_DEST_LOGICAL we'll probably always get a false returned.

This patch replaces the 0/1 settings of irq.dest_mode with the macros
to make them consistent.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 arch/x86/kvm/ioapic.c   | 9 ++++++---
 arch/x86/kvm/irq_comm.c | 7 ++++---
 arch/x86/kvm/x86.c      | 2 +-
 3 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kvm/ioapic.c b/arch/x86/kvm/ioapic.c
index 9fd2dd89a1c5..1e091637d5d5 100644
--- a/arch/x86/kvm/ioapic.c
+++ b/arch/x86/kvm/ioapic.c
@@ -331,7 +331,8 @@ static void ioapic_write_indirect(struct kvm_ioapic *ioapic, u32 val)
 			irq.vector = e->fields.vector;
 			irq.delivery_mode = e->fields.delivery_mode << 8;
 			irq.dest_id = e->fields.dest_id;
-			irq.dest_mode = e->fields.dest_mode;
+			irq.dest_mode = e->fields.dest_mode ?
+			    APIC_DEST_LOGICAL : APIC_DEST_PHYSICAL;
 			bitmap_zero(&vcpu_bitmap, 16);
 			kvm_bitmap_or_dest_vcpus(ioapic->kvm, &irq,
 						 &vcpu_bitmap);
@@ -343,7 +344,8 @@ static void ioapic_write_indirect(struct kvm_ioapic *ioapic, u32 val)
 				 * keep ioapic_handled_vectors synchronized.
 				 */
 				irq.dest_id = old_dest_id;
-				irq.dest_mode = old_dest_mode;
+				irq.dest_mode = old_dest_mode ?
+				    APIC_DEST_LOGICAL : APIC_DEST_PHYSICAL;
 				kvm_bitmap_or_dest_vcpus(ioapic->kvm, &irq,
 							 &vcpu_bitmap);
 			}
@@ -369,7 +371,8 @@ static int ioapic_service(struct kvm_ioapic *ioapic, int irq, bool line_status)
 
 	irqe.dest_id = entry->fields.dest_id;
 	irqe.vector = entry->fields.vector;
-	irqe.dest_mode = entry->fields.dest_mode;
+	irqe.dest_mode = entry->fields.dest_mode ?
+	    APIC_DEST_LOGICAL : APIC_DEST_PHYSICAL;
 	irqe.trig_mode = entry->fields.trig_mode;
 	irqe.delivery_mode = entry->fields.delivery_mode << 8;
 	irqe.level = 1;
diff --git a/arch/x86/kvm/irq_comm.c b/arch/x86/kvm/irq_comm.c
index 8ecd48d31800..673b6afd6dbf 100644
--- a/arch/x86/kvm/irq_comm.c
+++ b/arch/x86/kvm/irq_comm.c
@@ -52,8 +52,8 @@ int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
 	unsigned long dest_vcpu_bitmap[BITS_TO_LONGS(KVM_MAX_VCPUS)];
 	unsigned int dest_vcpus = 0;
 
-	if (irq->dest_mode == 0 && irq->dest_id == 0xff &&
-			kvm_lowest_prio_delivery(irq)) {
+	if (irq->dest_mode == APIC_DEST_PHYSICAL &&
+	    irq->dest_id == 0xff && kvm_lowest_prio_delivery(irq)) {
 		printk(KERN_INFO "kvm: apic: phys broadcast and lowest prio\n");
 		irq->delivery_mode = APIC_DM_FIXED;
 	}
@@ -114,7 +114,8 @@ void kvm_set_msi_irq(struct kvm *kvm, struct kvm_kernel_irq_routing_entry *e,
 		irq->dest_id |= MSI_ADDR_EXT_DEST_ID(e->msi.address_hi);
 	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 = (1 << MSI_ADDR_DEST_MODE_SHIFT) & e->msi.address_lo ?
+	    APIC_DEST_LOGICAL : APIC_DEST_PHYSICAL;
 	irq->trig_mode = (1 << MSI_DATA_TRIGGER_SHIFT) & e->msi.data;
 	irq->delivery_mode = e->msi.data & 0x700;
 	irq->msi_redir_hint = ((e->msi.address_lo
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 3ed167e039e5..3b00d662dc14 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -7356,7 +7356,7 @@ static void kvm_pv_kick_cpu_op(struct kvm *kvm, unsigned long flags, int apicid)
 	struct kvm_lapic_irq lapic_irq;
 
 	lapic_irq.shorthand = 0;
-	lapic_irq.dest_mode = 0;
+	lapic_irq.dest_mode = APIC_DEST_PHYSICAL;
 	lapic_irq.level = 0;
 	lapic_irq.dest_id = apicid;
 	lapic_irq.msi_redir_hint = false;
-- 
2.21.0

[PATCH v2 3/3] KVM: X86: Fixup kvm_apic_match_dest() dest_mode parameter

[Peter Xu]

The problem is the same as the previous patch on that we've got too
many ways to define a dest_mode, but logically we should only pass in
APIC_DEST_* macros for this helper.

To make it easier, simply define dest_mode of kvm_apic_match_dest() to
be a boolean to make it right while we can avoid to touch the callers.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 arch/x86/kvm/lapic.c | 5 +++--
 arch/x86/kvm/lapic.h | 2 +-
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index cf9177b4a07f..80732892c709 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -791,8 +791,9 @@ static u32 kvm_apic_mda(struct kvm_vcpu *vcpu, unsigned int dest_id,
 	return dest_id;
 }
 
+/* Set dest_mode to true for logical mode, false for physical mode */
 bool kvm_apic_match_dest(struct kvm_vcpu *vcpu, struct kvm_lapic *source,
-			   int short_hand, unsigned int dest, int dest_mode)
+			   int short_hand, unsigned int dest, bool dest_mode)
 {
 	struct kvm_lapic *target = vcpu->arch.apic;
 	u32 mda = kvm_apic_mda(vcpu, dest, source, target);
@@ -800,7 +801,7 @@ bool kvm_apic_match_dest(struct kvm_vcpu *vcpu, struct kvm_lapic *source,
 	ASSERT(target);
 	switch (short_hand) {
 	case APIC_DEST_NOSHORT:
-		if (dest_mode == APIC_DEST_PHYSICAL)
+		if (dest_mode == false)
 			return kvm_apic_match_physical_addr(target, mda);
 		else
 			return kvm_apic_match_logical_addr(target, mda);
diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
index 19b36196e2ff..c0b472ed87ad 100644
--- a/arch/x86/kvm/lapic.h
+++ b/arch/x86/kvm/lapic.h
@@ -82,7 +82,7 @@ int kvm_lapic_reg_write(struct kvm_lapic *apic, u32 reg, u32 val);
 int kvm_lapic_reg_read(struct kvm_lapic *apic, u32 offset, int len,
 		       void *data);
 bool kvm_apic_match_dest(struct kvm_vcpu *vcpu, struct kvm_lapic *source,
-			   int short_hand, unsigned int dest, int dest_mode);
+			   int short_hand, unsigned int dest, bool dest_mode);
 int kvm_apic_compare_prio(struct kvm_vcpu *vcpu1, struct kvm_vcpu *vcpu2);
 int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
 			     struct kvm_lapic_irq *irq,
-- 
2.21.0

Re: [PATCH v2 3/3] KVM: X86: Fixup kvm_apic_match_dest() dest_mode parameter

[Vitaly Kuznetsov]

Peter Xu <peterx@redhat.com> writes:

> The problem is the same as the previous patch on that we've got too
> many ways to define a dest_mode, but logically we should only pass in
> APIC_DEST_* macros for this helper.

Using 'the previous patch' in changelog is OK until it comes to
backporting as the order can change. I'd suggest to spell out "KVM: X86:
Use APIC_DEST_* macros properly" explicitly.

>
> To make it easier, simply define dest_mode of kvm_apic_match_dest() to
> be a boolean to make it right while we can avoid to touch the callers.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  arch/x86/kvm/lapic.c | 5 +++--
>  arch/x86/kvm/lapic.h | 2 +-
>  2 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index cf9177b4a07f..80732892c709 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -791,8 +791,9 @@ static u32 kvm_apic_mda(struct kvm_vcpu *vcpu, unsigned int dest_id,
>  	return dest_id;
>  }
>  
> +/* Set dest_mode to true for logical mode, false for physical mode */
>  bool kvm_apic_match_dest(struct kvm_vcpu *vcpu, struct kvm_lapic *source,
> -			   int short_hand, unsigned int dest, int dest_mode)
> +			   int short_hand, unsigned int dest, bool dest_mode)
>  {
>  	struct kvm_lapic *target = vcpu->arch.apic;
>  	u32 mda = kvm_apic_mda(vcpu, dest, source, target);
> @@ -800,7 +801,7 @@ bool kvm_apic_match_dest(struct kvm_vcpu *vcpu, struct kvm_lapic *source,
>  	ASSERT(target);
>  	switch (short_hand) {
>  	case APIC_DEST_NOSHORT:
> -		if (dest_mode == APIC_DEST_PHYSICAL)
> +		if (dest_mode == false)

I must admit this seriously harm the readability of the code for
me. Just look at the 

 if (dest_mode == false)

line without a context and try to say what's being checked. I can't.

I see to solutions:
1) Adhere to the APIC_DEST_PHYSICAL/APIC_DEST_LOGICAL (basically - just
check against "dest_mode == APIC_DEST_LOGICAL" in the else branch)
2) Rename the dest_mode parameter to 'dest_mode_is_phys' or something
like that.

>  			return kvm_apic_match_physical_addr(target, mda);
>  		else
>  			return kvm_apic_match_logical_addr(target, mda);
> diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
> index 19b36196e2ff..c0b472ed87ad 100644
> --- a/arch/x86/kvm/lapic.h
> +++ b/arch/x86/kvm/lapic.h
> @@ -82,7 +82,7 @@ int kvm_lapic_reg_write(struct kvm_lapic *apic, u32 reg, u32 val);
>  int kvm_lapic_reg_read(struct kvm_lapic *apic, u32 offset, int len,
>  		       void *data);
>  bool kvm_apic_match_dest(struct kvm_vcpu *vcpu, struct kvm_lapic *source,
> -			   int short_hand, unsigned int dest, int dest_mode);
> +			   int short_hand, unsigned int dest, bool dest_mode);
>  int kvm_apic_compare_prio(struct kvm_vcpu *vcpu1, struct kvm_vcpu *vcpu2);
>  int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
>  			     struct kvm_lapic_irq *irq,

-- 
Vitaly

Re: [PATCH v2 3/3] KVM: X86: Fixup kvm_apic_match_dest() dest_mode parameter

[Sean Christopherson]

On Mon, Dec 02, 2019 at 10:18:00AM +0100, Vitaly Kuznetsov wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > The problem is the same as the previous patch on that we've got too
> > many ways to define a dest_mode, but logically we should only pass in
> > APIC_DEST_* macros for this helper.
> 
> Using 'the previous patch' in changelog is OK until it comes to
> backporting as the order can change. I'd suggest to spell out "KVM: X86:
> Use APIC_DEST_* macros properly" explicitly.

Even that is bad practice IMO.  Unless there is an explicit dependency on
a previous patch, which does not seem to be the case here, the changelog
should fully describe and justify the patch without referencing a previous
patch/commit.

Case in point, I haven't looked at the previous patch yet and have no idea
why *this* patch is needed or what it's trying to accomplish.

> >
> > To make it easier, simply define dest_mode of kvm_apic_match_dest() to
> > be a boolean to make it right while we can avoid to touch the callers.
> >
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  arch/x86/kvm/lapic.c | 5 +++--
> >  arch/x86/kvm/lapic.h | 2 +-
> >  2 files changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> > index cf9177b4a07f..80732892c709 100644
> > --- a/arch/x86/kvm/lapic.c
> > +++ b/arch/x86/kvm/lapic.c
> > @@ -791,8 +791,9 @@ static u32 kvm_apic_mda(struct kvm_vcpu *vcpu, unsigned int dest_id,
> >  	return dest_id;
> >  }
> >  
> > +/* Set dest_mode to true for logical mode, false for physical mode */
> >  bool kvm_apic_match_dest(struct kvm_vcpu *vcpu, struct kvm_lapic *source,
> > -			   int short_hand, unsigned int dest, int dest_mode)
> > +			   int short_hand, unsigned int dest, bool dest_mode)
> >  {
> >  	struct kvm_lapic *target = vcpu->arch.apic;
> >  	u32 mda = kvm_apic_mda(vcpu, dest, source, target);
> > @@ -800,7 +801,7 @@ bool kvm_apic_match_dest(struct kvm_vcpu *vcpu, struct kvm_lapic *source,
> >  	ASSERT(target);
> >  	switch (short_hand) {
> >  	case APIC_DEST_NOSHORT:
> > -		if (dest_mode == APIC_DEST_PHYSICAL)
> > +		if (dest_mode == false)
> 
> I must admit this seriously harm the readability of the code for
> me. Just look at the 
> 
>  if (dest_mode == false)
> 
> line without a context and try to say what's being checked. I can't.
> 
> I see to solutions:
> 1) Adhere to the APIC_DEST_PHYSICAL/APIC_DEST_LOGICAL (basically - just
> check against "dest_mode == APIC_DEST_LOGICAL" in the else branch)
> 2) Rename the dest_mode parameter to 'dest_mode_is_phys' or something
> like that.

For #2, it should be "logical" instead of "phys" as APIC_DEST_PHYSICAL is
the zero value.

There's also a third option:

  3) Add a WARN_ON_ONCE and fix the IO APIC callers, e.g.:

	WARN_ON_ONCE(dest_mode != APIC_DEST_PHYSICAL ||
		     dest_mode != APIC_DEST_LOGICAL);

	if (dest_mode == APIC_DEST_PHYSICAL)
		return kvm_apic_match_physical_addr(target, mda);
	else
		return kvm_apic_match_logical_addr(target, mda);

Part of me likes the simplicity of #2, but on the other hand I don't like
the inconsistency with respect to @short_hand and @dest, which take in
"full" values.  E.g. @short_hand would also be problematic for a caller
that uses a bitfield.

Side topic, the I/O APIC callers should explicitly pass APIC_DEST_NOSHORT
instead of 0.

> >  			return kvm_apic_match_physical_addr(target, mda);
> >  		else
> >  			return kvm_apic_match_logical_addr(target, mda);
> > diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
> > index 19b36196e2ff..c0b472ed87ad 100644
> > --- a/arch/x86/kvm/lapic.h
> > +++ b/arch/x86/kvm/lapic.h
> > @@ -82,7 +82,7 @@ int kvm_lapic_reg_write(struct kvm_lapic *apic, u32 reg, u32 val);
> >  int kvm_lapic_reg_read(struct kvm_lapic *apic, u32 offset, int len,
> >  		       void *data);
> >  bool kvm_apic_match_dest(struct kvm_vcpu *vcpu, struct kvm_lapic *source,
> > -			   int short_hand, unsigned int dest, int dest_mode);
> > +			   int short_hand, unsigned int dest, bool dest_mode);
> >  int kvm_apic_compare_prio(struct kvm_vcpu *vcpu1, struct kvm_vcpu *vcpu2);
> >  int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
> >  			     struct kvm_lapic_irq *irq,
> 
> -- 
> Vitaly
> 

Re: [PATCH v2 3/3] KVM: X86: Fixup kvm_apic_match_dest() dest_mode parameter

[Peter Xu]

On Mon, Dec 02, 2019 at 09:31:52AM -0800, Sean Christopherson wrote:
> On Mon, Dec 02, 2019 at 10:18:00AM +0100, Vitaly Kuznetsov wrote:
> > Peter Xu <peterx@redhat.com> writes:
> > 
> > > The problem is the same as the previous patch on that we've got too
> > > many ways to define a dest_mode, but logically we should only pass in
> > > APIC_DEST_* macros for this helper.
> > 
> > Using 'the previous patch' in changelog is OK until it comes to
> > backporting as the order can change. I'd suggest to spell out "KVM: X86:
> > Use APIC_DEST_* macros properly" explicitly.
> 
> Even that is bad practice IMO.  Unless there is an explicit dependency on
> a previous patch, which does not seem to be the case here, the changelog
> should fully describe and justify the patch without referencing a previous
> patch/commit.
> 
> Case in point, I haven't looked at the previous patch yet and have no idea
> why *this* patch is needed or what it's trying to accomplish.

I'll improve both commit messages.

> 
> > >
> > > To make it easier, simply define dest_mode of kvm_apic_match_dest() to
> > > be a boolean to make it right while we can avoid to touch the callers.
> > >
> > > Signed-off-by: Peter Xu <peterx@redhat.com>
> > > ---
> > >  arch/x86/kvm/lapic.c | 5 +++--
> > >  arch/x86/kvm/lapic.h | 2 +-
> > >  2 files changed, 4 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> > > index cf9177b4a07f..80732892c709 100644
> > > --- a/arch/x86/kvm/lapic.c
> > > +++ b/arch/x86/kvm/lapic.c
> > > @@ -791,8 +791,9 @@ static u32 kvm_apic_mda(struct kvm_vcpu *vcpu, unsigned int dest_id,
> > >  	return dest_id;
> > >  }
> > >  
> > > +/* Set dest_mode to true for logical mode, false for physical mode */
> > >  bool kvm_apic_match_dest(struct kvm_vcpu *vcpu, struct kvm_lapic *source,
> > > -			   int short_hand, unsigned int dest, int dest_mode)
> > > +			   int short_hand, unsigned int dest, bool dest_mode)
> > >  {
> > >  	struct kvm_lapic *target = vcpu->arch.apic;
> > >  	u32 mda = kvm_apic_mda(vcpu, dest, source, target);
> > > @@ -800,7 +801,7 @@ bool kvm_apic_match_dest(struct kvm_vcpu *vcpu, struct kvm_lapic *source,
> > >  	ASSERT(target);
> > >  	switch (short_hand) {
> > >  	case APIC_DEST_NOSHORT:
> > > -		if (dest_mode == APIC_DEST_PHYSICAL)
> > > +		if (dest_mode == false)
> > 
> > I must admit this seriously harm the readability of the code for
> > me. Just look at the 
> > 
> >  if (dest_mode == false)
> > 
> > line without a context and try to say what's being checked. I can't.
> > 
> > I see to solutions:
> > 1) Adhere to the APIC_DEST_PHYSICAL/APIC_DEST_LOGICAL (basically - just
> > check against "dest_mode == APIC_DEST_LOGICAL" in the else branch)
> > 2) Rename the dest_mode parameter to 'dest_mode_is_phys' or something
> > like that.
> 
> For #2, it should be "logical" instead of "phys" as APIC_DEST_PHYSICAL is
> the zero value.
> 
> There's also a third option:
> 
>   3) Add a WARN_ON_ONCE and fix the IO APIC callers, e.g.:
> 
> 	WARN_ON_ONCE(dest_mode != APIC_DEST_PHYSICAL ||
> 		     dest_mode != APIC_DEST_LOGICAL);
> 
> 	if (dest_mode == APIC_DEST_PHYSICAL)
> 		return kvm_apic_match_physical_addr(target, mda);
> 	else
> 		return kvm_apic_match_logical_addr(target, mda);
> 
> Part of me likes the simplicity of #2, but on the other hand I don't like
> the inconsistency with respect to @short_hand and @dest, which take in
> "full" values.  E.g. @short_hand would also be problematic for a caller
> that uses a bitfield.

IMHO the best way is that we should always use a boolean for dest mode
internally because it's always a true or false flag, and we only
convert it to other forms when needed (e.g. when applying that bit to
an IOAPIC entry).  But here I think I'll go with the 3rd option to
avoid code churns (I think it's also what Vitaly suggested as the 1st
option).

> 
> Side topic, the I/O APIC callers should explicitly pass APIC_DEST_NOSHORT
> instead of 0.

I'll fix that too.

(I also missed suggested-by/reported-by for Vitaly)

Thank you both for your reviews,

-- 
Peter Xu