Re: [PATCH 2/2] arm: export platform_op XENPF_settime

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

Hi Stefano,

You forgot to CC Daniel for the XSM part. Please use
scripts/get_maintainers.pl to get the relevant maintainers.

On 05/11/15 16:57, Stefano Stabellini wrote:
> Call update_domain_wallclock_time at domain initialization.

It's not really what you are doing in the code. You are calling
update_domain_wallclock_time when the first vCPU is initialized.

Also some rationale to explain why this call should be done here would
be good.

Finally, I'm a bit surprised that you only need to call
update_domain_wallclock_time when the domain is created. x86 needs to
call in various places.

For instance we may want to call update_domain_wallclock_time in
construct_dom0 before clearing the pause flags. This is because the
wallclock may be out of sync as construction DOM0 takes some time.

> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  xen/arch/arm/Makefile             |    1 +
>  xen/arch/arm/domain.c             |    3 ++
>  xen/arch/arm/platform_hypercall.c |   62 +++++++++++++++++++++++++++++++++++++
>  xen/arch/arm/traps.c              |    1 +
>  xen/include/xsm/dummy.h           |   12 +++----
>  xen/include/xsm/xsm.h             |   13 ++++----

You also have to fix xsm/flask/hooks.c.

>  6 files changed, 80 insertions(+), 12 deletions(-)
>  create mode 100644 xen/arch/arm/platform_hypercall.c

[..]

> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index b2bfc7d..ac9b1b3 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -742,6 +742,9 @@ int arch_set_info_guest(
>      v->arch.ttbr1 = ctxt->ttbr1;
>      v->arch.ttbcr = ctxt->ttbcr;
>  
> +    if ( v->vcpu_id == 0 )
> +        update_domain_wallclock_time(v->domain);
> +
>      v->is_initialised = 1;
>  
>      if ( ctxt->flags & VGCF_online )
> diff --git a/xen/arch/arm/platform_hypercall.c b/xen/arch/arm/platform_hypercall.c
> new file mode 100644
> index 0000000..f60d7b3
> --- /dev/null
> +++ b/xen/arch/arm/platform_hypercall.c
> @@ -0,0 +1,62 @@
> +/******************************************************************************
> + * platform_hypercall.c
> + * 
> + * Hardware platform operations. Intended for use by domain-0 kernel.
> + * 
> + * Copyright (c) 2015, Citrix
> + */
> +
> +#include <xen/config.h>
> +#include <xen/types.h>
> +#include <xen/sched.h>
> +#include <xen/guest_access.h>
> +#include <xen/spinlock.h>
> +#include <public/platform.h>
> +#include <xsm/xsm.h>
> +#include <asm/current.h>
> +#include <asm/event.h>
> +
> +DEFINE_SPINLOCK(xenpf_lock);
> +
> +long do_platform_op(XEN_GUEST_HANDLE_PARAM(xen_platform_op_t) u_xenpf_op)
> +{

Would it make sense to introduce a common platform code which take care
of common hypercall? See for instance do_domctl and arch_do_domctl.

> +    long ret;
> +    struct xen_platform_op curop, *op = &curop;
> +
> +    if ( copy_from_guest(op, u_xenpf_op, 1) )
> +        return -EFAULT;
> +
> +    if ( op->interface_version != XENPF_INTERFACE_VERSION )
> +        return -EACCES;
> +
> +    ret = xsm_platform_op(XSM_PRIV, op->cmd);
> +    if ( ret )
> +        return ret;
> +
> +    spin_lock(&xenpf_lock);
> +
> +    switch ( op->cmd )
> +    {
> +    case XENPF_settime32:
> +        do_settime(op->u.settime32.secs,
> +                   op->u.settime32.nsecs,
> +                   op->u.settime32.system_time);
> +        break;

Do we really want to support settime32 on ARM?

> +
> +    case XENPF_settime64:
> +        if ( likely(!op->u.settime64.mbz) )
> +            do_settime(op->u.settime64.secs,
> +                       op->u.settime64.nsecs,
> +                       op->u.settime64.system_time);
> +        else
> +            ret = -EINVAL;
> +        break;
> +
> +    default:
> +        ret = -ENOSYS;
> +        break;
> +    }
> +
> +    spin_unlock(&xenpf_lock);
> +    return ret;
> +}

Regards,

-- 
Julien Grall