| Date | Wed, 15 Dec 2021 15:31:16 -0800 | From | Josh Poimboeuf <> | Subject | Re: [PATCH 08/26] x86/tdx: Handle in-kernel MMIO |
| |
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
|