KVM Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v2 0/3] KVM: X86: Cleanups on dest_mode and headers
@ 2019-11-29 16:32 Peter Xu
  2019-11-29 16:32 ` [PATCH v2 1/3] KVM: X86: Some cleanups in ioapic.h/lapic.h Peter Xu
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Peter Xu @ 2019-11-29 16:32 UTC (permalink / raw)
  To: linux-kernel, kvm
  Cc: Nitesh Narayan Lal, Paolo Bonzini, Sean Christopherson, peterx,
	Vitaly Kuznetsov

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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v2 1/3] KVM: X86: Some cleanups in ioapic.h/lapic.h
  2019-11-29 16:32 [PATCH v2 0/3] KVM: X86: Cleanups on dest_mode and headers Peter Xu
@ 2019-11-29 16:32 ` Peter Xu
  2019-12-02  9:27   ` Vitaly Kuznetsov
  2019-11-29 16:32 ` [PATCH v2 2/3] KVM: X86: Use APIC_DEST_* macros properly Peter Xu
  2019-11-29 16:32 ` [PATCH v2 3/3] KVM: X86: Fixup kvm_apic_match_dest() dest_mode parameter Peter Xu
  2 siblings, 1 reply; 10+ messages in thread
From: Peter Xu @ 2019-11-29 16:32 UTC (permalink / raw)
  To: linux-kernel, kvm
  Cc: Nitesh Narayan Lal, Paolo Bonzini, Sean Christopherson, peterx,
	Vitaly Kuznetsov

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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v2 2/3] KVM: X86: Use APIC_DEST_* macros properly
  2019-11-29 16:32 [PATCH v2 0/3] KVM: X86: Cleanups on dest_mode and headers Peter Xu
  2019-11-29 16:32 ` [PATCH v2 1/3] KVM: X86: Some cleanups in ioapic.h/lapic.h Peter Xu
@ 2019-11-29 16:32 ` Peter Xu
  2019-11-29 16:32 ` [PATCH v2 3/3] KVM: X86: Fixup kvm_apic_match_dest() dest_mode parameter Peter Xu
  2 siblings, 0 replies; 10+ messages in thread
From: Peter Xu @ 2019-11-29 16:32 UTC (permalink / raw)
  To: linux-kernel, kvm
  Cc: Nitesh Narayan Lal, Paolo Bonzini, Sean Christopherson, peterx,
	Vitaly Kuznetsov

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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v2 3/3] KVM: X86: Fixup kvm_apic_match_dest() dest_mode parameter
  2019-11-29 16:32 [PATCH v2 0/3] KVM: X86: Cleanups on dest_mode and headers Peter Xu
  2019-11-29 16:32 ` [PATCH v2 1/3] KVM: X86: Some cleanups in ioapic.h/lapic.h Peter Xu
  2019-11-29 16:32 ` [PATCH v2 2/3] KVM: X86: Use APIC_DEST_* macros properly Peter Xu
@ 2019-11-29 16:32 ` Peter Xu
  2019-12-02  9:18   ` Vitaly Kuznetsov
  2 siblings, 1 reply; 10+ messages in thread
From: Peter Xu @ 2019-11-29 16:32 UTC (permalink / raw)
  To: linux-kernel, kvm
  Cc: Nitesh Narayan Lal, Paolo Bonzini, Sean Christopherson, peterx,
	Vitaly Kuznetsov

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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 3/3] KVM: X86: Fixup kvm_apic_match_dest() dest_mode parameter
  2019-11-29 16:32 ` [PATCH v2 3/3] KVM: X86: Fixup kvm_apic_match_dest() dest_mode parameter Peter Xu
@ 2019-12-02  9:18   ` Vitaly Kuznetsov
  2019-12-02 17:31     ` Sean Christopherson
  0 siblings, 1 reply; 10+ messages in thread
From: Vitaly Kuznetsov @ 2019-12-02  9:18 UTC (permalink / raw)
  To: Peter Xu, linux-kernel, kvm
  Cc: Nitesh Narayan Lal, Paolo Bonzini, Sean Christopherson, peterx

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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 1/3] KVM: X86: Some cleanups in ioapic.h/lapic.h
  2019-11-29 16:32 ` [PATCH v2 1/3] KVM: X86: Some cleanups in ioapic.h/lapic.h Peter Xu
