KVM: X86: Cleanups on dest_mode and headers
Subsystems (4)
v3 cover KVM: X86: Cleanups on dest_mode and headers
v3:
- address all the comments from both Vitaly and Sean
- since at it, added patches:
"KVM: X86: Fix kvm_bitmap_or_dest_vcpus() to use irq shorthand" "KVM: X86: Drop KVM_APIC_SHORT_MASK and KVM_APIC_DEST_MASK"
Each patch explains itself.
Please have a look, thanks.
Peter Xu (5):
KVM: X86: Fix kvm_bitmap_or_dest_vcpus() to use irq shorthand
KVM: X86: Move irrelevant declarations out of ioapic.h
KVM: X86: Use APIC_DEST_* macros properly in kvm_lapic_irq.dest_mode
KVM: X86: Drop KVM_APIC_SHORT_MASK and KVM_APIC_DEST_MASK
KVM: X86: Fix callers of kvm_apic_match_dest() to use correct macros
arch/x86/include/asm/kvm_host.h | 5 +++++ arch/x86/kvm/hyperv.c | 1 + arch/x86/kvm/ioapic.c | 19 ++++++++++++------- arch/x86/kvm/ioapic.h | 6 ------ arch/x86/kvm/irq.h | 3 +++ arch/x86/kvm/irq_comm.c | 10 ++++++---- arch/x86/kvm/lapic.c | 5 +---- arch/x86/kvm/lapic.h | 7 ++++--- arch/x86/kvm/svm.c | 4 ++-- arch/x86/kvm/x86.c | 2 +- 10 files changed, 35 insertions(+), 27 deletions(-)
Signature · 1 line
2.21.0
PATCH v3 1/5 KVM: X86: Fix kvm_bitmap_or_dest_vcpus() to use irq shorthand
The 3rd parameter of kvm_apic_match_dest() is the irq shorthand, rather than the irq delivery mode.
--- arch/x86/kvm/lapic.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
arch/x86/kvm/lapic.c +1 -1
@@ -1151,7 +1151,7 @@ void kvm_bitmap_or_dest_vcpus(struct kvm *kvm, struct kvm_lapic_irq *irq,if (!kvm_apic_present (vcpu))continue ;if (!kvm_apic_match_dest (vcpu,NULL , - irq->delivery_mode, + irq->shorthand, irq->dest_id, irq->dest_mode))continue ;
Signature · 1 line
2.21.0
Re: [PATCH v3 1/5] KVM: X86: Fix kvm_bitmap_or_dest_vcpus() to use irq shorthand
Quoted 4 lines · Peter Xu
The 3rd parameter of
kvm_apic_match_dest()is the irq shorthand, rather than the irq delivery mode.Fixes 7ee30bc132c683d06a6d9e360e39e483e3990708
Better expressed as
Quoted 10 lines
Signed-off-by Peter Xu <[email protected]>--- arch/x86/kvm/lapic.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)arch/x86/kvm/lapic.c +0
-0
arch/x86/kvm/lapic.c void kvm_bitmap_or_dest_vcpus(struct kvm *kvm, struct kvm_lapic_irq *irq, · line 1157
Quoted 8 lines
arch/x86/kvm/lapic.c +1
-1@@ -1151,7 +1151,7 @@ void kvm_bitmap_or_dest_vcpus(struct kvm *kvm, struct kvm_lapic_irq *irq,if (!kvm_apic_present (vcpu))continue ;if (!kvm_apic_match_dest (vcpu,NULL , - irq->delivery_mode, + irq->shorthand, irq->dest_id, irq->dest_mode))continue ;
Signature · 1 line
Vitaly
PATCH v3 2/5 KVM: X86: Move irrelevant declarations out of ioapic.h
In reply to Peter Xukvm_apic_match_dest() is declared in both ioapic.h and lapic.h. Removing the declaration in ioapic.h.
kvm_apic_compare_prio() is declared in ioapic.h but defined in lapic.c. Moving the declaration to lapic.h.
kvm_irq_delivery_to_apic() is declared in ioapic.h but defined in irq_comm.c. Moving the declaration to irq.h.
While at it, include irq.h in hyperv.c because it needs to use kvm_irq_delivery_to_apic().
--- arch/x86/kvm/hyperv.c | 1 + arch/x86/kvm/ioapic.h | 6 ------ arch/x86/kvm/irq.h | 3 +++ arch/x86/kvm/lapic.h | 2 +- 4 files changed, 5 insertions(+), 7 deletions(-)
arch/x86/kvm/hyperv.c +1 -0
@@ -33,6 +33,7 @@#include <trace/events/kvm.h> #include "trace.h" +#include "irq.h" #define KVM_HV_MAX_SPARSE_VCPU_SET_BITS DIV_ROUND_UP(KVM_MAX_VCPUS, 64)
arch/x86/kvm/ioapic.h +0 -6
@@ -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/irq.h +3 -0
@@ -113,5 +113,8 @@ int apic_has_pending_timer(struct kvm_vcpu *vcpu);int kvm_setup_default_irq_routing (struct kvm *kvm);int kvm_setup_empty_irq_routing (struct kvm *kvm); +int kvm_irq_delivery_to_apic (struct kvm *kvm,struct kvm_lapic *src, +struct kvm_lapic_irq *irq, +struct dest_map *dest_map);#endif
arch/x86/kvm/lapic.h +1 -1
@@ -83,7 +83,7 @@ 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);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
2.21.0
Re: [PATCH v3 2/5] KVM: X86: Move irrelevant declarations out of ioapic.h
Quoted 8 lines · Peter Xu
kvm_apic_match_dest()is declared in bothioapic.handlapic.h. Removing the declaration inioapic.h.
kvm_apic_compare_prio()is declared inioapic.hbut defined inlapic.c. Moving the declaration tolapic.h.
kvm_irq_delivery_to_apic()is declared inioapic.hbut defined inirq_comm.c. Moving the declaration toirq.h.
Nitpicking: 'imperative mode' requested by Sean would be "remove the declaration", "move the declaration",...
Quoted 3 lines
While at it, include
irq.hinhyperv.cbecause it needs to usekvm_irq_delivery_to_apic().
"While at it" is being used when you are trying to squeeze in a (small) unrelated change (fix a typo, rename a variable,...) but here it's not the case: including irq.h to hyperv.c is mandatory (to not break the build).
"Include irq.h in hyperv.c to support the change" would do (but honestly I don't see much value in the statement so I'd rather omit in in the changelog).
Quoted 63 lines
Signed-off-by Peter Xu <[email protected]>--- arch/x86/kvm/hyperv.c | 1 + arch/x86/kvm/ioapic.h | 6 ------ arch/x86/kvm/irq.h | 3 +++ arch/x86/kvm/lapic.h | 2 +- 4 files changed, 5 insertions(+), 7 deletions(-)arch/x86/kvm/hyperv.c +1
-0@@ -33,6 +33,7 @@#include <trace/events/kvm.h> #include "trace.h" +#include "irq.h" #define KVM_HV_MAX_SPARSE_VCPU_SET_BITS DIV_ROUND_UP(KVM_MAX_VCPUS, 64)arch/x86/kvm/ioapic.h +0
-6@@ -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/irq.h +3
-0@@ -113,5 +113,8 @@ int apic_has_pending_timer(struct kvm_vcpu *vcpu);int kvm_setup_default_irq_routing (struct kvm *kvm);int kvm_setup_empty_irq_routing (struct kvm *kvm); +int kvm_irq_delivery_to_apic (struct kvm *kvm,struct kvm_lapic *src, +struct kvm_lapic_irq *irq, +struct dest_map *dest_map);#endif arch/x86/kvm/lapic.h +0
-0
arch/x86/kvm/lapic.h int kvm_lapic_reg_read(struct kvm_lapic *apic, u32 offset, int len, · line 89
Quoted 8 lines
arch/x86/kvm/lapic.h +1
-1@@ -83,7 +83,7 @@ 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);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
PATCH v3 3/5 KVM: X86: Use APIC_DEST_* macros properly in kvm_lapic_irq.dest_mode
In reply to Peter XuWe were using either APIC_DEST_PHYSICAL|APIC_DEST_LOGICAL or 0|1 to fill in kvm_lapic_irq.dest_mode. It's fine 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. We'll have problem when we want to start checking against APIC_DEST_PHYSICAL, which does not equals to 1.
This patch firstly introduces kvm_lapic_irq_dest_mode() helper to take any boolean of destination mode and return the APIC_DEST_* macro. Then, it replaces the 0|1 settings of irq.dest_mode with the helper.
--- arch/x86/include/asm/kvm_host.h | 5 +++++ arch/x86/kvm/ioapic.c | 8 +++++--- arch/x86/kvm/irq_comm.c | 7 ++++--- arch/x86/kvm/x86.c | 2 +- 4 files changed, 15 insertions(+), 7 deletions(-)
arch/x86/include/asm/kvm_host.h +5 -0
@@ -1022,6 +1022,11 @@ struct kvm_lapic_irq {bool msi_redir_hint; }; +static inline u16 kvm_lapic_irq_dest_mode (bool dest_mode) +{ +return dest_mode ?APIC_DEST_LOGICAL :APIC_DEST_PHYSICAL ; +} +struct kvm_x86_ops {int (*cpu_has_kvm_support)(void );/* __init */ int (*disabled_by_bios)(void );/* __init */
arch/x86/kvm/ioapic.c +5 -3
@@ -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 = +kvm_lapic_irq_dest_mode (e->fields.dest_mode);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 = +kvm_lapic_irq_dest_mode (e->fields.dest_mode);kvm_bitmap_or_dest_vcpus (ioapic->kvm, &irq, &vcpu_bitmap); }
@@ -369,7 +371,7 @@ 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 =kvm_lapic_irq_dest_mode (entry->fields.dest_mode); irqe.trig_mode = entry->fields.trig_mode; irqe.delivery_mode = entry->fields.delivery_mode <<8 ; irqe.level =1 ;
arch/x86/kvm/irq_comm.c +4 -3
@@ -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 =kvm_lapic_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->msi_redir_hint = ((e->msi.address_lo
arch/x86/kvm/x86.c +1 -1
@@ -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;
Signature · 1 line
2.21.0
Vitaly Kuznetsov
maintainerRe: [PATCH v3 3/5] KVM: X86: Use APIC_DEST_* macros properly in kvm_lapic_irq.dest_mode
Quoted 5 lines · Peter Xu
We were using either APIC_DEST_PHYSICAL|APIC_DEST_LOGICAL or 0|1 to fill in kvm_lapic_irq.dest_mode. It's fine only because in most cases when we check against
dest_modeit's againstAPIC_DEST_PHYSICAL(which equals to 0). However, that's not consistent. We'll have problem when we want to start checking againstAPIC_DEST_PHYSICAL
APIC_DEST_LOGICAL
Quoted 79 lines
which does not equals to 1.
This patch firstly introduces
kvm_lapic_irq_dest_mode()helper to take any boolean of destination mode and return the APIC_DEST_* macro. Then, it replaces the 0|1 settings of irq.dest_mode with the helper.Signed-off-by Peter Xu <[email protected]>--- arch/x86/include/asm/kvm_host.h | 5 +++++ arch/x86/kvm/ioapic.c | 8 +++++--- arch/x86/kvm/irq_comm.c | 7 ++++--- arch/x86/kvm/x86.c | 2 +- 4 files changed, 15 insertions(+), 7 deletions(-)arch/x86/include/asm/kvm_host.h +5
-0@@ -1022,6 +1022,11 @@ struct kvm_lapic_irq {bool msi_redir_hint; }; +static inline u16 kvm_lapic_irq_dest_mode (bool dest_mode) +{ +return dest_mode ?APIC_DEST_LOGICAL :APIC_DEST_PHYSICAL ; +} +struct kvm_x86_ops {int (*cpu_has_kvm_support)(void );/* __init */ int (*disabled_by_bios)(void );/* __init */ arch/x86/kvm/ioapic.c +5
-3@@ -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 = +kvm_lapic_irq_dest_mode (e->fields.dest_mode);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 = +kvm_lapic_irq_dest_mode (e->fields.dest_mode);kvm_bitmap_or_dest_vcpus (ioapic->kvm, &irq, &vcpu_bitmap); }@@ -369,7 +371,7 @@ 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 =kvm_lapic_irq_dest_mode (entry->fields.dest_mode); irqe.trig_mode = entry->fields.trig_mode; irqe.delivery_mode = entry->fields.delivery_mode <<8 ; irqe.level =1 ;arch/x86/kvm/irq_comm.c +2
-2@@ -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 ; }
arch/x86/kvm/irq_comm.c void kvm_set_msi_irq(struct kvm *kvm, struct kvm_kernel_irq_routing_entry *e, · line 118
Quoted 6 lines
arch/x86/kvm/irq_comm.c +2
-1@@ -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 =kvm_lapic_irq_dest_mode ( + (1 <<MSI_ADDR_DEST_MODE_SHIFT ) & e->msi.address_lo);
This usage is a bit fishy (I understand that it works, but), kvm_lapic_irq_dest_mode()'s input is bool (0/1) but here we're passing something different.
I'm not sure kvm_lapic_irq_dest_mode() is even needed here, but in case it is I'd suggest to add '!!':
kvm_lapic_irq_dest_mode(!!((1 << MSI_ADDR_DEST_MODE_SHIFT) & e->msi.address_lo))
to make things explicit. I don't like how it looks though.
Quoted 16 lines
arch/x86/kvm/irq_comm.c +0
-0irq->trig_mode = (1 <<MSI_DATA_TRIGGER_SHIFT ) & e->msi.data; irq->delivery_mode = e->msi.data &0x700 ; irq->msi_redir_hint = ((e->msi.address_loarch/x86/kvm/x86.c +1
-1@@ -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;
Signature · 1 line
Vitaly
Re: [PATCH v3 3/5] KVM: X86: Use APIC_DEST_* macros properly in kvm_lapic_irq.dest_mode
Quoted 9 lines · Vitaly Kuznetsov
Quoted 5 lines · Peter Xu
We were using either APIC_DEST_PHYSICAL|APIC_DEST_LOGICAL or 0|1 to fill in kvm_lapic_irq.dest_mode. It's fine only because in most cases when we check against
dest_modeit's againstAPIC_DEST_PHYSICAL(which equals to 0). However, that's not consistent. We'll have problem when we want to start checking againstAPIC_DEST_PHYSICAL
APIC_DEST_LOGICAL
Fixed.
Quoted 7 lines
arch/x86/kvm/irq_comm.c void kvm_set_msi_irq(struct kvm *kvm, struct kvm_kernel_irq_routing_entry *e, · line 118
Quoted 2 lines
arch/x86/kvm/irq_comm.c +2
-0+ irq->dest_mode =kvm_lapic_irq_dest_mode ( + (1 <<MSI_ADDR_DEST_MODE_SHIFT ) & e->msi.address_lo);This usage is a bit fishy (I understand that it works, but), kvm_lapic_irq_dest_mode()'s input is bool (0/1) but here we're passing something different.
Quoted 6 lines
I'm not sure
kvm_lapic_irq_dest_mode()is even needed here, but in case it is I'd suggest to add '!!':
kvm_lapic_irq_dest_mode(!!((1 << MSI_ADDR_DEST_MODE_SHIFT) & e->msi.address_lo))to make things explicit. I don't like how it looks though.
IMHO it's the same (converting uint to _Bool will be the same as "!!", also A ? B : C will be another, so we probably wrote this three times, each of them will translate to a similar pattern of "cmpl + setne" asm code). But sure I can add them if you prefer.
Thanks,
Signature · 1 line
Peter Xu
PATCH v3 4/5 KVM: X86: Drop KVM_APIC_SHORT_MASK and KVM_APIC_DEST_MASK
In reply to Peter XuWe have both APIC_SHORT_MASK and KVM_APIC_SHORT_MASK defined for the shorthand mask. Similarly, we have both APIC_DEST_MASK and KVM_APIC_DEST_MASK defined for the destination mode mask.
Drop the KVM_APIC_* macros and replace the only user of them to use the APIC_DEST_* macros instead. At the meantime, move APIC_SHORT_MASK and APIC_DEST_MASK from lapic.c to lapic.h.
--- arch/x86/kvm/lapic.c | 3 --- arch/x86/kvm/lapic.h | 5 +++-- arch/x86/kvm/svm.c | 4 ++-- 3 files changed, 5 insertions(+), 7 deletions(-)
arch/x86/kvm/lapic.c +0 -3
@@ -56,9 +56,6 @@#define APIC_VERSION (0x14UL | ((KVM_APIC_LVT_NUM - 1) << 16))#define LAPIC_MMIO_LENGTH (1 << 12)/* followed define is not in apicdef.h */ -#define APIC_SHORT_MASK 0xc0000 -#define APIC_DEST_NOSHORT 0x0 -#define APIC_DEST_MASK 0x800#define MAX_APIC_VECTOR 256#define APIC_VECTORS_PER_REG 32
arch/x86/kvm/lapic.h +3 -2
@@ -10,8 +10,9 @@#define KVM_APIC_SIPI 1#define KVM_APIC_LVT_NUM 6 -#define KVM_APIC_SHORT_MASK 0xc0000 -#define KVM_APIC_DEST_MASK 0x800 +#define APIC_SHORT_MASK 0xc0000 +#define APIC_DEST_NOSHORT 0x0 +#define APIC_DEST_MASK 0x800#define APIC_BUS_CYCLE_NS 1#define APIC_BUS_FREQUENCY (1000000000ULL / APIC_BUS_CYCLE_NS)
arch/x86/kvm/svm.c +2 -2
@@ -4519,9 +4519,9 @@ static int avic_incomplete_ipi_interception(struct vcpu_svm *svm)*/ kvm_for_each_vcpu (i, vcpu, kvm) {bool m =kvm_apic_match_dest (vcpu, apic, - icrl &KVM_APIC_SHORT_MASK , + icrl &APIC_SHORT_MASK ,GET_APIC_DEST_FIELD (icrh), - icrl &KVM_APIC_DEST_MASK ); + icrl &APIC_DEST_MASK );if (m && !avic_vcpu_is_running (vcpu))kvm_vcpu_wake_up (vcpu);
Signature · 1 line
2.21.0
Re: [PATCH v3 4/5] KVM: X86: Drop KVM_APIC_SHORT_MASK and KVM_APIC_DEST_MASK
Quoted 51 lines · Peter Xu
We have both
APIC_SHORT_MASKandKVM_APIC_SHORT_MASKdefined for the shorthand mask. Similarly, we have bothAPIC_DEST_MASKandKVM_APIC_DEST_MASKdefined for the destination mode mask.Drop the KVM_APIC_* macros and replace the only user of them to use the APIC_DEST_* macros instead. At the meantime, move
APIC_SHORT_MASKandAPIC_DEST_MASKfromlapic.ctolapic.h.Signed-off-by Peter Xu <[email protected]>--- arch/x86/kvm/lapic.c | 3 --- arch/x86/kvm/lapic.h | 5 +++-- arch/x86/kvm/svm.c | 4 ++-- 3 files changed, 5 insertions(+), 7 deletions(-)arch/x86/kvm/lapic.c +0
-3@@ -56,9 +56,6 @@#define APIC_VERSION (0x14UL | ((KVM_APIC_LVT_NUM - 1) << 16))#define LAPIC_MMIO_LENGTH (1 << 12)/* followed define is not in apicdef.h */ -#define APIC_SHORT_MASK 0xc0000 -#define APIC_DEST_NOSHORT 0x0 -#define APIC_DEST_MASK 0x800#define MAX_APIC_VECTOR 256#define APIC_VECTORS_PER_REG 32arch/x86/kvm/lapic.h +3
-2@@ -10,8 +10,9 @@#define KVM_APIC_SIPI 1#define KVM_APIC_LVT_NUM 6 -#define KVM_APIC_SHORT_MASK 0xc0000 -#define KVM_APIC_DEST_MASK 0x800 +#define APIC_SHORT_MASK 0xc0000 +#define APIC_DEST_NOSHORT 0x0 +#define APIC_DEST_MASK 0x800#define APIC_BUS_CYCLE_NS 1#define APIC_BUS_FREQUENCY (1000000000ULL / APIC_BUS_CYCLE_NS)arch/x86/kvm/svm.c +0
-0@@ -4519,9 +4519,9 @@ static int avic_incomplete_ipi_interception(struct vcpu_svm *svm)*/
arch/x86/kvm/svm.c static int avic_incomplete_ipi_interception(struct vcpu_svm *svm) · line 4527
Quoted 10 lines
arch/x86/kvm/svm.c +2
-2kvm_for_each_vcpu (i, vcpu, kvm) {bool m =kvm_apic_match_dest (vcpu, apic, - icrl &KVM_APIC_SHORT_MASK , + icrl &APIC_SHORT_MASK ,GET_APIC_DEST_FIELD (icrh), - icrl &KVM_APIC_DEST_MASK ); + icrl &APIC_DEST_MASK );if (m && !avic_vcpu_is_running (vcpu))kvm_vcpu_wake_up (vcpu);
Personal taste but I would've preserved KVM_ prefix. The patch itself looks correct, so
Signature · 1 line
Vitaly
Re: [PATCH v3 4/5] KVM: X86: Drop KVM_APIC_SHORT_MASK and KVM_APIC_DEST_MASK
Quoted 10 lines · Vitaly Kuznetsov
Quoted 10 lines
arch/x86/kvm/svm.c +2
-2@@ -4519,9 +4519,9 @@ static int avic_incomplete_ipi_interception(struct vcpu_svm *svm)*/ kvm_for_each_vcpu (i, vcpu, kvm) {bool m =kvm_apic_match_dest (vcpu, apic, - icrl &KVM_APIC_SHORT_MASK , + icrl &APIC_SHORT_MASK ,GET_APIC_DEST_FIELD (icrh), - icrl &KVM_APIC_DEST_MASK ); + icrl &APIC_DEST_MASK );
Quoted 5 lines
arch/x86/kvm/svm.c static int avic_incomplete_ipi_interception(struct vcpu_svm *svm) · line 4527
Quoted 2 lines
arch/x86/kvm/svm.c +0
-0if (m && !avic_vcpu_is_running (vcpu))kvm_vcpu_wake_up (vcpu);Personal taste but I would've preserved
KVM_prefix. The patch itself looks correct, so
KVM apic uses apicdefs.h a lot, so I was trying to match them (APIC_*) with it.
Quoted 2 lines
Reviewed-by Vitaly Kuznetsov <[email protected]>
Thanks for the reviews,
Signature · 1 line
Peter Xu
PATCH v3 5/5 KVM: X86: Fix callers of kvm_apic_match_dest() to use correct macros
In reply to Peter XuCallers of kvm_apic_match_dest() should always pass in APIC_DEST_* macros for either dest_mode and short_hand parameters. Fix up all the callers of kvm_apic_match_dest() that are not following the rule.
--- arch/x86/kvm/ioapic.c | 11 +++++++---- arch/x86/kvm/irq_comm.c | 3 ++- 2 files changed, 9 insertions(+), 5 deletions(-)
arch/x86/kvm/ioapic.c +7 -4
@@ -108,8 +108,9 @@ static void __rtc_irq_eoi_tracking_restore_one(struct kvm_vcpu *vcpu)union kvm_ioapic_redirect_entry *e; e = &ioapic->redirtbl[RTC_GSI ]; -if (!kvm_apic_match_dest (vcpu,NULL ,0 , e->fields.dest_id, - e->fields.dest_mode)) +if (!kvm_apic_match_dest (vcpu,NULL ,APIC_DEST_NOSHORT , + e->fields.dest_id, +kvm_lapic_irq_dest_mode (e->fields.dest_mode)))return ; new_val =kvm_apic_pending_eoi (vcpu, e->fields.vector);
@@ -237,6 +238,7 @@ void kvm_ioapic_scan_entry(struct kvm_vcpu *vcpu, ulong *ioapic_handled_vectors)struct dest_map *dest_map = &ioapic->rtc_status.dest_map;union kvm_ioapic_redirect_entry *e;int index; +u16 dm;spin_lock (&ioapic->lock);
@@ -250,8 +252,9 @@ void kvm_ioapic_scan_entry(struct kvm_vcpu *vcpu, ulong *ioapic_handled_vectors)if (e->fields.trig_mode ==IOAPIC_LEVEL_TRIG ||kvm_irq_has_notifier (ioapic->kvm,KVM_IRQCHIP_IOAPIC , index) || index ==RTC_GSI ) { -if (kvm_apic_match_dest (vcpu,NULL ,0 , - e->fields.dest_id, e->fields.dest_mode) || + dm =kvm_lapic_irq_dest_mode (e->fields.dest_mode); +if (kvm_apic_match_dest (vcpu,NULL ,APIC_DEST_NOSHORT , + e->fields.dest_id, dm) ||kvm_apic_pending_eoi (vcpu, e->fields.vector))__set_bit (e->fields.vector, ioapic_handled_vectors);
arch/x86/kvm/irq_comm.c +2 -1
@@ -417,7 +417,8 @@ void kvm_scan_ioapic_routes(struct kvm_vcpu *vcpu,kvm_set_msi_irq (vcpu->kvm, entry, &irq); -if (irq.level &&kvm_apic_match_dest (vcpu,NULL ,0 , +if (irq.level && +kvm_apic_match_dest (vcpu,NULL ,APIC_DEST_NOSHORT , irq.dest_id, irq.dest_mode))__set_bit (irq.vector, ioapic_handled_vectors); }
Signature · 1 line
2.21.0
Re: [PATCH v3 5/5] KVM: X86: Fix callers of kvm_apic_match_dest() to use correct macros
Quoted 37 lines · Peter Xu
Callers of
kvm_apic_match_dest()should always pass in APIC_DEST_* macros for eitherdest_modeandshort_handparameters. Fix up all the callers ofkvm_apic_match_dest()that are not following the rule.Reported-by Sean Christopherson <[email protected]>Reported-by Vitaly Kuznetsov <[email protected]>Signed-off-by Peter Xu <[email protected]>--- arch/x86/kvm/ioapic.c | 11 +++++++---- arch/x86/kvm/irq_comm.c | 3 ++- 2 files changed, 9 insertions(+), 5 deletions(-)arch/x86/kvm/ioapic.c +4
-2@@ -108,8 +108,9 @@ static void __rtc_irq_eoi_tracking_restore_one(struct kvm_vcpu *vcpu)union kvm_ioapic_redirect_entry *e; e = &ioapic->redirtbl[RTC_GSI ]; -if (!kvm_apic_match_dest (vcpu,NULL ,0 , e->fields.dest_id, - e->fields.dest_mode)) +if (!kvm_apic_match_dest (vcpu,NULL ,APIC_DEST_NOSHORT , + e->fields.dest_id, +kvm_lapic_irq_dest_mode (e->fields.dest_mode)))return ; new_val =kvm_apic_pending_eoi (vcpu, e->fields.vector);@@ -237,6 +238,7 @@ void kvm_ioapic_scan_entry(struct kvm_vcpu *vcpu, ulong *ioapic_handled_vectors)struct dest_map *dest_map = &ioapic->rtc_status.dest_map;union kvm_ioapic_redirect_entry *e;int index; +u16 dm;spin_lock (&ioapic->lock);
arch/x86/kvm/ioapic.c void kvm_ioapic_scan_entry(struct kvm_vcpu *vcpu, ulong *ioapic_handled_vectors) · line 255
Quoted 6 lines
arch/x86/kvm/ioapic.c +1
-2@@ -250,8 +252,9 @@ void kvm_ioapic_scan_entry(struct kvm_vcpu *vcpu, ulong *ioapic_handled_vectors)if (e->fields.trig_mode ==IOAPIC_LEVEL_TRIG ||kvm_irq_has_notifier (ioapic->kvm,KVM_IRQCHIP_IOAPIC , index) || index ==RTC_GSI ) { - if (kvm_apic_match_dest(vcpu, NULL, 0, - e->fields.dest_id, e->fields.dest_mode) || + dm =kvm_lapic_irq_dest_mode (e->fields.dest_mode);
Nit: you could've defined 'dm' right here in the block (after '{') but in any case I'd suggest to stick to 'dest_mode' and not shorten it to 'dm' for consistency.
Quoted 10 lines
arch/x86/kvm/ioapic.c +2
-0+if (kvm_apic_match_dest (vcpu,NULL ,APIC_DEST_NOSHORT , + e->fields.dest_id, dm) ||kvm_apic_pending_eoi (vcpu, e->fields.vector))__set_bit (e->fields.vector, ioapic_handled_vectors);arch/x86/kvm/irq_comm.c +0
-0
arch/x86/kvm/irq_comm.c void kvm_scan_ioapic_routes(struct kvm_vcpu *vcpu, · line 424
Quoted 9 lines
arch/x86/kvm/irq_comm.c +2
-1@@ -417,7 +417,8 @@ void kvm_scan_ioapic_routes(struct kvm_vcpu *vcpu,kvm_set_msi_irq (vcpu->kvm, entry, &irq); -if (irq.level &&kvm_apic_match_dest (vcpu,NULL ,0 , +if (irq.level && +kvm_apic_match_dest (vcpu,NULL ,APIC_DEST_NOSHORT , irq.dest_id, irq.dest_mode))__set_bit (irq.vector, ioapic_handled_vectors); }
Signature · 1 line
Vitaly
Re: [PATCH v3 5/5] KVM: X86: Fix callers of kvm_apic_match_dest() to use correct macros
Quoted 63 lines · Vitaly Kuznetsov
Quoted 37 lines · Peter Xu
Callers of
kvm_apic_match_dest()should always pass in APIC_DEST_* macros for eitherdest_modeandshort_handparameters. Fix up all the callers ofkvm_apic_match_dest()that are not following the rule.Reported-by Sean Christopherson <[email protected]>Reported-by Vitaly Kuznetsov <[email protected]>Signed-off-by Peter Xu <[email protected]>--- arch/x86/kvm/ioapic.c | 11 +++++++---- arch/x86/kvm/irq_comm.c | 3 ++- 2 files changed, 9 insertions(+), 5 deletions(-)arch/x86/kvm/ioapic.c +4
-2@@ -108,8 +108,9 @@ static void __rtc_irq_eoi_tracking_restore_one(struct kvm_vcpu *vcpu)union kvm_ioapic_redirect_entry *e; e = &ioapic->redirtbl[RTC_GSI ]; -if (!kvm_apic_match_dest (vcpu,NULL ,0 , e->fields.dest_id, - e->fields.dest_mode)) +if (!kvm_apic_match_dest (vcpu,NULL ,APIC_DEST_NOSHORT , + e->fields.dest_id, +kvm_lapic_irq_dest_mode (e->fields.dest_mode)))return ; new_val =kvm_apic_pending_eoi (vcpu, e->fields.vector);@@ -237,6 +238,7 @@ void kvm_ioapic_scan_entry(struct kvm_vcpu *vcpu, ulong *ioapic_handled_vectors)struct dest_map *dest_map = &ioapic->rtc_status.dest_map;union kvm_ioapic_redirect_entry *e;int index; +u16 dm;spin_lock (&ioapic->lock);arch/x86/kvm/ioapic.c void kvm_ioapic_scan_entry(struct kvm_vcpu *vcpu, ulong *ioapic_handled_vectors) · line 255
Quoted 6 lines
arch/x86/kvm/ioapic.c +1
-2@@ -250,8 +252,9 @@ void kvm_ioapic_scan_entry(struct kvm_vcpu *vcpu, ulong *ioapic_handled_vectors)if (e->fields.trig_mode ==IOAPIC_LEVEL_TRIG ||kvm_irq_has_notifier (ioapic->kvm,KVM_IRQCHIP_IOAPIC , index) || index ==RTC_GSI ) { - if (kvm_apic_match_dest(vcpu, NULL, 0, - e->fields.dest_id, e->fields.dest_mode) || + dm =kvm_lapic_irq_dest_mode (e->fields.dest_mode);Nit: you could've defined 'dm' right here in the block (after '{') but in any case I'd suggest to stick to '
dest_mode' and not shorten it to 'dm' for consistency.Quoted 13 lines
arch/x86/kvm/ioapic.c +2
-0+if (kvm_apic_match_dest (vcpu,NULL ,APIC_DEST_NOSHORT , + e->fields.dest_id, dm) ||kvm_apic_pending_eoi (vcpu, e->fields.vector))__set_bit (e->fields.vector, ioapic_handled_vectors);arch/x86/kvm/irq_comm.c +0
-0@@ -417,7 +417,8 @@ void kvm_scan_ioapic_routes(struct kvm_vcpu *vcpu,kvm_set_msi_irq (vcpu->kvm, entry, &irq);
Quoted 8 lines
arch/x86/kvm/irq_comm.c void kvm_scan_ioapic_routes(struct kvm_vcpu *vcpu, · line 424
Quoted 6 lines
arch/x86/kvm/irq_comm.c +2
-1-if (irq.level &&kvm_apic_match_dest (vcpu,NULL ,0 , +if (irq.level && +kvm_apic_match_dest (vcpu,NULL ,APIC_DEST_NOSHORT , irq.dest_id, irq.dest_mode))__set_bit (irq.vector, ioapic_handled_vectors); }Reviewed-by Vitaly Kuznetsov <[email protected]>
I'll move the declaration in with your r-b. 'dm' is a silly trick of mine to avoid the 80-char line limit. Thanks,
Signature · 1 line
Peter Xu
Re: [PATCH v3 5/5] KVM: X86: Fix callers of kvm_apic_match_dest() to use correct macros
Quoted 35 lines · Peter Xu
Quoted 25 lines · Vitaly Kuznetsov
arch/x86/kvm/ioapic.c void kvm_ioapic_scan_entry(struct kvm_vcpu *vcpu, ulong *ioapic_handled_vectors) · line 255
Quoted 7 lines
arch/x86/kvm/ioapic.c +1
-2@@ -250,8 +252,9 @@ void kvm_ioapic_scan_entry(struct kvm_vcpu *vcpu, ulong *ioapic_handled_vectors)if (e->fields.trig_mode ==IOAPIC_LEVEL_TRIG ||kvm_irq_has_notifier (ioapic->kvm,KVM_IRQCHIP_IOAPIC , index) || index ==RTC_GSI ) { - if (kvm_apic_match_dest(vcpu, NULL, 0, - e->fields.dest_id, e->fields.dest_mode) || + dm =kvm_lapic_irq_dest_mode (e->fields.dest_mode);Nit: you could've defined 'dm' right here in the block (after '{') but in any case I'd suggest to stick to '
dest_mode' and not shorten it to 'dm' for consistency.Quoted 13 lines
arch/x86/kvm/ioapic.c +2
-0+if (kvm_apic_match_dest (vcpu,NULL ,APIC_DEST_NOSHORT , + e->fields.dest_id, dm) ||kvm_apic_pending_eoi (vcpu, e->fields.vector))__set_bit (e->fields.vector, ioapic_handled_vectors);arch/x86/kvm/irq_comm.c +0
-0@@ -417,7 +417,8 @@ void kvm_scan_ioapic_routes(struct kvm_vcpu *vcpu,kvm_set_msi_irq (vcpu->kvm, entry, &irq);Quoted 8 lines
arch/x86/kvm/irq_comm.c void kvm_scan_ioapic_routes(struct kvm_vcpu *vcpu, · line 424
Quoted 6 lines
arch/x86/kvm/irq_comm.c +2
-1-if (irq.level &&kvm_apic_match_dest (vcpu,NULL ,0 , +if (irq.level && +kvm_apic_match_dest (vcpu,NULL ,APIC_DEST_NOSHORT , irq.dest_id, irq.dest_mode))__set_bit (irq.vector, ioapic_handled_vectors); }Reviewed-by Vitaly Kuznetsov <[email protected]>
Quoted 2 lines
I'll move the declaration in with your r-b. 'dm' is a silly trick of mine to avoid the 80-char line limit. Thanks,
The 80-char limit isn't an unbreakable rule, it's ok for a line to run a few chars over when there is no better alternative.