Messages in this thread | | | Date | Thu, 16 Dec 2021 02:37:06 +0300 | From | "Kirill A. Shutemov" <> | Subject | Re: [PATCH 08/26] x86/tdx: Handle in-kernel MMIO |
| |
On Wed, Dec 15, 2021 at 03:31:16PM -0800, Josh Poimboeuf wrote: > 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.
Okay, fair enough. I will make them consistent.
-- Kirill A. Shutemov
| |