@ 2019-12-02  9:27   ` Vitaly Kuznetsov
  2019-12-02 17:47     ` Sean Christopherson
  0 siblings, 1 reply; 10+ messages in thread
From: Vitaly Kuznetsov @ 2019-12-02  9:27 UTC (permalink / raw)
  To: Peter Xu, linux-kernel, kvm
  Cc: Nitesh Narayan Lal, Paolo Bonzini, Sean Christopherson, peterx

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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 3/3] KVM: X86: Fixup kvm_apic_match_dest() dest_mode parameter
  2019-12-02  9:18   ` Vitaly Kuznetsov
@ 2019-12-02 17:31     ` Sean Christopherson
  2019-12-02 18:59       ` Peter Xu
  0 siblings, 1 reply; 10+ messages in thread
From: Sean Christopherson @ 2019-12-02 17:31 UTC (permalink / raw)
  To: Vitaly Kuznetsov
  Cc: Peter Xu, linux-kernel, kvm, Nitesh Narayan Lal, Paolo Bonzini

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
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 1/3] KVM: X86: Some cleanups in ioapic.h/lapic.h
  2019-12-02  9:27   ` Vitaly Kuznetsov
@ 2019-12-02 17:47     ` Sean Christopherson
  2019-12-02 19:13       ` Peter Xu
  0 siblings, 1 reply; 10+ messages in thread
From: Sean Christopherson @ 2019-12-02 17:47 UTC (permalink / raw)
  To: Vitaly Kuznetsov
  Cc: Peter Xu, linux-kernel, kvm, Nitesh Narayan Lal, Paolo Bonzini

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
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 3/3] KVM: X86: Fixup kvm_apic_match_dest() dest_mode parameter
  2019-12-02 17:31     ` Sean Christopherson
@ 2019-12-02 18:59       ` Peter Xu
  0 siblings, 0 replies; 10+ messages in thread
From: Peter Xu @ 2019-12-02 18:59 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: Vitaly Kuznetsov, linux-kernel, kvm, Nitesh Narayan Lal, Paolo Bonzini

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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 1/3] KVM: X86: Some cleanups in ioapic.h/lapic.h
  2019-12-02 17:47     ` Sean Christopherson
@ 2019-12-02 19:13       ` Peter Xu
  0 siblings, 0 replies; 10+ messages in thread
From: Peter Xu @ 2019-12-02 19:13 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: Vitaly Kuznetsov, linux-kernel, kvm, Nitesh Narayan Lal, Paolo Bonzini

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


^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, back to index

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-29 16:32 [PATCH v2 0/3] KVM: X86: Cleanups on dest_mode and headers Peter Xu
2019-11-29 16:32 ` [PATCH v2 1/3] KVM: X86: Some cleanups in ioapic.h/lapic.h Peter Xu
2019-12-02  9:27   ` Vitaly Kuznetsov
2019-12-02 17:47     ` Sean Christopherson
2019-12-02 19:13       ` Peter Xu
2019-11-29 16:32 ` [PATCH v2 2/3] KVM: X86: Use APIC_DEST_* macros properly Peter Xu
2019-11-29 16:32 ` [PATCH v2 3/3] KVM: X86: Fixup kvm_apic_match_dest() dest_mode parameter Peter Xu
2019-12-02  9:18   ` Vitaly Kuznetsov
2019-12-02 17:31     ` Sean Christopherson
2019-12-02 18:59       ` Peter Xu

KVM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/kvm/0 kvm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 kvm kvm/ https://lore.kernel.org/kvm \
		kvm@vger.kernel.org
	public-inbox-index kvm

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.kvm


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git