Message ID | 20191128193211.32684-1-peterx@redhat.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | KVM: X86: Use APIC_DEST_* macros properly | expand |
Peter Xu <peterx@redhat.com> writes: > 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. > > CC: Paolo Bonzini <pbonzini@redhat.com> > CC: Sean Christopherson <sean.j.christopherson@intel.com> > CC: Vitaly Kuznetsov <vkuznets@redhat.com> > CC: Nitesh Narayan Lal <nitesh@redhat.com> > 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; dest_mode is being passed to kvm_apic_match_dest() where we do: case APIC_DEST_NOSHORT: if (dest_mode == APIC_DEST_PHYSICAL) return kvm_apic_match_physical_addr(target, mda); else return kvm_apic_match_logical_addr(target, mda); I'd suggest we fix this too then (and BUG() in case it's neither).
On Fri, Nov 29, 2019 at 03:23:36PM +0100, Vitaly Kuznetsov wrote: > Peter Xu <peterx@redhat.com> writes: > > > 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. > > > > CC: Paolo Bonzini <pbonzini@redhat.com> > > CC: Sean Christopherson <sean.j.christopherson@intel.com> > > CC: Vitaly Kuznetsov <vkuznets@redhat.com> > > CC: Nitesh Narayan Lal <nitesh@redhat.com> > > 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; > > dest_mode is being passed to kvm_apic_match_dest() where we do: > > case APIC_DEST_NOSHORT: > if (dest_mode == APIC_DEST_PHYSICAL) > return kvm_apic_match_physical_addr(target, mda); > else > return kvm_apic_match_logical_addr(target, mda); > > I'd suggest we fix this too then (and BUG() in case it's neither). That's true. Let me add another patch to fix it (probably easier by changing the type of dest_mode param in kvm_apic_match_dest from int to bool). Thanks,
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;
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. CC: Paolo Bonzini <pbonzini@redhat.com> CC: Sean Christopherson <sean.j.christopherson@intel.com> CC: Vitaly Kuznetsov <vkuznets@redhat.com> CC: Nitesh Narayan Lal <nitesh@redhat.com> 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(-)