Re: [PATCH 4/7] arm: smccc: handle SMCs according to SMCCC

[Julien Grall <julien.grall@arm.com>]

Hi Volodymyr,

CC "THE REST" maintainers to get an opinion on the public headers.

On 08/08/17 21:08, 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 `vsmc.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.
>
> But, before SMC is forwarded to standard SMCCC handler, it can be routed
> to a domain monitor, if one is installed.
>
> 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>
> ---
>  - Updated description to indicate that this patch affects only SMC call path.
>  - added "xen_" prefix to definitions in include/public/arch-arm/smc.h
>  - moved do_trap_smc() into vsmc.c from traps.c
>  - replaced all tabs with spaces

I would have really appreciated a summary of the discussion we had on 
the previous version regarding the bindings. This is a real blocker for 
this series and should not be ignored.

>
> ---
>  xen/arch/arm/Makefile             |   1 +
>  xen/arch/arm/traps.c              |  19 +-----
>  xen/arch/arm/vsmc.c               | 139 ++++++++++++++++++++++++++++++++++++++
>  xen/include/asm-arm/vsmc.h        |  94 ++++++++++++++++++++++++++
>  xen/include/public/arch-arm/smc.h |  68 +++++++++++++++++++
>  5 files changed, 303 insertions(+), 18 deletions(-)
>  create mode 100644 xen/arch/arm/vsmc.c
>  create mode 100644 xen/include/asm-arm/vsmc.h
>  create mode 100644 xen/include/public/arch-arm/smc.h
>
> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
> index 49e1fb2..4efd01c 100644
> --- a/xen/arch/arm/Makefile
> +++ b/xen/arch/arm/Makefile
> @@ -50,6 +50,7 @@ obj-$(CONFIG_HAS_GICV3) += vgic-v3.o
>  obj-$(CONFIG_HAS_ITS) += vgic-v3-its.o
>  obj-y += vm_event.o
>  obj-y += vtimer.o
> +obj-y += vsmc.o
>  obj-y += vpsci.o
>  obj-y += vuart.o
>
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index e14e7c0..b119dc0 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -43,7 +43,7 @@
>  #include <asm/mmio.h>
>  #include <asm/cpufeature.h>
>  #include <asm/flushtlb.h>
> -#include <asm/monitor.h>
> +#include <asm/vsmc.h>
>
>  #include "decode.h"
>  #include "vtimer.h"
> @@ -2769,23 +2769,6 @@ static void do_trap_data_abort_guest(struct cpu_user_regs *regs,
>      inject_dabt_exception(regs, info.gva, hsr.len);
>  }
>
> -static void do_trap_smc(struct cpu_user_regs *regs, const union hsr hsr)
> -{
> -    int rc = 0;
> -
> -    if ( !check_conditional_instr(regs, hsr) )
> -    {
> -        advance_pc(regs, hsr);
> -        return;
> -    }
> -
> -    if ( current->domain->arch.monitor.privileged_call_enabled )
> -        rc = monitor_smc();
> -
> -    if ( rc != 1 )
> -        inject_undef_exception(regs, hsr);
> -}
> -
>  static void enter_hypervisor_head(struct cpu_user_regs *regs)
>  {
>      if ( guest_mode(regs) )
> diff --git a/xen/arch/arm/vsmc.c b/xen/arch/arm/vsmc.c
> new file mode 100644
> index 0000000..5ef6167
> --- /dev/null
> +++ b/xen/arch/arm/vsmc.c
> @@ -0,0 +1,139 @@
> +/*
> + * xen/arch/arm/vsmc.c
> + *
> + * Generic handler for SMC and HVC calls according to
> + * ARM SMC calling convention
> + *
> + * 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/lib.h>
> +#include <xen/types.h>
> +#include <public/arch-arm/smc.h>
> +#include <asm/monitor.h>
> +#include <asm/vsmc.h>
> +#include <asm/regs.h>

<asm/regs.h> should be before <asm/vsmc.h> to keep the headers ordered 
alphabetically.

> +
> +/* Number of functions currently supported by Hypervisor Service. */
> +#define XEN_SMCCC_FUNCTION_COUNT 3
> +
> +/* SMCCC interface for hypervisor. Tell about itself. */
> +static bool handle_hypervisor(struct cpu_user_regs *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;
> +}
> +
> +/*
> + * vsmc_handle_call() - handle SMC/HVC call according to ARM SMCCC.
> + * returns true if that was valid SMCCC call (even if function number
> + * was unkown).
> + */
> +static bool vsmc_handle_call(struct cpu_user_regs *regs)

Something is wrong here. The comment says "Handle SMC/HVC" but the name 
of the function is "vsmc".

> +{
> +    bool handled = false;
> +    const union hsr hsr = { .bits = regs->hsr };
> +
> +    /*
> +     * Check immediate value for HVC32, HVC64 and SMC64.
> +     * It is not so easy to check immediate value for SMC32,
> +     * because it is not stored in HSR.ISS field. To get immediate
> +     * value we need to dissasemble instruction at current pc, which

s/dissasemble/disassemble/

> +     * is expensive. So we will assume that it is 0x0.
> +     */
> +    switch ( hsr.ec )
> +    {
> +    case HSR_EC_HVC32:
> +    case HSR_EC_HVC64:
> +    case HSR_EC_SMC64:
> +        if ( (hsr.iss & 0xFF) != 0)

The immediate is 16 bits. So it should be 0xFFFF. It would also be nice 
to have a comment explaining it and probably a define.

> +            return false;
> +        break;
> +    case HSR_EC_SMC32:
> +        break;
> +    default:
> +        return false;
> +    }
> +
> +    /* 64 bit calls are allowed only from 64 bit domains */

Missing full stop.

> +    if ( ARM_SMCCC_IS_64(get_user_reg(regs, 0)) &&
> +         is_32bit_domain(current->domain) )
> +    {
> +        set_user_reg(regs, 0, ARM_SMCCC_ERR_UNKNOWN_FUNCTION);
> +        return true;
> +    }
> +
> +    switch ( ARM_SMCCC_OWNER_NUM(get_user_reg(regs, 0)) )
> +    {
> +    case ARM_SMCCC_OWNER_HYPERVISOR:
> +        handled = handle_hypervisor(regs);
> +        break;
> +    }
> +
> +    if ( !handled )
> +    {
> +        gprintk(XENLOG_INFO, "Unhandled SMC/HVC: %08"PRIregister"\n",
> +                get_user_reg(regs, 0));
> +        /* Inform caller that function is not supported */
> +        set_user_reg(regs, 0, ARM_SMCCC_ERR_UNKNOWN_FUNCTION);
> +    }
> +
> +    return true;
> +}
> +
> +/* This function will be called from traps.c */
> +void do_trap_smc(struct cpu_user_regs *regs, const union hsr hsr)
> +{
> +    int rc = 0;
> +
> +    if ( !check_conditional_instr(regs, hsr) )
> +    {
> +        advance_pc(regs, hsr);
> +        return;
> +    }
> +
> +    /* If monitor is enabled, let it handle the call */
> +    if ( current->domain->arch.monitor.privileged_call_enabled )
> +        rc = monitor_smc();
> +
> +    if ( rc == 1 )
> +        return;
> +
> +    /* Use standard routines to handle the call */
> +    if ( vsmc_handle_call(regs) )
> +        advance_pc(regs, hsr);
> +    else
> +        inject_undef_exception(regs, hsr);
> +}
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/include/asm-arm/vsmc.h b/xen/include/asm-arm/vsmc.h
> new file mode 100644
> index 0000000..7baabef
> --- /dev/null
> +++ b/xen/include/asm-arm/vsmc.h
> @@ -0,0 +1,94 @@
> +/*
> + * 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_VSMC_H__
> +#define __ASM_ARM_VSMC_H__
> +
> +#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              0U
> +#define ARM_SMCCC_FAST_CALL             1U
> +#define ARM_SMCCC_TYPE_SHIFT            31
> +
> +#define ARM_SMCCC_SMC_32                0U
> +#define ARM_SMCCC_SMC_64                1U
> +#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
> +
> +/* Check if this is fast call */
> +#define ARM_SMCCC_IS_FAST_CALL(smc_val) \
> +    ((smc_val) & (ARM_SMCCC_FAST_CALL << ARM_SMCCC_TYPE_SHIFT))
> +
> +/* Check if this is 64 bit call  */
> +#define ARM_SMCCC_IS_64(smc_val)                                        \
> +    ((smc_val) & (ARM_SMCCC_SMC_64 << ARM_SMCCC_CALL_CONV_SHIFT))
> +
> +/* Get function number from function identifier */
> +#define ARM_SMCCC_FUNC_NUM(smc_val)     ((smc_val) & ARM_SMCCC_FUNC_MASK)
> +
> +/* Get service owner number from function identifier */
> +#define ARM_SMCCC_OWNER_NUM(smc_val)                                    \
> +    (((smc_val) >> ARM_SMCCC_OWNER_SHIFT) & ARM_SMCCC_OWNER_MASK)
> +
> +/*
> + * Construct function identifier from call type (fast or standard),
> + * calling convention (32 or 64 bit), service owner and function number
> + */
> +#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))
> +
> +/* List of known service owners */
> +#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
> +
> +/* List of generic function numbers */
> +#define ARM_SMCCC_FUNC_CALL_COUNT       0xFF00
> +#define ARM_SMCCC_FUNC_CALL_UID         0xFF01
> +#define ARM_SMCCC_FUNC_CALL_REVISION    0xFF03
> +
> +/* Only one error code defined in SMCCC */
> +#define ARM_SMCCC_ERR_UNKNOWN_FUNCTION  (-1)
> +
> +void do_trap_smc(struct cpu_user_regs *regs, const union hsr hsr);
> +
> +#endif  /* __ASM_ARM_VSMC_H__ */
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:b
> + */
> diff --git a/xen/include/public/arch-arm/smc.h b/xen/include/public/arch-arm/smc.h
> new file mode 100644
> index 0000000..42f3165
> --- /dev/null
> +++ b/xen/include/public/arch-arm/smc.h
> @@ -0,0 +1,68 @@
> +/*
> + * smc.h
> + *
> + * SMC/HVC interface in accordance with SMC Calling Convention.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to
> + * deal in the Software without restriction, including without limitation the
> + * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
> + * sell copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
> + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> + * DEALINGS IN THE SOFTWARE.
> + *
> + * Copyright 2017 (C) EPAM Systems
> + */
> +
> +#ifndef __XEN_PUBLIC_ARCH_ARM_SMC_H__
> +#define __XEN_PUBLIC_ARCH_ARM_SMC_H__
> +
> +typedef struct {
> +    uint32_t a[4];
> +} xen_arm_smccc_uid;
> +
> +#define XEN_ARM_SMCCC_UID(a, b, c, d0, d1, d2, d3, d4, d5, d6, d7)      \
> +    ((xen_arm_smccc_uid) {{(a), ((b) << 16 | (c) ),                     \
> +        ((d0) << 24 | (d1) << 16 | (d2) << 8 | (d3) << 0),              \
> +        ((d4) << 24 | (d5) << 16 | (d6) << 8 | (d7) << 0)}})
> +
> +/*
> + * Hypervisor Service version.
> + *
> + * We can't use XEN version here, because of SMCCC requirements:
> + * 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.
> + *
> + * Those values are subjected to change, when interface will be extended.
> + * They should not be stored in public/asm-arm/smc.h because they should
> + * be queried by guest using SMC/HVC interface.
> + */
> +#define XEN_SMCCC_MAJOR_REVISION 0
> +#define XEN_SMCCC_MINOR_REVISION 1
> +
> +/* Hypervisor Service UID. Randomly generated with 3rd party tool.  */
> +#define XEN_SMCCC_UID XEN_ARM_SMCCC_UID(0xa71812dc, 0xc698, 0x4369, \
> +                                        0x9a, 0xcf, 0x79, 0xd1, \
> +                                        0x8d, 0xde, 0xe6, 0x67)
> +
> +#endif  /* __XEN_PUBLIC_ARCH_ARM_SMC_H__ */
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:b
> + */
>

Cheers,

-- 
Julien Grall

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