On Mon, Jan 06, 2020 at 02:12:46PM +0000, Alexandru Elisei wrote: > Hi, > > On 1/6/20 1:17 PM, Andrew Jones wrote: > > On Mon, Jan 06, 2020 at 11:41:49AM +0000, Mark Rutland wrote: > >> On Mon, Jan 06, 2020 at 10:41:55AM +0000, Alexandru Elisei wrote: > >>> On 1/2/20 6:11 PM, Andre Przywara wrote: > >>>> On Tue, 31 Dec 2019 16:09:37 +0000 > >>>> Alexandru Elisei <alexandru.elisei@xxxxxxx> wrote: > >>>>> +.global asm_cpu_psci_cpu_die > >>>>> +asm_cpu_psci_cpu_die: > >>>>> + ldr r0, =PSCI_0_2_FN_CPU_OFF > >>>>> + hvc #0 > >>>>> + b . > >>>> I am wondering if this implementation is actually too simple. Both > >>>> the current implementation and the kernel clear at least the first > >>>> three arguments to 0. > >>>> I failed to find a requirement for doing this (nothing in the SMCCC > >>>> or the PSCI spec), but I guess it would make sense when looking at > >>>> forward compatibility. > >>> The SMC calling convention only specifies the values for the arguments that are > >>> used by a function, not the values for all possible arguments. kvm-unit-tests sets > >>> the other arguments to 0 because the function prototype that does the actual SMC > >>> call takes 4 arguments. The value 0 is a random value that was chosen for those > >>> unused parameters. For example, it could have been a random number on each call. > >> That's correct. > >> > >> A caller can leave arbitrary values in non-argument registers, in the > >> same manner as a caller of an AAPCS function. The callee should not > >> consume those values as they are not arguments. > >> > >>> Let me put it another way. Suggesting that unused arguments should be set to 0 is > >>> the same as suggesting that normal C function that adheres to procedure call > >>> standard for arm64 should always have 8 arguments, and for a particular function > >>> that doesn't use all of them, they should be set to 0 by the caller. > >> Heh, same rationale. :) > > This is a good rationale for the function to not zero parameters. > > > >>> @Mark Rutland has worked on the SMC implementation for the Linux kernel, if he > >>> wants to chime in on this. > >>> > >>> Thanks, > >>> Alex > >>>> At the very least it's a change in behaviour (ignoring the missing printf). > >>>> So shall we just clear r1, r2 and r3 here? (Same for arm64 below) > >> There's no need to zero non-argument registers, and it could potentially > >> mask bugs in callees, so I don't think it's a good idea to do so. > >> > >> If you really want to test that the callee is robust and correct, it > >> would be better to randomize the content of non-argument regsiters to > >> fuzz the callee. > >> > > But this indicates there is risk that we'll be less robust if we don't > > zero the parameters. Since this function is a common utility function and > > kvm-unit-tests targets KVM, QEMU/tcg, and anything else that somebody > > wants to try and target, then if there's any chance that zeroing unused > > parameters is more robust, I believe we should do that here. If we want to > > We agree that zero'ing unused parameters is not required by the specification, > right? Definitely > After that, I think it depends how you see kvm-unit-tests. I am of the > opinion that as a testing tool, if not zero'ing parameters (I'm not talking here > about fuzzing) causes an error in whatever piece of software you are running, then > that piece of software is not specification compliant and kvm-unit-tests has done > its job. We generally do our best to make sure the supporting code in the framework is robust and fails cleanly (usually with asserts). If there's reason to believe that the SUT may not implement a specification correctly, but we have test results proving it at least works under certain constrains, then we should probably keep the support code within those constraints. We can also write independent test cases to check that the SUT implements the specification completely. The reason for this approach is because not every user of kvm-unit-tests is willing to debug a random, potentially difficult to isolate failure when they're attempting to use the framework to test something unrelated. > > Either way, I'm not advocating changing our PSCI code. I'm fine with dropping this > patch for now (I mentioned this in another thread), so we can resume this > conversation when I rework it. Sounds good. And, like I said, I can't speak to the risk of not zeroing. Perhaps there's not enough concern here to bother with it. Maybe we could write those PSCI fuzzing tests sooner than later, confirm that on a reasonable sample of targets that there's no risk in not zeroing, and then implement all supporting functions as we wish. Thanks, drew > > Thanks, > Alex > > test/fuzz the PSCI/SMC emulation with kvm-unit-tests, then we can write > > explicit test cases to do that. > > > > I can't speak to the risk of not zeroing, but due to the way we've been > > calling PSCI functions with C, I can say up until now we always have. > > > > Thanks, > > drew > > >
