Re: [PATCHv2 08/29] x86/tdx: Handle in-kernel MMIO

[Kirill A. Shutemov]

On Mon, Jan 24, 2022 at 11:30:08AM -0800, Josh Poimboeuf wrote:
> On Mon, Jan 24, 2022 at 06:01:54PM +0300, Kirill A. Shutemov wrote:
> > +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);
> > +}
> > +
> > +static int tdx_handle_mmio(struct pt_regs *regs, struct ve_info *ve)
> > +{
> > +	char buffer[MAX_INSN_SIZE];
> > +	unsigned long *reg, val = 0;
> > +	struct insn insn = {};
> > +	enum mmio_type mmio;
> > +	int size;
> > +	bool err;
> > +
> > +	if (copy_from_kernel_nofault(buffer, (void *)regs->ip, MAX_INSN_SIZE))
> > +		return -EFAULT;
> > +
> > +	if (insn_decode(&insn, buffer, MAX_INSN_SIZE, INSN_MODE_64))
> > +		return -EFAULT;
> > +
> > +	mmio = insn_decode_mmio(&insn, &size);
> > +	if (WARN_ON_ONCE(mmio == MMIO_DECODE_FAILED))
> > +		return -EFAULT;
> > +
> > +	if (mmio != MMIO_WRITE_IMM && mmio != MMIO_MOVS) {
> > +		reg = insn_get_modrm_reg_ptr(&insn, regs);
> > +		if (!reg)
> > +			return -EFAULT;
> > +	}
> > +
> > +	switch (mmio) {
> > +	case MMIO_WRITE:
> > +		memcpy(&val, reg, size);
> > +		err = tdx_mmio_write(size, ve->gpa, &val);
> > +		break;
> 
> The return code conventions are still all mismatched and confusing:
> 
> - Most tdx_handle_*() handlers return bool (success == true)
> 
> - tdx_handle_mmio() returns int (success > 0)

Right, all tdx_handle_* are consistent: success > 0.

> - tdx_mmio*() helpers return bool (success == false)

And what is wrong with that? Why do you mix functions that called in
different contexts and expect them to have matching semantics?

> I still don't see any benefit in arbitrarily mixing three different
> return conventions, none of which matches the typical kernel style for
> returning errors, unless the goal is to confuse the reader and invite
> bugs.

Okay, we have an disagreement here.

I picked a way to communicate function result as I see best fits the
situation. It is a judgement call.

I will adjust code if maintainers see it differently from me. But until
then I don't see anything wrong here.

> There is precedent in traps.c for some handle_*() functions to return
> bool (success == true), so if the goal is to align with that
> semi-convention, that's ok.  But at the very least, please do it
> consistently:
> 
>   - change tdx_mmio*() to return true on success;
> 
>   - change tdx_handle_mmio() to return bool, with 'len' passed as an
>     argument.

Hard no.

Returning a value via passed argument is the last resort for cases when
more than one value has to be returned. In this case the function is
perfectly capable to communicate result via single return value.

I don't see a reason to complicate the code to satisfy some "typical
kernel style".

> Or, even better, just change them all to return 0 on success like 99+%
> of error-returning kernel functions.

Citation needed. 99+% looks like an overstatement to me.

-- 
 Kirill A. Shutemov