Re: [kvm-unit-tests PATCH 00/18] arm/arm64: add smp support

Re: [kvm-unit-tests PATCH 00/18] arm/arm64: add smp support

[Christoffer Dall]

On Sun, Feb 01, 2015 at 07:34:28PM +0100, Andrew Jones wrote:
> This series extends the kvm-unit-tests/arm[64] framework to support smp.
> A break down of the patches is as follows
> 
> 01-02: prepare general framework for smp use
> 03-06: arm/arm64 fixups not 100% related to this series,
>        but need to post some time...
> 07-09: add thread_info (for per-thread data) and suck some global
>        data into it
> 10-11: add cpumask support (for per-cpu data) and suck some more
>        global data in
>    12: add arm64 simple spinlock implementation
> 13-14: add some PSCI support
> 15-16: further prep for smp_boot_secondary
>    17: finally add smp_boot_secondary
>    18: as usual, add a selftest to make sure it all works
> 
> These patches are also available here:
> https://github.com/rhdrjones/kvm-unit-tests/tree/arm/smp
> 
I've tested these patches on Juno and they seem to run fine, however,
since we don't support big.LITTLE yet, you have to run them under
'taskset <mask>', but the config script uses $(getconf
_NPROCESSORS_CONF), which returns 6, and QEMU fails.  The interesting
bit is that the unit-tests still report 'PASS' - not sure why.

I'm fine with merging these and fixing anything up as we go along - this
is a test harness after all and I don't have time for an in-depth review
of these patches right now, sorry.

-Christoffer

Re: [PATCH 03/18] arm: fixups: add barriers, actually set MAIR

[Christoffer Dall]

On Sun, Feb 01, 2015 at 07:34:31PM +0100, Andrew Jones wrote:
> Sprinkle in some more isbs after context-changing operations,
> as the ARM ARM states we should. I haven't seen any problems
> without them, but we should do it right. Also, *actually* set
> the MAIR in asm_mmu_enable. We were reading, not writing...
> Luckily this was just spotted while adding the isbs, rather
> than leading to a nightmare debug session some day...
> 
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
>  arm/cstart.S        | 8 ++++++--
>  lib/arm/processor.c | 1 +
>  2 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/arm/cstart.S b/arm/cstart.S
> index da496e9eae7e0..39fac8f1e1bd8 100644
> --- a/arm/cstart.S
> +++ b/arm/cstart.S
> @@ -50,6 +50,7 @@ start:
>  .macro set_mode_stack mode, stack
>  	add	\stack, #S_FRAME_SIZE
>  	msr	cpsr_c, #(\mode | PSR_I_BIT | PSR_F_BIT)
> +	isb
>  	mov	sp, \stack
>  .endm
>  
> @@ -70,6 +71,7 @@ exceptions_init:
>  	set_mode_stack	FIQ_MODE, r1
>  
>  	msr	cpsr_cxsf, r2		@ back to svc mode
> +	isb
>  	mov	pc, lr
>  
I don't think you really need those after the cpsr, but whatever.

-Christoffer

Re: [kvm-unit-tests PATCH 00/18] arm/arm64: add smp support

[Andrew Jones]

On Thu, Feb 26, 2015 at 12:34:02PM +0100, Christoffer Dall wrote:
> On Sun, Feb 01, 2015 at 07:34:28PM +0100, Andrew Jones wrote:
> > This series extends the kvm-unit-tests/arm[64] framework to support smp.
> > A break down of the patches is as follows
> > 
> > 01-02: prepare general framework for smp use
> > 03-06: arm/arm64 fixups not 100% related to this series,
> >        but need to post some time...
> > 07-09: add thread_info (for per-thread data) and suck some global
> >        data into it
> > 10-11: add cpumask support (for per-cpu data) and suck some more
> >        global data in
> >    12: add arm64 simple spinlock implementation
> > 13-14: add some PSCI support
> > 15-16: further prep for smp_boot_secondary
> >    17: finally add smp_boot_secondary
> >    18: as usual, add a selftest to make sure it all works
> > 
> > These patches are also available here:
> > https://github.com/rhdrjones/kvm-unit-tests/tree/arm/smp
> > 
> I've tested these patches on Juno and they seem to run fine, however,
> since we don't support big.LITTLE yet, you have to run them under
> 'taskset <mask>', but the config script uses $(getconf
> _NPROCESSORS_CONF), which returns 6, and QEMU fails.  The interesting

Should I try to read the number of host cpus from some other source?
If you know something I can read that also works on big.LITTLE, then
I can change it now.

> bit is that the unit-tests still report 'PASS' - not sure why.

Ah, this is due to the weird way qemu's debugexit device sets its exit
code

hw/misc/debugexit.c:debug_exit_write()
{
  exit((val << 1) | 1);
}

To be consistent with that we made chr-testdev do the same thing (see
backends/testdev.c:testdev_eat_packet():case 'q'). Now, the
kvm-unit-tests run_tests.sh script knows about that, so it has

  eval $cmdline >> test.log
  if [ $? -le 1 ]; then
     echo -e "\e[32mPASS\e[0m $1"
  else
     echo -e "\e[31mFAIL\e[0m $1"
  fi

