Re: [PATCH 1/2] arm: smccc: handle SMCs/HVCs according to SMCCC

[Volodymyr Babchuk <volodymyr_babchuk@epam.com>]

Hi Julien,

On 15.06.17 13:48, Julien Grall wrote:
> Hi Volodymyr,
> 
> On 14/06/17 15:10, Volodymyr Babchuk wrote:
>> SMCCC (SMC Call Convention) describes how to handle both HVCs and SMCs.
>> SMCCC states that both HVC and SMC are valid conduits to call to a 
>> different
>> firmware functions. Thus, for example PSCI calls can be made both by
>> SMC or HVC. Also SMCCC defines function number coding for such calls.
>> Besides functional calls there are query calls, which allows underling
>> OS determine version, UID and number of functions provided by service
>> provider.
>>
>> This patch adds new file `smccc.c`, which handles both generic SMCs
>> and HVC according to SMC. At this moment it implements only one
>> service: Standard Hypervisor Service.
>>
>> Standard Hypervisor Service only supports query calls, so caller can
>> ask about hypervisor UID and determine that it is XEN running.
>>
>> This change allows more generic handling for SMCs and HVCs and it can
>> be easily extended to support new services and functions.
>>
>> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
>> Reviewed-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>> Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>> ---
>>  xen/arch/arm/Makefile       |  1 +
>>  xen/arch/arm/smccc.c        | 96 
>> +++++++++++++++++++++++++++++++++++++++++++++
>>  xen/arch/arm/traps.c        | 10 ++++-
>>  xen/include/asm-arm/smccc.h | 89 
>> +++++++++++++++++++++++++++++++++++++++++
>>  4 files changed, 194 insertions(+), 2 deletions(-)
>>  create mode 100644 xen/arch/arm/smccc.c
>>  create mode 100644 xen/include/asm-arm/smccc.h
>>
>> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
>> index 49e1fb2..b8728cf 100644
>> --- a/xen/arch/arm/Makefile
>> +++ b/xen/arch/arm/Makefile
>> @@ -39,6 +39,7 @@ obj-y += psci.o
>>  obj-y += setup.o
>>  obj-y += shutdown.o
>>  obj-y += smc.o
>> +obj-y += smccc.o
>>  obj-y += smp.o
>>  obj-y += smpboot.o
>>  obj-y += sysctl.o
>> diff --git a/xen/arch/arm/smccc.c b/xen/arch/arm/smccc.c
>> new file mode 100644
>> index 0000000..5d10964
>> --- /dev/null
>> +++ b/xen/arch/arm/smccc.c
> 
> I would name this file vsmccc.c to show it is about virtual SMC. Also, I 
> would have expected pretty everyone to use the SMCC, so I would even 
> name the file vsmc.c
> 
>> @@ -0,0 +1,96 @@
>> +/*
>> + * xen/arch/arm/smccc.c
>> + *
>> + * Generic handler for SMC and HVC calls according to
>> + * ARM SMC callling convention
> 
> s/callling/calling/
> 
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
> 
> I know that some of the other headers are wrong about the GPL license. 
> But Xen is GPLv2 only. Please update the copyright accordingly. I.e:
> 
>   * This program is free software; you can redistribute it and/or modify
>   * it under the terms of the GNU General Public License version 2 as
>   * published by the Free Software Foundation.
> 
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +
>> +#include <xen/config.h>
>> +#include <xen/lib.h>
>> +#include <xen/perfc.h>
> 
> Why this is included here? You don't use it.
> 
>> +/* Need to include xen/sched.h before asm/domain.h or it breaks build*/
> 
> xen/sched.h will include asm/domain.h. So no need to include the latter 
> here.
> 
>> +#include <xen/sched.h>
>> +#include <xen/stdbool.h>
>> +#include <xen/types.h>
>> +#include <asm/domain.h>
>> +#include <asm/psci.h>
> 
> You don't use this header here.
> 
>> +#include <asm/smccc.h>
>> +#include <asm/regs.h>
>> +
>> +#define XEN_SMCCC_UID ARM_SMCCC_UID(0xa71812dc, 0xc698, 0x4369, \
>> +                                    0x9a, 0xcf, 0x79, 0xd1, \
>> +                                    0x8d, 0xde, 0xe6, 0x67)
> 
> Please mention that this value was generated. This would avoid to wonder 
> where this value comes from.
> 
>> +
>> +/*
>> + * We can't use XEN version here:
>> + * Major revision should change every time SMC/HVC function is removed.
>> + * Minor revision should change every time SMC/HVC function is added.
>> + * So, it is SMCCC protocol revision code, not XEN version
> 
> It would be nice to say this is a requirement of the spec. Also missing 
> full stop.
> 
>> + */
>> +#define XEN_SMCCC_MAJOR_REVISION 0
>> +#define XEN_SMCCC_MINOR_REVISION 1
> 
> I first thought the revision was 0.1.3 and was about to ask why. But 
> then noticed XEN_SMCC_FUNCTION_COUNT is not part of the revision.
> 
> So please add a newline for clarity.
> 
>> +#define XEN_SMCCC_FUNCTION_COUNT 3
>> +
>> +/* SMCCC interface for hypervisor. Tell about self */
> 
> Tell about itself. + missing full stop.
> 
>> +static bool handle_hypervisor(struct cpu_user_regs *regs, const union 
>> hsr hsr)
> 
> hsr is already part of regs.
> 
>> +{
>> +    switch ( ARM_SMCCC_FUNC_NUM(get_user_reg(regs, 0)) )
>> +    {
>> +    case ARM_SMCCC_FUNC_CALL_COUNT:
>> +        set_user_reg(regs, 0, XEN_SMCCC_FUNCTION_COUNT);
>> +        return true;
>> +    case ARM_SMCCC_FUNC_CALL_UID:
>> +        set_user_reg(regs, 0, XEN_SMCCC_UID.a[0]);
>> +        set_user_reg(regs, 1, XEN_SMCCC_UID.a[1]);
>> +        set_user_reg(regs, 2, XEN_SMCCC_UID.a[2]);
>> +        set_user_reg(regs, 3, XEN_SMCCC_UID.a[3]);
>> +        return true;
>> +    case ARM_SMCCC_FUNC_CALL_REVISION:
>> +        set_user_reg(regs, 0, XEN_SMCCC_MAJOR_REVISION);
>> +        set_user_reg(regs, 1, XEN_SMCCC_MINOR_REVISION);
>> +        return true;
>> +    }
>> +    return false;
>> +}
>> +
>> +/**
>> + * smccc_handle_call() - handle SMC/HVC call according to ARM SMCCC
>> + */
>> +void smccc_handle_call(struct cpu_user_regs *regs, const union hsr hsr)
> 
> hsr is already part of regs.
> 
>> +{
>> +    bool handled = false;
>> +
> 
> I am a bit surprised, I don't see any check to prevent a 32-bit guest to 
> use SMC64 call.
> 
> Furthermore, based from the SMCCC spec (see 2.9 in ARM DEN 0028B), the 
> compliant SMC calls should have the immediate value of zero.
> 
> 
>> +    switch ( ARM_SMCCC_OWNER_NUM(get_user_reg(regs, 0)) )
>> +    {
>> +    case ARM_SMCCC_OWNER_HYPERVISOR:
>> +        handled = handle_hypervisor(regs, hsr);
>> +        break;
>> +    }
>> +
>> +    if ( !handled )
>> +    {
>> +        printk("Uhandled SMC/HVC: %08"PRIregister"\n", 
>> get_user_reg(regs, 0));
> 
> s/Uhandled/Unhandled/
> 
> Also, please don't use printk. They are not ratelimited. You want to use 
> gprintk here.
> 
>> +        /* Inform caller that function is not supported */
>> +        set_user_reg(regs, 0, ARM_SMCCC_ERR_UNKNOWN_FUNCTION);
>> +    }
>> +}
>> +
>> +/*
>> + * Local variables:
>> + * mode: C
>> + * c-file-style: "BSD"
>> + * c-basic-offset: 4
>> + * indent-tabs-mode: nil
>> + * End:
>> + */
>> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
>> index 6cf9ee7..2d0b058 100644
>> --- a/xen/arch/arm/traps.c
>> +++ b/xen/arch/arm/traps.c
>> @@ -44,6 +44,7 @@
>>  #include <asm/cpufeature.h>
>>  #include <asm/flushtlb.h>
>>  #include <asm/monitor.h>
>> +#include <asm/smccc.h>
>>
>>  #include "decode.h"
>>  #include "vtimer.h"
>> @@ -2781,11 +2782,16 @@ static void do_trap_smc(struct cpu_user_regs 
>> *regs, const union hsr hsr)
>>  {
> 
> I think it would make sense to push this function in the new file.
Unfortunately, I can't do this, because it uses local functions such as
inject_undef_exception() or advance_pc().

> Also, I was expecting some change in the HVC path as you say that this 
> will be used for both HVC and SMC.
Actually, I plan to use this particular function to handle only SMCs,
because it does SMC-specific tasks, such as calling to a monitor.

HVCs will be handled in different call path in the next patch,
because currently HVC callpath is used by PSCI code.

>>      int rc = 0;
>>
>> +    /* Let monitor to handle the call */
>>      if ( current->domain->arch.monitor.privileged_call_enabled )
>>          rc = monitor_smc();
>>
>> -    if ( rc != 1 )
>> -        inject_undef_exception(regs, hsr);
>> +    if ( rc == 1 )
>> +        return;
> 
> It would be nice to explain both in the commit message and the code that 
> if monitor is enabled, then all SMCs will be forwarded to the monitor app.
> 
>> +
>> +    /* Use standard routines to handle the call */
>> +    smccc_handle_call(regs, hsr);
> 
> It is allowed by the architecture to trap to conditional SMC 
> instructions that fail their condition code check (see G1-4435 in ARM 
> DDI 0487B.a). So you want to check why it has trapped before calling the 
> handler.
> 
>> +    advance_pc(regs, hsr);
>>  }
>>
>>  static void enter_hypervisor_head(struct cpu_user_regs *regs)
>> diff --git a/xen/include/asm-arm/smccc.h b/xen/include/asm-arm/smccc.h
>> new file mode 100644
>> index 0000000..9342d5e
>> --- /dev/null
>> +++ b/xen/include/asm-arm/smccc.h
>> @@ -0,0 +1,89 @@
>> +/*
>> + * Copyright (c) 2017, EPAM Systems
>> + *
>> + * This software is licensed under the terms of the GNU General Public
>> + * License version 2, as published by the Free Software Foundation, and
>> + * may be copied, distributed, and modified under those terms.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + */
>> +#ifndef __ASM_ARM_SMCCC_H_
> 
> It should be __ASM_ARM_SMCC_H__
> 
>> +#define __ASM_ARM_SMCCC_H_
> 
> Ditto.
> 
>> +
>> +#include <xen/types.h>
>> +
>> +/*
>> + * This file provides common defines for ARM SMC Calling Convention as
>> + * specified in
>> + * http://infocenter.arm.com/help/topic/com.arm.doc.den0028a/index.html
>> + */
>> +
>> +#define ARM_SMCCC_STD_CALL        0
> 
> Is this file coming from Linux? If so, it should be mention. If not, 
> please use soft tab and not hard tab. This is valid in all this file.
> 
>> +#define ARM_SMCCC_FAST_CALL        1
>> +#define ARM_SMCCC_TYPE_SHIFT        31
>> +
>> +#define ARM_SMCCC_SMC_32        0
>> +#define ARM_SMCCC_SMC_64        1
>> +#define ARM_SMCCC_CALL_CONV_SHIFT    30
>> +
>> +#define ARM_SMCCC_OWNER_MASK        0x3F
>> +#define ARM_SMCCC_OWNER_SHIFT        24
>> +
>> +#define ARM_SMCCC_FUNC_MASK        0xFFFF
>> +
>> +#define ARM_SMCCC_IS_FAST_CALL(smc_val)    \
>> +    ((smc_val) & (ARM_SMCCC_FAST_CALL << ARM_SMCCC_TYPE_SHIFT))
>> +#define ARM_SMCCC_IS_64(smc_val) \
>> +    ((smc_val) & (ARM_SMCCC_SMC_64 << ARM_SMCCC_CALL_CONV_SHIFT))
>> +#define ARM_SMCCC_FUNC_NUM(smc_val)    ((smc_val) & ARM_SMCCC_FUNC_MASK)
>> +#define ARM_SMCCC_OWNER_NUM(smc_val) \
>> +    (((smc_val) >> ARM_SMCCC_OWNER_SHIFT) & ARM_SMCCC_OWNER_MASK)
>> +
>> +#define ARM_SMCCC_CALL_VAL(type, calling_convention, owner, func_num) \
>> +    (((type) << ARM_SMCCC_TYPE_SHIFT) | \
>> +    ((calling_convention) << ARM_SMCCC_CALL_CONV_SHIFT) | \
>> +    (((owner) & ARM_SMCCC_OWNER_MASK) << ARM_SMCCC_OWNER_SHIFT) | \
>> +    ((func_num) & ARM_SMCCC_FUNC_MASK))
> 
> I would appreciate a bit more documentation of those macros as they are 
> a bit difficult to parse. Also some newline would be nice for clarity.
> 
>> +
>> +#define ARM_SMCCC_OWNER_ARCH        0
>> +#define ARM_SMCCC_OWNER_CPU        1
>> +#define ARM_SMCCC_OWNER_SIP        2
>> +#define ARM_SMCCC_OWNER_OEM        3
>> +#define ARM_SMCCC_OWNER_STANDARD    4
>> +#define ARM_SMCCC_OWNER_HYPERVISOR    5
>> +#define ARM_SMCCC_OWNER_TRUSTED_APP    48
>> +#define ARM_SMCCC_OWNER_TRUSTED_APP_END    49
>> +#define ARM_SMCCC_OWNER_TRUSTED_OS    50
>> +#define ARM_SMCCC_OWNER_TRUSTED_OS_END    63
>> +
>> +#define ARM_SMCCC_FUNC_CALL_COUNT    0xFF00
>> +#define ARM_SMCCC_FUNC_CALL_UID        0xFF01
>> +#define ARM_SMCCC_FUNC_CALL_REVISION    0xFF03
>> +
>> +#define ARM_SMCCC_ERR_UNKNOWN_FUNCTION    (-1)
>> +
>> +typedef struct {
>> +    uint32_t a[4];
>> +} arm_smccc_uid;
>> +
>> +#define ARM_SMCCC_UID(a, b, c, d0, d1, d2, d3, d4, d5, d6, d7)        \
>> +    ((arm_smccc_uid) {{(a), ((b) << 16 | (c) ),            \
>> +               ((d0) << 24 | (d1) << 16 | (d2) << 8 | (d3) << 0), \
>> +               ((d4) << 24 | (d5) << 16 | (d6) << 8 | (d7) << 0)}})
>> +
>> +void smccc_handle_call(struct cpu_user_regs *regs, const union hsr hsr);
>> +
>> +#endif
> 
> #endif /* __ASM_ARM_SMCC_H__
> 
>> +
>> +/*
>> + * Local variables:
>> + * mode: C
>> + * c-file-style: "BSD"
>> + * c-basic-offset: 4
>> + * indent-tabs-mode: nil
>> + * End:
>> + */
>>
> 
> Cheers,
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel