Re: [PATCH] KVM: arm64/sve: Fix vq_present() macro to yield a bool
On Thu, Jul 04, 2019 at 02:24:42PM +0200, Paolo Bonzini wrote: > On 04/07/19 10:20, Marc Zyngier wrote: > > +KVM, Paolo and Radim, > > > > Guys, do you mind picking this single patch and sending it to Linus? > > That's the only fix left for 5.2. Alternatively, I can send you a pull > > request, but it feels overkill. > > Sure, will do. > > Paolo Thanks all for the quick turnaround! [...] Cheers ---Dave ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH] KVM: arm64/sve: Fix vq_present() macro to yield a bool
On 04/07/19 10:20, Marc Zyngier wrote: > +KVM, Paolo and Radim, > > Guys, do you mind picking this single patch and sending it to Linus? > That's the only fix left for 5.2. Alternatively, I can send you a pull > request, but it feels overkill. Sure, will do. Paolo > Either way, please let me know. > > Thanks, > > M. > > On Wed, 03 Jul 2019 18:42:50 +0100, > Dave Martin wrote: >> >> From: Zhang Lei >> >> The original implementation of vq_present() relied on aggressive >> inlining in order for the compiler to know that the code is >> correct, due to some const-casting issues. This was causing sparse >> and clang to complain, while GCC compiled cleanly. >> >> Commit 0c529ff789bc addressed this problem, but since vq_present() >> is no longer a function, there is now no implicit casting of the >> returned value to the return type (bool). >> >> In set_sve_vls(), this uncast bit value is compared against a bool, >> and so may spuriously compare as unequal when both are nonzero. As >> a result, KVM may reject valid SVE vector length configurations as >> invalid, and vice versa. >> >> Fix it by forcing the returned value to a bool. >> >> Signed-off-by: Zhang Lei >> Fixes: 0c529ff789bc ("KVM: arm64: Implement vq_present() as a macro") >> Signed-off-by: Dave Martin [commit message rewrite] >> Cc: Viresh Kumar >> >> --- >> >> Posting this under Zhang Lei's authorship, due to the need to turn this >> fix around quickly. The fix is as per the original suggestion [1]. >> >> Originally observed with the QEMU KVM SVE support under review: >> https://lists.gnu.org/archive/html/qemu-devel/2019-06/msg04945.html >> >> Bug reproduced and fix tested on the Arm Fast Model, with >> http://linux-arm.org/git?p=kvmtool-dm.git;a=shortlog;h=refs/heads/sve/v3/head >> (After rerunning util/update_headers.sh.) >> >> (the --sve-vls command line argument was removed in v4 of the >> kvmtool patches). >> >> [1] >> http://lists.infradead.org/pipermail/linux-arm-kernel/2019-July/664633.html >> --- >> arch/arm64/kvm/guest.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c >> index c2afa79..dfd6264 100644 >> --- a/arch/arm64/kvm/guest.c >> +++ b/arch/arm64/kvm/guest.c >> @@ -208,7 +208,7 @@ static int set_core_reg(struct kvm_vcpu *vcpu, const >> struct kvm_one_reg *reg) >> >> #define vq_word(vq) (((vq) - SVE_VQ_MIN) / 64) >> #define vq_mask(vq) ((u64)1 << ((vq) - SVE_VQ_MIN) % 64) >> -#define vq_present(vqs, vq) ((vqs)[vq_word(vq)] & vq_mask(vq)) >> +#define vq_present(vqs, vq) (!!((vqs)[vq_word(vq)] & vq_mask(vq))) >> >> static int get_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg) >> { >> -- >> 2.1.4 >> > ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH] KVM: arm64/sve: Fix vq_present() macro to yield a bool
On Thu, Jul 04, 2019 at 10:04:08AM +, Zhang, Lei wrote: > Hi guys, > > I have started up KVM guest os successfully with SVE feature with Dave' patch. > > Tested-by: Zhang Lei Thanks for verifying. It's really your fix, I only wrote a commit message for it :) [...] Cheers ---Dave ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
RE: [PATCH] KVM: arm64/sve: Fix vq_present() macro to yield a bool
Hi guys, I have started up KVM guest os successfully with SVE feature with Dave' patch. Tested-by: Zhang Lei > -Original Message- > From: Dave Martin > Sent: Thursday, July 04, 2019 5:35 PM > To: Viresh Kumar > Cc: Okamoto, Takayuki/岡本 高幸 ; Christoffer > Dall ; Ard Biesheuvel ; Marc > Zyngier ; Catalin Marinas > ; Will Deacon ; Zhang, > Lei/張 雷 ; Julien Grall ; > kvmarm@lists.cs.columbia.edu; linux-arm-ker...@lists.infradead.org > Subject: Re: [PATCH] KVM: arm64/sve: Fix vq_present() macro to yield a bool > > On Thu, Jul 04, 2019 at 08:32:52AM +0530, Viresh Kumar wrote: > > On 03-07-19, 18:42, Dave Martin wrote: > > > From: Zhang Lei > > > > > > The original implementation of vq_present() relied on aggressive > > > inlining in order for the compiler to know that the code is correct, > > > due to some const-casting issues. This was causing sparse and clang > > > to complain, while GCC compiled cleanly. > > > > > > Commit 0c529ff789bc addressed this problem, but since vq_present() > > > is no longer a function, there is now no implicit casting of the > > > returned value to the return type (bool). > > > > > > In set_sve_vls(), this uncast bit value is compared against a bool, > > > and so may spuriously compare as unequal when both are nonzero. As > > > a result, KVM may reject valid SVE vector length configurations as > > > invalid, and vice versa. > > > > > > Fix it by forcing the returned value to a bool. > > > > > > Signed-off-by: Zhang Lei > > > Fixes: 0c529ff789bc ("KVM: arm64: Implement vq_present() as a > > > macro") > > > Signed-off-by: Dave Martin [commit message > > > rewrite] > > > Cc: Viresh Kumar > > > > > > --- > > > > > > Posting this under Zhang Lei's authorship, due to the need to turn > > > this fix around quickly. The fix is as per the original suggestion [1]. > > > > > > Originally observed with the QEMU KVM SVE support under review: > > > https://lists.gnu.org/archive/html/qemu-devel/2019-06/msg04945.html > > > > > > Bug reproduced and fix tested on the Arm Fast Model, with > > > http://linux-arm.org/git?p=kvmtool-dm.git;a=shortlog;h=refs/heads/sv > > > e/v3/head (After rerunning util/update_headers.sh.) > > > > > > (the --sve-vls command line argument was removed in v4 of the > > > kvmtool patches). > > > > > > [1] > > > http://lists.infradead.org/pipermail/linux-arm-kernel/2019-July/6646 > > > 33.html > > > --- > > > arch/arm64/kvm/guest.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c index > > > c2afa79..dfd6264 100644 > > > --- a/arch/arm64/kvm/guest.c > > > +++ b/arch/arm64/kvm/guest.c > > > @@ -208,7 +208,7 @@ static int set_core_reg(struct kvm_vcpu *vcpu, > > > const struct kvm_one_reg *reg) > > > > > > #define vq_word(vq) (((vq) - SVE_VQ_MIN) / 64) #define vq_mask(vq) > > > ((u64)1 << ((vq) - SVE_VQ_MIN) % 64) -#define vq_present(vqs, vq) > > > ((vqs)[vq_word(vq)] & vq_mask(vq)) > > > +#define vq_present(vqs, vq) (!!((vqs)[vq_word(vq)] & vq_mask(vq))) > > > > > > static int get_sve_vls(struct kvm_vcpu *vcpu, const struct > > > kvm_one_reg *reg) { > > > > It was a really nice bug :) > > > > Reviewed-by: Viresh Kumar > > Thanks for the quick review! > > Maybe it makes sense to write equality comparisons on bools as !x == !y to be > more defensive against this kind of thing. Anyway, probably best to leave > this > as-is while the dust settles. > > Cheers > ---Dave ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH] KVM: arm64/sve: Fix vq_present() macro to yield a bool
On Thu, Jul 04, 2019 at 08:32:52AM +0530, Viresh Kumar wrote: > On 03-07-19, 18:42, Dave Martin wrote: > > From: Zhang Lei > > > > The original implementation of vq_present() relied on aggressive > > inlining in order for the compiler to know that the code is > > correct, due to some const-casting issues. This was causing sparse > > and clang to complain, while GCC compiled cleanly. > > > > Commit 0c529ff789bc addressed this problem, but since vq_present() > > is no longer a function, there is now no implicit casting of the > > returned value to the return type (bool). > > > > In set_sve_vls(), this uncast bit value is compared against a bool, > > and so may spuriously compare as unequal when both are nonzero. As > > a result, KVM may reject valid SVE vector length configurations as > > invalid, and vice versa. > > > > Fix it by forcing the returned value to a bool. > > > > Signed-off-by: Zhang Lei > > Fixes: 0c529ff789bc ("KVM: arm64: Implement vq_present() as a macro") > > Signed-off-by: Dave Martin [commit message rewrite] > > Cc: Viresh Kumar > > > > --- > > > > Posting this under Zhang Lei's authorship, due to the need to turn this > > fix around quickly. The fix is as per the original suggestion [1]. > > > > Originally observed with the QEMU KVM SVE support under review: > > https://lists.gnu.org/archive/html/qemu-devel/2019-06/msg04945.html > > > > Bug reproduced and fix tested on the Arm Fast Model, with > > http://linux-arm.org/git?p=kvmtool-dm.git;a=shortlog;h=refs/heads/sve/v3/head > > (After rerunning util/update_headers.sh.) > > > > (the --sve-vls command line argument was removed in v4 of the > > kvmtool patches). > > > > [1] > > http://lists.infradead.org/pipermail/linux-arm-kernel/2019-July/664633.html > > --- > > arch/arm64/kvm/guest.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c > > index c2afa79..dfd6264 100644 > > --- a/arch/arm64/kvm/guest.c > > +++ b/arch/arm64/kvm/guest.c > > @@ -208,7 +208,7 @@ static int set_core_reg(struct kvm_vcpu *vcpu, const > > struct kvm_one_reg *reg) > > > > #define vq_word(vq) (((vq) - SVE_VQ_MIN) / 64) > > #define vq_mask(vq) ((u64)1 << ((vq) - SVE_VQ_MIN) % 64) > > -#define vq_present(vqs, vq) ((vqs)[vq_word(vq)] & vq_mask(vq)) > > +#define vq_present(vqs, vq) (!!((vqs)[vq_word(vq)] & vq_mask(vq))) > > > > static int get_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg > > *reg) > > { > > It was a really nice bug :) > > Reviewed-by: Viresh Kumar Thanks for the quick review! Maybe it makes sense to write equality comparisons on bools as !x == !y to be more defensive against this kind of thing. Anyway, probably best to leave this as-is while the dust settles. Cheers ---Dave ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH] KVM: arm64/sve: Fix vq_present() macro to yield a bool
+KVM, Paolo and Radim, Guys, do you mind picking this single patch and sending it to Linus? That's the only fix left for 5.2. Alternatively, I can send you a pull request, but it feels overkill. Either way, please let me know. Thanks, M. On Wed, 03 Jul 2019 18:42:50 +0100, Dave Martin wrote: > > From: Zhang Lei > > The original implementation of vq_present() relied on aggressive > inlining in order for the compiler to know that the code is > correct, due to some const-casting issues. This was causing sparse > and clang to complain, while GCC compiled cleanly. > > Commit 0c529ff789bc addressed this problem, but since vq_present() > is no longer a function, there is now no implicit casting of the > returned value to the return type (bool). > > In set_sve_vls(), this uncast bit value is compared against a bool, > and so may spuriously compare as unequal when both are nonzero. As > a result, KVM may reject valid SVE vector length configurations as > invalid, and vice versa. > > Fix it by forcing the returned value to a bool. > > Signed-off-by: Zhang Lei > Fixes: 0c529ff789bc ("KVM: arm64: Implement vq_present() as a macro") > Signed-off-by: Dave Martin [commit message rewrite] > Cc: Viresh Kumar > > --- > > Posting this under Zhang Lei's authorship, due to the need to turn this > fix around quickly. The fix is as per the original suggestion [1]. > > Originally observed with the QEMU KVM SVE support under review: > https://lists.gnu.org/archive/html/qemu-devel/2019-06/msg04945.html > > Bug reproduced and fix tested on the Arm Fast Model, with > http://linux-arm.org/git?p=kvmtool-dm.git;a=shortlog;h=refs/heads/sve/v3/head > (After rerunning util/update_headers.sh.) > > (the --sve-vls command line argument was removed in v4 of the > kvmtool patches). > > [1] > http://lists.infradead.org/pipermail/linux-arm-kernel/2019-July/664633.html > --- > arch/arm64/kvm/guest.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c > index c2afa79..dfd6264 100644 > --- a/arch/arm64/kvm/guest.c > +++ b/arch/arm64/kvm/guest.c > @@ -208,7 +208,7 @@ static int set_core_reg(struct kvm_vcpu *vcpu, const > struct kvm_one_reg *reg) > > #define vq_word(vq) (((vq) - SVE_VQ_MIN) / 64) > #define vq_mask(vq) ((u64)1 << ((vq) - SVE_VQ_MIN) % 64) > -#define vq_present(vqs, vq) ((vqs)[vq_word(vq)] & vq_mask(vq)) > +#define vq_present(vqs, vq) (!!((vqs)[vq_word(vq)] & vq_mask(vq))) > > static int get_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg) > { > -- > 2.1.4 > -- Jazz is not dead, it just smells funny. ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH] KVM: arm64/sve: Fix vq_present() macro to yield a bool
On 03-07-19, 18:42, Dave Martin wrote: > From: Zhang Lei > > The original implementation of vq_present() relied on aggressive > inlining in order for the compiler to know that the code is > correct, due to some const-casting issues. This was causing sparse > and clang to complain, while GCC compiled cleanly. > > Commit 0c529ff789bc addressed this problem, but since vq_present() > is no longer a function, there is now no implicit casting of the > returned value to the return type (bool). > > In set_sve_vls(), this uncast bit value is compared against a bool, > and so may spuriously compare as unequal when both are nonzero. As > a result, KVM may reject valid SVE vector length configurations as > invalid, and vice versa. > > Fix it by forcing the returned value to a bool. > > Signed-off-by: Zhang Lei > Fixes: 0c529ff789bc ("KVM: arm64: Implement vq_present() as a macro") > Signed-off-by: Dave Martin [commit message rewrite] > Cc: Viresh Kumar > > --- > > Posting this under Zhang Lei's authorship, due to the need to turn this > fix around quickly. The fix is as per the original suggestion [1]. > > Originally observed with the QEMU KVM SVE support under review: > https://lists.gnu.org/archive/html/qemu-devel/2019-06/msg04945.html > > Bug reproduced and fix tested on the Arm Fast Model, with > http://linux-arm.org/git?p=kvmtool-dm.git;a=shortlog;h=refs/heads/sve/v3/head > (After rerunning util/update_headers.sh.) > > (the --sve-vls command line argument was removed in v4 of the > kvmtool patches). > > [1] > http://lists.infradead.org/pipermail/linux-arm-kernel/2019-July/664633.html > --- > arch/arm64/kvm/guest.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c > index c2afa79..dfd6264 100644 > --- a/arch/arm64/kvm/guest.c > +++ b/arch/arm64/kvm/guest.c > @@ -208,7 +208,7 @@ static int set_core_reg(struct kvm_vcpu *vcpu, const > struct kvm_one_reg *reg) > > #define vq_word(vq) (((vq) - SVE_VQ_MIN) / 64) > #define vq_mask(vq) ((u64)1 << ((vq) - SVE_VQ_MIN) % 64) > -#define vq_present(vqs, vq) ((vqs)[vq_word(vq)] & vq_mask(vq)) > +#define vq_present(vqs, vq) (!!((vqs)[vq_word(vq)] & vq_mask(vq))) > > static int get_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg) > { It was a really nice bug :) Reviewed-by: Viresh Kumar -- viresh ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
[PATCH] KVM: arm64/sve: Fix vq_present() macro to yield a bool
From: Zhang Lei The original implementation of vq_present() relied on aggressive inlining in order for the compiler to know that the code is correct, due to some const-casting issues. This was causing sparse and clang to complain, while GCC compiled cleanly. Commit 0c529ff789bc addressed this problem, but since vq_present() is no longer a function, there is now no implicit casting of the returned value to the return type (bool). In set_sve_vls(), this uncast bit value is compared against a bool, and so may spuriously compare as unequal when both are nonzero. As a result, KVM may reject valid SVE vector length configurations as invalid, and vice versa. Fix it by forcing the returned value to a bool. Signed-off-by: Zhang Lei Fixes: 0c529ff789bc ("KVM: arm64: Implement vq_present() as a macro") Signed-off-by: Dave Martin [commit message rewrite] Cc: Viresh Kumar --- Posting this under Zhang Lei's authorship, due to the need to turn this fix around quickly. The fix is as per the original suggestion [1]. Originally observed with the QEMU KVM SVE support under review: https://lists.gnu.org/archive/html/qemu-devel/2019-06/msg04945.html Bug reproduced and fix tested on the Arm Fast Model, with http://linux-arm.org/git?p=kvmtool-dm.git;a=shortlog;h=refs/heads/sve/v3/head (After rerunning util/update_headers.sh.) (the --sve-vls command line argument was removed in v4 of the kvmtool patches). [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2019-July/664633.html --- arch/arm64/kvm/guest.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c index c2afa79..dfd6264 100644 --- a/arch/arm64/kvm/guest.c +++ b/arch/arm64/kvm/guest.c @@ -208,7 +208,7 @@ static int set_core_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg) #define vq_word(vq) (((vq) - SVE_VQ_MIN) / 64) #define vq_mask(vq) ((u64)1 << ((vq) - SVE_VQ_MIN) % 64) -#define vq_present(vqs, vq) ((vqs)[vq_word(vq)] & vq_mask(vq)) +#define vq_present(vqs, vq) (!!((vqs)[vq_word(vq)] & vq_mask(vq))) static int get_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg) { -- 2.1.4 ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm