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

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

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?


>>>>> 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.
>>>
>>> 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.

> 
> 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