Yes, this sucks, as we can't tell the difference between qemu failing
to run the test, and exiting with 1 vs. the test running, passing -
exiting with (0 << 1) | 1. It's too bad debugexit didn't set a higher
bit (like 5 or 6) to flag a "debug exit". Maybe it's not too late to
change it? Paolo?

> 
> I'm fine with merging these and fixing anything up as we go along - this
> is a test harness after all and I don't have time for an in-depth review
> of these patches right now, sorry.

Thanks!
drew

Re: [kvm-unit-tests PATCH 00/18] arm/arm64: add smp support

[Paolo Bonzini]

On 26/02/2015 14:50, Andrew Jones wrote:
> Yes, this sucks, as we can't tell the difference between qemu failing
> to run the test, and exiting with 1 vs. the test running, passing -
> exiting with (0 << 1) | 1. It's too bad debugexit didn't set a higher
> bit (like 5 or 6) to flag a "debug exit". Maybe it's not too late to
> change it? Paolo?

That would indeed have been a better idea. :(  I'm not sure it's worth
it because in theory exit codes of 1 should never happen in
kvm-unit-tests.  That said, it does suck when they happen due to a bug
in the harness.

Paolo

Re: [kvm-unit-tests PATCH 00/18] arm/arm64: add smp support

[Marcelo Tosatti]

On Sun, Feb 01, 2015 at 07:34:28PM +0100, Andrew Jones wrote:
> This series extends the kvm-unit-tests/arm[64] framework to support smp.
> A break down of the patches is as follows
> 
> 01-02: prepare general framework for smp use
> 03-06: arm/arm64 fixups not 100% related to this series,
>        but need to post some time...
> 07-09: add thread_info (for per-thread data) and suck some global
>        data into it
> 10-11: add cpumask support (for per-cpu data) and suck some more
>        global data in
>    12: add arm64 simple spinlock implementation
> 13-14: add some PSCI support
> 15-16: further prep for smp_boot_secondary
>    17: finally add smp_boot_secondary
>    18: as usual, add a selftest to make sure it all works
> 
> These patches are also available here:
> https://github.com/rhdrjones/kvm-unit-tests/tree/arm/smp
> 
> Thanks in advance for reviews!
> 
> 
> Andrew Jones (18):
>   x86: expose spin_lock/unlock to lib code
>   lib/report: guard access to counters
>   arm: fixups: add barriers, actually set MAIR
>   arm64: fixup: use id_aa64mmfr0_el1 to set tcr
>   arm/arm64: processor.[ch] cleanups
>   arm/arm64: get rid of get_sp()
>   arm/arm64: introduce thread_info
>   arm/arm64: add per thread user_mode flag
>   arm/arm64: maintain per thread exception handlers
>   arm/arm64: add simple cpumask API
>   arm/arm64: make mmu_on per cpu
>   arm64: implement spinlocks
>   arm/arm64: import include/uapi/linux/psci.h
>   arm/arm64: add some PSCI API
>   arm/arm64: add cpu_relax() and friends
>   arm: clarify comment about exception stack use
>   arm/arm64: add smp_boot_secondary
>   arm/arm64: Add smp selftest
> 
>  arm/cstart.S                 |  47 +++++++++++++----
>  arm/cstart64.S               |  48 +++++++++++-------
>  arm/flat.lds                 |   6 +++
>  arm/selftest.c               |  70 +++++++++++++++++++++++--
>  arm/unittests.cfg            |  11 +++-
>  config/config-arm-common.mak |   3 ++
>  config/config-arm64.mak      |   1 +
>  lib/arm/asm-offsets.c        |   3 ++
>  lib/arm/asm/barrier.h        |   5 ++
>  lib/arm/asm/bitops.h         |  53 +++++++++++++++++++
>  lib/arm/asm/cpumask.h        | 118 +++++++++++++++++++++++++++++++++++++++++++
>  lib/arm/asm/mmu-api.h        |   1 +
>  lib/arm/asm/processor.h      |  12 ++++-
>  lib/arm/asm/psci.h           |  15 ++++++
>  lib/arm/asm/smp.h            |  56 ++++++++++++++++++++
>  lib/arm/asm/thread_info.h    |  60 ++++++++++++++++++++++
>  lib/arm/asm/uapi-psci.h      |  73 ++++++++++++++++++++++++++
>  lib/arm/bitops.c             |  81 +++++++++++++++++++++++++++++
>  lib/arm/mmu.c                |  15 ++++--
>  lib/arm/processor.c          |  44 +++++++++++-----
>  lib/arm/psci.c               |  49 ++++++++++++++++++
>  lib/arm/setup.c              |  11 +++-
>  lib/arm/smp.c                |  57 +++++++++++++++++++++
>  lib/arm64/asm-offsets.c      |   2 +
>  lib/arm64/asm/barrier.h      |   5 ++
>  lib/arm64/asm/bitops.h       |  51 +++++++++++++++++++
>  lib/arm64/asm/cpumask.h      |   1 +
>  lib/arm64/asm/processor.h    |  18 ++++++-
>  lib/arm64/asm/psci.h         |  15 ++++++
>  lib/arm64/asm/smp.h          |   1 +
>  lib/arm64/asm/spinlock.h     |   8 +--
>  lib/arm64/asm/thread_info.h  |   1 +
>  lib/arm64/asm/uapi-psci.h    |   1 +
>  lib/arm64/processor.c        |  81 +++++++++++++++++++----------
>  lib/arm64/spinlock.c         |  43 ++++++++++++++++
>  lib/report.c                 |  16 ++++++
>  lib/x86/asm/spinlock.h       |  11 ++++
>  lib/x86/smp.h                |   7 +--
>  38 files changed, 1008 insertions(+), 92 deletions(-)
>  create mode 100644 lib/arm/asm/bitops.h
>  create mode 100644 lib/arm/asm/cpumask.h
>  create mode 100644 lib/arm/asm/psci.h
>  create mode 100644 lib/arm/asm/smp.h
>  create mode 100644 lib/arm/asm/thread_info.h
>  create mode 100644 lib/arm/asm/uapi-psci.h
>  create mode 100644 lib/arm/bitops.c
>  create mode 100644 lib/arm/psci.c
>  create mode 100644 lib/arm/smp.c
>  create mode 100644 lib/arm64/asm/bitops.h
>  create mode 100644 lib/arm64/asm/cpumask.h
>  create mode 100644 lib/arm64/asm/psci.h
>  create mode 100644 lib/arm64/asm/smp.h
>  create mode 100644 lib/arm64/asm/thread_info.h
>  create mode 100644 lib/arm64/asm/uapi-psci.h
>  create mode 100644 lib/arm64/spinlock.c
>  create mode 100644 lib/x86/asm/spinlock.h
> 
> -- 
> 1.9.3

Applied, thanks.

Re: [kvm-unit-tests PATCH 00/18] arm/arm64: add smp support

[Christoffer Dall]

On Thu, Feb 26, 2015 at 02:50:38PM +0100, Andrew Jones wrote:
> On Thu, Feb 26, 2015 at 12:34:02PM +0100, Christoffer Dall wrote:
> > On Sun, Feb 01, 2015 at 07:34:28PM +0100, Andrew Jones wrote:
> > > This series extends the kvm-unit-tests/arm[64] framework to support smp.
> > > A break down of the patches is as follows
> > > 
> > > 01-02: prepare general framework for smp use
> > > 03-06: arm/arm64 fixups not 100% related to this series,
> > >        but need to post some time...
> > > 07-09: add thread_info (for per-thread data) and suck some global
> > >        data into it
> > > 10-11: add cpumask support (for per-cpu data) and suck some more
> > >        global data in
> > >    12: add arm64 simple spinlock implementation
> > > 13-14: add some PSCI support
> > > 15-16: further prep for smp_boot_secondary
> > >    17: finally add smp_boot_secondary
> > >    18: as usual, add a selftest to make sure it all works
> > > 
> > > These patches are also available here:
> > > https://github.com/rhdrjones/kvm-unit-tests/tree/arm/smp
> > > 
> > I've tested these patches on Juno and they seem to run fine, however,
> > since we don't support big.LITTLE yet, you have to run them under
> > 'taskset <mask>', but the config script uses $(getconf
> > _NPROCESSORS_CONF), which returns 6, and QEMU fails.  The interesting
> 
> Should I try to read the number of host cpus from some other source?
> If you know something I can read that also works on big.LITTLE, then
> I can change it now.
> 

I have no idea what the right scripting fix would be. But we should
really fix big.LITTLE support in KVM.  Hmmm.

> > bit is that the unit-tests still report 'PASS' - not sure why.
> 
> Ah, this is due to the weird way qemu's debugexit device sets its exit
> code
> 
> hw/misc/debugexit.c:debug_exit_write()
> {
>   exit((val << 1) | 1);
> }
> 
> To be consistent with that we made chr-testdev do the same thing (see
> backends/testdev.c:testdev_eat_packet():case 'q'). Now, the
> kvm-unit-tests run_tests.sh script knows about that, so it has
> 
>   eval $cmdline >> test.log
>   if [ $? -le 1 ]; then
>      echo -e "\e[32mPASS\e[0m $1"
>   else
>      echo -e "\e[31mFAIL\e[0m $1"
>   fi
> 
> Yes, this sucks, as we can't tell the difference between qemu failing
> to run the test, and exiting with 1 vs. the test running, passing -
> exiting with (0 << 1) | 1. It's too bad debugexit didn't set a higher
> bit (like 5 or 6) to flag a "debug exit". Maybe it's not too late to
> change it? Paolo?
> 

This would be really good to address somehow, because we don't want
to report that everything is happy when the test harness broke, that
really goes against the whole idea of this work.

Thanks,
-Christoffer