From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id B8655C32771 for ; Wed, 21 Sep 2022 23:46:43 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229877AbiIUXql (ORCPT ); Wed, 21 Sep 2022 19:46:41 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:58494 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229649AbiIUXqj (ORCPT ); Wed, 21 Sep 2022 19:46:39 -0400 Received: from mail-pf1-x436.google.com (mail-pf1-x436.google.com [IPv6:2607:f8b0:4864:20::436]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id D3D12A0604 for ; Wed, 21 Sep 2022 16:46:38 -0700 (PDT) Received: by mail-pf1-x436.google.com with SMTP id 9so7527507pfz.12 for ; Wed, 21 Sep 2022 16:46:38 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date; bh=oLmfKWuUTNL21IQ6HESQaaXbmI+ko3s/zbVnpQNTyfQ=; b=ZhPjG3hJi+MYcKRihGUdAC68xvTWiMoW7SPD8OTcOZ26+dN5JNCX/1w6nFfyBCMstE 5HYD4Fse3n9roNiceTXE44Upoj73OH2Cq728vb6uC7wyB1ELBnRp3QOIas5WBcfDDtZW yGRjURMv19EhBh2rt5Wo+TBFMeOe50xrac1nFYO5ruNLvMCpxjlZjTnlUAwipjgnm1Qr guH3EBQ4F/A3bEWfskdvaLhydQ9rr+SiKQoTVq1/gvGGHky725DBu8BYhOgkv7ojic15 LdbhRd81llw9FC383cjWQzz9qSVioH+77jgRKVTCjN52Cx0QDwCaR7JId0drMgzjd18G VQVg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date; bh=oLmfKWuUTNL21IQ6HESQaaXbmI+ko3s/zbVnpQNTyfQ=; b=b19VhyXirbdUuAb0m5E+08cEKmeiSaWIXU2qKxEYGkxUJRhNlTXbdC8muT8CN04e8D Gd6v9PCc2kAwbP5+nIV2OUh8XA0BzzO+1+3NCM9kQv9u/2NuS7eNwJFwGDrv3pV/X13I auFz8LcMXmuHfWrQ0SMJ8Uzl1FtqMOlD+jO8gTy418RnGXSxwXNKWFDhHgCNjmmFU1Fo VYnCcPddGVAERF0jeKf9+o8QDJy7oRYixF0+VBUyF80YYeVn85sAl80HaI7N2GGKmjMk fdzwms+ttEW+SbbVdLTAbuW1xsKTYTy0jzUj+35MthlBeYKpczlF/L6XleAnx3nNXeUj fXBQ== X-Gm-Message-State: ACrzQf0VLH7b0zrVWAk2ee44tsawO+/GT6m9/a93qxlGrEyn2uBFH4v5 0jjcUnD7xoUPQSEIohMppDToMQ== X-Google-Smtp-Source: AMsMyM6XbKjJMUzlns8HLCwuDBcHPQZk6TBhuf7BAseDGdPOhjE8XWL38BgMGblaP5ZfSFJoRXpHQw== X-Received: by 2002:a05:6a00:1249:b0:543:aa0a:9c0a with SMTP id u9-20020a056a00124900b00543aa0a9c0amr580002pfi.2.1663803998248; Wed, 21 Sep 2022 16:46:38 -0700 (PDT) Received: from google.com (7.104.168.34.bc.googleusercontent.com. [34.168.104.7]) by smtp.gmail.com with ESMTPSA id z4-20020a1709027e8400b00177e5d83d3dsm2579190pla.170.2022.09.21.16.46.37 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 21 Sep 2022 16:46:37 -0700 (PDT) Date: Wed, 21 Sep 2022 23:46:34 +0000 From: Sean Christopherson To: Jim Mattson Cc: Vipin Sharma , pbonzini@redhat.com, vkuznets@redhat.com, kvm@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] KVM: selftests: Fix hyperv_features test failure when built on Clang Message-ID: References: <20220921231151.2321058-1-vipinsh@google.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Sep 21, 2022, Jim Mattson wrote: > On Wed, Sep 21, 2022 at 4:11 PM Vipin Sharma wrote: > > > > hyperv_features test fails when built on Clang. It throws error: > > > > Failed guest assert: !hcall->ud_expected || res == hcall->expect at > > x86_64/hyperv_features.c:90 > > > > On GCC, EAX is set to 0 before the hypercall whereas in Clang it is not, > > this causes EAX to have garbage value when hypercall is returned in Clang > > binary. > > > > Fix by executing the guest assertion only when ud_expected is false. TL;DR: please rewrite the changelog to explain the actual bug (checking the result when the hypercall is expected to fault) and the fix, and only mention the gcc vs. clang behavior as a footnote. As Jim pointed out, the bug has nothing to do with clang. Ha, figured out why gcc passes; it uses RAX as the scratch reg that the asm blob loads into R8, i.e. loads RAX with @output_address. So ignore my earlier suggestion of: *hv_status = -EFAULT, even better is to do: diff --git a/tools/testing/selftests/kvm/x86_64/hyperv_features.c b/tools/testing/selftests/kvm/x86_64/hyperv_features.c index 79ab0152d281..673085f6edfd 100644 --- a/tools/testing/selftests/kvm/x86_64/hyperv_features.c +++ b/tools/testing/selftests/kvm/x86_64/hyperv_features.c @@ -26,7 +26,8 @@ static inline uint8_t hypercall(u64 control, vm_vaddr_t input_address, : "=a" (*hv_status), "+c" (control), "+d" (input_address), KVM_ASM_SAFE_OUTPUTS(vector) - : [output_address] "r"(output_address) + : [output_address] "r"(output_address), + "a"(-EFAULT) : "cc", "memory", "r8", KVM_ASM_SAFE_CLOBBERS); return vector; } so that there is zero chance of getting a false positive due to the compiler (but not KVM) modifying RAX. Anyways, this bug is not clang's fault, with above patch it fails as "expected". ==== Test Assertion Failure ==== x86_64/hyperv_features.c:622: false pid=283847 tid=283847 errno=4 - Interrupted system call 1 0x0000000000402842: guest_test_hcalls_access at hyperv_features.c:622 2 (inlined by) main at hyperv_features.c:642 3 0x00007f23fc513082: ?? ??:0 4 0x0000000000402ebd: _start at ??:? Failed guest assert: !hcall->ud_expected || res == hcall->expect at x86_64/hyperv_features.c:90 arg1 = 0, arg2 = fffffff2 > > Fixes: cc5851c6be86 ("KVM: selftests: Use exception fixup for #UD/#GP Hyper-V MSR/hcall tests") > > Signed-off-by: Vipin Sharma > > Suggested-by: Sean Christopherson > > > > --- > > tools/testing/selftests/kvm/x86_64/hyperv_features.c | 8 ++++---- > > 1 file changed, 4 insertions(+), 4 deletions(-) > > In case Sean doesn't point it out, be wary of starting a shortlog with > "Fix." You may later regret it. Heh, I think our record is "Really, really fix xyz" for a shortlog. > Also, I think the "clang" part is a red herring. You are fixing a > latent bug in the code. Ya, it's definitely a good idea to call out why a bug was missed, but clang is not to blame here, at all.