Re: [Xen-devel] [PATCH v2 12/13] xl: add "tee" option for xl.cfg

[Julien Grall <julien.grall@xxxxxxx>]

Hi Volodymyr,

On 03/09/18 17:54, Volodymyr Babchuk wrote:
> This boolean option controls if TEE access is enabled for the domain.
> If access is enabled, xl will call xc_dom_tee_enable(...) to ask
> hypervisor to enable TEE support.
>
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@xxxxxxxx>
> ---
>   docs/man/xl.cfg.pod.5.in    | 10 ++++++++++
>   tools/libxl/libxl_arm.c     | 30 +++++++++++++++++-------------
>   tools/libxl/libxl_create.c  |  1 +
>   tools/libxl/libxl_types.idl |  1 +
>   tools/xl/xl_parse.c         |  1 +
>   5 files changed, 30 insertions(+), 13 deletions(-)
>
> diff --git a/docs/man/xl.cfg.pod.5.in b/docs/man/xl.cfg.pod.5.in
> index b727181..eac7f2b 100644
> --- a/docs/man/xl.cfg.pod.5.in
> +++ b/docs/man/xl.cfg.pod.5.in
> @@ -2792,6 +2792,16 @@ Currently, only the "sbsa_uart" model is supported for 
> ARM.
>   
>   =back
>   
> +=over 4
> +
> +=item B<tee=BOOLEAN>
> +
> +Enable TEE support for the guest. Currently only OP-TEE is supported. If this
> +option is enabled, xl will create guest, which can access TEE. Also
> +OP-TEE node will be emitted into guest's device tree.
> +
> +=back
> +
>   =head3 x86
>   
>   =over 4
> diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
> index baa0d38..7f1d509 100644
> --- a/tools/libxl/libxl_arm.c
> +++ b/tools/libxl/libxl_arm.c
> @@ -1077,23 +1077,27 @@ int libxl__arch_build_dom_finish(libxl__gc *gc,
>   {
>       int rc = 0, ret;
>   
> -    if (info->arch_arm.vuart != LIBXL_VUART_TYPE_SBSA_UART) {
> -        rc = 0;
> -        goto out;
> +    if (info->arch_arm.vuart == LIBXL_VUART_TYPE_SBSA_UART) {
> +        ret = xc_dom_vuart_init(CTX->xch,
> +                                XEN_DOMCTL_VUART_TYPE_VPL011,
> +                                dom->guest_domid,
> +                                dom->console_domid,
> +                                dom->vuart_gfn,
> +                                &state->vuart_port);
> +        if (ret < 0) {
> +            rc = ERROR_FAIL;
> +            LOG(ERROR, "xc_dom_vuart_init failed\n");
> +        }
I am not sure to understand this re-work. If xc_dom_vuart_init fail, why 
would you want to continue?
I think you want to keep goto out here here.

>       }
>   
> -    ret = xc_dom_vuart_init(CTX->xch,
> -                            XEN_DOMCTL_VUART_TYPE_VPL011,
> -                            dom->guest_domid,
> -                            dom->console_domid,
> -                            dom->vuart_gfn,
> -                            &state->vuart_port);
> -    if (ret < 0) {
> -        rc = ERROR_FAIL;
> -        LOG(ERROR, "xc_dom_vuart_init failed\n");
> +    if (libxl_defbool_val(info->tee)) {
> +        ret = xc_dom_tee_enable(CTX->xch, dom->guest_domid);
> +        if (ret < 0) {
> +            rc = ERROR_FAIL;
> +            LOG(ERROR, "xc_dom_tee_enable failed\n");
> +        }
>       }
>   
> -out:
>       return rc;
>   }
>   
> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> index b7b44e2..d76a294 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -217,6 +217,7 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
>   
>       libxl__arch_domain_build_info_acpi_setdefault(b_info);
>       libxl_defbool_setdefault(&b_info->dm_restrict, false);
> +    libxl_defbool_setdefault(&b_info->tee, false);
>   
>       switch (b_info->type) {
>       case LIBXL_DOMAIN_TYPE_HVM:
> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index 4a38580..8cd35ce 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -613,6 +613,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
>       # Alternate p2m is not bound to any architecture or guest type, as it is
>       # supported by x86 HVM and ARM support is planned.
>       ("altp2m", libxl_altp2m_mode),
> +    ("tee",  libxl_defbool),
The option TEE is described in the Arm section, but the option is 
declared for all architectures here. Do you see any use on x86?
Also, new option should be accompanied with a define (e.g 
LIBXL_HAVE_BUILDINFO_TEE) in libxl.h informing toolstack (e.g libvirt) 
that the option is available.
Lastly, I would consider to introduce an enum here with for now only the 
options: NONE, NATIVE. This would give us some freedom to potentially 
emulate TEE in the future or even choose the TEE (i.e in Secure EL2 
case) without re-defining a new parameters.
Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel