Date:   Wed, 15 Dec 2021 15:31:16 -0800
From:   Josh Poimboeuf <jpoimboe@redhat.com>
To:     "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
Cc:     tglx@linutronix.de, mingo@redhat.com, bp@alien8.de,
        dave.hansen@intel.com, luto@kernel.org, peterz@infradead.org,
        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, knsathya@kernel.org, pbonzini@redhat.com,
        sdeep@vmware.com, seanjc@google.com, tony.luck@intel.com,
        vkuznets@redhat.com, wanpengli@tencent.com, x86@kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH 08/26] x86/tdx: Handle in-kernel MMIO
Message-ID: <20211215233116.d2opfoei42viqdty@treble>
References: <20211214150304.62613-1-kirill.shutemov@linux.intel.com>
 <20211214150304.62613-9-kirill.shutemov@linux.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20211214150304.62613-9-kirill.shutemov@linux.intel.com>
Precedence: bulk

On Tue, Dec 14, 2021 at 06:02:46PM +0300, Kirill A. Shutemov wrote:
> @@ -155,6 +157,108 @@ static bool tdx_handle_cpuid(struct pt_regs *regs)
>  	return true;
>  }
>  
> +static bool tdx_mmio(int size, bool write, unsigned long addr,
> +		     unsigned long *val)
> +{
> +	struct tdx_hypercall_output out;
> +	u64 err;
> +
> +	err = _tdx_hypercall(EXIT_REASON_EPT_VIOLATION, size, write,
> +			     addr, *val, &out);
> +	if (err)
> +		return true;
> +
> +	*val = out.r11;
> +	return false;
> +}
> +
> +static bool tdx_mmio_read(int size, unsigned long addr, unsigned long *val)
> +{
> +	return tdx_mmio(size, false, addr, val);
> +}
> +
> +static bool tdx_mmio_write(int size, unsigned long addr, unsigned long *val)
> +{
> +	return tdx_mmio(size, true, addr, val);
> +}

These bool functions return false on success.  Conversely, other
functions in this file return true on success.  That inconsistency is
really confusing for the callers and is bound to introduce bugs
eventually.

> +static int tdx_handle_mmio(struct pt_regs *regs, struct ve_info *ve)

Similarly, tdx_handle_mmio() returns (int) 0 for success, while other
tdx_handle_*() functions return (bool) true for success.  Also
confusing.

The most robust option would be for all the functions to follow the
typical kernel convention of returning (int) 0 on success.  It works for
99.99% of the kernel.  Why mess with success? (pun intended)

Otherwise it's just pointless added cognitive overhead, trying to keep
track of what success means, for each individual function.

-- 
Josh