Message ID | 20191129163234.18902-4-peterx@redhat.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | KVM: X86: Cleanups on dest_mode and headers | expand |
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,
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 >
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,
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,
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(-)