| Message ID | 1502222922-25821-5-git-send-email-volodymyr_babchuk@epam.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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,
>>> On 09.08.17 at 12:10, <julien.grall@arm.com> wrote: > CC "THE REST" maintainers to get an opinion on the public headers. Please be more specific as to what you expect - the only public header affected here is ARM-specific. > On 08/08/17 21:08, Volodymyr Babchuk wrote: >> +#ifndef __XEN_PUBLIC_ARCH_ARM_SMC_H__ >> +#define __XEN_PUBLIC_ARCH_ARM_SMC_H__ >> + >> +typedef struct { >> + uint32_t a[4]; >> +} xen_arm_smccc_uid; This is not the normal way of encoding a UID type. >> +#define XEN_ARM_SMCCC_UID(a, b, c, d0, d1, d2, d3, d4, d5, d6, d7) \ >> + ((xen_arm_smccc_uid) {{(a), ((b) << 16 | (c) ), \ This is not C89 compatible. >> + ((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. I don't understand this explanation - how is the situation here different from some arbitrary, non-toolstack-only hypercall? >> + * 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 The comment says the values shouldn't be put here, but then they're still being put here? >> +/* 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) Why is the right side using _ARM_ in its name, but the macro being defined isn't? Jan
On 09.08.17 14:58, Jan Beulich wrote: >>>> On 09.08.17 at 12:10, <julien.grall@arm.com> wrote: >> CC "THE REST" maintainers to get an opinion on the public headers. > > Please be more specific as to what you expect - the only public > header affected here is ARM-specific. > >> On 08/08/17 21:08, Volodymyr Babchuk wrote: >>> +#ifndef __XEN_PUBLIC_ARCH_ARM_SMC_H__ >>> +#define __XEN_PUBLIC_ARCH_ARM_SMC_H__ >>> + >>> +typedef struct { >>> + uint32_t a[4]; >>> +} xen_arm_smccc_uid; > > This is not the normal way of encoding a UID type. Just to be clear: you are proposing to store UID in such struct struct uuid_t { unsigned32 time_low; unsigned16 time_mid; unsigned16 time_hi_and_version; unsigned8 clock_seq_hi_and_reserved; unsigned8 clock_seq_low; byte node[6]; }; right? >>> +#define XEN_ARM_SMCCC_UID(a, b, c, d0, d1, d2, d3, d4, d5, d6, d7) \ >>> + ((xen_arm_smccc_uid) {{(a), ((b) << 16 | (c) ), \ > > This is not C89 compatible. Oops, sorry. Didn't knew that XEN should be C89 compatible. Is there any guide for novices? I didn't found anything useful in docs/ (not even coding style document). On wiki I have found only "Submitting_Xen_Project_Patches" page, which is very helpful, but it does not cover topics like which C standard to use. >>> + ((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. > > I don't understand this explanation - how is the situation here > different from some arbitrary, non-toolstack-only hypercall? Because this is generic interface that should be supported by all hypervisors, including XEN. Think about this as a way for a guest to determine under which hypervisor it operates, and which functions it provides. >>> + * 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 > > The comment says the values shouldn't be put here, but then > they're still being put here? Yeah, sorry. Missed this. >>> +/* 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) > > Why is the right side using _ARM_ in its name, but the macro being > defined isn't? > > Jan >
>>> On 09.08.17 at 23:39, <volodymyr_babchuk@epam.com> wrote: > On 09.08.17 14:58, Jan Beulich wrote: >>>>> On 09.08.17 at 12:10, <julien.grall@arm.com> wrote: >>> On 08/08/17 21:08, Volodymyr Babchuk wrote: >>>> +#ifndef __XEN_PUBLIC_ARCH_ARM_SMC_H__ >>>> +#define __XEN_PUBLIC_ARCH_ARM_SMC_H__ >>>> + >>>> +typedef struct { >>>> + uint32_t a[4]; >>>> +} xen_arm_smccc_uid; >> >> This is not the normal way of encoding a UID type. > Just to be clear: you are proposing to store UID in such struct > struct uuid_t { > unsigned32 time_low; > unsigned16 time_mid; > unsigned16 time_hi_and_version; > unsigned8 clock_seq_hi_and_reserved; > unsigned8 clock_seq_low; > byte node[6]; > }; > right? Type-wise yes; the names of the fields look uncommon to me. >>>> +#define XEN_ARM_SMCCC_UID(a, b, c, d0, d1, d2, d3, d4, d5, d6, d7) \ >>>> + ((xen_arm_smccc_uid) {{(a), ((b) << 16 | (c) ), \ >> >> This is not C89 compatible. > Oops, sorry. Didn't knew that XEN should be C89 compatible. > Is there any guide for novices? I didn't found anything useful in docs/ > (not even coding style document). On wiki I have found only > "Submitting_Xen_Project_Patches" page, which is very helpful, but it > does not cover topics like which C standard to use. The public headers are required to the C89 compatible; Xen code in general is fine to use extensions. >>>> + ((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. >> >> I don't understand this explanation - how is the situation here >> different from some arbitrary, non-toolstack-only hypercall? > Because this is generic interface that should be supported by all > hypervisors, including XEN. Think about this as a way for a guest to > determine under which hypervisor it operates, and which functions it > provides. In which case - why the XEN_ prefixes? Jan
Hi, On 10/08/17 08:30, Jan Beulich wrote: >>>> On 09.08.17 at 23:39, <volodymyr_babchuk@epam.com> wrote: >> On 09.08.17 14:58, Jan Beulich wrote: >>>>>> On 09.08.17 at 12:10, <julien.grall@arm.com> wrote: >>>> On 08/08/17 21:08, Volodymyr Babchuk wrote: >>>>> +#ifndef __XEN_PUBLIC_ARCH_ARM_SMC_H__ >>>>> +#define __XEN_PUBLIC_ARCH_ARM_SMC_H__ >>>>> + >>>>> +typedef struct { >>>>> + uint32_t a[4]; >>>>> +} xen_arm_smccc_uid; >>> >>> This is not the normal way of encoding a UID type. >> Just to be clear: you are proposing to store UID in such struct >> struct uuid_t { >> unsigned32 time_low; >> unsigned16 time_mid; >> unsigned16 time_hi_and_version; >> unsigned8 clock_seq_hi_and_reserved; >> unsigned8 clock_seq_low; >> byte node[6]; >> }; >> right? > > Type-wise yes; the names of the fields look uncommon to me. > >>>>> +#define XEN_ARM_SMCCC_UID(a, b, c, d0, d1, d2, d3, d4, d5, d6, d7) \ >>>>> + ((xen_arm_smccc_uid) {{(a), ((b) << 16 | (c) ), \ >>> >>> This is not C89 compatible. >> Oops, sorry. Didn't knew that XEN should be C89 compatible. >> Is there any guide for novices? I didn't found anything useful in docs/ >> (not even coding style document). On wiki I have found only >> "Submitting_Xen_Project_Patches" page, which is very helpful, but it >> does not cover topics like which C standard to use. > > The public headers are required to the C89 compatible; Xen > code in general is fine to use extensions. > >>>>> + ((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. >>> >>> I don't understand this explanation - how is the situation here >>> different from some arbitrary, non-toolstack-only hypercall? >> Because this is generic interface that should be supported by all >> hypervisors, including XEN. Think about this as a way for a guest to >> determine under which hypervisor it operates, and which functions it >> provides. > > In which case - why the XEN_ prefixes? Because the version depends on the interface implemented which is Xen specific. You have to match the UID (Xen Specific) and the version to know what is actually supported. Cheers,
Hi Julien, On 09.08.17 13:10, Julien Grall wrote: > 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. > While I agree that question about bindings is important, I can't see how it affects this patch series. This patch series does not break anything. Because 1. This series add only new feature: generic hypervisor service with no immediate use. All ARM guests are already aware that they are running on XEN. All ARM guests know that they call *only* PSCI. 2. I see importance of this patch series for embedded platforms, where developer exactly knows what software of which version he/she will run. I doubt that server platforms will need something beyond PSCI, which I preserved as is. A I'm not denying importance of SMC bindings, but I think it is not blocker for my patches. We can add bindings later, when there will be consensus on how they should look. In meantime SMC handler can be used by anyone who knows that is available.
On 10/08/17 16:33, Volodymyr Babchuk wrote: > Hi Julien, > > On 09.08.17 13:10, Julien Grall wrote: >> 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. >> > > While I agree that question about bindings is important, I can't see how > it affects this patch series. This patch series does not break anything. > Because > > 1. This series add only new feature: generic hypervisor service with no > immediate use. All ARM guests are already aware that they are running on > XEN. All ARM guests know that they call *only* PSCI. > > 2. I see importance of this patch series for embedded platforms, where > developer exactly knows what software of which version he/she will run. > I doubt that server platforms will need something beyond PSCI, which I > preserved as is. I disagree here. SMCCC could be used to provide Dom0 a way to interact with the firmware if necessary. > > A I'm not denying importance of SMC bindings, but I think it is not > blocker for my patches. We can add bindings later, when there will be > consensus on how they should look. In meantime SMC handler can be used > by anyone who knows that is available. I am not in favor on getting something merged in Xen until we agree on the way for the guest to know it is there. It means you have to carry hack in your kernel in order to use SMC. Maybe this is fine for you, but I don't want to make this assumption on Xen upstream today. This is a change in the interface that should be notified to the guest. If we expose it without providing a bindings (or something), we have no way to revert/disable it. Imagine we want to disable SMC in the future. How a guest will know that - until Xen 4.10 SMC was not existing, - between Xen 4.10 and Xen 4.x you can use them - after Xen 4.y they can be disabled. All changes should be detected through the firmware tables (DT, ACPI) or another Xen method (i.e XENFEAT_*). For instance, the guest has to parse the firmware tables in order to know PSCI is available. Cheers,
On 10.08.17 19:11, Julien Grall wrote: > > > On 10/08/17 16:33, Volodymyr Babchuk wrote: >> Hi Julien, >> >> On 09.08.17 13:10, Julien Grall wrote: >>> 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. >>> >> >> While I agree that question about bindings is important, I can't see how >> it affects this patch series. This patch series does not break anything. >> Because >> >> 1. This series add only new feature: generic hypervisor service with no >> immediate use. All ARM guests are already aware that they are running on >> XEN. All ARM guests know that they call *only* PSCI. >> >> 2. I see importance of this patch series for embedded platforms, where >> developer exactly knows what software of which version he/she will run. >> I doubt that server platforms will need something beyond PSCI, which I >> preserved as is. > > I disagree here. SMCCC could be used to provide Dom0 a way to interact > with the firmware if necessary. AFAIK, Dom0 usually is built with particular version of XEN in mind (or at least minimal XEN version). >> >> A I'm not denying importance of SMC bindings, but I think it is not >> blocker for my patches. We can add bindings later, when there will be >> consensus on how they should look. In meantime SMC handler can be used >> by anyone who knows that is available. > > I am not in favor on getting something merged in Xen until we agree on > the way for the guest to know it is there. I think that SMC implementation will be the same, regardless the way we can tell guest that it is available. At this time guests can safely assume that SMCCC is not implemented in XEN. This wouldn't break anything. > It means you have to carry > hack in your kernel in order to use SMC. Maybe this is fine for you, but > I don't want to make this assumption on Xen upstream today. Modern linux kernel uses SMC for PSCI calls and OP-TEE calls. In both cases it reads DT to get conduit method (SMC/HVC). So there are already bindings for generic uses. Other uses are platform-specific (okay, probably there can be a problem). > This is a change in the interface that should be notified to the guest. > If we expose it without providing a bindings (or something), we have no > way to revert/disable it. Imagine we want to disable SMC in the future. Natural way was to disable Security Extensions in PFR1 register. This was not done and now we have curious situation: guest thinks that SMC is available, but then it gets Undefined Instruction exception when it tries to invoke SMC. > How a guest will know that > - until Xen 4.10 SMC was not existing, > - between Xen 4.10 and Xen 4.x you can use them > - after Xen 4.y they can be disabled. It is a broader question: how software can know that SMCCC is available on a platform? Not SMC, but SMCCC as a protocol. Probably, there should be some generic way to tell Linux/Windows/XEN/Windriver Hypervisor/etc that they can rely on SMCCC spec. I think that it is question to ARM guys (including you), because this affects all ARM machines. > All changes should be detected through the firmware tables (DT, ACPI) or > another Xen method (i.e XENFEAT_*). For instance, the guest has to parse > the firmware tables in order to know PSCI is available. Yep. The same does OP-TEE code. It parses DT to get conduit. Probably, this is wrong approach. Should all SMCCC calls use the same conduit?. Should platform provide conduit for each SMCCC service owner?
Hi, On 10/08/17 18:40, Volodymyr Babchuk wrote: > > > On 10.08.17 19:11, Julien Grall wrote: >> >> >> On 10/08/17 16:33, Volodymyr Babchuk wrote: >>> Hi Julien, >>> >>> On 09.08.17 13:10, Julien Grall wrote: >>>> 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. >>>> >>> >>> While I agree that question about bindings is important, I can't see how >>> it affects this patch series. This patch series does not break anything. >>> Because >>> >>> 1. This series add only new feature: generic hypervisor service with no >>> immediate use. All ARM guests are already aware that they are running on >>> XEN. All ARM guests know that they call *only* PSCI. >>> >>> 2. I see importance of this patch series for embedded platforms, where >>> developer exactly knows what software of which version he/she will run. >>> I doubt that server platforms will need something beyond PSCI, which I >>> preserved as is. >> >> I disagree here. SMCCC could be used to provide Dom0 a way to interact >> with the firmware if necessary. > AFAIK, Dom0 usually is built with particular version of XEN in mind (or > at least minimal XEN version). That's not true. Dom0 is a generic kernel able to probe everything from the firmware tables and Xen interface. I use the exact Linux kernel on multiple platforms with no issue. The Hypervisor and Dom0 (Linux, FreeBSD) are different projects with different release cadence. We provide a stable interface that is backward compatible (returning error code for non-existent hypercalls). So a Linux released in 10 years should still work on Xen 4.10 and adapt to the features available. > >>> >>> A I'm not denying importance of SMC bindings, but I think it is not >>> blocker for my patches. We can add bindings later, when there will be >>> consensus on how they should look. In meantime SMC handler can be used >>> by anyone who knows that is available. >> >> I am not in favor on getting something merged in Xen until we agree on >> the way for the guest to know it is there. > I think that SMC implementation will be the same, regardless the way we > can tell guest that it is available. At this time guests can safely > assume that SMCCC is not implemented in XEN. This wouldn't break anything. So you suggest to merge this series but says "The guest should not assume the presence of SMC"? This is rather a bit odd... > >> It means you have to carry hack in your kernel in order to use SMC. >> Maybe this is fine for you, but I don't want to make this assumption >> on Xen upstream today. > Modern linux kernel uses SMC for PSCI calls and OP-TEE calls. In both > cases it reads DT to get conduit method (SMC/HVC). So there are already > bindings for generic uses. Other uses are platform-specific (okay, > probably there can be a problem). > >> This is a change in the interface that should be notified to the >> guest. If we expose it without providing a bindings (or something), we >> have no way to revert/disable it. Imagine we want to disable SMC in >> the future. > Natural way was to disable Security Extensions in PFR1 register. This > was not done and now we have curious situation: guest thinks that SMC is > available, but then it gets Undefined Instruction exception when it > tries to invoke SMC. Security extensions does not mean that SMCCC is present... Even if we had returned an error, you don't know if it was from the SMCCC protocol or my foo protocol. It would be valid (though a bit odd) for a firmware to inject an undefined instruction if it is unable to understand the SMC. The main problem today is neither HVC #0 nor SMC #0 are SMCCC compliant. They are let's call it XEN Protocol compliant. KVM has the same problem. So you clearly need a way to say: "HVCs and SMCs are SMCCC compliant, you can go ahead probing the different components". As I said, I am not asking you to write the binding. I am asking you to have in mind other use cases. If the binding takes to much time, then a solution would be to add a XENFEAT_*. > >> How a guest will know that >> - until Xen 4.10 SMC was not existing, >> - between Xen 4.10 and Xen 4.x you can use them >> - after Xen 4.y they can be disabled. > It is a broader question: how software can know that SMCCC is available > on a platform? Not SMC, but SMCCC as a protocol. Probably, there should > be some generic way to tell Linux/Windows/XEN/Windriver Hypervisor/etc > that they can rely on SMCCC spec. I think that it is question to ARM > guys (including you), because this affects all ARM machines. I spent quite some time to explain that in an e-mail to the previous version... See the thread [1]. > >> All changes should be detected through the firmware tables (DT, ACPI) >> or another Xen method (i.e XENFEAT_*). For instance, the guest has to >> parse the firmware tables in order to know PSCI is available. > Yep. The same does OP-TEE code. It parses DT to get conduit. Probably, > this is wrong approach. Should all SMCCC calls use the same conduit?. > Should platform provide conduit for each SMCCC service owner? Technically a guest should only use HVC. I am happy to allow the SMC to support unmodified OS that don't use DT. Cheers, [1] https://lists.xenproject.org/archives/html/xen-devel/2017-08/msg00068.html
Hi, On 10.08.17 21:18, Julien Grall wrote: > Hi, > > On 10/08/17 18:40, Volodymyr Babchuk wrote: >> >> >> On 10.08.17 19:11, Julien Grall wrote: >>> >>> >>> On 10/08/17 16:33, Volodymyr Babchuk wrote: >>>> Hi Julien, >>>> >>>> On 09.08.17 13:10, Julien Grall wrote: >>>>> 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. >>>>> >>>> >>>> While I agree that question about bindings is important, I can't see >>>> how >>>> it affects this patch series. This patch series does not break >>>> anything. >>>> Because >>>> >>>> 1. This series add only new feature: generic hypervisor service with no >>>> immediate use. All ARM guests are already aware that they are >>>> running on >>>> XEN. All ARM guests know that they call *only* PSCI. >>>> >>>> 2. I see importance of this patch series for embedded platforms, where >>>> developer exactly knows what software of which version he/she will run. >>>> I doubt that server platforms will need something beyond PSCI, which I >>>> preserved as is. >>> >>> I disagree here. SMCCC could be used to provide Dom0 a way to interact >>> with the firmware if necessary. >> AFAIK, Dom0 usually is built with particular version of XEN in mind (or >> at least minimal XEN version). > > That's not true. Dom0 is a generic kernel able to probe everything from > the firmware tables and Xen interface. I use the exact Linux kernel on > multiple platforms with no issue. Okay, I was speaking about embedded use case. > The Hypervisor and Dom0 (Linux, FreeBSD) are different projects with > different release cadence. We provide a stable interface that is > backward compatible (returning error code for non-existent hypercalls). > So a Linux released in 10 years should still work on Xen 4.10 and adapt > to the features available. I just want to clarify this. At least four hypercalls are not backward compatible: platform_op, sysctl, domctl and flask_op. I had a problem with this, when played with MiniOS-based monitor. Sure, your kernel will boot up, but some well known XEN services will be absent. This have nothing with SMC bindings problem, though. >> >>>> >>>> A I'm not denying importance of SMC bindings, but I think it is not >>>> blocker for my patches. We can add bindings later, when there will be >>>> consensus on how they should look. In meantime SMC handler can be used >>>> by anyone who knows that is available. >>> >>> I am not in favor on getting something merged in Xen until we agree on >>> the way for the guest to know it is there. >> I think that SMC implementation will be the same, regardless the way we >> can tell guest that it is available. At this time guests can safely >> assume that SMCCC is not implemented in XEN. This wouldn't break >> anything. > > So you suggest to merge this series but says "The guest should not > assume the presence of SMC"? This is rather a bit odd... Basically yes. This is one of the options. If guest knowns for sure that SMCCC is available - it can use it. If it is unsure - then it does not use it. In meantime we can develop a generic way to tell guest that SMCCC is present on a platform. I just don't want to bloat up this patch series. There are already 7 patches and looks like there will be more. But I can add another patch/two with bindings if you insist. This is not a big deal. I just have concerns about this right now. >> >>> It means you have to carry hack in your kernel in order to use SMC. >>> Maybe this is fine for you, but I don't want to make this assumption >>> on Xen upstream today. >> Modern linux kernel uses SMC for PSCI calls and OP-TEE calls. In both >> cases it reads DT to get conduit method (SMC/HVC). So there are already >> bindings for generic uses. Other uses are platform-specific (okay, >> probably there can be a problem). >> >>> This is a change in the interface that should be notified to the >>> guest. If we expose it without providing a bindings (or something), we >>> have no way to revert/disable it. Imagine we want to disable SMC in >>> the future. >> Natural way was to disable Security Extensions in PFR1 register. This >> was not done and now we have curious situation: guest thinks that SMC is >> available, but then it gets Undefined Instruction exception when it >> tries to invoke SMC. > > Security extensions does not mean that SMCCC is present... Even if we > had returned an error, you don't know if it was from the SMCCC protocol > or my foo protocol. > It would be valid (though a bit odd) for a firmware to inject an > undefined instruction if it is unable to understand the SMC. Okay, I can see logic there. This is a strange way to return an error, but it is not prohibited in reference manual at least. > The main problem today is neither HVC #0 nor SMC #0 are SMCCC compliant. > They are let's call it XEN Protocol compliant. KVM has the same problem. > So you clearly need a way to say: "HVCs and SMCs are SMCCC compliant, > you can go ahead probing the different components". Yes, I agree with this. I just trying to say, that it is not only XEN problem, it is platform-wide. > As I said, I am not asking you to write the binding. I am asking you to > have in mind other use cases. If the binding takes to much time, then a > solution would be to add a XENFEAT_*. I can write DT binding. Problem is that this binding should be adopted by all OSes, hypervisors and firmwares. More on this below. >> >>> How a guest will know that >>> - until Xen 4.10 SMC was not existing, >>> - between Xen 4.10 and Xen 4.x you can use them >>> - after Xen 4.y they can be disabled. >> It is a broader question: how software can know that SMCCC is available >> on a platform? Not SMC, but SMCCC as a protocol. Probably, there should >> be some generic way to tell Linux/Windows/XEN/Windriver Hypervisor/etc >> that they can rely on SMCCC spec. I think that it is question to ARM >> guys (including you), because this affects all ARM machines. > > I spent quite some time to explain that in an e-mail to the previous > version... See the thread [1]. Yes, I've seen this thread, naturally. But I can't find there answer to my question. As I said, this binding (or any other way to tell software about SMCCC) should not be XEN-specific. As you mentioned, KVM have the exactly same problem. I think, bare-metal platforms suffer from similar issue. So, in my opinion, it will be great to have agreement on SMCCC discovery from all players. So, when I boot my kernel on any ARM machine, it can tell for sure if SMCCC is supported. What to you think? We can stick to XEN-only approach, like XENFEAT_* or "xen,smccc" in DT. But is this right? >> >>> All changes should be detected through the firmware tables (DT, ACPI) >>> or another Xen method (i.e XENFEAT_*). For instance, the guest has to >>> parse the firmware tables in order to know PSCI is available. >> Yep. The same does OP-TEE code. It parses DT to get conduit. Probably, >> this is wrong approach. Should all SMCCC calls use the same conduit?. >> Should platform provide conduit for each SMCCC service owner? > > Technically a guest should only use HVC. I am happy to allow the SMC to > support unmodified OS that don't use DT. > Cheers, > > [1] > https://lists.xenproject.org/archives/html/xen-devel/2017-08/msg00068.html >
On 10/08/2017 21:09, Volodymyr Babchuk wrote: > Hi, > > On 10.08.17 21:18, Julien Grall wrote: >> Hi, >> >> On 10/08/17 18:40, Volodymyr Babchuk wrote: >>> >>> >>> On 10.08.17 19:11, Julien Grall wrote: >>>> >>>> >>>> On 10/08/17 16:33, Volodymyr Babchuk wrote: >>>>> Hi Julien, >>>>> >>>>> On 09.08.17 13:10, Julien Grall wrote: >>>>>> 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. >>>>>> >>>>> >>>>> While I agree that question about bindings is important, I can't >>>>> see how >>>>> it affects this patch series. This patch series does not break >>>>> anything. >>>>> Because >>>>> >>>>> 1. This series add only new feature: generic hypervisor service >>>>> with no >>>>> immediate use. All ARM guests are already aware that they are >>>>> running on >>>>> XEN. All ARM guests know that they call *only* PSCI. >>>>> >>>>> 2. I see importance of this patch series for embedded platforms, where >>>>> developer exactly knows what software of which version he/she will >>>>> run. >>>>> I doubt that server platforms will need something beyond PSCI, which I >>>>> preserved as is. >>>> >>>> I disagree here. SMCCC could be used to provide Dom0 a way to interact >>>> with the firmware if necessary. >>> AFAIK, Dom0 usually is built with particular version of XEN in mind (or >>> at least minimal XEN version). >> >> That's not true. Dom0 is a generic kernel able to probe everything >> from the firmware tables and Xen interface. I use the exact Linux >> kernel on multiple platforms with no issue. > Okay, I was speaking about embedded use case. Bear in mind that Xen is not only embedded. When you upstream a new feature you have to think about how this could be used by anyone. > >> The Hypervisor and Dom0 (Linux, FreeBSD) are different projects with >> different release cadence. We provide a stable interface that is >> backward compatible (returning error code for non-existent >> hypercalls). So a Linux released in 10 years should still work on Xen >> 4.10 and adapt to the features available. > I just want to clarify this. At least four hypercalls are not backward > compatible: platform_op, sysctl, domctl and flask_op. I had a problem > with this, when played with MiniOS-based monitor. Sure, your kernel will > boot up, but some well known XEN services will be absent. > This have nothing with SMC bindings problem, though. I am not sure to follow here... Were they ever be supported on ARM? If not, then it is no a backward compatibility issue. You cannot assume that all the hypercalls existing on x86 will be available on ARM. For a list of known available/supported hypercalls for ARM see: public/include/arch-arm.h. Also bear in mind that some hypercalls are not part of the stable ABI. For instance domctls are not stable and it is well-known that you have to rebuild your app for every new Xen versions... But domctls will never be used in Linux Kernel as they only meant to be used to control a domain. > >>> >>>>> >>>>> A I'm not denying importance of SMC bindings, but I think it is not >>>>> blocker for my patches. We can add bindings later, when there will be >>>>> consensus on how they should look. In meantime SMC handler can be used >>>>> by anyone who knows that is available. >>>> >>>> I am not in favor on getting something merged in Xen until we agree on >>>> the way for the guest to know it is there. >>> I think that SMC implementation will be the same, regardless the way we >>> can tell guest that it is available. At this time guests can safely >>> assume that SMCCC is not implemented in XEN. This wouldn't break >>> anything. >> >> So you suggest to merge this series but says "The guest should not >> assume the presence of SMC"? This is rather a bit odd... > Basically yes. This is one of the options. If guest knowns for sure that > SMCCC is available - it can use it. If it is unsure - then it does not > use it. > In meantime we can develop a generic way to tell guest that SMCCC is > present on a platform. > I just don't want to bloat up this patch series. There are already 7 > patches and looks like there will be more. Let's be honest, a 7 patches series is quite small :P. > But I can add another patch/two with bindings if you insist. This is not > a big deal. I just have concerns about this right now. > >>> >>>> It means you have to carry hack in your kernel in order to use SMC. >>>> Maybe this is fine for you, but I don't want to make this assumption >>>> on Xen upstream today. >>> Modern linux kernel uses SMC for PSCI calls and OP-TEE calls. In both >>> cases it reads DT to get conduit method (SMC/HVC). So there are already >>> bindings for generic uses. Other uses are platform-specific (okay, >>> probably there can be a problem). >>> >>>> This is a change in the interface that should be notified to the >>>> guest. If we expose it without providing a bindings (or something), we >>>> have no way to revert/disable it. Imagine we want to disable SMC in >>>> the future. >>> Natural way was to disable Security Extensions in PFR1 register. This >>> was not done and now we have curious situation: guest thinks that SMC is >>> available, but then it gets Undefined Instruction exception when it >>> tries to invoke SMC. >> >> Security extensions does not mean that SMCCC is present... Even if we >> had returned an error, you don't know if it was from the SMCCC >> protocol or my foo protocol. > >> It would be valid (though a bit odd) for a firmware to inject an >> undefined instruction if it is unable to understand the SMC. > Okay, I can see logic there. This is a strange way to return an error, > but it is not prohibited in reference manual at least. > >> The main problem today is neither HVC #0 nor SMC #0 are SMCCC >> compliant. They are let's call it XEN Protocol compliant. KVM has the >> same problem. > >> So you clearly need a way to say: "HVCs and SMCs are SMCCC compliant, >> you can go ahead probing the different components". > Yes, I agree with this. I just trying to say, that it is not only XEN > problem, it is platform-wide. I am well aware about it... and kept saying that from the beginning. Except that nobody really thought about it because embedded are mostly focused to get things working for a specific set of configuration. > >> As I said, I am not asking you to write the binding. I am asking you >> to have in mind other use cases. If the binding takes to much time, >> then a solution would be to add a XENFEAT_*. > I can write DT binding. Problem is that this binding should be adopted > by all OSes, hypervisors and firmwares. More on this below. > >>> >>>> How a guest will know that >>>> - until Xen 4.10 SMC was not existing, >>>> - between Xen 4.10 and Xen 4.x you can use them >>>> - after Xen 4.y they can be disabled. >>> It is a broader question: how software can know that SMCCC is available >>> on a platform? Not SMC, but SMCCC as a protocol. Probably, there should >>> be some generic way to tell Linux/Windows/XEN/Windriver Hypervisor/etc >>> that they can rely on SMCCC spec. I think that it is question to ARM >>> guys (including you), because this affects all ARM machines. >> >> I spent quite some time to explain that in an e-mail to the previous >> version... See the thread [1]. > Yes, I've seen this thread, naturally. But I can't find there answer to > my question. Because there is none of the moment and it is still in discussion... > As I said, this binding (or any other way to tell software about SMCCC) > should not be XEN-specific. As you mentioned, KVM have the exactly same > problem. I think, bare-metal platforms suffer from similar issue. So, in > my opinion, it will be great to have agreement on SMCCC discovery from > all players. So, when I boot my kernel on any ARM machine, it can tell > for sure if SMCCC is supported. What to you think? There are already discussion going-on. It is known and taken care. > > We can stick to XEN-only approach, like XENFEAT_* or "xen,smccc" in DT. > But is this right? >>> >>>> All changes should be detected through the firmware tables (DT, ACPI) >>>> or another Xen method (i.e XENFEAT_*). For instance, the guest has to >>>> parse the firmware tables in order to know PSCI is available. >>> Yep. The same does OP-TEE code. It parses DT to get conduit. Probably, >>> this is wrong approach. Should all SMCCC calls use the same conduit?. >>> Should platform provide conduit for each SMCCC service owner? >> >> Technically a guest should only use HVC. I am happy to allow the SMC >> to support unmodified OS that don't use DT. >> Cheers, >> >> [1] >> https://lists.xenproject.org/archives/html/xen-devel/2017-08/msg00068.html >> >> Cheers,
Hi Volodymyr, On 10/08/17 22:09, Julien Grall wrote: >> >> We can stick to XEN-only approach, like XENFEAT_* or "xen,smccc" in DT. >> But is this right? Answering to this question after some thoughts and discussion around. The generic approach is indeed the best solution, however I am afraid it will take some times to get ready and might not be ready for Xen 4.10. As I mentioned earlier, I don't want to merge this without any way to discover the existence of SMCCC in the hypervisor. I also understand that you would like to get this merged in Xen 4.10. So the alternative we have is to provide a XEN-only approach for the time being. It has to work for both ACPI and DT, this likely means we want a XENFEAT_* flag. That alternative would be supersede when the generic approach will be available. Though we would have to keep both around, but it is it not a big deal. Does it make sense? Cheers,
Hi Julien, On 11.08.17 00:09, Julien Grall wrote: > > > On 10/08/2017 21:09, Volodymyr Babchuk wrote: >> Hi, >> >> On 10.08.17 21:18, Julien Grall wrote: >>> Hi, >>> >>> On 10/08/17 18:40, Volodymyr Babchuk wrote: >>>> >>>> >>>> On 10.08.17 19:11, Julien Grall wrote: >>>>> >>>>> >>>>> On 10/08/17 16:33, Volodymyr Babchuk wrote: >>>>>> Hi Julien, >>>>>> >>>>>> On 09.08.17 13:10, Julien Grall wrote: >>>>>>> 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. >>>>>>> >>>>>> >>>>>> While I agree that question about bindings is important, I can't >>>>>> see how >>>>>> it affects this patch series. This patch series does not break >>>>>> anything. >>>>>> Because >>>>>> >>>>>> 1. This series add only new feature: generic hypervisor service >>>>>> with no >>>>>> immediate use. All ARM guests are already aware that they are >>>>>> running on >>>>>> XEN. All ARM guests know that they call *only* PSCI. >>>>>> >>>>>> 2. I see importance of this patch series for embedded platforms, >>>>>> where >>>>>> developer exactly knows what software of which version he/she will >>>>>> run. >>>>>> I doubt that server platforms will need something beyond PSCI, >>>>>> which I >>>>>> preserved as is. >>>>> >>>>> I disagree here. SMCCC could be used to provide Dom0 a way to interact >>>>> with the firmware if necessary. >>>> AFAIK, Dom0 usually is built with particular version of XEN in mind (or >>>> at least minimal XEN version). >>> >>> That's not true. Dom0 is a generic kernel able to probe everything >>> from the firmware tables and Xen interface. I use the exact Linux >>> kernel on multiple platforms with no issue. >> Okay, I was speaking about embedded use case. > > Bear in mind that Xen is not only embedded. When you upstream a new > feature you have to think about how this could be used by anyone. >> >>> The Hypervisor and Dom0 (Linux, FreeBSD) are different projects with >>> different release cadence. We provide a stable interface that is >>> backward compatible (returning error code for non-existent >>> hypercalls). So a Linux released in 10 years should still work on Xen >>> 4.10 and adapt to the features available. >> I just want to clarify this. At least four hypercalls are not backward >> compatible: platform_op, sysctl, domctl and flask_op. I had a problem >> with this, when played with MiniOS-based monitor. Sure, your kernel will >> boot up, but some well known XEN services will be absent. >> This have nothing with SMC bindings problem, though. > > I am not sure to follow here... Were they ever be supported on ARM? If > not, then it is no a backward compatibility issue. You cannot assume > that all the hypercalls existing on x86 will be available on ARM. > > For a list of known available/supported hypercalls for ARM see: > public/include/arch-arm.h. > > Also bear in mind that some hypercalls are not part of the stable ABI. > For instance domctls are not stable and it is well-known that you have > to rebuild your app for every new Xen versions... > > But domctls will never be used in Linux Kernel as they only meant to be > used to control a domain. > >> >>>> >>>>>> >>>>>> A I'm not denying importance of SMC bindings, but I think it is not >>>>>> blocker for my patches. We can add bindings later, when there will be >>>>>> consensus on how they should look. In meantime SMC handler can be >>>>>> used >>>>>> by anyone who knows that is available. >>>>> >>>>> I am not in favor on getting something merged in Xen until we agree on >>>>> the way for the guest to know it is there. >>>> I think that SMC implementation will be the same, regardless the way we >>>> can tell guest that it is available. At this time guests can safely >>>> assume that SMCCC is not implemented in XEN. This wouldn't break >>>> anything. >>> >>> So you suggest to merge this series but says "The guest should not >>> assume the presence of SMC"? This is rather a bit odd... >> Basically yes. This is one of the options. If guest knowns for sure that >> SMCCC is available - it can use it. If it is unsure - then it does not >> use it. >> In meantime we can develop a generic way to tell guest that SMCCC is >> present on a platform. >> I just don't want to bloat up this patch series. There are already 7 >> patches and looks like there will be more. > > Let's be honest, a 7 patches series is quite small :P. Aaaah, okay :) >> But I can add another patch/two with bindings if you insist. This is not >> a big deal. I just have concerns about this right now. >> >>>> >>>>> It means you have to carry hack in your kernel in order to use SMC. >>>>> Maybe this is fine for you, but I don't want to make this assumption >>>>> on Xen upstream today. >>>> Modern linux kernel uses SMC for PSCI calls and OP-TEE calls. In both >>>> cases it reads DT to get conduit method (SMC/HVC). So there are already >>>> bindings for generic uses. Other uses are platform-specific (okay, >>>> probably there can be a problem). >>>> >>>>> This is a change in the interface that should be notified to the >>>>> guest. If we expose it without providing a bindings (or something), we >>>>> have no way to revert/disable it. Imagine we want to disable SMC in >>>>> the future. >>>> Natural way was to disable Security Extensions in PFR1 register. This >>>> was not done and now we have curious situation: guest thinks that >>>> SMC is >>>> available, but then it gets Undefined Instruction exception when it >>>> tries to invoke SMC. >>> >>> Security extensions does not mean that SMCCC is present... Even if we >>> had returned an error, you don't know if it was from the SMCCC >>> protocol or my foo protocol. >> >>> It would be valid (though a bit odd) for a firmware to inject an >>> undefined instruction if it is unable to understand the SMC. >> Okay, I can see logic there. This is a strange way to return an error, >> but it is not prohibited in reference manual at least. >> >>> The main problem today is neither HVC #0 nor SMC #0 are SMCCC >>> compliant. They are let's call it XEN Protocol compliant. KVM has the >>> same problem. >> >>> So you clearly need a way to say: "HVCs and SMCs are SMCCC compliant, >>> you can go ahead probing the different components". >> Yes, I agree with this. I just trying to say, that it is not only XEN >> problem, it is platform-wide. > > I am well aware about it... and kept saying that from the beginning. > Except that nobody really thought about it because embedded are mostly > focused to get things working for a specific set of configuration. Yes, I see. Okay, I'm curious on how such thinks should be worked out. E.g. XEN ML is not the best place to discuss platform-wide questions. >> >>> As I said, I am not asking you to write the binding. I am asking you >>> to have in mind other use cases. If the binding takes to much time, >>> then a solution would be to add a XENFEAT_*. >> I can write DT binding. Problem is that this binding should be adopted >> by all OSes, hypervisors and firmwares. More on this below. >> >>>> >>>>> How a guest will know that >>>>> - until Xen 4.10 SMC was not existing, >>>>> - between Xen 4.10 and Xen 4.x you can use them >>>>> - after Xen 4.y they can be disabled. >>>> It is a broader question: how software can know that SMCCC is available >>>> on a platform? Not SMC, but SMCCC as a protocol. Probably, there should >>>> be some generic way to tell Linux/Windows/XEN/Windriver Hypervisor/etc >>>> that they can rely on SMCCC spec. I think that it is question to ARM >>>> guys (including you), because this affects all ARM machines. >>> >>> I spent quite some time to explain that in an e-mail to the previous >>> version... See the thread [1]. >> Yes, I've seen this thread, naturally. But I can't find there answer to >> my question. > > Because there is none of the moment and it is still in discussion... > >> As I said, this binding (or any other way to tell software about SMCCC) >> should not be XEN-specific. As you mentioned, KVM have the exactly same >> problem. I think, bare-metal platforms suffer from similar issue. So, in >> my opinion, it will be great to have agreement on SMCCC discovery from >> all players. So, when I boot my kernel on any ARM machine, it can tell >> for sure if SMCCC is supported. What to you think? > > There are already discussion going-on. It is known and taken care. Are you referring to that thread at [1]? Or to some internal discussion? >> >> We can stick to XEN-only approach, like XENFEAT_* or "xen,smccc" in DT. >> But is this right? >>>> >>>>> All changes should be detected through the firmware tables (DT, ACPI) >>>>> or another Xen method (i.e XENFEAT_*). For instance, the guest has to >>>>> parse the firmware tables in order to know PSCI is available. >>>> Yep. The same does OP-TEE code. It parses DT to get conduit. Probably, >>>> this is wrong approach. Should all SMCCC calls use the same conduit?. >>>> Should platform provide conduit for each SMCCC service owner? >>> >>> Technically a guest should only use HVC. I am happy to allow the SMC >>> to support unmodified OS that don't use DT. >>> Cheers, >>> >>> [1] >>> https://lists.xenproject.org/archives/html/xen-devel/2017-08/msg00068.html >>> >>> >>> > > Cheers, >
Hello Jan, On Wed, Aug 09, 2017 at 05:58:19AM -0600, Jan Beulich wrote: > > > On 08/08/17 21:08, Volodymyr Babchuk wrote: > >> +#ifndef __XEN_PUBLIC_ARCH_ARM_SMC_H__ > >> +#define __XEN_PUBLIC_ARCH_ARM_SMC_H__ > >> + > >> +typedef struct { > >> + uint32_t a[4]; > >> +} xen_arm_smccc_uid; > > This is not the normal way of encoding a UID type. I thought about this. According to RFC 4122, UUID should be defined like this: struct xen_uuid_rfc_4122 { u32 time_low; /* low part of timestamp */ u16 time_mid; /* mid part of timestamp */ u16 time_hi_and_version; /* high part of timestamp and version */ u8 clock_seq_hi_and_reserved; /* clock seq hi and variant */ u8 clock_seq_low; /* clock seq low */ u8 node[6]; /* nodes */ }; This resembles structure of RFC, but it is highly inconvenient to use. The most used operation for UUIDs is comparison, I think. Next popular operations are serialization and deserialization. All those are very trivial, if you are using array instead of separate fields. I just checked Linux kernel, it uses array of 16 u8s. I used array of four u32s because this is how it is represented in SMC convention. Now I'm going to create separate public header for UUIDs. And I'm not sure that RFC 4122 approach is the best. Serialization code for that structure will require some fiddling with binary shifts. Personally I stick to the Linux way (uint8_t data[16]). So, I'm interested in maintainers opinion. > >> +#define XEN_ARM_SMCCC_UID(a, b, c, d0, d1, d2, d3, d4, d5, d6, d7) \ > >> + ((xen_arm_smccc_uid) {{(a), ((b) << 16 | (c) ), \ > > This is not C89 compatible. I'm sorry, but I not quite sure why this is not C89 compatible. According to [1] C89 supports initializer lists. > > >> + ((d0) << 24 | (d1) << 16 | (d2) << 8 | (d3) << 0), \ > >> + ((d4) << 24 | (d5) << 16 | (d6) << 8 | (d7) << 0)}}) > >> + [1] http://port70.net/~nsz/c/c89/c89-draft.html#3.5.7
>>> On 16.08.17 at 23:41, <volodymyr_babchuk@epam.com> wrote: > Hello Jan, > > On Wed, Aug 09, 2017 at 05:58:19AM -0600, Jan Beulich wrote: > >> >> > On 08/08/17 21:08, Volodymyr Babchuk wrote: >> >> +#ifndef __XEN_PUBLIC_ARCH_ARM_SMC_H__ >> >> +#define __XEN_PUBLIC_ARCH_ARM_SMC_H__ >> >> + >> >> +typedef struct { >> >> + uint32_t a[4]; >> >> +} xen_arm_smccc_uid; >> >> This is not the normal way of encoding a UID type. > I thought about this. According to RFC 4122, UUID should be defined like > this: > struct xen_uuid_rfc_4122 { > u32 time_low; /* low part of timestamp */ > u16 time_mid; /* mid part of timestamp */ > u16 time_hi_and_version; /* high part of timestamp and version */ > u8 clock_seq_hi_and_reserved; /* clock seq hi and variant */ > u8 clock_seq_low; /* clock seq low */ > u8 node[6]; /* nodes */ > }; > > This resembles structure of RFC, but it is highly inconvenient to use. The most > used operation for UUIDs is comparison, I think. Next popular operations are > serialization and deserialization. All those are very trivial, if you are using > array instead of separate fields. I just checked Linux kernel, it uses array > of 16 u8s. I used array of four u32s because this is how it is represented in > SMC convention. In our tree, see e.g. struct xenpf_efi_guid and EFI_GUID plus the "conversion" function between them (cast_guid()). > Now I'm going to create separate public header for UUIDs. I don't see the need for a separate header. >And I'm not sure > that RFC 4122 approach is the best. Serialization code for that structure > will require some fiddling with binary shifts. Personally I stick to the > Linux way (uint8_t data[16]). > So, I'm interested in maintainers opinion. > >> >> +#define XEN_ARM_SMCCC_UID(a, b, c, d0, d1, d2, d3, d4, d5, d6, d7) \ >> >> + ((xen_arm_smccc_uid) {{(a), ((b) << 16 | (c) ), \ >> >> This is not C89 compatible. > > I'm sorry, but I not quite sure why this is not C89 compatible. According to [1] > C89 supports initializer lists. It's the (<type>){<initializers>} style which C89 doesn't support afaik, and ... >> >> + ((d0) << 24 | (d1) << 16 | (d2) << 8 | (d3) << 0), \ >> >> + ((d4) << 24 | (d5) << 16 | (d6) << 8 | (d7) << 0)}}) >> >> + > > [1] http://port70.net/~nsz/c/c89/c89-draft.html#3.5.7 ... this also doesn't have anything like that. Jan
Hi Jan, On Thu, Aug 17, 2017 at 01:45:54AM -0600, Jan Beulich wrote: > >>> On 16.08.17 at 23:41, <volodymyr_babchuk@epam.com> wrote: > > Hello Jan, > > > > On Wed, Aug 09, 2017 at 05:58:19AM -0600, Jan Beulich wrote: > > > >> > >> > On 08/08/17 21:08, Volodymyr Babchuk wrote: > >> >> +#ifndef __XEN_PUBLIC_ARCH_ARM_SMC_H__ > >> >> +#define __XEN_PUBLIC_ARCH_ARM_SMC_H__ > >> >> + > >> >> +typedef struct { > >> >> + uint32_t a[4]; > >> >> +} xen_arm_smccc_uid; > >> > >> This is not the normal way of encoding a UID type. > > I thought about this. According to RFC 4122, UUID should be defined like > > this: > > struct xen_uuid_rfc_4122 { > > u32 time_low; /* low part of timestamp */ > > u16 time_mid; /* mid part of timestamp */ > > u16 time_hi_and_version; /* high part of timestamp and version */ > > u8 clock_seq_hi_and_reserved; /* clock seq hi and variant */ > > u8 clock_seq_low; /* clock seq low */ > > u8 node[6]; /* nodes */ > > }; > > > > This resembles structure of RFC, but it is highly inconvenient to use. The most > > used operation for UUIDs is comparison, I think. Next popular operations are > > serialization and deserialization. All those are very trivial, if you are using > > array instead of separate fields. I just checked Linux kernel, it uses array > > of 16 u8s. I used array of four u32s because this is how it is represented in > > SMC convention. > > In our tree, see e.g. struct xenpf_efi_guid and EFI_GUID plus the > "conversion" function between them (cast_guid()). There are two EFI_GUID definitions in xen tree. One is in tools/libxc/xc_efi.h, where guid defined in linux style: typedef struct { uint8_t b[16]; } efi_guid_t; Another is in xen/include/efi/efidef.h, and it is defined in Microsoft style: typedef struct { UINT32 Data1; UINT16 Data2; UINT16 Data3; UINT8 Data4[8]; } EFI_GUID; I assume, that you meant second one. Anyways, both of them are for EFI code and are not publicly exported. > > Now I'm going to create separate public header for UUIDs. > > I don't see the need for a separate header. Look, I was asked to provide XEN SMCCC UUID in public headers. To do that, I need some structure to hold UUID. There are two ways: I can either introduce public struct xen_uuid, which later can be used by anyone. In this case it should have some generic structure (RFC4122 compatible, Linux compatible, Microsoft compatible, or some XEN way). In another words, everyone should be happy with it. Either it can be SMCCC-only structure. Then it can live in public SMCCC header, it can have definition that is suitable for SMCCC use, and I'm not obligued to make it compatible with any other UUID definition standard. According to SMCCC, UUID is presented as four 32-bit fields. This is exactly what I did there. So, if you are saying, that there are no need for a separate header, then I can use the second approach, right? > >And I'm not sure > > that RFC 4122 approach is the best. Serialization code for that structure > > will require some fiddling with binary shifts. Personally I stick to the > > Linux way (uint8_t data[16]). > > So, I'm interested in maintainers opinion. > > > >> >> +#define XEN_ARM_SMCCC_UID(a, b, c, d0, d1, d2, d3, d4, d5, d6, d7) \ > >> >> + ((xen_arm_smccc_uid) {{(a), ((b) << 16 | (c) ), \ > >> > >> This is not C89 compatible. > > > > I'm sorry, but I not quite sure why this is not C89 compatible. According to [1] > > C89 supports initializer lists. > > It's the (<type>){<initializers>} style which C89 doesn't support > afaik, and ... I wrote this small test program: #include <stdio.h> struct test { int a[2]; }; int main (int argc, char* argv[]) { printf("%d\n", ((struct test){{1,2}}).a[0]); return 0; } It is compiles with gcc --std=c89 without warnings. > >> >> + ((d0) << 24 | (d1) << 16 | (d2) << 8 | (d3) << 0), \ > >> >> + ((d4) << 24 | (d5) << 16 | (d6) << 8 | (d7) << 0)}}) > >> >> + > > > > [1] http://port70.net/~nsz/c/c89/c89-draft.html#3.5.7 > > ... this also doesn't have anything like that. I'm sorry, I don't get what do you mean under "this". > Jan >
Hi, On 17/08/17 13:35, Volodymyr Babchuk wrote: > I wrote this small test program: > > #include <stdio.h> > > struct test > { > int a[2]; > }; > > int main (int argc, char* argv[]) > { > printf("%d\n", ((struct test){{1,2}}).a[0]); > return 0; > } > > It is compiles with gcc --std=c89 without warnings. This is because GCC supports compounds literals in C89 as an extension (see [1]). > >>>>>> + ((d0) << 24 | (d1) << 16 | (d2) << 8 | (d3) << 0), \ >>>>>> + ((d4) << 24 | (d5) << 16 | (d6) << 8 | (d7) << 0)}}) >>>>>> + >>> >>> [1] http://port70.net/~nsz/c/c89/c89-draft.html#3.5.7 >> >> ... this also doesn't have anything like that. > I'm sorry, I don't get what do you mean under "this". The web page does not mention (<type>){<initializers>}. Which is normal because this also appears in c99 (see [2]). > >> Jan >> Cheers, [1] https://gcc.gnu.org/onlinedocs/gcc-4.3.1/gcc/Compound-Literals.html [2] http://en.cppreference.com/w/c/language/compound_literal
>>> On 17.08.17 at 14:35, <volodymyr_babchuk@epam.com> wrote: > On Thu, Aug 17, 2017 at 01:45:54AM -0600, Jan Beulich wrote: >> >>> On 16.08.17 at 23:41, <volodymyr_babchuk@epam.com> wrote: >> > On Wed, Aug 09, 2017 at 05:58:19AM -0600, Jan Beulich wrote: >> >> > On 08/08/17 21:08, Volodymyr Babchuk wrote: >> >> >> +#ifndef __XEN_PUBLIC_ARCH_ARM_SMC_H__ >> >> >> +#define __XEN_PUBLIC_ARCH_ARM_SMC_H__ >> >> >> + >> >> >> +typedef struct { >> >> >> + uint32_t a[4]; >> >> >> +} xen_arm_smccc_uid; >> >> >> >> This is not the normal way of encoding a UID type. >> > I thought about this. According to RFC 4122, UUID should be defined like >> > this: >> > struct xen_uuid_rfc_4122 { >> > u32 time_low; /* low part of timestamp */ >> > u16 time_mid; /* mid part of timestamp */ >> > u16 time_hi_and_version; /* high part of timestamp and version */ >> > u8 clock_seq_hi_and_reserved; /* clock seq hi and variant */ >> > u8 clock_seq_low; /* clock seq low */ >> > u8 node[6]; /* nodes */ >> > }; >> > >> > This resembles structure of RFC, but it is highly inconvenient to use. The most >> > used operation for UUIDs is comparison, I think. Next popular operations are >> > serialization and deserialization. All those are very trivial, if you are using >> > array instead of separate fields. I just checked Linux kernel, it uses array >> > of 16 u8s. I used array of four u32s because this is how it is represented in >> > SMC convention. >> >> In our tree, see e.g. struct xenpf_efi_guid and EFI_GUID plus the >> "conversion" function between them (cast_guid()). > There are two EFI_GUID definitions in xen tree. One is in > tools/libxc/xc_efi.h, When I talk about the Xen tree, I normally mean the xen/ sub-tree of xen.git. > where guid defined in linux style: > typedef struct { > uint8_t b[16]; > } efi_guid_t; > > Another is in xen/include/efi/efidef.h, and it is defined in Microsoft > style: > typedef struct { > UINT32 Data1; > UINT16 Data2; > UINT16 Data3; > UINT8 Data4[8]; > } EFI_GUID; > > I assume, that you meant second one. > Anyways, both of them are for EFI code and are not publicly exported. But struct xenpf_efi_guid is. >> > Now I'm going to create separate public header for UUIDs. >> >> I don't see the need for a separate header. > Look, I was asked to provide XEN SMCCC UUID in public headers. To do that, > I need some structure to hold UUID. There are two ways: I can either > introduce public struct xen_uuid, which later can be used by anyone. > In this case it should have some generic structure (RFC4122 compatible, > Linux compatible, Microsoft compatible, or some XEN way). In another words, > everyone should be happy with it. Right, but that _still_ does not require you to add a new header. I'd see such a relatively generic type go into xen.h. > Either it can be SMCCC-only structure. Then it can live in public SMCCC header, > it can have definition that is suitable for SMCCC use, and I'm not obligued > to make it compatible with any other UUID definition standard. According to > SMCCC, UUID is presented as four 32-bit fields. This is exactly what I did > there. > > So, if you are saying, that there are no need for a separate header, then I > can use the second approach, right? As per above - no, not really. >> >And I'm not sure >> > that RFC 4122 approach is the best. Serialization code for that structure >> > will require some fiddling with binary shifts. Personally I stick to the >> > Linux way (uint8_t data[16]). >> > So, I'm interested in maintainers opinion. >> > >> >> >> +#define XEN_ARM_SMCCC_UID(a, b, c, d0, d1, d2, d3, d4, d5, d6, d7) \ >> >> >> + ((xen_arm_smccc_uid) {{(a), ((b) << 16 | (c) ), \ >> >> >> >> This is not C89 compatible. >> > >> > I'm sorry, but I not quite sure why this is not C89 compatible. According to [1] >> > C89 supports initializer lists. >> >> It's the (<type>){<initializers>} style which C89 doesn't support >> afaik, and ... > I wrote this small test program: > > #include <stdio.h> > > struct test > { > int a[2]; > }; > > int main (int argc, char* argv[]) > { > printf("%d\n", ((struct test){{1,2}}).a[0]); > return 0; > } > > It is compiles with gcc --std=c89 without warnings. Sure, because afaik gcc keeps its extensions enabled in that mode. Try a compiler that is _not_ gcc or C99 compatible. >> >> >> + ((d0) << 24 | (d1) << 16 | (d2) << 8 | (d3) << 0), \ >> >> >> + ((d4) << 24 | (d5) << 16 | (d6) << 8 | (d7) << 0)}}) >> >> >> + >> > >> > [1] http://port70.net/~nsz/c/c89/c89-draft.html#3.5.7 >> >> ... this also doesn't have anything like that. > I'm sorry, I don't get what do you mean under "this". It means the document you referred to. Jan
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> + +/* 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) +{ + 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 + * 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) + return false; + break; + case HSR_EC_SMC32: + break; + default: + return false; + } + + /* 64 bit calls are allowed only from 64 bit domains */ + 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 + */