Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
From:   Thomas Gleixner <tglx@linutronix.de>
To:     "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>,
        mingo@redhat.com, bp@alien8.de, dave.hansen@intel.com,
        luto@kernel.org, peterz@infradead.org
Cc:     sathyanarayanan.kuppuswamy@linux.intel.com, aarcange@redhat.com,
        ak@linux.intel.com, dan.j.williams@intel.com, david@redhat.com,
        hpa@zytor.com, jgross@suse.com, jmattson@google.com,
        joro@8bytes.org, jpoimboe@redhat.com, knsathya@kernel.org,
        pbonzini@redhat.com, sdeep@vmware.com, seanjc@google.com,
        tony.luck@intel.com, vkuznets@redhat.com, wanpengli@tencent.com,
        thomas.lendacky@amd.com, brijesh.singh@amd.com, x86@kernel.org,
        linux-kernel@vger.kernel.org,
        "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        Borislav Petkov <bp@suse.de>
Subject: Re: [PATCHv6 04/30] x86/tdx: Extend the confidential computing API
 to support TDX guests
In-Reply-To: <20220316020856.24435-5-kirill.shutemov@linux.intel.com>
References: <20220316020856.24435-1-kirill.shutemov@linux.intel.com>
 <20220316020856.24435-5-kirill.shutemov@linux.intel.com>
Date:   Thu, 17 Mar 2022 01:01:54 +0100
Message-ID: <87fsnh30r1.ffs@tglx>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Wed, Mar 16 2022 at 05:08, Kirill A. Shutemov wrote:
> +
> +static void get_info(unsigned int *gpa_width)

get_info() is really not a descriptive function name. Also why does this
need a pointer and does not return gpa_width?

>=20=20
>  	setup_force_cpu_cap(X86_FEATURE_TDX_GUEST);
>=20=20
> +	get_info(&gpa_width);
> +
> +	cc_set_vendor(CC_VENDOR_INTEL);
> +
> +	/*
> +	 * The highest bit of a guest physical address is the "sharing" bit.
> +	 * Set it for shared pages and clear it for private pages.
> +	 */
> +	cc_set_mask(BIT_ULL(gpa_width - 1));

The code ordering here is silly:

    1) Retrieve gpa width
    2) Set CC vendor
    3) Use gpa width

static u64 get_cc_mask(void)
{
        gpa_width =3D magic();

        /*
         * Comment explaining the return value
         */
        return BIT_ULL(gpa_width - 1);
}

and

        cc_set_vendor(CC_VENDOR_INTEL);
        cc_set_mask(get_cc_mask());

would be too intuitive, right?

Thanks,

        tglx

---

=E2=80=9CMy tastes are simple: I am easily satisfied with the best.=E2=80=9D
                                     =E2=80=95 Winston S. Churchill=20