Re: [Xen-devel] [PATCH v5 06/10] arm: smccc: handle SMCs according to SMCCC

[Julien Grall <julien.grall@xxxxxxx>]

Hi,

On 08/31/2017 09:09 PM, 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 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, UUID and number of functions provided by service
> provider.
>
> This patch adds new file `vsmc.c`, which handles both generic SMCs
> and HVC according to SMCCC. At this moment it implements only one
> service: Standard Hypervisor Service.
>
> At this time 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@xxxxxxxx>
> Reviewed-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
> Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> ---
>
>   * reworked fill_uuid() function
>   * dropped vsmc.h header. Function prototypes moved to traps.h
>   * public/arch-arm/smc.h header renamed to smccc.h
>   * introduced `register_t funcid` in vsmccc_handle_call()x
>   * spelling fixes
>   * coding style fixes
>
> ---
> xen/arch/arm/Makefile               |   1 +
>   xen/arch/arm/traps.c                |  17 ----
>   xen/arch/arm/vsmc.c                 | 168 ++++++++++++++++++++++++++++++++++++
>   xen/include/asm-arm/traps.h         |   3 +
>   xen/include/public/arch-arm/smccc.h |  58 +++++++++++++
>   5 files changed, 230 insertions(+), 17 deletions(-)
>   create mode 100644 xen/arch/arm/vsmc.c
>   create mode 100644 xen/include/public/arch-arm/smccc.h
>
> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
> index de00c5e..3d7dde9 100644
> --- a/xen/arch/arm/Makefile
> +++ b/xen/arch/arm/Makefile
> @@ -51,6 +51,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 9132fe1..f3b64b4 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -2155,23 +2155,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..97a6be3
> --- /dev/null
> +++ b/xen/arch/arm/vsmc.c
> @@ -0,0 +1,168 @@
> +/*
> + * 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/smccc.h>
> +#include <asm/monitor.h>
> +#include <asm/regs.h>
> +#include <asm/smccc.h>
> +#include <asm/traps.h>
> +
> +/* Number of functions currently supported by Hypervisor Service. */
> +#define XEN_SMCCC_FUNCTION_COUNT 3
> +
> +static void fill_uuid(struct cpu_user_regs *regs, const xen_uuid_t *u)
Actually why do you pass a pointer for u? This requires every caller to 
introduce temporary variable because the UUID is usually a define.
With your current solution each caller as to do:

xen_uuid_t foo = MY_UUID;

fill_uuid(regs, &foo);

return true;

What I suggested in the previous version is to get fill_uuid return 
true. So you make each caller simpler.
Cheers,

--
Julien Grall

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