Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Date:   Fri, 22 Apr 2022 17:59:06 +0000
From:   Oliver Upton <oupton@google.com>
To:     Gavin Shan <gshan@redhat.com>
Cc:     kvmarm@lists.cs.columbia.edu, linux-kernel@vger.kernel.org,
        eauger@redhat.com, Jonathan.Cameron@huawei.com,
        vkuznets@redhat.com, will@kernel.org, shannon.zhaosl@gmail.com,
        james.morse@arm.com, mark.rutland@arm.com, maz@kernel.org,
        pbonzini@redhat.com, shan.gavin@gmail.com
Subject: Re: [PATCH v6 02/18] KVM: arm64: Route hypercalls based on their
 owner
Message-ID: <YmLs6t8iUn+BH6mo@google.com>
References: <20220403153911.12332-1-gshan@redhat.com>
 <20220403153911.12332-3-gshan@redhat.com>
 <YmETmWvPPQvHpQwP@google.com>
 <2519e2fa-4d6a-a5f8-1057-6b1820853036@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <2519e2fa-4d6a-a5f8-1057-6b1820853036@redhat.com>
Precedence: bulk

On Fri, Apr 22, 2022 at 08:20:50PM +0800, Gavin Shan wrote:
> Hi Oliver,
> 
> On 4/21/22 4:19 PM, Oliver Upton wrote:
> > On Sun, Apr 03, 2022 at 11:38:55PM +0800, Gavin Shan wrote:
> > > kvm_hvc_call_handler() directly handles the incoming hypercall, or
> > > and routes it based on its (function) ID. kvm_psci_call() becomes
> > > the gate keeper to handle the hypercall that can't be handled by
> > > any one else. It makes kvm_hvc_call_handler() a bit messy.
> > > 
> > > This reorgnizes the code to route the hypercall to the corresponding
> > > handler based on its owner.
> > 
> > nit: write changelogs in the imperative:
> > 
> > Reorganize the code to ...
> > 
> 
> Thanks again for your review. It will be corrected in next respin.
> By the way, could you help to review the rest when you have free
> cycles? :)

Yup, I've been thinking on the rest of the series just to make sure the
feedback I give is sane.

> > > The hypercall may be handled directly
> > > in the handler or routed further to the corresponding functionality.
> > > The (function) ID is always verified before it's routed to the
> > > corresponding functionality. By the way, @func_id is repalced by
> > > @func, to be consistent with by smccc_get_function().
> > > 
> > > PSCI is the only exception, those hypercalls defined by 0.2 or
> > > beyond are routed to the handler for Standard Secure Service, but
> > > those defined in 0.1 are routed to the handler for Standard
> > > Hypervisor Service.
> > > 
> > > Suggested-by: Oliver Upton <oupton@google.com>
> > > Signed-off-by: Gavin Shan <gshan@redhat.com>
> > > ---
> > >   arch/arm64/kvm/hypercalls.c | 199 +++++++++++++++++++++++-------------
> > >   1 file changed, 127 insertions(+), 72 deletions(-)
> > > 
> > > diff --git a/arch/arm64/kvm/hypercalls.c b/arch/arm64/kvm/hypercalls.c
> > > index 8438fd79e3f0..b659387d8919 100644
> > > --- a/arch/arm64/kvm/hypercalls.c
> > > +++ b/arch/arm64/kvm/hypercalls.c
> > 
> > [...]
> > 
> > > +static int kvm_hvc_standard(struct kvm_vcpu *vcpu, u32 func)
> > > +{
> > > +	u64 val = SMCCC_RET_NOT_SUPPORTED;
> > > +
> > > +	switch (func) {
> > > +	case ARM_SMCCC_TRNG_VERSION ... ARM_SMCCC_TRNG_RND32:
> > > +	case ARM_SMCCC_TRNG_RND64:
> > > +		return kvm_trng_call(vcpu);
> > > +	case PSCI_0_2_FN_PSCI_VERSION ... PSCI_0_2_FN_SYSTEM_RESET:
> > > +	case PSCI_0_2_FN64_CPU_SUSPEND ... PSCI_0_2_FN64_MIGRATE_INFO_UP_CPU:
> > > +	case PSCI_1_0_FN_PSCI_FEATURES ... PSCI_1_0_FN_SET_SUSPEND_MODE:
> > > +	case PSCI_1_0_FN64_SYSTEM_SUSPEND:
> > > +	case PSCI_1_1_FN_SYSTEM_RESET2:
> > > +	case PSCI_1_1_FN64_SYSTEM_RESET2:
> > 
> > Isn't it known from the SMCCC what range of hypercall numbers PSCI and
> > TRNG fall under, respectively?
> > 
> > https://developer.arm.com/documentation/den0028/e/
> > 
> > See sections 6.3 and 6.4.
> > 
> 
> Bit#30 of the function ID is the call convention indication, which is
> either 32 or 64-bits. For TRNG's function IDs, its 32-bits and 64-bits
> variants are discrete. Besides, the spec reserves more functions IDs
> than what range we're using. It means we don't have symbols to match
> the reserved ranges. So it looks good to me for TRNG cases.
> 
> For PSCI, it can be simplified as below, according to the defination
> in include/uapi/linux/psci.h:
> 
>     case PSCI_0_2_FN_PSCI_VERSION ...
>          PSCI_1_1_FN_SYSTEM_RESET2:     /* 32-bits */
>     case PSCI_0_2_FN64_CPU_SUSPEND ...
>          PSCI_1_1_FN64_SYSTEM_RESET2:   /* 64-bits */

Right, but this still requires that we go back and update this switch
statement every time we add a new PSCI call, which is exactly what I was
hoping we could avoid. Doing this based exactly on the spec reduces the
burden for future changes, and keeps all relevant context in a single
spot.

  #define SMCCC_STD_PSCI_RANGE_START	0x0000
  #define SMCCC_STD_PSCI_RANGE_END	0x001f
  #define SMCCC_STD_TRNG_RANGE_START	0x0050
  #define SMCCC_STD_TRNG_RANGE_END	0x005f

  switch (ARM_SMCCC_FUNC_NUM(function_id)) {
          case SMCCC_STD_PSCI_RANGE_START ... SMCCC_STD_PSCI_RANGE_END:
	          return kvm_psci_call(vcpu);
          case SMCCC_STD_TRNG_RANGE_START ... SMCCC_STD_TRNG_RANGE_END:
	  	  return kvm_trng_call(vcpu);

	 ...
  }

[...]

> > > +	case KVM_PSCI_FN_CPU_SUSPEND ... KVM_PSCI_FN_MIGRATE:
> > > +		return kvm_psci_call(vcpu);
> > 
> > You might want to handle these from the main call handler with a giant
> > disclaimer that these values predate SMCCC and therefore collide with
> > the standard hypervisor service range.
> > 
> > [...]
> > 
> 
> I probably just keep it as it is to follow the rule: to route
> based on the owner strictly. Besides, there are 3 levels to
> handle SMCCCs after this patch is applied, which corresponds
> to 3 handlers as main/owner/function. It sounds more natural
> for reader to follow the implementation in this way.

I think this makes it much more confusing for the reader, as you'd be
hard pressed to find these function IDs in the SMCCC spec. Since their
values are outside of the specification, it is confusing to only address
them after these switch statements have decided that they belong to a
particular service owner as they do not.

--
Thanks,
Oliver