On Fri, Dec 3, 2021 at 11:00 AM Tom Lendacky <thomas.lendacky@xxxxxxx> wrote: > > On 12/3/21 10:39 AM, Sean Christopherson wrote: > > On Thu, Dec 02, 2021, Tom Lendacky wrote: > > >> > >> - return -EINVAL; > >> + return false; > > > > I'd really prefer that this helper continue to return 0/-EINVAL, there's no hint > > in the function name that this return true/false. And given the usage, there's > > no advantage to returning true/false. On the contrary, if there's a future > > condition where this needs to exit to userspace, we'll end up switching this all > > back to int. > > I don't have any objection to that. I think Sean's review makes a pretty compelling case that we should keep the int return value for `setup_vmgexit_scratch()`. In particular, failing to allocate a host kernel buffer definitely seems like a host error that should return to userspace. Though, failing to read the guest GPA seems less clear cut on who is at fault (host vs. guest), as Tom mentioned. My understanding from the commit description is that the entire point of the patch is to protect the guest from mis-behaving guest userspace code. So I would think that if we have a case like mapping the guest GPA that could fail due to the guest or the host, we should probably go ahead and use the new GHCB error codes to return back to the guest in this case. But either way, having an int return code seems like the way to go for `setup_vmgexit_scratch()`. Because if we are wrong in either direction, it's a trivial fix. It's not as obvious to me that converting `sev_es_validate_vmgexit()` to return a bool is not cleaner than returning an int. This function seems to pretty much just process the GHCB buffer as a self-contained set of bits. So it's hard to imagine how this could fail in a way where exiting to userspace is the right thing to do. That being said, I do not have a strong objection to returning an int. Sean is right that an int is definitely more future proof. And I'm sure Sean (and Tom) have much better insight into how validating the bits written into the GHCB could potentially require code that could justify exiting out to userspace.
