Re: [Xen-devel] Fwd: [xen-4.10-testing bisection] complete test-armhf-armhf-xl-arndale

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

Hi Jan,

On 21/06/2019 09:58, Jan Beulich wrote:
>>>> On 20.06.19 at 19:24, <julien.grall@arm.com> wrote:
>> Actually I may have found the error. I feel quite ashamed I didn't spot
>> this during review and when the bisector fingered it.
>>
>> staging-4.11 and staging.4.12 didn't have get_cycles implemented (i.e it
>> returned 0). During the backport, get_cycles() got suddenly implemented
>> (aside the isb()) so it now returns the number of cycles.
> 
> Stefano, how can this be a valid backport under the given title? The
> (imo) only correct way of backporting that hunk would have been to
> simply drop it, adding isb() instead of the switch to call the function
> in the two other places.
> 
> To both of you: How certain are you that the subsequent type
> change is really all that's needed, and that the sudden change in
> behavior of get_cycles() won't have other undue side effects?

Here the current callers:

42sh> ack get_cycles

include/asm-arm/time.h
12:static inline cycles_t get_cycles (void)

include/asm-x86/time.h
29:static inline cycles_t get_cycles(void)

arch/arm/time.c
152:    boot_count = get_cycles();
193:    uint64_t ticks = get_cycles() - boot_count;

arch/x86/irq.c
898:        tsc_in = tb_init_done ? get_cycles() : 0;
900:        TRACE_3D(TRC_HW_IRQ_HANDLED, irq, tsc_in, get_cycles());
921:        tsc_in = tb_init_done ? get_cycles() : 0;
923:        TRACE_3D(TRC_HW_IRQ_HANDLED, irq, tsc_in, get_cycles());

common/tmem.c
275:    pgp->timestamp = get_cycles();
389:    life = get_cycles() - pgp->timestamp;

common/keyhandler.c
389:    per_cpu(read_cycles_time, cpu) = get_cycles();
412:    per_cpu(read_cycles_time, cpu) = get_cycles();

common/trace.c
595:        u64 tsc = (u64)get_cycles();
778:            this_cpu(lost_records_first_tsc)=(u64)get_cycles();

The two callers we care the most are in arch/arm/time.c. They were added with 
the patch backported and I know this is fine because the value before was 64-bit 
as well. So there are no change except for the isb() here.

I am not sure anyone ever tested/used tmem.c on Arm (and it is dropped on recent 
release). So that's not going to make much difference.

common/keyhandler.c, this is mostly for debugging.

common/trace.c does not matter as tracing does not work on Arm before 4.12.

So I am pretty confident that this change will not provoke more side effects 
than an isb() directly in the code.

Cheers,

--
Julien Grall

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