Hi guys, I have started up KVM guest os successfully with SVE feature with Dave' patch.
Tested-by: Zhang Lei <[email protected] > > -----Original Message----- > From: Dave Martin <[email protected]> > Sent: Thursday, July 04, 2019 5:35 PM > To: Viresh Kumar <[email protected]> > Cc: Okamoto, Takayuki/岡本 高幸 <[email protected]>; Christoffer > Dall <[email protected]>; Ard Biesheuvel <[email protected]>; Marc > Zyngier <[email protected]>; Catalin Marinas > <[email protected]>; Will Deacon <[email protected]>; Zhang, > Lei/張 雷 <[email protected]>; Julien Grall <[email protected]>; > [email protected]; [email protected] > 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 <[email protected]> > > > > > > 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 <[email protected]> > > > Fixes: 0c529ff789bc ("KVM: arm64: Implement vq_present() as a > > > macro") > > > Signed-off-by: Dave Martin <[email protected]> [commit message > > > rewrite] > > > Cc: Viresh Kumar <[email protected]> > > > > > > --- > > > > > > 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 <[email protected]> > > 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 [email protected] https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
