Re: Re: [PATCH v3 4/9] KVM: PPC: Ultravisor: Add generic ultravisor call handler

[Ram Pai <linuxram@us.ibm.com>]

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 <linuxram@us.ibm.com>
> > 
> > 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