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

[Volodymyr Babchuk <volodymyr_babchuk@xxxxxxxx>]

Hello Julien,

Thank you for review. It is my first time, when I'm submitting patch to 
XEN, so I have some questions.
On 15.06.17 13:48, Julien Grall wrote:
> 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@xxxxxxxx>
> > Reviewed-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
> > Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> > ---
> >  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.
Should we return ARM_SMCCC_ERR_UNKNOWN_FUNCTION code in this case? Or 
inject undefined instruction?
> Furthermore, based from the SMCCC spec (see 2.9 in ARM DEN 0028B), the 
> compliant SMC calls should have the immediate value of zero.
Looks like HSR does not hold immediate value (as if in case of HVC/SVC). 
That means that I need to map memory at PC and decode instruction 
manually. It is a bit complex, I think. Should we do this?
> > +    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.
>
> Also, I was expecting some change in the HVC path as you say that this 
> will be used for both HVC and SMC.
> >      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.Actually, part of this defines are from Linux, another defines was added 
by myself. What I should to in this case?

> > +#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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel