KVM: X86: Cleanups on dest_mode and headers

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 <[email protected]>

---
 arch/x86/kvm/ioapic.h | 6 ------
 arch/x86/kvm/lapic.h  | 5 ++++-
 2 files changed, 4 insertions(+), 7 deletions(-)

arch/x86/kvm/ioapic.h +0 -6

index ea1a4e0297da..2fb2e3c80724 100644
@@ -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,

arch/x86/kvm/lapic.h +4 -1

index 39925afdfcdc..19b36196e2ff 100644
@@ -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);

Quoted 2 lines · 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.

Quoted 1 line

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

Quoted 1 line

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.)

Quoted 46 lines

Signed-off-by Peter Xu <[email protected]>

---
 arch/x86/kvm/ioapic.h | 6 ------
 arch/x86/kvm/lapic.h  | 5 ++++-
 2 files changed, 4 insertions(+), 7 deletions(-)

arch/x86/kvm/ioapic.h +0 -6

index ea1a4e0297da..2fb2e3c80724 100644
@@ -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,

arch/x86/kvm/lapic.h +4 -1

index 39925afdfcdc..19b36196e2ff 100644
@@ -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);

Signature · 1 line

Vitaly

Quoted 5 lines

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.

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 <[email protected]>

---
 arch/x86/kvm/lapic.c | 5 +++--
 arch/x86/kvm/lapic.h | 2 +-
 2 files changed, 4 insertions(+), 3 deletions(-)

arch/x86/kvm/lapic.c +2 -1

index cf9177b4a07f..80732892c709 100644
@@ -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);

arch/x86/kvm/lapic.c +0 -0

 			return kvm_apic_match_physical_addr(target, mda);
 		else
 			return kvm_apic_match_logical_addr(target, mda);

arch/x86/kvm/lapic.h +1 -1

index 19b36196e2ff..c0b472ed87ad 100644
@@ -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,

Quoted 3 lines · 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.

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.

Quoted 26 lines

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 <[email protected]>

---
 arch/x86/kvm/lapic.c | 5 +++--
 arch/x86/kvm/lapic.h | 2 +-
 2 files changed, 4 insertions(+), 3 deletions(-)

arch/x86/kvm/lapic.c +2 -1

index cf9177b4a07f..80732892c709 100644
@@ -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);

arch/x86/kvm/lapic.c bool kvm_apic_match_dest(struct kvm_vcpu *vcpu, struct kvm_lapic *source, · line 804

Quoted 5 lines

arch/x86/kvm/lapic.c +1 -1

@@ -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.

Quoted 16 lines

arch/x86/kvm/lapic.c +0 -0

 			return kvm_apic_match_physical_addr(target, mda);
 		else
 			return kvm_apic_match_logical_addr(target, mda);

arch/x86/kvm/lapic.h +1 -1

index 19b36196e2ff..c0b472ed87ad 100644
@@ -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,

Signature · 1 line

Vitaly

Quoted 9 lines · Vitaly Kuznetsov

Quoted 3 lines · 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.

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.

Quoted 38 lines

Quoted 27 lines

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 <[email protected]>

---
 arch/x86/kvm/lapic.c | 5 +++--
 arch/x86/kvm/lapic.h | 2 +-
 2 files changed, 4 insertions(+), 3 deletions(-)

arch/x86/kvm/lapic.c +2 -1

index cf9177b4a07f..80732892c709 100644
@@ -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);

arch/x86/kvm/lapic.c bool kvm_apic_match_dest(struct kvm_vcpu *vcpu, struct kvm_lapic *source, · line 804

Quoted 5 lines

arch/x86/kvm/lapic.c +1 -1

@@ -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)

Quoted 7 lines

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.