From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ram Pai Date: Mon, 17 Jun 2019 23:51:46 +0000 Subject: Re: Re: [PATCH v3 4/9] KVM: PPC: Ultravisor: Add generic ultravisor call handler Message-Id: <20190617235146.GC10806@ram.ibm.com> List-Id: References: <20190606173614.32090-1-cclaudio@linux.ibm.com> <20190606173614.32090-5-cclaudio@linux.ibm.com> <20190617020632.yywfoqwfinjxs3pb@oak.ozlabs.ibm.com> In-Reply-To: <20190617020632.yywfoqwfinjxs3pb@oak.ozlabs.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Paul Mackerras Cc: Madhavan Srinivasan , Michael Anderson , Claudio Carvalho , kvm-ppc@vger.kernel.org, Bharata B Rao , linuxppc-dev@ozlabs.org, Sukadev Bhattiprolu , Thiago Bauermann , Anshuman Khandual On Mon, Jun 17, 2019 at 12:06:32PM +1000, Paul Mackerras wrote: > On Thu, Jun 06, 2019 at 02:36:09PM -0300, Claudio Carvalho wrote: > > From: Ram Pai > > > > Add the ucall() function, which can be used to make ultravisor calls > > with varied number of in and out arguments. Ultravisor calls can be made > > from the host or guests. > > > > This copies the implementation of plpar_hcall(). > > One point which I missed when I looked at this patch previously is > that the ABI that we're defining here is different from the hcall ABI > in that we are putting the ucall number in r0, whereas hcalls have the > hcall number in r3. That makes ucalls more like syscalls, which have > the syscall number in r0. So that last sentence quoted above is > somewhat misleading. > > The thing we need to consider is that when SMFCTRL[E] = 0, a ucall > instruction becomes a hcall (that is, sc 2 is executed as if it was > sc 1). In that case, the first argument to the ucall will be > interpreted as the hcall number. Mostly that will happen not to be a > valid hcall number, but sometimes it might unavoidably be a valid but > unintended hcall number. > > I think that will make it difficult to get ucalls to fail gracefully > in the case where SMF/PEF is disabled. It seems like the assignment > of ucall numbers was made so that they wouldn't overlap with valid > hcall numbers; presumably that was so that we could tell when an hcall > was actually intended to be a ucall. However, using a different GPR > to pass the ucall number defeats that. Right this is a valid point. Glad that you caught it, otherwise it would have become a difficult to fix it in the future. > > I realize that there is ultravisor code in development that takes the > ucall number in r0, and also that having the ucall number in r3 would > possibly make life more difficult for the place where we call > UV_RETURN in assembler code. Its called from one place in the hypervisor, and the changes look simple. - LOAD_REG_IMMEDIATE(r0, UV_RETURN) + LOAD_REG_IMMEDIATE(r3, UV_RETURN) ld r7, VCPU_GPR(R7)(r4) ld r6, VCPU_GPR(R6)(r4) ld r4, VCPU_GPR(R4)(r4) What am i missing? > Nevertheless, perhaps we should consider > changing the ABI to be like the hcall ABI before everything gets set > in concrete. yes. Thanks Paul! RP