Re: [Xen-devel] [PATCH 2/2] arm: export platform_op XENPF_settime

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Tue, 10 Nov 2015, Julien Grall wrote:
> On 10/11/15 14:27, Stefano Stabellini wrote:
> > On Tue, 10 Nov 2015, Julien Grall wrote:
> >> Hi Stefano,
> >>
> >> On 09/11/15 17:09, Stefano Stabellini wrote:
> >>> On Thu, 5 Nov 2015, Julien Grall wrote:
> >>>> 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.
> >>>
> >>> That's not necessary: the wallclock in Xen is the number
> >>> of seconds since 1970 at the time the physical machine booted, plus the
> >>> domain specific offset, so it is not subject to quick incremental
> >>> changes, like a monotonic clock.
> >>
> >> Well, building dom0 takes more than one sec, even on big platform.
> >>
> >> And if it's not subject to quick incremental, what's the point to call
> >> update_domain_wallclock_time in an odd way in arch_set_info_guest rather
> >> than in arch_domain_create?
> > 
> > Only to make sure that is called after domain_vtimer_init.
> > In fact I could move it to arch_domain_create right after it. That might
> > be better?
> 
> Yes, it would make more sense.
> 
> I'm also wondering if we can directly do the call in domain_vtimer_init?

Maybe that's not a good idea given that wallclock and monotonic clocks
are usually separate, but we could certainly move setting
time_offset_seconds there.

 
> >>>>> Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
> >>>>> Signed-off-by: Ian Campbell <ian.campbell@xxxxxxxxxx>
> >>>>> ---
> >>>>>  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.
> >>>
> >>> Uhm.. OK
> >>>
> >>>
> >>>>>  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.
> >>>
> >>> In this case I don't think so. I don't see the other existing
> >>> platform_ops being used on arm.
> >>
> >> Are you sure? I can see some of the sub-hypercall implemented for ARM
> >> too such as XENPF_efi_runtime_call, XENPF_change_freq,
> >> XENPF_getidletime, XENPF_cpu_{online,offline}...
> >>
> >> I'm not asking for implementing all of them now, but just preparing an
> >> infrastructure for later similar to the domctl version.
> > 
> > The spin_trylock call is not useful on ARM and many of the other ops are
> > not either. In addition the implementation of XENPF_settime64 is
> > slightly different too.
> 
> That's a call to forget changing the locking when it will be required
> and may lead to security issue.
> 
> It really doesn't hurt us to do the spin_trylock solution today.

All right


> > I don't think there is any value in trying to share the do_platform_op
> > function now. But maybe in the future.
> 
> Ok.
> 
> -- 
> Julien Grall
> 

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