Re: [kvm-unit-tests PATCH v9 07/31] scripts: allow machine option to be specified in unittests.cfg

[Thomas Huth]

On 08/05/2024 14.58, Thomas Huth wrote:

On 08/05/2024 14.55, Thomas Huth wrote:

On 08/05/2024 14.27, Nicholas Piggin wrote:

On Wed May 8, 2024 at 1:08 AM AEST, Thomas Huth wrote:

On 04/05/2024 14.28, Nicholas Piggin wrote:

This allows different machines with different requirements to be
supported by run_tests.sh, similarly to how different accelerators
are handled.

Acked-by: Thomas Huth 
Acked-by: Andrew Jones 
Signed-off-by: Nicholas Piggin 
---
   docs/unittests.txt   |  7 +++
   scripts/common.bash  |  8 ++--
   scripts/runtime.bash | 16 
   3 files changed, 25 insertions(+), 6 deletions(-)

diff --git a/docs/unittests.txt b/docs/unittests.txt
index 7cf2c55ad..6449efd78 100644
--- a/docs/unittests.txt
+++ b/docs/unittests.txt
@@ -42,6 +42,13 @@ For / directories that support multiple 
architectures, this restricts

   the test to the specified arch. By default, the test will run on any
   architecture.
+machine
+---
+For those architectures that support multiple machine types, this 
restricts

+the test to the specified machine. By default, the test will run on
+any machine type. (Note, the machine can be specified with the MACHINE=
+environment variable, and defaults to the architecture's default.)
+
   smp
   ---
   smp = 
diff --git a/scripts/common.bash b/scripts/common.bash
index 5e9ad53e2..3aa557c8c 100644
--- a/scripts/common.bash
+++ b/scripts/common.bash
@@ -10,6 +10,7 @@ function for_each_unittest()
   local opts
   local groups
   local arch
+    local machine
   local check
   local accel
   local timeout
@@ -21,7 +22,7 @@ function for_each_unittest()
   if [[ "$line" =~ ^\[(.*)\]$ ]]; then
   rematch=${BASH_REMATCH[1]}
   if [ -n "${testname}" ]; then
-    $(arch_cmd) "$cmd" "$testname" "$groups" "$smp" 
"$kernel" "$opts" "$arch" "$check" "$accel" "$timeout"
+    $(arch_cmd) "$cmd" "$testname" "$groups" "$smp" 
"$kernel" "$opts" "$arch" "$machine" "$check" "$accel" "$timeout"

   fi
   testname=$rematch
   smp=1
@@ -29,6 +30,7 @@ function for_each_unittest()
   opts=""
   groups=""
   arch=""
+    machine=""
   check=""
   accel=""
   timeout=""
@@ -58,6 +60,8 @@ function for_each_unittest()
   groups=${BASH_REMATCH[1]}
   elif [[ $line =~ ^arch\ *=\ *(.*)$ ]]; then
   arch=${BASH_REMATCH[1]}
+    elif [[ $line =~ ^machine\ *=\ *(.*)$ ]]; then
+    machine=${BASH_REMATCH[1]}
   elif [[ $line =~ ^check\ *=\ *(.*)$ ]]; then
   check=${BASH_REMATCH[1]}
   elif [[ $line =~ ^accel\ *=\ *(.*)$ ]]; then
@@ -67,7 +71,7 @@ function for_each_unittest()
   fi
   done
   if [ -n "${testname}" ]; then
-    $(arch_cmd) "$cmd" "$testname" "$groups" "$smp" "$kernel" 
"$opts" "$arch" "$check" "$accel" "$timeout"
+    $(arch_cmd) "$cmd" "$testname" "$groups" "$smp" "$kernel" 
"$opts" "$arch" "$machine" "$check" "$accel" "$timeout"

   fi
   exec {fd}<&-
   }
diff --git a/scripts/runtime.bash b/scripts/runtime.bash
index 177b62166..0c96d6ea2 100644
--- a/scripts/runtime.bash
+++ b/scripts/runtime.bash
@@ -32,7 +32,7 @@ premature_failure()
   get_cmdline()
   {
   local kernel=$1
-    echo "TESTNAME=$testname TIMEOUT=$timeout ACCEL=$accel 
$RUNTIME_arch_run $kernel -smp $smp $opts"
+    echo "TESTNAME=$testname TIMEOUT=$timeout MACHINE=$machine 
ACCEL=$accel $RUNTIME_arch_run $kernel -smp $smp $opts"

   }
   skip_nodefault()
@@ -80,9 +80,10 @@ function run()
   local kernel="$4"
   local opts="$5"
   local arch="$6"
-    local check="${CHECK:-$7}"
-    local accel="$8"
-    local timeout="${9:-$TIMEOUT}" # unittests.cfg overrides the default
+    local machine="$7"
+    local check="${CHECK:-$8}"
+    local accel="$9"
+    local timeout="${10:-$TIMEOUT}" # unittests.cfg overrides the default
   if [ "${CONFIG_EFI}" == "y" ]; then
   kernel=${kernel/%.flat/.efi}
@@ -116,6 +117,13 @@ function run()
   return 2
   fi
+    if [ -n "$machine" ] && [ -n "$MACHINE" ] && [ "$machine" != 
"$MACHINE" ]; then

+    print_result "SKI

Re: [kvm-unit-tests PATCH v9 07/31] scripts: allow machine option to be specified in unittests.cfg

[Thomas Huth]

On 08/05/2024 14.55, Thomas Huth wrote:

On 08/05/2024 14.27, Nicholas Piggin wrote:

On Wed May 8, 2024 at 1:08 AM AEST, Thomas Huth wrote:

On 04/05/2024 14.28, Nicholas Piggin wrote:

This allows different machines with different requirements to be
supported by run_tests.sh, similarly to how different accelerators
are handled.

Acked-by: Thomas Huth 
Acked-by: Andrew Jones 
Signed-off-by: Nicholas Piggin 
---
   docs/unittests.txt   |  7 +++
   scripts/common.bash  |  8 ++--
   scripts/runtime.bash | 16 
   3 files changed, 25 insertions(+), 6 deletions(-)

diff --git a/docs/unittests.txt b/docs/unittests.txt
index 7cf2c55ad..6449efd78 100644
--- a/docs/unittests.txt
+++ b/docs/unittests.txt
@@ -42,6 +42,13 @@ For / directories that support multiple 
architectures, this restricts

   the test to the specified arch. By default, the test will run on any
   architecture.
+machine
+---
+For those architectures that support multiple machine types, this 
restricts

+the test to the specified machine. By default, the test will run on
+any machine type. (Note, the machine can be specified with the MACHINE=
+environment variable, and defaults to the architecture's default.)
+
   smp
   ---
   smp = 
diff --git a/scripts/common.bash b/scripts/common.bash
index 5e9ad53e2..3aa557c8c 100644
--- a/scripts/common.bash
+++ b/scripts/common.bash
@@ -10,6 +10,7 @@ function for_each_unittest()
   local opts
   local groups
   local arch
+    local machine
   local check
   local accel
   local timeout
@@ -21,7 +22,7 @@ function for_each_unittest()
   if [[ "$line" =~ ^\[(.*)\]$ ]]; then
   rematch=${BASH_REMATCH[1]}
   if [ -n "${testname}" ]; then
-    $(arch_cmd) "$cmd" "$testname" "$groups" "$smp" 
"$kernel" "$opts" "$arch" "$check" "$accel" "$timeout"
+    $(arch_cmd) "$cmd" "$testname" "$groups" "$smp" 
"$kernel" "$opts" "$arch" "$machine" "$check" "$accel" "$timeout"

   fi
   testname=$rematch
   smp=1
@@ -29,6 +30,7 @@ function for_each_unittest()
   opts=""
   groups=""
   arch=""
+    machine=""
   check=""
   accel=""
   timeout=""
@@ -58,6 +60,8 @@ function for_each_unittest()
   groups=${BASH_REMATCH[1]}
   elif [[ $line =~ ^arch\ *=\ *(.*)$ ]]; then
   arch=${BASH_REMATCH[1]}
+    elif [[ $line =~ ^machine\ *=\ *(.*)$ ]]; then
+    machine=${BASH_REMATCH[1]}
   elif [[ $line =~ ^check\ *=\ *(.*)$ ]]; then
   check=${BASH_REMATCH[1]}
   elif [[ $line =~ ^accel\ *=\ *(.*)$ ]]; then
@@ -67,7 +71,7 @@ function for_each_unittest()
   fi
   done
   if [ -n "${testname}" ]; then
-    $(arch_cmd) "$cmd" "$testname" "$groups" "$smp" "$kernel" 
"$opts" "$arch" "$check" "$accel" "$timeout"
+    $(arch_cmd) "$cmd" "$testname" "$groups" "$smp" "$kernel" 
"$opts" "$arch" "$machine" "$check" "$accel" "$timeout"

   fi
   exec {fd}<&-
   }
diff --git a/scripts/runtime.bash b/scripts/runtime.bash
index 177b62166..0c96d6ea2 100644
--- a/scripts/runtime.bash
+++ b/scripts/runtime.bash
@@ -32,7 +32,7 @@ premature_failure()
   get_cmdline()
   {
   local kernel=$1
-    echo "TESTNAME=$testname TIMEOUT=$timeout ACCEL=$accel 
$RUNTIME_arch_run $kernel -smp $smp $opts"
+    echo "TESTNAME=$testname TIMEOUT=$timeout MACHINE=$machine 
ACCEL=$accel $RUNTIME_arch_run $kernel -smp $smp $opts"

   }
   skip_nodefault()
@@ -80,9 +80,10 @@ function run()
   local kernel="$4"
   local opts="$5"
   local arch="$6"
-    local check="${CHECK:-$7}"
-    local accel="$8"
-    local timeout="${9:-$TIMEOUT}" # unittests.cfg overrides the default
+    local machine="$7"
+    local check="${CHECK:-$8}"
+    local accel="$9"
+    local timeout="${10:-$TIMEOUT}" # unittests.cfg overrides the default
   if [ "${CONFIG_EFI}" == "y" ]; then
   kernel=${kernel/%.flat/.efi}
@@ -116,6 +117,13 @@ function run()
   return 2
   fi
+    if [ -n "$machine" ] && [ -n "$MACHINE" ] && [ "$machine" != 
"$MACHINE" ]; then

+    print_result "SKIP" $testname "" "$m

Re: [kvm-unit-tests PATCH v9 07/31] scripts: allow machine option to be specified in unittests.cfg

[Thomas Huth]

On 08/05/2024 14.27, Nicholas Piggin wrote:

On Wed May 8, 2024 at 1:08 AM AEST, Thomas Huth wrote:

On 04/05/2024 14.28, Nicholas Piggin wrote:

This allows different machines with different requirements to be
supported by run_tests.sh, similarly to how different accelerators
are handled.

Acked-by: Thomas Huth 
Acked-by: Andrew Jones 
Signed-off-by: Nicholas Piggin 
---
   docs/unittests.txt   |  7 +++
   scripts/common.bash  |  8 ++--
   scripts/runtime.bash | 16 
   3 files changed, 25 insertions(+), 6 deletions(-)

diff --git a/docs/unittests.txt b/docs/unittests.txt
index 7cf2c55ad..6449efd78 100644
--- a/docs/unittests.txt
+++ b/docs/unittests.txt
@@ -42,6 +42,13 @@ For / directories that support multiple architectures, 
this restricts
   the test to the specified arch. By default, the test will run on any
   architecture.
   
+machine

+---
+For those architectures that support multiple machine types, this restricts
+the test to the specified machine. By default, the test will run on
+any machine type. (Note, the machine can be specified with the MACHINE=
+environment variable, and defaults to the architecture's default.)
+
   smp
   ---
   smp = 
diff --git a/scripts/common.bash b/scripts/common.bash
index 5e9ad53e2..3aa557c8c 100644
--- a/scripts/common.bash
+++ b/scripts/common.bash
@@ -10,6 +10,7 @@ function for_each_unittest()
local opts
local groups
local arch
+   local machine
local check
local accel
local timeout
@@ -21,7 +22,7 @@ function for_each_unittest()
if [[ "$line" =~ ^\[(.*)\]$ ]]; then
rematch=${BASH_REMATCH[1]}
if [ -n "${testname}" ]; then
-   $(arch_cmd) "$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" 
"$check" "$accel" "$timeout"
+   $(arch_cmd) "$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" 
"$machine" "$check" "$accel" "$timeout"
fi
testname=$rematch
smp=1
@@ -29,6 +30,7 @@ function for_each_unittest()
opts=""
groups=""
arch=""
+   machine=""
check=""
accel=""
timeout=""
@@ -58,6 +60,8 @@ function for_each_unittest()
groups=${BASH_REMATCH[1]}
elif [[ $line =~ ^arch\ *=\ *(.*)$ ]]; then
arch=${BASH_REMATCH[1]}
+   elif [[ $line =~ ^machine\ *=\ *(.*)$ ]]; then
+   machine=${BASH_REMATCH[1]}
elif [[ $line =~ ^check\ *=\ *(.*)$ ]]; then
check=${BASH_REMATCH[1]}
elif [[ $line =~ ^accel\ *=\ *(.*)$ ]]; then
@@ -67,7 +71,7 @@ function for_each_unittest()
fi
done
if [ -n "${testname}" ]; then
-   $(arch_cmd) "$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" 
"$accel" "$timeout"
+   $(arch_cmd) "$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$machine" 
"$check" "$accel" "$timeout"
fi
exec {fd}<&-
   }
diff --git a/scripts/runtime.bash b/scripts/runtime.bash
index 177b62166..0c96d6ea2 100644
--- a/scripts/runtime.bash
+++ b/scripts/runtime.bash
@@ -32,7 +32,7 @@ premature_failure()
   get_cmdline()
   {
   local kernel=$1
-echo "TESTNAME=$testname TIMEOUT=$timeout ACCEL=$accel $RUNTIME_arch_run 
$kernel -smp $smp $opts"
+echo "TESTNAME=$testname TIMEOUT=$timeout MACHINE=$machine ACCEL=$accel 
$RUNTIME_arch_run $kernel -smp $smp $opts"
   }
   
   skip_nodefault()

@@ -80,9 +80,10 @@ function run()
   local kernel="$4"
   local opts="$5"
   local arch="$6"
-local check="${CHECK:-$7}"
-local accel="$8"
-local timeout="${9:-$TIMEOUT}" # unittests.cfg overrides the default
+local machine="$7"
+local check="${CHECK:-$8}"
+local accel="$9"
+local timeout="${10:-$TIMEOUT}" # unittests.cfg overrides the default
   
   if [ "${CONFIG_EFI}" == "y" ]; then

   kernel=${kernel/%.flat/.efi}
@@ -116,6 +117,13 @@ function run()
   return 2
   fi
  

Re: [kvm-unit-tests PATCH v9 07/31] scripts: allow machine option to be specified in unittests.cfg

[Thomas Huth]

On 04/05/2024 14.28, Nicholas Piggin wrote:

This allows different machines with different requirements to be
supported by run_tests.sh, similarly to how different accelerators
are handled.

Acked-by: Thomas Huth 
Acked-by: Andrew Jones 
Signed-off-by: Nicholas Piggin 
---
  docs/unittests.txt   |  7 +++
  scripts/common.bash  |  8 ++--
  scripts/runtime.bash | 16 
  3 files changed, 25 insertions(+), 6 deletions(-)

diff --git a/docs/unittests.txt b/docs/unittests.txt
index 7cf2c55ad..6449efd78 100644
--- a/docs/unittests.txt
+++ b/docs/unittests.txt
@@ -42,6 +42,13 @@ For / directories that support multiple architectures, 
this restricts
  the test to the specified arch. By default, the test will run on any
  architecture.
  
+machine

+---
+For those architectures that support multiple machine types, this restricts
+the test to the specified machine. By default, the test will run on
+any machine type. (Note, the machine can be specified with the MACHINE=
+environment variable, and defaults to the architecture's default.)
+
  smp
  ---
  smp = 
diff --git a/scripts/common.bash b/scripts/common.bash
index 5e9ad53e2..3aa557c8c 100644
--- a/scripts/common.bash
+++ b/scripts/common.bash
@@ -10,6 +10,7 @@ function for_each_unittest()
local opts
local groups
local arch
+   local machine
local check
local accel
local timeout
@@ -21,7 +22,7 @@ function for_each_unittest()
if [[ "$line" =~ ^\[(.*)\]$ ]]; then
rematch=${BASH_REMATCH[1]}
if [ -n "${testname}" ]; then
-   $(arch_cmd) "$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" 
"$check" "$accel" "$timeout"
+   $(arch_cmd) "$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" 
"$machine" "$check" "$accel" "$timeout"
fi
testname=$rematch
smp=1
@@ -29,6 +30,7 @@ function for_each_unittest()
opts=""
groups=""
arch=""
+   machine=""
check=""
accel=""
timeout=""
@@ -58,6 +60,8 @@ function for_each_unittest()
groups=${BASH_REMATCH[1]}
elif [[ $line =~ ^arch\ *=\ *(.*)$ ]]; then
arch=${BASH_REMATCH[1]}
+   elif [[ $line =~ ^machine\ *=\ *(.*)$ ]]; then
+   machine=${BASH_REMATCH[1]}
elif [[ $line =~ ^check\ *=\ *(.*)$ ]]; then
check=${BASH_REMATCH[1]}
elif [[ $line =~ ^accel\ *=\ *(.*)$ ]]; then
@@ -67,7 +71,7 @@ function for_each_unittest()
fi
done
if [ -n "${testname}" ]; then
-   $(arch_cmd) "$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" 
"$accel" "$timeout"
+   $(arch_cmd) "$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$machine" 
"$check" "$accel" "$timeout"
fi
exec {fd}<&-
  }
diff --git a/scripts/runtime.bash b/scripts/runtime.bash
index 177b62166..0c96d6ea2 100644
--- a/scripts/runtime.bash
+++ b/scripts/runtime.bash
@@ -32,7 +32,7 @@ premature_failure()
  get_cmdline()
  {
  local kernel=$1
-echo "TESTNAME=$testname TIMEOUT=$timeout ACCEL=$accel $RUNTIME_arch_run 
$kernel -smp $smp $opts"
+echo "TESTNAME=$testname TIMEOUT=$timeout MACHINE=$machine ACCEL=$accel 
$RUNTIME_arch_run $kernel -smp $smp $opts"
  }
  
  skip_nodefault()

@@ -80,9 +80,10 @@ function run()
  local kernel="$4"
  local opts="$5"
  local arch="$6"
-local check="${CHECK:-$7}"
-local accel="$8"
-local timeout="${9:-$TIMEOUT}" # unittests.cfg overrides the default
+local machine="$7"
+local check="${CHECK:-$8}"
+local accel="$9"
+local timeout="${10:-$TIMEOUT}" # unittests.cfg overrides the default
  
  if [ "${CONFIG_EFI}" == "y" ]; then

  kernel=${kernel/%.flat/.efi}
@@ -116,6 +117,13 @@ function run()
  return 2
  fi
  
+if [ -n "$machine" ] && [ -n "$MACHINE" ] && [ "$machine" != "$MACHINE&quo

Re: [kvm-unit-tests PATCH v9 17/31] powerpc: Add cpu_relax

[Thomas Huth]

On 04/05/2024 14.28, Nicholas Piggin wrote:

Add a cpu_relax variant that uses SMT priority nop instructions like
Linux. This was split out of the SMP patch because it affects the sprs
test case.

Signed-off-by: Nicholas Piggin 
---
  lib/ppc64/asm/barrier.h | 1 +
  powerpc/sprs.c  | 4 ++--
  2 files changed, 3 insertions(+), 2 deletions(-)



Reviewed-by: Thomas Huth 

Re: [kvm-unit-tests PATCH v9 12/31] powerpc: general interrupt tests

[Thomas Huth]

On 04/05/2024 14.28, Nicholas Piggin wrote:

Add basic testing of various kinds of interrupts, machine check,
page fault, illegal, decrementer, trace, syscall, etc.

This has a known failure on QEMU TCG pseries machines where MSR[ME]
can be incorrectly set to 0.

Signed-off-by: Nicholas Piggin 
---
  lib/powerpc/asm/processor.h |   4 +
  lib/powerpc/asm/reg.h   |  17 ++
  lib/powerpc/setup.c |  11 +
  lib/ppc64/asm/ptrace.h  |  16 ++
  powerpc/Makefile.common |   3 +-
  powerpc/interrupts.c| 414 
  powerpc/unittests.cfg   |   3 +
  7 files changed, 467 insertions(+), 1 deletion(-)
  create mode 100644 powerpc/interrupts.c


Acked-by: Thomas Huth 

Re: [kvm-unit-tests PATCH v9 03/31] powerpc: Mark known failing tests as kfail

[Thomas Huth]

On 07/05/2024 06.07, Nicholas Piggin wrote:

On Mon May 6, 2024 at 5:37 PM AEST, Thomas Huth wrote:

On 04/05/2024 14.28, Nicholas Piggin wrote:

Mark the failing h_cede_tm and spapr_vpa tests as kfail.

Signed-off-by: Nicholas Piggin 
---
   powerpc/spapr_vpa.c | 3 ++-
   powerpc/tm.c| 3 ++-
   2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/powerpc/spapr_vpa.c b/powerpc/spapr_vpa.c
index c2075e157..46fa0485c 100644
--- a/powerpc/spapr_vpa.c
+++ b/powerpc/spapr_vpa.c
@@ -150,7 +150,8 @@ static void test_vpa(void)
report_fail("Could not deregister after registration");
   
   	disp_count1 = be32_to_cpu(vpa->vp_dispatch_count);

-   report(disp_count1 % 2 == 1, "Dispatch count is odd after deregister");
+   /* TCG known fail, could be wrong test, must verify against PowerVM */
+   report_kfail(true, disp_count1 % 2 == 1, "Dispatch count is odd after 
deregister");


Using "true" as first argument looks rather pointless - then you could also
simply delete the test completely if it can never be tested reliably.

Thus could you please introduce a helper function is_tcg() that could be
used to check whether we run under TCG (and not KVM)? I think you could
check for "linux,kvm" in the "compatible" property in /hypervisor in the
device tree to see whether we're running in KVM mode or in TCG mode.


This I added in patch 30.

The reason for the suboptimal patch ordering was just me being lazy and
avoiding rebasing annoyance. I'd written a bunch of failing test cases
for QEMU work, but hadn't done the kvm/tcg test yet. It had a few
conflicts so I put it at the end... can rebase if you'd really prefer.


Ah, ok, no need to rebase then, as long it's there in the end, it's fine.

 Thanks,
  Thomas

Re: [kvm-unit-tests PATCH v9 02/31] report: Add known failure reporting option

[Thomas Huth]

On 06/05/2024 10.01, Andrew Jones wrote:

On Mon, May 06, 2024 at 09:25:37AM GMT, Thomas Huth wrote:

On 04/05/2024 14.28, Nicholas Piggin wrote:

There are times we would like to test a function that is known to fail
in some conditions due to a bug in implementation (QEMU, KVM, or even
hardware). It would be nice to count these as known failures and not
report a summary failure.

xfail is not the same thing, xfail means failure is required and a pass
causes the test to fail. So add kfail for known failures.


Actually, I wonder whether that's not rather a bug in report_xfail()
instead. Currently, when you call report_xfail(true, ...), the result is
*always* counted as a failure, either as an expected failure (if the test
really failed), or as a normal failure (if the test succeeded). What's the
point of counting a successful test as a failure??

Andrew, you've originally introduced report_xfail in commit a5af7b8a67e,
could you please comment on this?



An expected failure passes when the test fails and fails when the test
passes, i.e.

   XFAIL == PASS (but separately accounted with 'xfailures')
   XPASS == FAIL

If we expect something to fail and it passes then this may be due to the
thing being fixed, so we should change the test to expect success, or
due to the test being written incorrectly for our expectations. Either
way, when an expected failure doesn't fail, it means our expectations are
wrong and we need to be alerted to that, hence a FAIL is reported.


Ok, so this was on purpose, indeed. Maybe we should add this information in 
a comment right in front of the function, so that others don't scratch their 
head, too?


Anyway, this patch here is fine then:
Reviewed-by: Thomas Huth 

Re: [kvm-unit-tests PATCH v9 03/31] powerpc: Mark known failing tests as kfail

[Thomas Huth]

On 04/05/2024 14.28, Nicholas Piggin wrote:

Mark the failing h_cede_tm and spapr_vpa tests as kfail.

Signed-off-by: Nicholas Piggin 
---
  powerpc/spapr_vpa.c | 3 ++-
  powerpc/tm.c| 3 ++-
  2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/powerpc/spapr_vpa.c b/powerpc/spapr_vpa.c
index c2075e157..46fa0485c 100644
--- a/powerpc/spapr_vpa.c
+++ b/powerpc/spapr_vpa.c
@@ -150,7 +150,8 @@ static void test_vpa(void)
report_fail("Could not deregister after registration");
  
  	disp_count1 = be32_to_cpu(vpa->vp_dispatch_count);

-   report(disp_count1 % 2 == 1, "Dispatch count is odd after deregister");
+   /* TCG known fail, could be wrong test, must verify against PowerVM */
+   report_kfail(true, disp_count1 % 2 == 1, "Dispatch count is odd after 
deregister");


Using "true" as first argument looks rather pointless - then you could also 
simply delete the test completely if it can never be tested reliably.


Thus could you please introduce a helper function is_tcg() that could be 
used to check whether we run under TCG (and not KVM)? I think you could 
check for "linux,kvm" in the "compatible" property in /hypervisor in the 
device tree to see whether we're running in KVM mode or in TCG mode.



report_prefix_pop();
  }
diff --git a/powerpc/tm.c b/powerpc/tm.c
index 6b1ceeb6e..d9e7f455d 100644
--- a/powerpc/tm.c
+++ b/powerpc/tm.c
@@ -133,7 +133,8 @@ int main(int argc, char **argv)
report_skip("TM is not available");
goto done;
}
-   report(cpus_with_tm == nr_cpus,
+   /* KVM does not report TM in secondary threads in POWER9 */
+   report_kfail(true, cpus_with_tm == nr_cpus,
   "TM available in all 'ibm,pa-features' properties");


Could you check the PVR for POWER9 here instead of using "true" as first 
parameter?


 Thomas

Re: [kvm-unit-tests PATCH v9 02/31] report: Add known failure reporting option

[Thomas Huth]

On 04/05/2024 14.28, Nicholas Piggin wrote:

There are times we would like to test a function that is known to fail
in some conditions due to a bug in implementation (QEMU, KVM, or even
hardware). It would be nice to count these as known failures and not
report a summary failure.

xfail is not the same thing, xfail means failure is required and a pass
causes the test to fail. So add kfail for known failures.


Actually, I wonder whether that's not rather a bug in report_xfail() 
instead. Currently, when you call report_xfail(true, ...), the result is 
*always* counted as a failure, either as an expected failure (if the test 
really failed), or as a normal failure (if the test succeeded). What's the 
point of counting a successful test as a failure??


Andrew, you've originally introduced report_xfail in commit a5af7b8a67e, 
could you please comment on this?


IMHO we should rather do something like this instead:

diff --git a/lib/report.c b/lib/report.c
--- a/lib/report.c
+++ b/lib/report.c
@@ -98,7 +98,7 @@ static void va_report(const char *msg_fmt,
skipped++;
else if (xfail && !pass)
xfailures++;
-   else if (xfail || !pass)
+   else if (!xfail && !pass)
failures++;

spin_unlock();

 Thomas

Re: [kvm-unit-tests PATCH v9 01/31] doc: update unittests doc

[Thomas Huth]

On 04/05/2024 14.28, Nicholas Piggin wrote:

This adds a few minor fixes.

Signed-off-by: Nicholas Piggin 
---
  docs/unittests.txt | 12 +++-
  1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/docs/unittests.txt b/docs/unittests.txt
index 3192a60ec..7cf2c55ad 100644
--- a/docs/unittests.txt
+++ b/docs/unittests.txt
@@ -15,8 +15,8 @@ unittests.cfg format
  
  # is the comment symbol, all following contents of the line is ignored.
  
-Each unit test is defined with a [unit-test-name] line, followed by

-a set of parameters that control how the test case is run. The name is
+Each unit test is defined with a [unit-test-name] line, followed by a
+set of parameters that control how the test case is run. The name is
  arbitrary and appears in the status reporting output.
  
  Parameters appear on their own lines under the test name, and have a

@@ -62,8 +62,8 @@ groups
  groups =   ...
  
  Used to group the test cases for the `run_tests.sh -g ...` run group

-option. Adding a test to the nodefault group will cause it to not be
-run by default.
+option. The group name is arbitrary, aside from the nodefault group
+which makes the test to not be run by default.
  
  accel

  -
@@ -82,8 +82,10 @@ Optional timeout in seconds, after which the test will be 
killed and fail.
  
  check

  -
-check = =<
+check = =
  
  Check a file for a particular value before running a test. The check line

  can contain multiple files to check separated by a space, but each check
  parameter needs to be of the form =
+
+The path and value can not contain space, =, or shell wildcard characters.


Could you comment on my feedback here, please:

 https://lore.kernel.org/kvm/951ccd88-0e39-4379-8d86-718e72594...@redhat.com/

 Thanks,
  Thomas

Re: [kvm-unit-tests PATCH v8 06/35] gitlab-ci: Run migration selftest on s390x and powerpc

[Thomas Huth]

On 16/04/2024 09.55, Thomas Huth wrote:

On 16/04/2024 09.18, Thomas Huth wrote:

On 11/04/2024 21.22, Thomas Huth wrote:

On 08/04/2024 18.06, Nico Boehr wrote:

Quoting Nicholas Piggin (2024-04-05 10:35:07)

The migration harness is complicated and easy to break so CI will
be helpful.

Signed-off-by: Nicholas Piggin 
---
  .gitlab-ci.yml  | 32 +++-
  s390x/unittests.cfg |  8 
  2 files changed, 31 insertions(+), 9 deletions(-)

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index ff34b1f50..60b3cdfd2 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml

[...]

@@ -135,7 +147,7 @@ build-riscv64:
  build-s390x:
   extends: .outoftree_template
   script:
- - dnf install -y qemu-system-s390x gcc-s390x-linux-gnu
+ - dnf install -y qemu-system-s390x gcc-s390x-linux-gnu nmap-ncat
   - mkdir build
   - cd build
   - ../configure --arch=s390x --cross-prefix=s390x-linux-gnu-
@@ -161,6 +173,8 @@ build-s390x:
    sclp-1g
    sclp-3g
    selftest-setup
+  selftest-migration-kvm


We're running under TCG in the Gitlab CI. I'm a little bit confused why
we're running a KVM-only test here.


The build-s390x job is TCG, indeed, but we have the "s390x-kvm" job that 
runs on a KVM-capable s390x host, so it could be added there?


I now gave it a try and it seems to work, so I updated this patch and 
pushed it to the repository now.


Hmm, "selftest-migration" now was failing once here:

  https://gitlab.com/kvm-unit-tests/kvm-unit-tests/-/jobs/6633865591

Let's keep an eye on it, and if it is not stable enough, we might need to 
disable it in the CI again...


And it just failed again:

 https://gitlab.com/kvm-unit-tests/kvm-unit-tests/-/jobs/6635395811

... not sure whether this is due to the slow CI machine or whether there is 
still a bug lurking around somewhere, but anyway, I disabled it now for the 
CI again to avoid that other MRs get affected.


 Thomas

Re: [kvm-unit-tests PATCH v8 11/35] powerpc/sprs: Specify SPRs with data rather than code

[Thomas Huth]

On 05/04/2024 10.35, Nicholas Piggin wrote:

A significant rework that builds an array of 'struct spr', where each
element describes an SPR. This makes various metadata about the SPR
like name and access type easier to carry and use.

Hypervisor privileged registers are described despite not being used
at the moment for completeness, but also the code might one day be
reused for a hypervisor-privileged test.

Acked-by: Thomas Huth 
Signed-off-by: Nicholas Piggin 
---
  lib/powerpc/asm/reg.h |   2 +
  powerpc/sprs.c| 647 +-
  2 files changed, 457 insertions(+), 192 deletions(-)


I tried to merge this patch, but it seems to break running the tests on 
Ubuntu Focal with Clang 11 in the Travis-CI:


 https://app.travis-ci.com/github/huth/kvm-unit-tests/jobs/620577246

Could you please have a look?

 Thanks,
  Thomas

Re: [kvm-unit-tests PATCH v8 06/35] gitlab-ci: Run migration selftest on s390x and powerpc

[Thomas Huth]

On 16/04/2024 09.18, Thomas Huth wrote:

On 11/04/2024 21.22, Thomas Huth wrote:

On 08/04/2024 18.06, Nico Boehr wrote:

Quoting Nicholas Piggin (2024-04-05 10:35:07)

The migration harness is complicated and easy to break so CI will
be helpful.

Signed-off-by: Nicholas Piggin 
---
  .gitlab-ci.yml  | 32 +++-
  s390x/unittests.cfg |  8 
  2 files changed, 31 insertions(+), 9 deletions(-)

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index ff34b1f50..60b3cdfd2 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml

[...]

@@ -135,7 +147,7 @@ build-riscv64:
  build-s390x:
   extends: .outoftree_template
   script:
- - dnf install -y qemu-system-s390x gcc-s390x-linux-gnu
+ - dnf install -y qemu-system-s390x gcc-s390x-linux-gnu nmap-ncat
   - mkdir build
   - cd build
   - ../configure --arch=s390x --cross-prefix=s390x-linux-gnu-
@@ -161,6 +173,8 @@ build-s390x:
    sclp-1g
    sclp-3g
    selftest-setup
+  selftest-migration-kvm


We're running under TCG in the Gitlab CI. I'm a little bit confused why
we're running a KVM-only test here.


The build-s390x job is TCG, indeed, but we have the "s390x-kvm" job that 
runs on a KVM-capable s390x host, so it could be added there?


I now gave it a try and it seems to work, so I updated this patch and pushed 
it to the repository now.


Hmm, "selftest-migration" now was failing once here:

 https://gitlab.com/kvm-unit-tests/kvm-unit-tests/-/jobs/6633865591

Let's keep an eye on it, and if it is not stable enough, we might need to 
disable it in the CI again...


 Thomas

Re: [kvm-unit-tests PATCH v8 10/35] powerpc: interrupt stack backtracing

[Thomas Huth]

On 05/04/2024 10.35, Nicholas Piggin wrote:

Add support for backtracing across interrupt stacks, and add
interrupt frame backtrace for unhandled interrupts.

This requires a back-chain created from initial interrupt stack
frame to the r1 value of the interrupted context. A label is
added at the return location of the exception handler call, so
the unwinder can recognize the initial interrupt frame.

The additional cstart entry-frame is no longer required because
the unwinder now looks for frame == 0 as well as address == 0.

Signed-off-by: Nicholas Piggin 
---


Acked-by: Thomas Huth 

Re: [kvm-unit-tests PATCH v8 06/35] gitlab-ci: Run migration selftest on s390x and powerpc

[Thomas Huth]

On 11/04/2024 21.22, Thomas Huth wrote:

On 08/04/2024 18.06, Nico Boehr wrote:

Quoting Nicholas Piggin (2024-04-05 10:35:07)

The migration harness is complicated and easy to break so CI will
be helpful.

Signed-off-by: Nicholas Piggin 
---
  .gitlab-ci.yml  | 32 +++-
  s390x/unittests.cfg |  8 
  2 files changed, 31 insertions(+), 9 deletions(-)

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index ff34b1f50..60b3cdfd2 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml

[...]

@@ -135,7 +147,7 @@ build-riscv64:
  build-s390x:
   extends: .outoftree_template
   script:
- - dnf install -y qemu-system-s390x gcc-s390x-linux-gnu
+ - dnf install -y qemu-system-s390x gcc-s390x-linux-gnu nmap-ncat
   - mkdir build
   - cd build
   - ../configure --arch=s390x --cross-prefix=s390x-linux-gnu-
@@ -161,6 +173,8 @@ build-s390x:
    sclp-1g
    sclp-3g
    selftest-setup
+  selftest-migration-kvm


We're running under TCG in the Gitlab CI. I'm a little bit confused why
we're running a KVM-only test here.


The build-s390x job is TCG, indeed, but we have the "s390x-kvm" job that 
runs on a KVM-capable s390x host, so it could be added there?


I now gave it a try and it seems to work, so I updated this patch and pushed 
it to the repository now.


 Thomas

Re: [kvm-unit-tests PATCH v8 09/35] powerpc: Fix stack backtrace termination

[Thomas Huth]

On 05/04/2024 10.35, Nicholas Piggin wrote:

The backtrace handler terminates when it sees a NULL caller address,
but the powerpc stack setup does not keep such a NULL caller frame
at the start of the stack.

This happens to work on pseries because the memory at 0 is mapped and
it contains 0 at the location of the return address pointer if it
were a stack frame. But this is fragile, and does not work with powernv
where address 0 contains firmware instructions.

Use the existing dummy frame on stack as the NULL caller, and create a
new frame on stack for the entry code.

Signed-off-by: Nicholas Piggin 
---
  powerpc/cstart64.S | 15 +++
  1 file changed, 15 insertions(+)


Reviewed-by: Thomas Huth 

Re: [kvm-unit-tests PATCH v8 05/35] arch-run: Add a "continuous" migration option for tests

[Thomas Huth]

On 05/04/2024 10.35, Nicholas Piggin wrote:

The cooperative migration protocol is very good to control precise
pre and post conditions for a migration event. However in some cases
its intrusiveness to the test program, can mask problems and make
analysis more difficult.

For example to stress test migration vs concurrent complicated
memory access, including TLB refill, ram dirtying, etc., then the
tight spin at getchar() and resumption of the workload after
migration is unhelpful.

This adds a continuous migration mode that directs the harness to
perform migrations continually. This is added to the migration
selftests, which also sees cooperative migration iterations reduced
to avoid increasing test time too much.

Signed-off-by: Nicholas Piggin 
---
  common/selftest-migration.c | 16 +--
  lib/migrate.c   | 18 
  lib/migrate.h   |  3 ++
  scripts/arch-run.bash   | 55 -
  4 files changed, 82 insertions(+), 10 deletions(-)


Reviewed-by: Thomas Huth 

Re: [kvm-unit-tests PATCH v8 03/35] migration: Add a migrate_skip command

[Thomas Huth]

On 16/04/2024 05.22, Nicholas Piggin wrote:

On Tue Apr 9, 2024 at 1:59 AM AEST, Nico Boehr wrote:

Quoting Nicholas Piggin (2024-04-05 10:35:04)
[...]

diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash
index 39419d4e2..4a1aab48d 100644
--- a/scripts/arch-run.bash
+++ b/scripts/arch-run.bash

[...]

@@ -179,8 +189,11 @@ run_migration ()
 # Wait for test exit or further migration messages.
 if ! seen_migrate_msg ${src_out} ;  then
 sleep 0.1
-   else
+   elif grep -q "Now migrate the VM" < ${src_out} ; then
 do_migration || return $?
+   elif [ $skip_migration -eq 0 ] && grep -q "Skipped VM migration" 
< ${src_out} ; then
+   echo > ${src_infifo} # Resume src and carry on.
+   break;


If I understand the code correctly, this simply makes the test PASS when
migration is skipped, am I wrong?


This just gets the harness past the wait-for-migration phase, it
otherwise should not change behaviour.


If so, can we set ret=77 here so we get a nice SKIP?


The harness _should_ still scan the status value printed by the
test case when it exits. Is it not working as expected? We
certainly should be able to make it SKIP.


I just gave it a try (by modifying the selftest-migration-skip test 
accordingly), and it seems to work fine, so I think this patch here should 
be ok.


 Thomas

Re: [kvm-unit-tests PATCH v8 05/35] arch-run: Add a "continuous" migration option for tests

[Thomas Huth]

On 05/04/2024 10.35, Nicholas Piggin wrote:

The cooperative migration protocol is very good to control precise
pre and post conditions for a migration event. However in some cases
its intrusiveness to the test program, can mask problems and make
analysis more difficult.

For example to stress test migration vs concurrent complicated
memory access, including TLB refill, ram dirtying, etc., then the
tight spin at getchar() and resumption of the workload after
migration is unhelpful.

This adds a continuous migration mode that directs the harness to
perform migrations continually. This is added to the migration
selftests, which also sees cooperative migration iterations reduced
to avoid increasing test time too much.

Signed-off-by: Nicholas Piggin 
---
  common/selftest-migration.c | 16 +--
  lib/migrate.c   | 18 
  lib/migrate.h   |  3 ++
  scripts/arch-run.bash   | 55 -
  4 files changed, 82 insertions(+), 10 deletions(-)


Reviewed-by: Thomas Huth 

Re: [kvm-unit-tests PATCH v8 06/35] gitlab-ci: Run migration selftest on s390x and powerpc

[Thomas Huth]

On 08/04/2024 18.06, Nico Boehr wrote:

Quoting Nicholas Piggin (2024-04-05 10:35:07)

The migration harness is complicated and easy to break so CI will
be helpful.

Signed-off-by: Nicholas Piggin 
---
  .gitlab-ci.yml  | 32 +++-
  s390x/unittests.cfg |  8 
  2 files changed, 31 insertions(+), 9 deletions(-)

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index ff34b1f50..60b3cdfd2 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml

[...]

@@ -135,7 +147,7 @@ build-riscv64:
  build-s390x:
   extends: .outoftree_template
   script:
- - dnf install -y qemu-system-s390x gcc-s390x-linux-gnu
+ - dnf install -y qemu-system-s390x gcc-s390x-linux-gnu nmap-ncat
   - mkdir build
   - cd build
   - ../configure --arch=s390x --cross-prefix=s390x-linux-gnu-
@@ -161,6 +173,8 @@ build-s390x:
sclp-1g
sclp-3g
selftest-setup
+  selftest-migration-kvm


We're running under TCG in the Gitlab CI. I'm a little bit confused why
we're running a KVM-only test here.


The build-s390x job is TCG, indeed, but we have the "s390x-kvm" job that 
runs on a KVM-capable s390x host, so it could be added there?


 Thomas

Re: [kvm-unit-tests PATCH v7 07/35] common: add memory dirtying vs migration test

[Thomas Huth]

On 19/03/2024 08.58, Nicholas Piggin wrote:

This test stores to a bunch of pages and verifies previous stores,
while being continually migrated. Default runtime is 5 seconds.

Add this test to ppc64 and s390x builds. This can fail due to a QEMU
TCG physical memory dirty bitmap bug, so it is not enabled in unittests
for TCG yet.

The selftest-migration test time is reduced significantly because
this test

Signed-off-by: Nicholas Piggin 
---
  common/memory-verify.c  | 67 +
  common/selftest-migration.c |  8 ++---
  powerpc/Makefile.common |  1 +
  powerpc/memory-verify.c |  1 +
  powerpc/unittests.cfg   |  7 
  s390x/Makefile  |  1 +
  s390x/memory-verify.c   |  1 +
  s390x/unittests.cfg |  6 
  8 files changed, 88 insertions(+), 4 deletions(-)
  create mode 100644 common/memory-verify.c
  create mode 12 powerpc/memory-verify.c
  create mode 12 s390x/memory-verify.c

diff --git a/common/memory-verify.c b/common/memory-verify.c
new file mode 100644
index 0..e78fb4338
--- /dev/null
+++ b/common/memory-verify.c
@@ -0,0 +1,67 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Simple memory verification test, used to exercise dirty memory migration.
+ */
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define NR_PAGES 32
+
+static unsigned time_sec = 5;
+
+static void do_getopts(int argc, char **argv)
+{
+   int i;
+
+   for (i = 0; i < argc; ++i) {
+   if (strcmp(argv[i], "-t") == 0) {
+   i++;
+   if (i == argc)
+   break;
+   time_sec = atol(argv[i]);
+   }
+   }
+
+   printf("running for %d secs\n", time_sec);
+}
+
+int main(int argc, char **argv)
+{
+   void *mem = malloc(NR_PAGES*PAGE_SIZE);


Use alloc_pages(5) instead ? Or add at least some white spaces around "*".

Apart from that this patch looks sane to me, so with that line fixed:
Reviewed-by: Thomas Huth 

Re: [kvm-unit-tests PATCH v7 06/35] gitlab-ci: Run migration selftest on s390x and powerpc

[Thomas Huth]

On 19/03/2024 08.58, Nicholas Piggin wrote:

The migration harness is complicated and easy to break so CI will
be helpful.

Signed-off-by: Nicholas Piggin 
---
  .gitlab-ci.yml  | 18 +++---
  s390x/unittests.cfg |  8 
  2 files changed, 19 insertions(+), 7 deletions(-)

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index ff34b1f50..bd34da04f 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -92,26 +92,28 @@ build-arm:
  build-ppc64be:
   extends: .outoftree_template
   script:
- - dnf install -y qemu-system-ppc gcc-powerpc64-linux-gnu
+ - dnf install -y qemu-system-ppc gcc-powerpc64-linux-gnu nmap-ncat
   - mkdir build
   - cd build
   - ../configure --arch=ppc64 --endian=big --cross-prefix=powerpc64-linux-gnu-
   - make -j2
   - ACCEL=tcg ./run_tests.sh
- selftest-setup spapr_hcall rtas-get-time-of-day rtas-get-time-of-day-base
- rtas-set-time-of-day emulator
+ selftest-setup selftest-migration selftest-migration-skip spapr_hcall
+ rtas-get-time-of-day rtas-get-time-of-day-base rtas-set-time-of-day


I used to squash as much as possible into one line in the past, but nowadays 
I rather prefer one test per line (like it is done for s390x below), so that 
it is easier to identify the changes ...
So if you like, I think you could also put each test on a separate line here 
now (since you're touching all lines with tests here anyway).



+ emulator
   | tee results.txt
   - if grep -q FAIL results.txt ; then exit 1 ; fi
  
  build-ppc64le:

   extends: .intree_template
   script:
- - dnf install -y qemu-system-ppc gcc-powerpc64-linux-gnu
+ - dnf install -y qemu-system-ppc gcc-powerpc64-linux-gnu nmap-ncat
   - ./configure --arch=ppc64 --endian=little 
--cross-prefix=powerpc64-linux-gnu-
   - make -j2
   - ACCEL=tcg ./run_tests.sh
- selftest-setup spapr_hcall rtas-get-time-of-day rtas-get-time-of-day-base
- rtas-set-time-of-day emulator
+ selftest-setup selftest-migration selftest-migration-skip spapr_hcall
+ rtas-get-time-of-day rtas-get-time-of-day-base rtas-set-time-of-day
+ emulator
   | tee results.txt
   - if grep -q FAIL results.txt ; then exit 1 ; fi
  
@@ -135,7 +137,7 @@ build-riscv64:

  build-s390x:
   extends: .outoftree_template
   script:
- - dnf install -y qemu-system-s390x gcc-s390x-linux-gnu
+ - dnf install -y qemu-system-s390x gcc-s390x-linux-gnu nmap-ncat
   - mkdir build
   - cd build
   - ../configure --arch=s390x --cross-prefix=s390x-linux-gnu-
@@ -161,6 +163,8 @@ build-s390x:
sclp-1g
sclp-3g
selftest-setup
+  selftest-migration-kvm
+  selftest-migration-skip
sieve
smp
stsi
diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
index 49e3e4608..b79b99416 100644
--- a/s390x/unittests.cfg
+++ b/s390x/unittests.cfg
@@ -31,6 +31,14 @@ groups = selftest migration
  # 
https://lore.kernel.org/qemu-devel/20240219061731.232570-1-npig...@gmail.com/
  accel = kvm
  
+[selftest-migration-kvm]

+file = selftest-migration.elf
+groups = nodefault
+accel = kvm
+# This is a special test for gitlab-ci that can must not use TCG until the


"can" or "must"?


+# TCG migration fix has made its way into CI environment's QEMU.
+# https://lore.kernel.org/qemu-devel/20240219061731.232570-1-npig...@gmail.com/
+
  [selftest-migration-skip]
  file = selftest-migration.elf
  groups = selftest migration


 Thomas

Re: [kvm-unit-tests PATCH v7 01/35] arch-run: Add functions to help handle migration directives from test

[Thomas Huth]

On 19/03/2024 08.58, Nicholas Piggin wrote:

The migration harness will be expanded to deal with more commands
from the test, moving these checks into functions helps keep things
managable.

Signed-off-by: Nicholas Piggin 
---
  scripts/arch-run.bash | 20 +++-
  1 file changed, 15 insertions(+), 5 deletions(-)


Reviewed-by: Thomas Huth 

Re: [kvm-unit-tests PATCH 03/32] powerpc: Fix stack backtrace termination

[Thomas Huth]

On 05/03/2024 07.29, Nicholas Piggin wrote:

On Tue Feb 27, 2024 at 6:50 PM AEST, Thomas Huth wrote:

On 26/02/2024 11.11, Nicholas Piggin wrote:

...

/* save DTB pointer */
-   std r3, 56(r1)
+   SAVE_GPR(3,r1)


Isn't SAVE_GPR rather meant for the interrupt frame, not for the normal C
calling convention frames?

Sorry for asking dumb questions ... I still have a hard time understanding
the changes here... :-/


Ah, that was me being lazy and using an interrupt frame for the new
frame.


Ah, ok. It's super-confusing (at least for me) to see an interrupt frame 
here out of no reason... could you please either add proper comments here 
explaining this, or even better switch to a normal stack frame, please?


 Thanks,
  Thomas

Re: [kvm-unit-tests PATCH 7/7] common: add memory dirtying vs migration test

[Thomas Huth]

On 05/03/2024 03.50, Nicholas Piggin wrote:

On Mon Mar 4, 2024 at 4:22 PM AEST, Thomas Huth wrote:

On 26/02/2024 10.38, Nicholas Piggin wrote:

This test stores to a bunch of pages and verifies previous stores,
while being continually migrated. This can fail due to a QEMU TCG
physical memory dirty bitmap bug.


Good idea, but could we then please drop "continuous" test from
selftest-migration.c again? ... having two common tests to exercise the
continuous migration that take quite a bunch of seconds to finish sounds
like a waste of time in the long run to me.


Yeah if you like. I could shorten them up a bit. I did want to have
the selftests for just purely testing the harness with as little
"test" code as possible.


Ok, but then please shorten the selftest to ~ 2 seconds if possible, please.

 Thomas

Re: [kvm-unit-tests PATCH 6/7] gitlab-ci: Run migration selftest on s390x and powerpc

[Thomas Huth]

On 05/03/2024 03.38, Nicholas Piggin wrote:

On Sat Mar 2, 2024 at 12:16 AM AEST, Thomas Huth wrote:

On 26/02/2024 10.38, Nicholas Piggin wrote:

The migration harness is complicated and easy to break so CI will
be helpful.

Signed-off-by: Nicholas Piggin 
---
   .gitlab-ci.yml | 18 +++---
   1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 71d986e98..61f196d5d 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -64,26 +64,28 @@ build-arm:
   build-ppc64be:
extends: .outoftree_template
script:
- - dnf install -y qemu-system-ppc gcc-powerpc64-linux-gnu
+ - dnf install -y qemu-system-ppc gcc-powerpc64-linux-gnu nmap-ncat
- mkdir build
- cd build
- ../configure --arch=ppc64 --endian=big --cross-prefix=powerpc64-linux-gnu-
- make -j2
- ACCEL=tcg ./run_tests.sh
- selftest-setup spapr_hcall rtas-get-time-of-day rtas-get-time-of-day-base
- rtas-set-time-of-day emulator
+ selftest-setup selftest-migration selftest-migration-skip spapr_hcall
+ rtas-get-time-of-day rtas-get-time-of-day-base rtas-set-time-of-day
+ emulator
| tee results.txt
- if grep -q FAIL results.txt ; then exit 1 ; fi
   
   build-ppc64le:

extends: .intree_template
script:
- - dnf install -y qemu-system-ppc gcc-powerpc64-linux-gnu
+ - dnf install -y qemu-system-ppc gcc-powerpc64-linux-gnu nmap-ncat
- ./configure --arch=ppc64 --endian=little 
--cross-prefix=powerpc64-linux-gnu-
- make -j2
- ACCEL=tcg ./run_tests.sh
- selftest-setup spapr_hcall rtas-get-time-of-day rtas-get-time-of-day-base
- rtas-set-time-of-day emulator
+ selftest-setup selftest-migration selftest-migration-skip spapr_hcall
+ rtas-get-time-of-day rtas-get-time-of-day-base rtas-set-time-of-day
+ emulator
| tee results.txt
- if grep -q FAIL results.txt ; then exit 1 ; fi
   
@@ -107,7 +109,7 @@ build-riscv64:

   build-s390x:
extends: .outoftree_template
script:
- - dnf install -y qemu-system-s390x gcc-s390x-linux-gnu
+ - dnf install -y qemu-system-s390x gcc-s390x-linux-gnu nmap-ncat
- mkdir build
- cd build
- ../configure --arch=s390x --cross-prefix=s390x-linux-gnu-
@@ -133,6 +135,8 @@ build-s390x:
 sclp-1g
 sclp-3g
 selftest-setup
+  selftest-migration
+  selftest-migration-skip
 sieve
 smp
 stsi


While I can update the qemu binary for the s390x-kvm job, the build-* jobs
run in a container with a normal QEMU from the corresponding distros, so I
think this has to wait 'til we get distros that contain your QEMU TCG
migration fix.


Okay. powerpc *could* run into the TCG bug too, in practice it has not.
We could try enable it there to get migration into CI, and revert it if
it starts showing random failures?


Fine for me.

 Thomas

Re: [kvm-unit-tests PATCH 14/32] powerpc: general interrupt tests

[Thomas Huth]

On 05/03/2024 03.19, Nicholas Piggin wrote:

On Fri Mar 1, 2024 at 10:41 PM AEST, Thomas Huth wrote:

On 26/02/2024 11.12, Nicholas Piggin wrote:

Add basic testing of various kinds of interrupts, machine check,
page fault, illegal, decrementer, trace, syscall, etc.

This has a known failure on QEMU TCG pseries machines where MSR[ME]
can be incorrectly set to 0.


Two questions out of curiosity:

Any chance that this could be fixed easily in QEMU?


Yes I have a fix on the mailing list. It should get into 9.0 and
probably stable.


Ok, then it's IMHO not worth the effort to make the k-u-t work around this 
bug in older QEMU versions.



Or is there a way to detect TCG from within the test? (for example, we have
a host_is_tcg() function for s390x so we can e.g. use report_xfail() for
tests that are known to fail on TCG there)


I do have a half-done patch which adds exactly this.

One (minor) annoyance is that it doesn't seem possible to detect QEMU
version to add workarounds. E.g., we would like to test the fixed
functionality, but older qemu should not. Maybe that's going too much
into a rabbit hole. We *could* put a QEMU version into device tree
to deal with this though...


No, let's better not do this - hardwired version checks are often a bad 
idea, e.g. when a bug is in QEMU 8.0.0 and 8.1.0, it could be fixed in 8.0.1 
and then it could get really messy with the version checks...


 Thomas

Re: [kvm-unit-tests PATCH 14/32] powerpc: general interrupt tests

[Thomas Huth]

On 05/03/2024 03.30, Nicholas Piggin wrote:

On Fri Mar 1, 2024 at 11:45 PM AEST, Andrew Jones wrote:

On Fri, Mar 01, 2024 at 01:41:22PM +0100, Thomas Huth wrote:

On 26/02/2024 11.12, Nicholas Piggin wrote:

Add basic testing of various kinds of interrupts, machine check,
page fault, illegal, decrementer, trace, syscall, etc.

This has a known failure on QEMU TCG pseries machines where MSR[ME]
can be incorrectly set to 0.


Two questions out of curiosity:

Any chance that this could be fixed easily in QEMU?

Or is there a way to detect TCG from within the test? (for example, we have
a host_is_tcg() function for s390x so we can e.g. use report_xfail() for
tests that are known to fail on TCG there)


If there's nothing better, then it should be possible to check the
QEMU_ACCEL environment variable which will be there with the default
environ.




@@ -0,0 +1,415 @@
+/*
+ * Test interrupts
+ *
+ * Copyright 2024 Nicholas Piggin, IBM Corp.
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.


I know, we're using this line in a lot of source files ... but maybe we
should do better for new files at least: "LGPL, version 2" is a little bit
ambiguous: Does it mean the "Library GPL version 2.0" or the "Lesser GPL
version 2.1"? Maybe you could clarify by additionally providing a SPDX
identifier here, or by explicitly writing 2.0 or 2.1.


Let's only add SPDX identifiers to new files.


+1

Speaking of which, a bunch of these just got inherited from the file
that was copied to begin with (I tried not to remove copyright
notices unless there was really nothing of the original remaining).
So for new code/files, is there any particular preference for the
license to use? I don't much mind between the *GPL*. Looks like almost
all the SPDX code use GPL 2.0 only, but that could be just from
coming from Linux. I might just go with that.


k-u-t were originally licensed under the LGPL, but in the course of time, 
code has been copied from the Linux kernel here and there, so we updated the 
license to GPL 2.
So yes, for code that might have been copied from the kernel, we have to use 
GPL 2. For other code, it's up to the author to chose. (IIRC we once 
discussed that LGPL would be nice for the files in the lib/ directory, while 
GPL is fine for the other folders, but my memory might fail here and it was 
never written in stone anyway)


 Thomas

Re: [kvm-unit-tests PATCH 7/7] common: add memory dirtying vs migration test

[Thomas Huth]

On 26/02/2024 10.38, Nicholas Piggin wrote:

This test stores to a bunch of pages and verifies previous stores,
while being continually migrated. This can fail due to a QEMU TCG
physical memory dirty bitmap bug.


Good idea, but could we then please drop "continuous" test from 
selftest-migration.c again? ... having two common tests to exercise the 
continuous migration that take quite a bunch of seconds to finish sounds 
like a waste of time in the long run to me.



Signed-off-by: Nicholas Piggin 
---
  common/memory-verify.c  | 48 +
  powerpc/Makefile.common |  1 +
  powerpc/memory-verify.c |  1 +
  powerpc/unittests.cfg   |  7 ++
  s390x/Makefile  |  1 +
  s390x/memory-verify.c   |  1 +
  s390x/unittests.cfg |  6 ++
  7 files changed, 65 insertions(+)
  create mode 100644 common/memory-verify.c
  create mode 12 powerpc/memory-verify.c
  create mode 12 s390x/memory-verify.c

diff --git a/common/memory-verify.c b/common/memory-verify.c
new file mode 100644
index 0..7c4ec087b
--- /dev/null
+++ b/common/memory-verify.c
@@ -0,0 +1,48 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Simple memory verification test, used to exercise dirty memory migration.
+ *
+ */
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define NR_PAGES 32
+
+int main(int argc, char **argv)
+{
+   void *mem = malloc(NR_PAGES*PAGE_SIZE);
+   bool success = true;
+   uint64_t ms;
+   long i;
+
+   report_prefix_push("memory");
+
+   memset(mem, 0, NR_PAGES*PAGE_SIZE);
+
+   migrate_begin_continuous();
+   ms = get_clock_ms();
+   i = 0;
+   do {
+   int j;
+
+   for (j = 0; j < NR_PAGES*PAGE_SIZE; j += PAGE_SIZE) {
+   if (*(volatile long *)(mem + j) != i) {
+   success = false;
+   goto out;
+   }
+   *(volatile long *)(mem + j) = i + 1;
+   }
+   i++;
+   } while (get_clock_ms() - ms < 5000);


Maybe add a parameter so that the user can use different values for the 
runtime than always doing 5 seconds?


 Thomas


+out:
+   migrate_end_continuous();
+
+   report(success, "memory verification stress test");
+
+   report_prefix_pop();
+
+   return report_summary();
+}

Re: [kvm-unit-tests PATCH 5/7] arch-run: Add a "continuous" migration option for tests

[Thomas Huth]

On 26/02/2024 10.38, Nicholas Piggin wrote:

The cooperative migration protocol is very good to control precise
pre and post conditions for a migration event. However in some cases
its intrusiveness to the test program, can mask problems and make
analysis more difficult.

For example to stress test migration vs concurrent complicated
memory access, including TLB refill, ram dirtying, etc., then the
tight spin at getchar() and resumption of the workload after
migration is unhelpful.

This adds a continuous migration mode that directs the harness to
perform migrations continually. This is added to the migration
selftests, which also sees cooperative migration iterations reduced
to avoid increasing test time too much.

Signed-off-by: Nicholas Piggin 
---
  common/selftest-migration.c | 16 +--
  lib/migrate.c   | 18 
  lib/migrate.h   |  3 ++
  scripts/arch-run.bash   | 55 -
  4 files changed, 82 insertions(+), 10 deletions(-)

diff --git a/common/selftest-migration.c b/common/selftest-migration.c
index 0afd8581c..9a9b61835 100644
--- a/common/selftest-migration.c
+++ b/common/selftest-migration.c
@@ -9,12 +9,13 @@
   */
  #include 
  #include 
+#include 
  
-#define NR_MIGRATIONS 30

+#define NR_MIGRATIONS 15
  
  int main(int argc, char **argv)

  {
-   report_prefix_push("migration");
+   report_prefix_push("migration harness");
  
  	if (argc > 1 && !strcmp(argv[1], "skip")) {

migrate_skip();
@@ -24,7 +25,16 @@ int main(int argc, char **argv)
  
  		for (i = 0; i < NR_MIGRATIONS; i++)

migrate_quiet();
-   report(true, "simple harness stress");
+   report(true, "cooperative migration");
+
+   migrate_begin_continuous();
+   mdelay(2000);
+   migrate_end_continuous();
+   mdelay(1000);
+   migrate_begin_continuous();
+   mdelay(2000);
+   migrate_end_continuous();
+   report(true, "continuous migration");
}
  
  	report_prefix_pop();

diff --git a/lib/migrate.c b/lib/migrate.c
index 1d22196b7..770f76d5c 100644
--- a/lib/migrate.c
+++ b/lib/migrate.c
@@ -60,3 +60,21 @@ void migrate_skip(void)
puts("Skipped VM migration (quiet)\n");
(void)getchar();
  }
+
+void migrate_begin_continuous(void)
+{
+   puts("Begin continuous migration\n");
+   (void)getchar();
+}
+
+void migrate_end_continuous(void)
+{
+   /*
+* Migration can split this output between source and dest QEMU
+* output files, print twice and match once to always cope with
+* a split.
+*/
+   puts("End continuous migration\n");
+   puts("End continuous migration (quiet)\n");
+   (void)getchar();
+}
diff --git a/lib/migrate.h b/lib/migrate.h
index db6e0c501..35b6703a2 100644
--- a/lib/migrate.h
+++ b/lib/migrate.h
@@ -11,3 +11,6 @@ void migrate_quiet(void);
  void migrate_once(void);
  
  void migrate_skip(void);

+
+void migrate_begin_continuous(void);
+void migrate_end_continuous(void);
diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash
index d0f6f098f..5c7e72036 100644
--- a/scripts/arch-run.bash
+++ b/scripts/arch-run.bash
@@ -125,15 +125,17 @@ qmp_events ()
  filter_quiet_msgs ()
  {
grep -v "Now migrate the VM (quiet)" |
+   grep -v "Begin continuous migration (quiet)" |
+   grep -v "End continuous migration (quiet)" |
grep -v "Skipped VM migration (quiet)"
  }
  
  seen_migrate_msg ()

  {
if [ $skip_migration -eq 1 ]; then
-   grep -q -e "Now migrate the VM" < $1
+   grep -q -e "Now migrate the VM" -e "Begin continuous migration" 
< $1
else
-   grep -q -e "Now migrate the VM" -e "Skipped VM migration" < $1
+   grep -q -e "Now migrate the VM" -e "Begin continuous migration" -e 
"Skipped VM migration" < $1
fi
  }
  
@@ -161,6 +163,7 @@ run_migration ()

src_qmpout=/dev/null
dst_qmpout=/dev/null
skip_migration=0
+   continuous_migration=0
  
  	mkfifo ${src_outfifo}

mkfifo ${dst_outfifo}
@@ -186,9 +189,12 @@ run_migration ()
do_migration || return $?
  
  	while ps -p ${live_pid} > /dev/null ; do

-   # Wait for test exit or further migration messages.
-   if ! seen_migrate_msg ${src_out} ;  then
+   if [[ ${continuous_migration} -eq 1 ]] ; then


Here you're using "[[" for testing ...


+   do_migration || return $?
+   elif ! seen_migrate_msg ${src_out} ;  then
sleep 0.1
+   elif grep -q "Begin continuous migration" < ${src_out} ; then
+   do_migration || return $?
elif grep -q "Now migrate the VM" < ${src_out} ; then
do_migration || return $?
elif [ $skip_migration -eq 0 ] && grep -q "Skipped 

Re: [kvm-unit-tests PATCH 2/7] migration: Add a migrate_skip command

[Thomas Huth]

On 26/02/2024 10.38, Nicholas Piggin wrote:

Tests that are run with MIGRATION=yes but skip due to some requirement
not being met will show as a failure due to the harness requirement to
see one successful migration. The workaround for this is to migrate in
test's skip path. Add a new command that just tells the harness to not
expect a migration.

Signed-off-by: Nicholas Piggin 
---
  common/selftest-migration.c | 14 -
  lib/migrate.c   | 19 -
  lib/migrate.h   |  2 ++
  powerpc/unittests.cfg   |  6 ++
  s390x/unittests.cfg |  5 +
  scripts/arch-run.bash   | 41 +
  6 files changed, 73 insertions(+), 14 deletions(-)


Reviewed-by: Thomas Huth 

Re: [kvm-unit-tests PATCH 6/7] gitlab-ci: Run migration selftest on s390x and powerpc

[Thomas Huth]

On 26/02/2024 10.38, Nicholas Piggin wrote:

The migration harness is complicated and easy to break so CI will
be helpful.

Signed-off-by: Nicholas Piggin 
---
  .gitlab-ci.yml | 18 +++---
  1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 71d986e98..61f196d5d 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -64,26 +64,28 @@ build-arm:
  build-ppc64be:
   extends: .outoftree_template
   script:
- - dnf install -y qemu-system-ppc gcc-powerpc64-linux-gnu
+ - dnf install -y qemu-system-ppc gcc-powerpc64-linux-gnu nmap-ncat
   - mkdir build
   - cd build
   - ../configure --arch=ppc64 --endian=big --cross-prefix=powerpc64-linux-gnu-
   - make -j2
   - ACCEL=tcg ./run_tests.sh
- selftest-setup spapr_hcall rtas-get-time-of-day rtas-get-time-of-day-base
- rtas-set-time-of-day emulator
+ selftest-setup selftest-migration selftest-migration-skip spapr_hcall
+ rtas-get-time-of-day rtas-get-time-of-day-base rtas-set-time-of-day
+ emulator
   | tee results.txt
   - if grep -q FAIL results.txt ; then exit 1 ; fi
  
  build-ppc64le:

   extends: .intree_template
   script:
- - dnf install -y qemu-system-ppc gcc-powerpc64-linux-gnu
+ - dnf install -y qemu-system-ppc gcc-powerpc64-linux-gnu nmap-ncat
   - ./configure --arch=ppc64 --endian=little 
--cross-prefix=powerpc64-linux-gnu-
   - make -j2
   - ACCEL=tcg ./run_tests.sh
- selftest-setup spapr_hcall rtas-get-time-of-day rtas-get-time-of-day-base
- rtas-set-time-of-day emulator
+ selftest-setup selftest-migration selftest-migration-skip spapr_hcall
+ rtas-get-time-of-day rtas-get-time-of-day-base rtas-set-time-of-day
+ emulator
   | tee results.txt
   - if grep -q FAIL results.txt ; then exit 1 ; fi
  
@@ -107,7 +109,7 @@ build-riscv64:

  build-s390x:
   extends: .outoftree_template
   script:
- - dnf install -y qemu-system-s390x gcc-s390x-linux-gnu
+ - dnf install -y qemu-system-s390x gcc-s390x-linux-gnu nmap-ncat
   - mkdir build
   - cd build
   - ../configure --arch=s390x --cross-prefix=s390x-linux-gnu-
@@ -133,6 +135,8 @@ build-s390x:
sclp-1g
sclp-3g
selftest-setup
+  selftest-migration
+  selftest-migration-skip
sieve
smp
stsi


While I can update the qemu binary for the s390x-kvm job, the build-* jobs 
run in a container with a normal QEMU from the corresponding distros, so I 
think this has to wait 'til we get distros that contain your QEMU TCG 
migration fix.


 Thomas

Re: [kvm-unit-tests PATCH 4/7] powerpc: add asm/time.h header with delay and get_clock_us/ms

[Thomas Huth]

On 26/02/2024 10.38, Nicholas Piggin wrote:

This matches s390x clock and delay APIs, so common test code can start
using time facilities.

Signed-off-by: Nicholas Piggin 
---
  lib/powerpc/asm/processor.h | 21 -
  lib/powerpc/asm/time.h  | 30 ++
  lib/powerpc/processor.c | 11 +++
  lib/powerpc/smp.c   |  1 +
  lib/ppc64/asm/time.h|  1 +
  powerpc/spapr_vpa.c |  1 +
  powerpc/sprs.c  |  1 +
  powerpc/tm.c|  1 +
  8 files changed, 46 insertions(+), 21 deletions(-)
  create mode 100644 lib/powerpc/asm/time.h
  create mode 100644 lib/ppc64/asm/time.h


Reviewed-by: Thomas Huth 

Re: [kvm-unit-tests PATCH 14/32] powerpc: general interrupt tests

[Thomas Huth]

On 01/03/2024 14.45, Andrew Jones wrote:

On Fri, Mar 01, 2024 at 01:41:22PM +0100, Thomas Huth wrote:

On 26/02/2024 11.12, Nicholas Piggin wrote:

Add basic testing of various kinds of interrupts, machine check,
page fault, illegal, decrementer, trace, syscall, etc.

This has a known failure on QEMU TCG pseries machines where MSR[ME]
can be incorrectly set to 0.


Two questions out of curiosity:

Any chance that this could be fixed easily in QEMU?

Or is there a way to detect TCG from within the test? (for example, we have
a host_is_tcg() function for s390x so we can e.g. use report_xfail() for
tests that are known to fail on TCG there)


If there's nothing better, then it should be possible to check the
QEMU_ACCEL environment variable which will be there with the default
environ.


Well, but that's only available from the host side, not within the test 
(i.e. the guest). So that does not help much with report_xfail...
I was rather thinking of something like checking the device tree, e.g. for 
the compatible property in /hypervisor to see whether it's KVM or TCG...?


 Thomas

Re: [kvm-unit-tests PATCH 3/7] (arm|s390): Use migrate_skip in test cases

[Thomas Huth]

On 26/02/2024 10.38, Nicholas Piggin wrote:

Have tests use the new migrate_skip command in skip paths, rather than
calling migrate_once to prevent harness reporting an error.

s390x/migration.c adds a new command that looks like it was missing
previously.

Signed-off-by: Nicholas Piggin 
---
  arm/gic.c  | 21 -
  s390x/migration-cmm.c  |  8 
  s390x/migration-skey.c |  4 +++-
  s390x/migration.c  |  1 +
  4 files changed, 20 insertions(+), 14 deletions(-)


Reviewed-by: Thomas Huth 

Re: [kvm-unit-tests PATCH 1/7] arch-run: Keep infifo open

[Thomas Huth]

On 26/02/2024 10.38, Nicholas Piggin wrote:

The infifo fifo that is used to send characters to QEMU console is
only able to receive one character before the cat process exits.
Supporting interactions between test and harness involving multiple
characters requires the fifo to remain open.

This also allows us to let the cat out of the bag, simplifying the
input pipeline.


LOL, we rather let the cat out of the subshell now, but I like the play on 
words :-)



Signed-off-by: Nicholas Piggin 
---
  scripts/arch-run.bash | 12 ++--
  1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash
index 6daef3218..e5b36a07b 100644
--- a/scripts/arch-run.bash
+++ b/scripts/arch-run.bash
@@ -158,6 +158,11 @@ run_migration ()
mkfifo ${src_outfifo}
mkfifo ${dst_outfifo}
  
+	# Holding both ends of the input fifo open prevents opens from

+   # blocking and readers getting EOF when a writer closes it.
+   mkfifo ${dst_infifo}
+   exec {dst_infifo_fd}<>${dst_infifo}
+
eval "$migcmdline" \
-chardev socket,id=mon,path=${src_qmp},server=on,wait=off \
-mon chardev=mon,mode=control > ${src_outfifo} &
@@ -191,14 +196,10 @@ run_migration ()
  
  do_migration ()

  {
-   # We have to use cat to open the named FIFO, because named FIFO's,
-   # unlike pipes, will block on open() until the other end is also
-   # opened, and that totally breaks QEMU...
-   mkfifo ${dst_infifo}
eval "$migcmdline" \
-chardev socket,id=mon,path=${dst_qmp},server=on,wait=off \
-mon chardev=mon,mode=control -incoming unix:${dst_incoming} \
-   < <(cat ${dst_infifo}) > ${dst_outfifo} &
+   < ${dst_infifo} > ${dst_outfifo} &
incoming_pid=$!
cat ${dst_outfifo} | tee ${dst_out} | filter_quiet_msgs &
  
@@ -245,7 +246,6 @@ do_migration ()
  
  	# keypress to dst so getchar completes and test continues

echo > ${dst_infifo}
-   rm ${dst_infifo}


I assume it will not get deleted by the trap handler? ... sounds fine to me, 
so I dare to say:


Reviewed-by: Thomas Huth 

Re: [kvm-unit-tests PATCH 14/32] powerpc: general interrupt tests

[Thomas Huth]

On 26/02/2024 11.12, Nicholas Piggin wrote:

Add basic testing of various kinds of interrupts, machine check,
page fault, illegal, decrementer, trace, syscall, etc.

This has a known failure on QEMU TCG pseries machines where MSR[ME]
can be incorrectly set to 0.


Two questions out of curiosity:

Any chance that this could be fixed easily in QEMU?

Or is there a way to detect TCG from within the test? (for example, we have 
a host_is_tcg() function for s390x so we can e.g. use report_xfail() for 
tests that are known to fail on TCG there)



Signed-off-by: Nicholas Piggin 
---
  lib/powerpc/asm/processor.h |   4 +
  lib/powerpc/asm/reg.h   |  17 ++
  lib/powerpc/setup.c |  11 +
  lib/ppc64/asm/ptrace.h  |  16 ++
  powerpc/Makefile.common |   3 +-
  powerpc/interrupts.c| 415 
  powerpc/unittests.cfg   |   3 +
  7 files changed, 468 insertions(+), 1 deletion(-)
  create mode 100644 powerpc/interrupts.c

diff --git a/lib/powerpc/asm/processor.h b/lib/powerpc/asm/processor.h
index cf1b9d8ff..eed37d1f4 100644
--- a/lib/powerpc/asm/processor.h
+++ b/lib/powerpc/asm/processor.h
@@ -11,7 +11,11 @@ void do_handle_exception(struct pt_regs *regs);
  #endif /* __ASSEMBLY__ */
  
  extern bool cpu_has_hv;

+extern bool cpu_has_power_mce;
+extern bool cpu_has_siar;
  extern bool cpu_has_heai;
+extern bool cpu_has_prefix;
+extern bool cpu_has_sc_lev;
  
  static inline uint64_t mfspr(int nr)

  {
diff --git a/lib/powerpc/asm/reg.h b/lib/powerpc/asm/reg.h
index 782e75527..d6097f48f 100644
--- a/lib/powerpc/asm/reg.h
+++ b/lib/powerpc/asm/reg.h
@@ -5,8 +5,15 @@
  
  #define UL(x) _AC(x, UL)
  
+#define SPR_DSISR	0x012

+#define SPR_DAR0x013
+#define SPR_DEC0x016
  #define SPR_SRR0  0x01a
  #define SPR_SRR1  0x01b
+#define   SRR1_PREFIX  UL(0x2000)
+#define SPR_FSCR   0x099
+#define   FSCR_PREFIX  UL(0x2000)
+#define SPR_HFSCR  0x0be
  #define SPR_TB0x10c
  #define SPR_SPRG0 0x110
  #define SPR_SPRG1 0x111
@@ -22,12 +29,17 @@
  #define   PVR_VER_POWER8  UL(0x004d)
  #define   PVR_VER_POWER9  UL(0x004e)
  #define   PVR_VER_POWER10 UL(0x0080)
+#define SPR_HDEC   0x136
  #define SPR_HSRR0 0x13a
  #define SPR_HSRR1 0x13b
+#define SPR_LPCR   0x13e
+#define   LPCR_HDICE   UL(0x1)
+#define SPR_HEIR   0x153
  #define SPR_MMCR0 0x31b
  #define   MMCR0_FCUL(0x8000)
  #define   MMCR0_PMAE  UL(0x0400)
  #define   MMCR0_PMAO  UL(0x0080)
+#define SPR_SIAR   0x31c
  
  /* Machine State Register definitions: */

  #define MSR_LE_BIT0
@@ -35,6 +47,11 @@
  #define MSR_HV_BIT60  /* Hypervisor mode */
  #define MSR_SF_BIT63  /* 64-bit mode */
  
+#define MSR_DR		UL(0x0010)

+#define MSR_IR UL(0x0020)
+#define MSR_BE UL(0x0200)  /* Branch Trace Enable */
+#define MSR_SE UL(0x0400)  /* Single Step Enable */
+#define MSR_EE UL(0x8000)
  #define MSR_MEUL(0x1000)
  
  #endif

diff --git a/lib/powerpc/setup.c b/lib/powerpc/setup.c
index 3c81aee9e..9b665f59c 100644
--- a/lib/powerpc/setup.c
+++ b/lib/powerpc/setup.c
@@ -87,7 +87,11 @@ static void cpu_set(int fdtnode, u64 regval, void *info)
  }
  
  bool cpu_has_hv;

+bool cpu_has_power_mce; /* POWER CPU machine checks */
+bool cpu_has_siar;
  bool cpu_has_heai;
+bool cpu_has_prefix;
+bool cpu_has_sc_lev; /* sc interrupt has LEV field in SRR1 */
  
  static void cpu_init(void)

  {
@@ -112,15 +116,22 @@ static void cpu_init(void)
  
  	switch (mfspr(SPR_PVR) & PVR_VERSION_MASK) {

case PVR_VER_POWER10:
+   cpu_has_prefix = true;
+   cpu_has_sc_lev = true;
case PVR_VER_POWER9:
case PVR_VER_POWER8E:
case PVR_VER_POWER8NVL:
case PVR_VER_POWER8:
+   cpu_has_power_mce = true;
cpu_has_heai = true;
+   cpu_has_siar = true;
break;
default:
break;
}
+
+   if (!cpu_has_hv) /* HEIR is HV register */
+   cpu_has_heai = false;
  }
  
  static void mem_init(phys_addr_t freemem_start)

diff --git a/lib/ppc64/asm/ptrace.h b/lib/ppc64/asm/ptrace.h
index 12de7499b..db263a59e 100644
--- a/lib/ppc64/asm/ptrace.h
+++ b/lib/ppc64/asm/ptrace.h
@@ -5,6 +5,9 @@
  #define STACK_FRAME_OVERHEAD112 /* size of minimum stack frame */
  
  #ifndef __ASSEMBLY__

+
+#include 
+
  struct pt_regs {
unsigned long gpr[32];
unsigned long nip;
@@ -17,6 +20,19 @@ struct pt_regs {
unsigned long _pad; /* stack must be 16-byte aligned */
  };
  
+static inline bool regs_is_prefix(volatile struct pt_regs *regs)

+{
+   return regs->msr & SRR1_PREFIX;
+}
+
+static inline void regs_advance_insn(struct pt_regs *regs)
+{
+   if (regs_is_prefix(regs))
+   

Re: [kvm-unit-tests PATCH 12/32] powerpc: Fix emulator illegal instruction test for powernv

[Thomas Huth]

On 26/02/2024 11.11, Nicholas Piggin wrote:

Illegal instructions cause 0xe40 (HEAI) interrupts rather
than program interrupts.

Acked-by: Thomas Huth 
Signed-off-by: Nicholas Piggin 
---
  lib/powerpc/asm/processor.h |  1 +
  lib/powerpc/setup.c | 13 +
  powerpc/emulator.c  | 21 -
  3 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/lib/powerpc/asm/processor.h b/lib/powerpc/asm/processor.h
index 9d8061962..cf1b9d8ff 100644
--- a/lib/powerpc/asm/processor.h
+++ b/lib/powerpc/asm/processor.h
@@ -11,6 +11,7 @@ void do_handle_exception(struct pt_regs *regs);
  #endif /* __ASSEMBLY__ */
  
  extern bool cpu_has_hv;

+extern bool cpu_has_heai;
  
  static inline uint64_t mfspr(int nr)

  {
diff --git a/lib/powerpc/setup.c b/lib/powerpc/setup.c
index 89e5157f2..3c81aee9e 100644
--- a/lib/powerpc/setup.c
+++ b/lib/powerpc/setup.c
@@ -87,6 +87,7 @@ static void cpu_set(int fdtnode, u64 regval, void *info)
  }
  
  bool cpu_has_hv;

+bool cpu_has_heai;
  
  static void cpu_init(void)

  {
@@ -108,6 +109,18 @@ static void cpu_init(void)
hcall(H_SET_MODE, 0, 4, 0, 0);
  #endif
}
+
+   switch (mfspr(SPR_PVR) & PVR_VERSION_MASK) {
+   case PVR_VER_POWER10:
+   case PVR_VER_POWER9:
+   case PVR_VER_POWER8E:
+   case PVR_VER_POWER8NVL:
+   case PVR_VER_POWER8:
+   cpu_has_heai = true;
+   break;
+   default:
+   break;
+   }
  }
  
  static void mem_init(phys_addr_t freemem_start)

diff --git a/powerpc/emulator.c b/powerpc/emulator.c
index 39dd59645..c9b17f742 100644
--- a/powerpc/emulator.c
+++ b/powerpc/emulator.c
@@ -31,6 +31,20 @@ static void program_check_handler(struct pt_regs *regs, void 
*opaque)
regs->nip += 4;
  }
  
+static void heai_handler(struct pt_regs *regs, void *opaque)

+{
+   int *data = opaque;
+
+   if (verbose) {
+   printf("Detected invalid instruction %#018lx: %08x\n",
+  regs->nip, *(uint32_t*)regs->nip);
+   }
+
+   *data = 8; /* Illegal instruction */
+
+   regs->nip += 4;
+}
+
  static void alignment_handler(struct pt_regs *regs, void *opaque)
  {
int *data = opaque;
@@ -362,7 +376,12 @@ int main(int argc, char **argv)
  {
int i;
  
-	handle_exception(0x700, program_check_handler, (void *)_invalid);

+   if (cpu_has_heai) {
+   handle_exception(0xe40, heai_handler, (void *)_invalid);
+   handle_exception(0x700, program_check_handler, (void 
*)_invalid);
+   } else {
+   handle_exception(0x700, program_check_handler, (void 
*)_invalid);


The 0x700 line looks identical to the other part of the if-statement ... I'd 
suggest to leave it outside of the if-statement, drop the else-part and just 
set 0xe40 if cpu_has_heai.


 Thomas


+   }
handle_exception(0x600, alignment_handler, (void *));
  
  	for (i = 1; i < argc; i++) {

Re: [kvm-unit-tests PATCH 08/32] powerpc/sprs: Avoid taking PMU interrupts caused by register fuzzing

[Thomas Huth]

On 26/02/2024 11.11, Nicholas Piggin wrote:

Storing certain values in MMCR0 can cause PMU interrupts when msleep
enables MSR[EE], and this crashes the test. Freeze the PMU counters
and clear any PMU exception before calling msleep.

Signed-off-by: Nicholas Piggin 
---
  lib/powerpc/asm/reg.h |  4 
  powerpc/sprs.c| 17 +++--
  2 files changed, 15 insertions(+), 6 deletions(-)


Reviewed-by: Thomas Huth 

Re: [kvm-unit-tests PATCH 07/32] powerpc/sprs: Don't fail changed SPRs that are used by the test harness

[Thomas Huth]

On 26/02/2024 11.11, Nicholas Piggin wrote:

SPRs annotated with SPR_HARNESS can change between consecutive reads
because the test harness code has changed them. Avoid failing the
test in this case.

Signed-off-by: Nicholas Piggin 
---
  powerpc/sprs.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/powerpc/sprs.c b/powerpc/sprs.c
index 8253ea971..44edd0d7b 100644
--- a/powerpc/sprs.c
+++ b/powerpc/sprs.c
@@ -563,7 +563,7 @@ int main(int argc, char **argv)
if (before[i] >> 32)
pass = false;
}
-   if (!(sprs[i].type & SPR_ASYNC) && (before[i] != after[i]))
+   if (!(sprs[i].type & (SPR_HARNESS|SPR_ASYNC)) && (before[i] != 
after[i]))
pass = false;
  
  		if (sprs[i].width == 32 && !(before[i] >> 32) && !(after[i] >> 32))


I guess you could also squash this into the previous patch (to avoid 
problems with bisecting later?) ...


Anyway:
Reviewed-by: Thomas Huth 

Re: [kvm-unit-tests PATCH 05/32] powerpc: Cleanup SPR and MSR definitions

[Thomas Huth]

On 26/02/2024 11.11, Nicholas Piggin wrote:

Move SPR and MSR defines out of ppc_asm.h and processor.h and into a
new include, asm/reg.h.

Add a define for the PVR SPR and various processor versions, and replace
the open coded numbers in the sprs.c test case.

Signed-off-by: Nicholas Piggin 
---
  lib/powerpc/asm/ppc_asm.h   |  8 +---
  lib/powerpc/asm/processor.h |  7 +--
  lib/powerpc/asm/reg.h   | 30 ++
  lib/powerpc/asm/time.h  |  1 +
  lib/ppc64/asm/reg.h |  1 +
  powerpc/sprs.c  | 21 ++---
  6 files changed, 44 insertions(+), 24 deletions(-)
  create mode 100644 lib/powerpc/asm/reg.h
  create mode 100644 lib/ppc64/asm/reg.h


Reviewed-by: Thomas Huth 

Re: [kvm-unit-tests PATCH 04/32] powerpc: interrupt stack backtracing

[Thomas Huth]

On 26/02/2024 11.11, Nicholas Piggin wrote:

Add support for backtracing across interrupt stacks, and
add interrupt frame backtrace for unhandled interrupts.

Signed-off-by: Nicholas Piggin 
---
  lib/powerpc/processor.c |  4 ++-
  lib/ppc64/asm/stack.h   |  3 +++
  lib/ppc64/stack.c   | 55 +
  powerpc/Makefile.ppc64  |  1 +
  powerpc/cstart64.S  |  7 --
  5 files changed, 67 insertions(+), 3 deletions(-)
  create mode 100644 lib/ppc64/stack.c

diff --git a/lib/powerpc/processor.c b/lib/powerpc/processor.c
index ad0d95666..114584024 100644
--- a/lib/powerpc/processor.c
+++ b/lib/powerpc/processor.c
@@ -51,7 +51,9 @@ void do_handle_exception(struct pt_regs *regs)
return;
}
  
-	printf("unhandled cpu exception %#lx at NIA:0x%016lx MSR:0x%016lx\n", regs->trap, regs->nip, regs->msr);

+   printf("Unhandled cpu exception %#lx at NIA:0x%016lx MSR:0x%016lx\n",
+   regs->trap, regs->nip, regs->msr);
+   dump_frame_stack((void *)regs->nip, (void *)regs->gpr[1]);
abort();
  }
  
diff --git a/lib/ppc64/asm/stack.h b/lib/ppc64/asm/stack.h

index 9734bbb8f..94fd1021c 100644
--- a/lib/ppc64/asm/stack.h
+++ b/lib/ppc64/asm/stack.h
@@ -5,4 +5,7 @@
  #error Do not directly include . Just use .
  #endif
  
+#define HAVE_ARCH_BACKTRACE

+#define HAVE_ARCH_BACKTRACE_FRAME
+
  #endif
diff --git a/lib/ppc64/stack.c b/lib/ppc64/stack.c
new file mode 100644
index 0..fcb7fa860
--- /dev/null
+++ b/lib/ppc64/stack.c
@@ -0,0 +1,55 @@
+#include 
+#include 
+#include 
+
+extern char exception_stack_marker[];
+
+int backtrace_frame(const void *frame, const void **return_addrs, int 
max_depth)
+{
+   static int walking;
+   int depth = 0;
+   const unsigned long *bp = (unsigned long *)frame;
+   void *return_addr;
+
+   asm volatile("" ::: "lr"); /* Force it to save LR */
+
+   if (walking) {
+   printf("RECURSIVE STACK WALK!!!\n");
+   return 0;
+   }
+   walking = 1;
+
+   bp = (unsigned long *)bp[0];
+   return_addr = (void *)bp[2];
+
+   for (depth = 0; bp && depth < max_depth; depth++) {
+   return_addrs[depth] = return_addr;
+   if (return_addrs[depth] == 0)
+   break;
+   if (return_addrs[depth] == exception_stack_marker) {
+   struct pt_regs *regs;
+
+   regs = (void *)bp + STACK_FRAME_OVERHEAD;
+   bp = (unsigned long *)bp[0];
+   /* Represent interrupt frame with vector number */
+   return_addr = (void *)regs->trap;
+   if (depth + 1 < max_depth) {
+   depth++;
+   return_addrs[depth] = return_addr;
+   return_addr = (void *)regs->nip;
+   }
+   } else {
+   bp = (unsigned long *)bp[0];
+   return_addr = (void *)bp[2];
+   }
+   }
+
+   walking = 0;
+   return depth;
+}
+
+int backtrace(const void **return_addrs, int max_depth)
+{
+   return backtrace_frame(__builtin_frame_address(0), return_addrs,
+  max_depth);
+}
diff --git a/powerpc/Makefile.ppc64 b/powerpc/Makefile.ppc64
index b0ed2b104..eb682c226 100644
--- a/powerpc/Makefile.ppc64
+++ b/powerpc/Makefile.ppc64
@@ -17,6 +17,7 @@ cstart.o = $(TEST_DIR)/cstart64.o
  reloc.o  = $(TEST_DIR)/reloc64.o
  
  OBJDIRS += lib/ppc64

+cflatobjs += lib/ppc64/stack.o
  
  # ppc64 specific tests

  tests = $(TEST_DIR)/spapr_vpa.elf
diff --git a/powerpc/cstart64.S b/powerpc/cstart64.S
index 14ab0c6c8..278af84a6 100644
--- a/powerpc/cstart64.S
+++ b/powerpc/cstart64.S
@@ -188,6 +188,7 @@ call_handler:
.endr
mfsprg1 r0
std r0,GPR1(r1)
+   std r0,0(r1)
  
  	/* lr, xer, ccr */
  
@@ -206,12 +207,12 @@ call_handler:

subir31, r31, 0b - start_text
ld  r2, (p_toc_text - start_text)(r31)
  
-	/* FIXME: build stack frame */

-
/* call generic handler */
  
  	addi	r3,r1,STACK_FRAME_OVERHEAD

bl  do_handle_exception
+   .global exception_stack_marker
+exception_stack_marker:
  
  	/* restore context */
  
@@ -321,6 +322,7 @@ handler_trampoline:

/* nip and msr */
mfsrr0  r0
std r0, _NIP(r1)
+   std r0, INT_FRAME_SIZE+16(r1)


So if I got that right, INT_FRAME_SIZE+16 points to the stack frame of the 
function that was running before the interrupt handler? Is it such a good 
idea to change that here? Please add a comment here explaining this line. 
Thanks!




mfsrr1  r0
std r0, _MSR(r1)
@@ -337,6 +339,7 @@ handler_htrampoline:
/* nip and msr */
mfspr   r0, SPR_HSRR0
std r0, _NIP(r1)
+   std r0, INT_FRAME_SIZE+16(r1)


dito.


mfspr   r0, SPR_HSRR1
 

Re: [kvm-unit-tests PATCH 03/32] powerpc: Fix stack backtrace termination

[Thomas Huth]

On 27/02/2024 09.50, Thomas Huth wrote:

On 26/02/2024 11.11, Nicholas Piggin wrote:

The backtrace handler terminates when it sees a NULL caller address,
but the powerpc stack setup does not keep such a NULL caller frame
at the start of the stack.

This happens to work on pseries because the memory at 0 is mapped and
it contains 0 at the location of the return address pointer if it
were a stack frame. But this is fragile, and does not work with powernv
where address 0 contains firmware instructions.

Use the existing dummy frame on stack as the NULL caller, and create a
new frame on stack for the entry code.

Signed-off-by: Nicholas Piggin 
---
  powerpc/cstart64.S | 12 ++--
  1 file changed, 10 insertions(+), 2 deletions(-)


Thanks for tackling this! ... however, not doing powerpc work since years 
anymore, I have some ignorant questions below...



diff --git a/powerpc/cstart64.S b/powerpc/cstart64.S
index e18ae9a22..14ab0c6c8 100644
--- a/powerpc/cstart64.S
+++ b/powerpc/cstart64.S
@@ -46,8 +46,16 @@ start:
  add    r1, r1, r31
  add    r2, r2, r31
+    /* Zero backpointers in initial stack frame so backtrace() stops */
+    li    r0,0
+    std    r0,0(r1)


0(r1) is the back chain pointer ...


+    std    r0,16(r1)


... but what is 16(r1) ? I suppose that should be the "LR save word" ? But 
isn't that at 8(r1) instead?? (not sure whether I'm looking at the right ELF 
abi spec right now...)


Ok, I was looking at the wrong ELF spec, indeed (it was an ancient 32-bit 
spec, not the 64-bit ABI). Sorry for the confusion. Having a proper #define 
or a comment for the 16 here would still be helpful, though.


 Thomas

Re: [kvm-unit-tests PATCH 03/32] powerpc: Fix stack backtrace termination

[Thomas Huth]

On 26/02/2024 11.11, Nicholas Piggin wrote:

The backtrace handler terminates when it sees a NULL caller address,
but the powerpc stack setup does not keep such a NULL caller frame
at the start of the stack.

This happens to work on pseries because the memory at 0 is mapped and
it contains 0 at the location of the return address pointer if it
were a stack frame. But this is fragile, and does not work with powernv
where address 0 contains firmware instructions.

Use the existing dummy frame on stack as the NULL caller, and create a
new frame on stack for the entry code.

Signed-off-by: Nicholas Piggin 
---
  powerpc/cstart64.S | 12 ++--
  1 file changed, 10 insertions(+), 2 deletions(-)


Thanks for tackling this! ... however, not doing powerpc work since years 
anymore, I have some ignorant questions below...



diff --git a/powerpc/cstart64.S b/powerpc/cstart64.S
index e18ae9a22..14ab0c6c8 100644
--- a/powerpc/cstart64.S
+++ b/powerpc/cstart64.S
@@ -46,8 +46,16 @@ start:
add r1, r1, r31
add r2, r2, r31
  
+	/* Zero backpointers in initial stack frame so backtrace() stops */

+   li  r0,0
+   std r0,0(r1)


0(r1) is the back chain pointer ...


+   std r0,16(r1)


... but what is 16(r1) ? I suppose that should be the "LR save word" ? But 
isn't that at 8(r1) instead?? (not sure whether I'm looking at the right ELF 
abi spec right now...)


Anyway, a comment in the source would be helpful here.


+
+   /* Create entry frame */
+   stdur1,-INT_FRAME_SIZE(r1)


Since we already create an initial frame via stackptr from powerpc/flat.lds, 
do we really need to create this additional one here? Or does the one from 
flat.lds have to be completely empty, i.e. also no DTB pointer in it?



/* save DTB pointer */
-   std r3, 56(r1)
+   SAVE_GPR(3,r1)


Isn't SAVE_GPR rather meant for the interrupt frame, not for the normal C 
calling convention frames?


Sorry for asking dumb questions ... I still have a hard time understanding 
the changes here... :-/



/*
 * Call relocate. relocate is C code, but careful to not use
@@ -101,7 +109,7 @@ start:
stw r4, 0(r3)
  
  	/* complete setup */

-1: ld  r3, 56(r1)
+1: REST_GPR(3, r1)
bl  setup
  
  	/* run the test */


 Thomas

Re: [kvm-unit-tests PATCH 02/32] powerpc: Fix pseries getchar return value

[Thomas Huth]

On 26/02/2024 11.11, Nicholas Piggin wrote:

getchar() didn't get the shift value correct and never returned the
first character. This never really mattered since it was only ever
used for press-a-key-to-continue prompts. but it tripped me up when
debugging a QEMU console output problem.

Signed-off-by: Nicholas Piggin 
---
  lib/powerpc/hcall.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/powerpc/hcall.c b/lib/powerpc/hcall.c
index 711cb1b0f..b4d39ac65 100644
--- a/lib/powerpc/hcall.c
+++ b/lib/powerpc/hcall.c
@@ -43,5 +43,5 @@ int __getchar(void)
asm volatile (" sc 1 "  : "+r"(r3), "+r"(r4), "=r"(r5)
: "r"(r3),  "r"(r4));
  
-	return r3 == H_SUCCESS && r4 > 0 ? r5 >> 48 : -1;

+   return r3 == H_SUCCESS && r4 > 0 ? r5 >> 56 : -1;
  }


Reviewed-by: Thomas Huth 

Re: [kvm-unit-tests PATCH 01/32] powerpc: Fix KVM caps on POWER9 hosts

[Thomas Huth]

On 26/02/2024 11.11, Nicholas Piggin wrote:

KVM does not like to run on POWER9 hosts without cap-ccf-assist=off.

Signed-off-by: Nicholas Piggin 
---
  powerpc/run | 2 ++
  1 file changed, 2 insertions(+)

diff --git a/powerpc/run b/powerpc/run
index e469f1eb3..5cdb94194 100755
--- a/powerpc/run
+++ b/powerpc/run
@@ -24,6 +24,8 @@ M+=",accel=$ACCEL$ACCEL_PROPS"
  
  if [[ "$ACCEL" == "tcg" ]] ; then

M+=",cap-cfpc=broken,cap-sbbc=broken,cap-ibs=broken,cap-ccf-assist=off"
+elif [[ "$ACCEL" == "kvm" ]] ; then
+   M+=",cap-ccf-assist=off"
  fi


Since it is needed in both cases, you could also move it out of the 
if-statement and remove it from the tcg part.


Anyway,
Reviewed-by: Thomas Huth 

Re: [kvm-unit-tests PATCH v5 0/8] Multi-migration support

[Thomas Huth]

On 26/02/2024 09.10, Nicholas Piggin wrote:

On Fri Feb 23, 2024 at 5:06 PM AEST, Thomas Huth wrote:

On 21/02/2024 04.27, Nicholas Piggin wrote:

Now that strange arm64 hang is found to be QEMU bug, I'll repost.
Since arm64 requires Thomas's uart patch and it is worse affected
by the QEMU bug, I will just not build it on arm. The QEMU bug
still affects powerpc (and presumably s390x) but it's not causing
so much trouble for this test case.

I have another test case that can hit it reliably and doesn't
cause crashes but that takes some harness and common lib work so
I'll send that another time.

Since v4:
- Don't build selftest-migration on arm.
- Reduce selftest-migration iterations from 100 to 30 to make the
test run faster (it's ~0.5s per migration).


Thanks, I think the series is ready to go now ... we just have to wait for
your QEMU TCG migration fix to get merged first. Or should we maybe mark the
selftest-migration with "accel = kvm" for now and remove that line later
once QEMU has been fixed?


Could we merge it? I'm juggling a bunch of different things and prone to
lose track of something :\ I'll need to drum up a bit of interest to
review the QEMU fixes from those who know the code too, so that may take
some time.


Ok, I merged it, but with "accel = kvm" for the time being (otherwise this 
would be quite a pitfall for people trying to run the k-u-t with TCG when 
they don't know that they have to fetch a patch from the mailing list to get 
it working).



I left it out of arm unittests.cfg entirely, and s390 and powerpc seems
to work by luck enough to be useful for gitlab CI so I don't think there
is a chnage needed really unless you're paranoid.


At least the s390x test does not work reliably at all when running with TCG 
without your QEMU patch, so I think we really need the "accel = kvm" for the 
time being here.



I do have a later patch that adds a memory tester that does trigger it
right away on powerpc. I'll send that out after this series is merged...
but we do still have the issue that the gitlab CI image has the old QEMU
don't we? Until we update distro.


We only run selected tests in the gitlab-CI, so unless you add it to 
.gitlab-ci.yml, the selftest-migration test won't be run there.


 Thomas

Re: [kvm-unit-tests PATCH v5 0/8] Multi-migration support

[Thomas Huth]

On 21/02/2024 04.27, Nicholas Piggin wrote:

Now that strange arm64 hang is found to be QEMU bug, I'll repost.
Since arm64 requires Thomas's uart patch and it is worse affected
by the QEMU bug, I will just not build it on arm. The QEMU bug
still affects powerpc (and presumably s390x) but it's not causing
so much trouble for this test case.

I have another test case that can hit it reliably and doesn't
cause crashes but that takes some harness and common lib work so
I'll send that another time.

Since v4:
- Don't build selftest-migration on arm.
- Reduce selftest-migration iterations from 100 to 30 to make the
   test run faster (it's ~0.5s per migration).


Thanks, I think the series is ready to go now ... we just have to wait for 
your QEMU TCG migration fix to get merged first. Or should we maybe mark the 
selftest-migration with "accel = kvm" for now and remove that line later 
once QEMU has been fixed?


 Thomas

Re: [kvm-unit-tests PATCH v4 8/8] migration: add a migration selftest

[Thomas Huth]

On 17/02/2024 08.19, Nicholas Piggin wrote:

On Fri Feb 16, 2024 at 9:15 PM AEST, Thomas Huth wrote:

On 09/02/2024 10.11, Nicholas Piggin wrote:

Add a selftest for migration support in  guest library and test harness
code. It performs migrations in a tight loop to irritate races and bugs
in the test harness code.

Include the test in arm, s390, powerpc.

Acked-by: Claudio Imbrenda  (s390x)
Reviewed-by: Thomas Huth 
Signed-off-by: Nicholas Piggin 
---
   arm/Makefile.common  |  1 +
   arm/selftest-migration.c |  1 +
   arm/unittests.cfg|  6 ++


   Hi Nicholas,

I just gave the patches a try, but the arm test seems to fail for me: Only
the first getchar() seems to wait for a character, all the subsequent ones
don't wait anymore and just continue immediately ... is this working for
you? Or do I need another patch on top?


Hey sorry missed this comment

It does seem to work for me, I've mostly tested pseries but I did test
others too (that's how I saw the arm getchar limit).

How are you observing it not waiting for migration?


According to you other mail, I think you figured it out already, but just 
for the records: You can see it when running the guest manually, e.g. 
something like:


 qemu-system-aarch64 -nodefaults -machine virt -accel tcg -cpu cortex-a57 \
   -device virtio-serial-device -device virtconsole,chardev=ctd \
   -chardev testdev,id=ctd -device pci-testdev -display none \
   -serial mon:stdio -kernel arm/selftest-migration.flat -smp 1

Without my "lib/arm/io: Fix calling getchar() multiple times" patch, the 
guest only waits during the first getchar(), all the others simply return 
immediately.


 Thomas

Re: [kvm-unit-tests PATCH v4 8/8] migration: add a migration selftest

[Thomas Huth]

On 09/02/2024 10.11, Nicholas Piggin wrote:

Add a selftest for migration support in  guest library and test harness
code. It performs migrations in a tight loop to irritate races and bugs
in the test harness code.

Include the test in arm, s390, powerpc.

Acked-by: Claudio Imbrenda  (s390x)
Reviewed-by: Thomas Huth 
Signed-off-by: Nicholas Piggin 
---
  arm/Makefile.common  |  1 +
  arm/selftest-migration.c |  1 +
  arm/unittests.cfg|  6 ++
  common/selftest-migration.c  | 34 ++
  powerpc/Makefile.common  |  1 +
  powerpc/selftest-migration.c |  1 +
  powerpc/unittests.cfg|  4 
  s390x/Makefile   |  1 +
  s390x/selftest-migration.c   |  1 +
  s390x/unittests.cfg  |  4 
  10 files changed, 54 insertions(+)
  create mode 12 arm/selftest-migration.c
  create mode 100644 common/selftest-migration.c
  create mode 12 powerpc/selftest-migration.c
  create mode 12 s390x/selftest-migration.c

diff --git a/arm/Makefile.common b/arm/Makefile.common
index f828dbe0..f107c478 100644
--- a/arm/Makefile.common
+++ b/arm/Makefile.common
@@ -5,6 +5,7 @@
  #
  
  tests-common  = $(TEST_DIR)/selftest.$(exe)

+tests-common += $(TEST_DIR)/selftest-migration.$(exe)
  tests-common += $(TEST_DIR)/spinlock-test.$(exe)
  tests-common += $(TEST_DIR)/pci-test.$(exe)
  tests-common += $(TEST_DIR)/pmu.$(exe)
diff --git a/arm/selftest-migration.c b/arm/selftest-migration.c
new file mode 12
index ..bd1eb266
--- /dev/null
+++ b/arm/selftest-migration.c
@@ -0,0 +1 @@
+../common/selftest-migration.c
\ No newline at end of file
diff --git a/arm/unittests.cfg b/arm/unittests.cfg
index fe601cbb..db0e4c9b 100644
--- a/arm/unittests.cfg
+++ b/arm/unittests.cfg
@@ -55,6 +55,12 @@ smp = $MAX_SMP
  extra_params = -append 'smp'
  groups = selftest
  
+# Test migration

+[selftest-migration]
+file = selftest-migration.flat
+groups = selftest migration
+arch = arm64
+
  # Test PCI emulation
  [pci-test]
  file = pci-test.flat
diff --git a/common/selftest-migration.c b/common/selftest-migration.c
new file mode 100644
index ..f70c505f
--- /dev/null
+++ b/common/selftest-migration.c
@@ -0,0 +1,34 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Machine independent migration tests
+ *
+ * This is just a very simple test that is intended to stress the migration
+ * support in the test harness. This could be expanded to test more guest
+ * library code, but architecture-specific tests should be used to test
+ * migration of tricky machine state.
+ */
+#include 
+#include 
+
+#if defined(__arm__) || defined(__aarch64__)
+/* arm can only call getchar 15 times */
+#define NR_MIGRATIONS 15
+#else
+#define NR_MIGRATIONS 100
+#endif


FYI, I just wrote a patch that will hopefully fix the limitation to 15 times 
on arm:


 https://lore.kernel.org/kvm/20240216140210.70280-1-th...@redhat.com/T/#u

 Thomas

Re: [kvm-unit-tests PATCH v4 8/8] migration: add a migration selftest

[Thomas Huth]

On 09/02/2024 10.11, Nicholas Piggin wrote:

Add a selftest for migration support in  guest library and test harness
code. It performs migrations in a tight loop to irritate races and bugs
in the test harness code.

Include the test in arm, s390, powerpc.

Acked-by: Claudio Imbrenda  (s390x)
Reviewed-by: Thomas Huth 
Signed-off-by: Nicholas Piggin 
---
  arm/Makefile.common  |  1 +
  arm/selftest-migration.c |  1 +
  arm/unittests.cfg|  6 ++


 Hi Nicholas,

I just gave the patches a try, but the arm test seems to fail for me: Only 
the first getchar() seems to wait for a character, all the subsequent ones 
don't wait anymore and just continue immediately ... is this working for 
you? Or do I need another patch on top?


 Thanks,
  Thomas

Re: [kvm-unit-tests PATCH v4 6/8] migration: Add quiet migration support

[Thomas Huth]

On 09/02/2024 10.11, Nicholas Piggin wrote:

Console output required to support migration becomes quite noisy
when doing lots of migrations. Provide a migrate_quiet() call that
suppresses console output and doesn't log a message.

Signed-off-by: Nicholas Piggin 
---
  lib/migrate.c | 11 +++
  lib/migrate.h |  1 +
  scripts/arch-run.bash |  4 ++--
  3 files changed, 14 insertions(+), 2 deletions(-)


Reviewed-by: Thomas Huth 

Re: [kvm-unit-tests PATCH v4 4/8] migration: Support multiple migrations

[Thomas Huth]

On 09/02/2024 10.11, Nicholas Piggin wrote:

Support multiple migrations by flipping dest file/socket variables to
source after the migration is complete, ready to start again. A new
destination is created if the test outputs the migrate line again.
Test cases may now switch to calling migrate() one or more times.

Signed-off-by: Nicholas Piggin 
---
  lib/migrate.c |  8 ++--
  lib/migrate.h |  1 +
  scripts/arch-run.bash | 86 ---
  3 files changed, 77 insertions(+), 18 deletions(-)


Reviewed-by: Thomas Huth 

Re: [kvm-unit-tests PATCH v4 2/8] arch-run: Clean up initrd cleanup

[Thomas Huth]

On 09/02/2024 10.11, Nicholas Piggin wrote:

Rather than put a big script into the trap handler, have it call
a function.

Signed-off-by: Nicholas Piggin 
---
  scripts/arch-run.bash | 13 -
  1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash
index 11d47a85..c1dd67ab 100644
--- a/scripts/arch-run.bash
+++ b/scripts/arch-run.bash
@@ -269,10 +269,21 @@ search_qemu_binary ()
export PATH=$save_path
  }
  
+initrd_cleanup ()

+{
+   rm -f $KVM_UNIT_TESTS_ENV
+   if [ "$KVM_UNIT_TESTS_ENV_OLD" ]; then
+   export KVM_UNIT_TESTS_ENV="$KVM_UNIT_TESTS_ENV_OLD"
+   else
+   unset KVM_UNIT_TESTS_ENV
+   fi
+   unset KVM_UNIT_TESTS_ENV_OLD
+}
+
  initrd_create ()
  {
if [ "$ENVIRON_DEFAULT" = "yes" ]; then
-   trap_exit_push 'rm -f $KVM_UNIT_TESTS_ENV; [ "$KVM_UNIT_TESTS_ENV_OLD" ] 
&& export KVM_UNIT_TESTS_ENV="$KVM_UNIT_TESTS_ENV_OLD" || unset KVM_UNIT_TESTS_ENV; unset 
KVM_UNIT_TESTS_ENV_OLD'
+   trap_exit_push 'initrd_cleanup'
[ -f "$KVM_UNIT_TESTS_ENV" ] && export 
KVM_UNIT_TESTS_ENV_OLD="$KVM_UNIT_TESTS_ENV"
    export KVM_UNIT_TESTS_ENV=$(mktemp)
env_params


Reviewed-by: Thomas Huth 

Re: [kvm-unit-tests PATCH v3 4/8] migration: Support multiple migrations

[Thomas Huth]

On 09/02/2024 09.39, Nicholas Piggin wrote:

On Fri Feb 9, 2024 at 6:19 PM AEST, Thomas Huth wrote:

On 09/02/2024 08.01, Nicholas Piggin wrote:

Support multiple migrations by flipping dest file/socket variables to
source after the migration is complete, ready to start again. A new
destination is created if the test outputs the migrate line again.
Test cases may now switch to calling migrate() one or more times.

Signed-off-by: Nicholas Piggin 
---

...

diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash
index 3689d7c2..a914ba17 100644
--- a/scripts/arch-run.bash
+++ b/scripts/arch-run.bash
@@ -129,12 +129,16 @@ run_migration ()
return 77
fi
   
+	migcmdline=$@

+
trap 'trap - TERM ; kill 0 ; exit 2' INT TERM
-   trap 'rm -f ${migout1} ${migout_fifo1} ${migsock} ${qmp1} ${qmp2} 
${fifo}' RETURN EXIT
+   trap 'rm -f ${migout1} ${migout2} ${migout_fifo1} ${migout_fifo2} 
${migsock} ${qmp1} ${qmp2} ${fifo}' RETURN EXIT
   
   	migsock=$(mktemp -u -t mig-helper-socket.XX)

migout1=$(mktemp -t mig-helper-stdout1.XX)
migout_fifo1=$(mktemp -u -t mig-helper-fifo-stdout1.XX)
+   migout2=$(mktemp -t mig-helper-stdout2.XX)
+   migout_fifo2=$(mktemp -u -t mig-helper-fifo-stdout2.XX)
qmp1=$(mktemp -u -t mig-helper-qmp1.XX)
qmp2=$(mktemp -u -t mig-helper-qmp2.XX)
fifo=$(mktemp -u -t mig-helper-fifo.XX)
@@ -142,18 +146,61 @@ run_migration ()
qmpout2=/dev/null
   
   	mkfifo ${migout_fifo1}

-   eval "$@" -chardev socket,id=mon1,path=${qmp1},server=on,wait=off \
+   mkfifo ${migout_fifo2}
+
+   eval "$migcmdline" \
+   -chardev socket,id=mon1,path=${qmp1},server=on,wait=off \
-mon chardev=mon1,mode=control > ${migout_fifo1} &
live_pid=$!
cat ${migout_fifo1} | tee ${migout1} &
   
-	# We have to use cat to open the named FIFO, because named FIFO's, unlike

-   # pipes, will block on open() until the other end is also opened, and 
that
-   # totally breaks QEMU...
+   # The test must prompt the user to migrate, so wait for the "migrate"
+   # keyword
+   while ! grep -q -i "Now migrate the VM" < ${migout1} ; do
+   if ! ps -p ${live_pid} > /dev/null ; then
+   echo "ERROR: Test exit before migration point." >&2
+   qmp ${qmp1} '"quit"'> ${qmpout1} 2>/dev/null
+   return 3
+   fi
+   sleep 0.1
+   done
+
+   # This starts the first source QEMU in advance of the test reaching the
+   # migration point, since we expect at least one migration. Subsequent
+   # sources are started as the test hits migrate keywords.
+   do_migration || return $?
+
+   while ps -p ${live_pid} > /dev/null ; do
+   # Wait for EXIT or further migrations
+   if ! grep -q -i "Now migrate the VM" < ${migout1} ; then
+   sleep 0.1
+   else
+   do_migration || return $?
+   fi
+   done
+
+   wait ${live_pid}
+   ret=$?
+
+   while (( $(jobs -r | wc -l) > 0 )); do
+   sleep 0.1
+   done
+
+   return $ret
+}
+
+do_migration ()
+{
+   # We have to use cat to open the named FIFO, because named FIFO's,
+   # unlike pipes, will block on open() until the other end is also
+   # opened, and that totally breaks QEMU...
mkfifo ${fifo}
-   eval "$@" -chardev socket,id=mon2,path=${qmp2},server=on,wait=off \
-   -mon chardev=mon2,mode=control -incoming unix:${migsock} < <(cat 
${fifo}) &
+   eval "$migcmdline" \
+   -chardev socket,id=mon2,path=${qmp2},server=on,wait=off \
+   -mon chardev=mon2,mode=control -incoming unix:${migsock} \
+   < <(cat ${fifo}) > ${migout_fifo2} &
incoming_pid=$!
+   cat ${migout_fifo2} | tee ${migout2} &
   
   	# The test must prompt the user to migrate, so wait for the "migrate" keyword

while ! grep -q -i "Now migrate the VM" < ${migout1} ; do


So the old check for the "migrate" keyword is also still around?


It's just the comment is staleish, it only checks "Now migrate...".


Why do we
need to wait on two spots for the "Now mirgrate..." string now?


So that the it ensures we do one migration, subsequent ones are
optional.

I was thinking we could just remove that, and possibly even
remove the MIGRATION=yes/no paths and always just use the same
code here. But that's for another time.

Actually there is some weirdness here. There are *three* spots
where it waits for migration.


Yes, that's what I meant (I considered your two new additions like one

Re: [kvm-unit-tests PATCH v3 8/8] migration: add a migration selftest

[Thomas Huth]

On 09/02/2024 08.01, Nicholas Piggin wrote:

Add a selftest for migration support in  guest library and test harness
code. It performs migrations a tight loop to irritate races and bugs in


"*in* a tight loop" ?


the test harness code.

Include the test in arm, s390, powerpc.

Acked-by: Claudio Imbrenda  (s390x)
Signed-off-by: Nicholas Piggin 
---
This has flushed out several bugs in developing the multi migration test
harness code already.

Thanks,
Nick

  arm/Makefile.common  |  1 +
  arm/selftest-migration.c |  1 +
  arm/unittests.cfg|  6 ++
  common/selftest-migration.c  | 34 ++
  powerpc/Makefile.common  |  1 +
  powerpc/selftest-migration.c |  1 +
  powerpc/unittests.cfg|  4 
  s390x/Makefile   |  1 +
  s390x/selftest-migration.c   |  1 +
  s390x/unittests.cfg  |  4 
  10 files changed, 54 insertions(+)
  create mode 12 arm/selftest-migration.c
  create mode 100644 common/selftest-migration.c
  create mode 12 powerpc/selftest-migration.c
  create mode 12 s390x/selftest-migration.c

diff --git a/arm/Makefile.common b/arm/Makefile.common
index f828dbe0..f107c478 100644
--- a/arm/Makefile.common
+++ b/arm/Makefile.common
@@ -5,6 +5,7 @@
  #
  
  tests-common  = $(TEST_DIR)/selftest.$(exe)

+tests-common += $(TEST_DIR)/selftest-migration.$(exe)
  tests-common += $(TEST_DIR)/spinlock-test.$(exe)
  tests-common += $(TEST_DIR)/pci-test.$(exe)
  tests-common += $(TEST_DIR)/pmu.$(exe)
diff --git a/arm/selftest-migration.c b/arm/selftest-migration.c
new file mode 12
index ..bd1eb266
--- /dev/null
+++ b/arm/selftest-migration.c
@@ -0,0 +1 @@
+../common/selftest-migration.c
\ No newline at end of file
diff --git a/arm/unittests.cfg b/arm/unittests.cfg
index fe601cbb..1ffd9a82 100644
--- a/arm/unittests.cfg
+++ b/arm/unittests.cfg
@@ -55,6 +55,12 @@ smp = $MAX_SMP
  extra_params = -append 'smp'
  groups = selftest
  
+# Test migration

+[selftest-migration]
+file = selftest-migration.flat
+groups = selftest migration
+
+arch = arm64


Please swap the last two lines!


  # Test PCI emulation
  [pci-test]
  file = pci-test.flat


With the nits fixed:
Reviewed-by: Thomas Huth 

Re: [kvm-unit-tests PATCH v3 7/8] Add common/ directory for architecture-independent tests

[Thomas Huth]

On 09/02/2024 08.01, Nicholas Piggin wrote:

x86/sieve.c is used by s390x, arm, and riscv via symbolic link. Make a
new directory common/ for architecture-independent tests and move
sieve.c here.

Signed-off-by: Nicholas Piggin 
---
  arm/sieve.c|  2 +-
  common/sieve.c | 51 +
  riscv/sieve.c  |  2 +-
  s390x/sieve.c  |  2 +-
  x86/sieve.c| 52 +-
  5 files changed, 55 insertions(+), 54 deletions(-)
  create mode 100644 common/sieve.c
  mode change 100644 => 12 x86/sieve.c



Reviewed-by: Thomas Huth 

Re: [kvm-unit-tests PATCH v3 5/8] arch-run: rename migration variables

[Thomas Huth]

On 09/02/2024 08.01, Nicholas Piggin wrote:

Using 1 and 2 for source and destination is confusing, particularly
now with multiple migrations that flip between them. Do a rename
pass to tidy things up.

Signed-off-by: Nicholas Piggin 
---
  scripts/arch-run.bash | 115 +-
  1 file changed, 58 insertions(+), 57 deletions(-)



Acked-by: Thomas Huth 

Re: [kvm-unit-tests PATCH v3 4/8] migration: Support multiple migrations

[Thomas Huth]

On 09/02/2024 08.01, Nicholas Piggin wrote:

Support multiple migrations by flipping dest file/socket variables to
source after the migration is complete, ready to start again. A new
destination is created if the test outputs the migrate line again.
Test cases may now switch to calling migrate() one or more times.

Signed-off-by: Nicholas Piggin 
---

...

diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash
index 3689d7c2..a914ba17 100644
--- a/scripts/arch-run.bash
+++ b/scripts/arch-run.bash
@@ -129,12 +129,16 @@ run_migration ()
return 77
fi
  
+	migcmdline=$@

+
trap 'trap - TERM ; kill 0 ; exit 2' INT TERM
-   trap 'rm -f ${migout1} ${migout_fifo1} ${migsock} ${qmp1} ${qmp2} 
${fifo}' RETURN EXIT
+   trap 'rm -f ${migout1} ${migout2} ${migout_fifo1} ${migout_fifo2} 
${migsock} ${qmp1} ${qmp2} ${fifo}' RETURN EXIT
  
  	migsock=$(mktemp -u -t mig-helper-socket.XX)

migout1=$(mktemp -t mig-helper-stdout1.XX)
migout_fifo1=$(mktemp -u -t mig-helper-fifo-stdout1.XX)
+   migout2=$(mktemp -t mig-helper-stdout2.XX)
+   migout_fifo2=$(mktemp -u -t mig-helper-fifo-stdout2.XX)
qmp1=$(mktemp -u -t mig-helper-qmp1.XX)
qmp2=$(mktemp -u -t mig-helper-qmp2.XX)
fifo=$(mktemp -u -t mig-helper-fifo.XX)
@@ -142,18 +146,61 @@ run_migration ()
qmpout2=/dev/null
  
  	mkfifo ${migout_fifo1}

-   eval "$@" -chardev socket,id=mon1,path=${qmp1},server=on,wait=off \
+   mkfifo ${migout_fifo2}
+
+   eval "$migcmdline" \
+   -chardev socket,id=mon1,path=${qmp1},server=on,wait=off \
-mon chardev=mon1,mode=control > ${migout_fifo1} &
live_pid=$!
cat ${migout_fifo1} | tee ${migout1} &
  
-	# We have to use cat to open the named FIFO, because named FIFO's, unlike

-   # pipes, will block on open() until the other end is also opened, and 
that
-   # totally breaks QEMU...
+   # The test must prompt the user to migrate, so wait for the "migrate"
+   # keyword
+   while ! grep -q -i "Now migrate the VM" < ${migout1} ; do
+   if ! ps -p ${live_pid} > /dev/null ; then
+   echo "ERROR: Test exit before migration point." >&2
+   qmp ${qmp1} '"quit"'> ${qmpout1} 2>/dev/null
+   return 3
+   fi
+   sleep 0.1
+   done
+
+   # This starts the first source QEMU in advance of the test reaching the
+   # migration point, since we expect at least one migration. Subsequent
+   # sources are started as the test hits migrate keywords.
+   do_migration || return $?
+
+   while ps -p ${live_pid} > /dev/null ; do
+   # Wait for EXIT or further migrations
+   if ! grep -q -i "Now migrate the VM" < ${migout1} ; then
+   sleep 0.1
+   else
+   do_migration || return $?
+   fi
+   done
+
+   wait ${live_pid}
+   ret=$?
+
+   while (( $(jobs -r | wc -l) > 0 )); do
+   sleep 0.1
+   done
+
+   return $ret
+}
+
+do_migration ()
+{
+   # We have to use cat to open the named FIFO, because named FIFO's,
+   # unlike pipes, will block on open() until the other end is also
+   # opened, and that totally breaks QEMU...
mkfifo ${fifo}
-   eval "$@" -chardev socket,id=mon2,path=${qmp2},server=on,wait=off \
-   -mon chardev=mon2,mode=control -incoming unix:${migsock} < <(cat 
${fifo}) &
+   eval "$migcmdline" \
+   -chardev socket,id=mon2,path=${qmp2},server=on,wait=off \
+   -mon chardev=mon2,mode=control -incoming unix:${migsock} \
+   < <(cat ${fifo}) > ${migout_fifo2} &
incoming_pid=$!
+   cat ${migout_fifo2} | tee ${migout2} &
  
  	# The test must prompt the user to migrate, so wait for the "migrate" keyword

while ! grep -q -i "Now migrate the VM" < ${migout1} ; do


So the old check for the "migrate" keyword is also still around? Why do we 
need to wait on two spots for the "Now mirgrate..." string now?


 Thomas



@@ -164,7 +211,7 @@ run_migration ()
qmp ${qmp2} '"quit"'> ${qmpout2} 2>/dev/null
return 3
fi
-   sleep 1
+   sleep 0.1
done
  
  	# Wait until the destination has created the incoming and qmp sockets

@@ -176,7 +223,7 @@ run_migration ()
# Wait for the migration to complete
migstatus=`qmp ${qmp1} '"query-migrate"' | grep return`
while ! grep -q '"completed"' <<<"$migstatus" ; do
-   sleep 1
+   sleep 0.1
if ! migstatus=`qmp ${qmp1} '"query-migrate"'`; then
echo "ERROR: Querying migration state failed." >&2
echo > ${fifo}
@@ -192,14 +239,34 @@ run_migration ()
 

Re: [kvm-unit-tests PATCH v3 6/8] migration: Add quiet migration support

[Thomas Huth]

On 09/02/2024 08.01, Nicholas Piggin wrote:

Console output required to support migration becomes quite noisy
when doing lots of migrations. Provide a migrate_quiet() call that
suppresses console output and doesn't log a message.

Signed-off-by: Nicholas Piggin 
---
  lib/migrate.c | 12 
  lib/migrate.h |  1 +
  scripts/arch-run.bash |  4 ++--
  3 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/lib/migrate.c b/lib/migrate.c
index b7721659..4e0ab516 100644
--- a/lib/migrate.c
+++ b/lib/migrate.c
@@ -18,6 +18,18 @@ void migrate(void)
report_info("Migration complete");
  }
  
+/*

+ * Like migrate() but supporess output and logs, useful for intensive


s/supporess/suppress/


+ * migration stress testing without polluting logs. Test cases should
+ * provide relevant information about migration in failure reports.
+ */
+void migrate_quiet(void)
+{
+   puts("Now migrate the VM (quiet)\n");
+   (void)getchar();
+}
+
+


Remove one empty line, please!


  /*
   * Initiate migration and wait for it to complete.
   * If this function is called more than once, it is a no-op.
diff --git a/lib/migrate.h b/lib/migrate.h
index 2af06a72..95b9102b 100644
--- a/lib/migrate.h
+++ b/lib/migrate.h
@@ -7,4 +7,5 @@
   */
  
  void migrate(void);

+void migrate_quiet(void);
  void migrate_once(void);
diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash
index 0b45eb61..29cf9b0c 100644
--- a/scripts/arch-run.bash
+++ b/scripts/arch-run.bash
@@ -152,7 +152,7 @@ run_migration ()
-chardev socket,id=mon,path=${src_qmp},server=on,wait=off \
-mon chardev=mon,mode=control > ${src_outfifo} &
live_pid=$!
-   cat ${src_outfifo} | tee ${src_out} &
+   cat ${src_outfifo} | tee ${src_out} | grep -v "Now migrate the VM (quiet)" 
&
  
  	# The test must prompt the user to migrate, so wait for the "migrate"

# keyword
@@ -200,7 +200,7 @@ do_migration ()
-mon chardev=mon,mode=control -incoming unix:${dst_incoming} \
< <(cat ${dst_infifo}) > ${dst_outfifo} &
incoming_pid=$!
-   cat ${dst_outfifo} | tee ${dst_out} &
+   cat ${dst_outfifo} | tee ${dst_out} | grep -v "Now migrate the VM (quiet)" 
&
  
  	# The test must prompt the user to migrate, so wait for the "migrate" keyword

while ! grep -q -i "Now migrate the VM" < ${src_out} ; do


 Thomas

Re: [kvm-unit-tests PATCH v3 3/8] migration: use a more robust way to wait for background job

[Thomas Huth]

On 09/02/2024 08.01, Nicholas Piggin wrote:

Starting a pipeline of jobs in the background does not seem to have
a simple way to reliably find the pid of a particular process in the
pipeline (because not all processes are started when the shell
continues to execute).

The way PID of QEMU is derived can result in a failure waiting on a
PID that is not running. This is easier to hit with subsequent
multiple-migration support. Changing this to use $! by swapping the
pipeline for a fifo is more robust.

Signed-off-by: Nicholas Piggin 
---
  scripts/arch-run.bash | 15 +++
  1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash
index 1e903e83..3689d7c2 100644
--- a/scripts/arch-run.bash
+++ b/scripts/arch-run.bash
@@ -130,19 +130,22 @@ run_migration ()
fi
  
  	trap 'trap - TERM ; kill 0 ; exit 2' INT TERM

-   trap 'rm -f ${migout1} ${migsock} ${qmp1} ${qmp2} ${fifo}' RETURN EXIT
+   trap 'rm -f ${migout1} ${migout_fifo1} ${migsock} ${qmp1} ${qmp2} 
${fifo}' RETURN EXIT
  
  	migsock=$(mktemp -u -t mig-helper-socket.XX)

migout1=$(mktemp -t mig-helper-stdout1.XX)
+   migout_fifo1=$(mktemp -u -t mig-helper-fifo-stdout1.XX)
qmp1=$(mktemp -u -t mig-helper-qmp1.XX)
qmp2=$(mktemp -u -t mig-helper-qmp2.XX)
fifo=$(mktemp -u -t mig-helper-fifo.XX)
qmpout1=/dev/null
qmpout2=/dev/null
  
+	mkfifo ${migout_fifo1}

eval "$@" -chardev socket,id=mon1,path=${qmp1},server=on,wait=off \
-   -mon chardev=mon1,mode=control | tee ${migout1} &
-   live_pid=`jobs -l %+ | grep "eval" | awk '{print$2}'`
+   -mon chardev=mon1,mode=control > ${migout_fifo1} &
+   live_pid=$!
+   cat ${migout_fifo1} | tee ${migout1} &
  
  	# We have to use cat to open the named FIFO, because named FIFO's, unlike

# pipes, will block on open() until the other end is also opened, and 
that
@@ -150,7 +153,7 @@ run_migration ()
mkfifo ${fifo}
eval "$@" -chardev socket,id=mon2,path=${qmp2},server=on,wait=off \
-mon chardev=mon2,mode=control -incoming unix:${migsock} < <(cat 
${fifo}) &
-   incoming_pid=`jobs -l %+ | awk '{print$2}'`
+   incoming_pid=$!
  
  	# The test must prompt the user to migrate, so wait for the "migrate" keyword

while ! grep -q -i "Now migrate the VM" < ${migout1} ; do
@@ -164,6 +167,10 @@ run_migration ()
sleep 1
done
  
+	# Wait until the destination has created the incoming and qmp sockets

+   while ! [ -S ${migsock} ] ; do sleep 0.1 ; done
+   while ! [ -S ${qmp2} ] ; do sleep 0.1 ; done
+
qmp ${qmp1} '"migrate", "arguments": { "uri": "unix:'${migsock}'" }' > 
${qmpout1}
  
  	# Wait for the migration to complete


Reviewed-by: Thomas Huth 

Re: [kvm-unit-tests PATCH v3 2/8] arch-run: Clean up initrd cleanup

[Thomas Huth]

On 09/02/2024 08.01, Nicholas Piggin wrote:

Rather than put a big script into the trap handler, have it call
a function.

Signed-off-by: Nicholas Piggin 
---
  scripts/arch-run.bash | 13 -
  1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash
index 11d47a85..1e903e83 100644
--- a/scripts/arch-run.bash
+++ b/scripts/arch-run.bash
@@ -269,10 +269,21 @@ search_qemu_binary ()
export PATH=$save_path
  }
  
+initrd_cleanup ()

+{
+   rm -f $KVM_UNIT_TESTS_ENV
+   if [ "$KVM_UNIT_TESTS_ENV_OLD" ]; then
+   export KVM_UNIT_TESTS_ENV="$KVM_UNIT_TESTS_ENV_OLD"
+   else
+   unset KVM_UNIT_TESTS_ENV
+   unset KVM_UNIT_TESTS_ENV_OLD
+   fi
+}


Looking at the original code below, shouldn't this rather unset 
KVM_UNIT_TESTS_ENV_OLD after the "fi" statement?


 Thomas



  initrd_create ()
  {
if [ "$ENVIRON_DEFAULT" = "yes" ]; then
-   trap_exit_push 'rm -f $KVM_UNIT_TESTS_ENV; [ "$KVM_UNIT_TESTS_ENV_OLD" ] 
&& export KVM_UNIT_TESTS_ENV="$KVM_UNIT_TESTS_ENV_OLD" || unset KVM_UNIT_TESTS_ENV; unset 
KVM_UNIT_TESTS_ENV_OLD'
+   trap_exit_push 'initrd_cleanup'
[ -f "$KVM_UNIT_TESTS_ENV" ] && export 
KVM_UNIT_TESTS_ENV_OLD="$KVM_UNIT_TESTS_ENV"
export KVM_UNIT_TESTS_ENV=$(mktemp)
env_params

Re: [kvm-unit-tests PATCH v3 1/8] arch-run: Fix TRAP handler recursion to remove temporary files properly

[Thomas Huth]

On 09/02/2024 08.01, Nicholas Piggin wrote:

Migration files were not being removed when the QEMU process is
interrupted (e.g., with ^C). This is becaus the SIGINT propagates to the
bash TRAP handler, which recursively TRAPs due to the 'kill 0' in the
handler. This eventually crashes bash.

This can be observed by interrupting a long-running test program that is
run with MIGRATION=yes, /tmp/mig-helper-* files remain afterwards.

Removing TRAP recursion solves this problem and allows the EXIT handler
to run and clean up the files.

This also moves the trap handler before temp file creation, and expands
the name variables at trap-time rather than install-time, which closes
the small race between creation trap handler install.

Reviewed-by: Thomas Huth 
Signed-off-by: Nicholas Piggin 
---
  scripts/arch-run.bash | 12 ++--
  1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash
index d0864360..11d47a85 100644
--- a/scripts/arch-run.bash
+++ b/scripts/arch-run.bash
@@ -129,6 +129,9 @@ run_migration ()
return 77
fi
  
+	trap 'trap - TERM ; kill 0 ; exit 2' INT TERM

+   trap 'rm -f ${migout1} ${migsock} ${qmp1} ${qmp2} ${fifo}' RETURN EXIT
+
migsock=$(mktemp -u -t mig-helper-socket.XX)
migout1=$(mktemp -t mig-helper-stdout1.XX)
qmp1=$(mktemp -u -t mig-helper-qmp1.XX)
@@ -137,9 +140,6 @@ run_migration ()
qmpout1=/dev/null
qmpout2=/dev/null
  
-	trap 'kill 0; exit 2' INT TERM

-   trap 'rm -f ${migout1} ${migsock} ${qmp1} ${qmp2} ${fifo}' RETURN EXIT
-
eval "$@" -chardev socket,id=mon1,path=${qmp1},server=on,wait=off \
-mon chardev=mon1,mode=control | tee ${migout1} &
live_pid=`jobs -l %+ | grep "eval" | awk '{print$2}'`
@@ -209,11 +209,11 @@ run_panic ()
return 77
fi
  
-	qmp=$(mktemp -u -t panic-qmp.XX)

-
-   trap 'kill 0; exit 2' INT TERM
+   trap 'trap - TERM ; kill 0 ; exit 2' INT TERM
trap 'rm -f ${qmp}' RETURN EXIT
  
+	qmp=$(mktemp -u -t panic-qmp.XX)

+
# start VM stopped so we don't miss any events
eval "$@" -chardev socket,id=mon1,path=${qmp},server=on,wait=off \
-mon chardev=mon1,mode=control -S &


Reviewed-by: Thomas Huth 

Re: [kvm-unit-tests PATCH v2 2/9] arch-run: Clean up temporary files properly

[Thomas Huth]

On 02/02/2024 07.57, Nicholas Piggin wrote:

Migration files weren't being removed when tests were interrupted.
This improves the situation.

Signed-off-by: Nicholas Piggin 
---
  scripts/arch-run.bash | 12 +++-
  1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash
index d0864360..f22ead6f 100644
--- a/scripts/arch-run.bash
+++ b/scripts/arch-run.bash
@@ -134,12 +134,14 @@ run_migration ()
qmp1=$(mktemp -u -t mig-helper-qmp1.XX)
qmp2=$(mktemp -u -t mig-helper-qmp2.XX)
fifo=$(mktemp -u -t mig-helper-fifo.XX)
+
+   # race here between file creation and trap
+   trap "trap - TERM ; kill 0 ; exit 2" INT TERM
+   trap "rm -f ${migout1} ${migsock} ${qmp1} ${qmp2} ${fifo}" RETURN EXIT
+
qmpout1=/dev/null
qmpout2=/dev/null
  
-	trap 'kill 0; exit 2' INT TERM

-   trap 'rm -f ${migout1} ${migsock} ${qmp1} ${qmp2} ${fifo}' RETURN EXIT
-
eval "$@" -chardev socket,id=mon1,path=${qmp1},server=on,wait=off \
-mon chardev=mon1,mode=control | tee ${migout1} &
live_pid=`jobs -l %+ | grep "eval" | awk '{print$2}'`
@@ -211,8 +213,8 @@ run_panic ()
  
  	qmp=$(mktemp -u -t panic-qmp.XX)
  
-	trap 'kill 0; exit 2' INT TERM

-   trap 'rm -f ${qmp}' RETURN EXIT
+   trap "trap - TERM ; kill 0 ; exit 2" INT TERM
+   trap "rm -f ${qmp}" RETURN EXIT
  
  	# start VM stopped so we don't miss any events

eval "$@" -chardev socket,id=mon1,path=${qmp},server=on,wait=off \


So the point is that the "EXIT" trap wasn't executed without the "trap - 
TERM" in the other trap? ... ok, then your patch certainly makes sense.


Reviewed-by: Thomas Huth 

Re: [kvm-unit-tests PATCH v2 3/9] arch-run: Clean up initrd cleanup

[Thomas Huth]

On 02/02/2024 07.57, Nicholas Piggin wrote:

Rather than put a big script into the trap handler, have it call
a function.

Signed-off-by: Nicholas Piggin 
---
  scripts/arch-run.bash | 12 +++-
  1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash
index f22ead6f..cc7da7c5 100644
--- a/scripts/arch-run.bash
+++ b/scripts/arch-run.bash
@@ -271,10 +271,20 @@ search_qemu_binary ()
export PATH=$save_path
  }
  
+initrd_cleanup ()

+{
+   if [ "$KVM_UNIT_TESTS_ENV_OLD" ]; then
+   export KVM_UNIT_TESTS_ENV="$KVM_UNIT_TESTS_ENV_OLD"
+   else
+   unset KVM_UNIT_TESTS_ENV
+   unset KVM_UNIT_TESTS_ENV_OLD
+   fi
+}
+
  initrd_create ()
  {
if [ "$ENVIRON_DEFAULT" = "yes" ]; then
-   trap_exit_push 'rm -f $KVM_UNIT_TESTS_ENV; [ "$KVM_UNIT_TESTS_ENV_OLD" ] 
&& export KVM_UNIT_TESTS_ENV="$KVM_UNIT_TESTS_ENV_OLD" || unset KVM_UNIT_TESTS_ENV; unset 
KVM_UNIT_TESTS_ENV_OLD'
+   trap_exit_push 'rm -f $KVM_UNIT_TESTS_ENV; initrd_cleanup'



Why don't you move the 'rm -f $KVM_UNIT_TESTS_ENV' into the initrd_cleanup() 
function, too? ... that would IMHO make more sense for a function that is 
called *_cleanup() ?


 Thomas

Re: [kvm-unit-tests PATCH v2 1/9] (arm|powerpc|s390x): Makefile: Fix .aux.o generation

[Thomas Huth]

On 02/02/2024 10.30, Andrew Jones wrote:

On Fri, Feb 02, 2024 at 04:57:32PM +1000, Nicholas Piggin wrote:

Using all prerequisites for the source file results in the build
dying on the second time around with:

gcc: fatal error: cannot specify ‘-o’ with ‘-c’, ‘-S’ or ‘-E’ with multiple 
files

This is due to auxinfo.h becoming a prerequisite after the first
build recorded the dependency.


D'oh, of course I only tried to run "make" once when testing that patch :-/


diff --git a/arm/Makefile.common b/arm/Makefile.common
index 54cb4a63..c2ee568c 100644
--- a/arm/Makefile.common
+++ b/arm/Makefile.common
@@ -71,7 +71,7 @@ FLATLIBS = $(libcflat) $(LIBFDT_archive) $(libeabi)
  
  ifeq ($(CONFIG_EFI),y)

  %.aux.o: $(SRCDIR)/lib/auxinfo.c
-   $(CC) $(CFLAGS) -c -o $@ $^ \
+   $(CC) $(CFLAGS) -c -o $@ $< \
-DPROGNAME=\"$(@:.aux.o=.efi)\" -DAUXFLAGS=$(AUXFLAGS)


There are two instances of the %.aux.o target in arm/Makefile.common. We
need to fix both. We can actually pull the target out of the two arms of
the CONFIG_EFI if-else, though, by changing the .efi/.flat to .$(exe).


I went ahead and pushed this patch with the trivial fix for the else-branch 
to the repo to unbreak the build. If you think it's worthwhile to unify the 
target, please provide a patch to do so, thanks!


 Thomas

Re: [kvm-unit-tests PATCH v5 29/29] powerpc: Add timebase tests

[Thomas Huth]

On 16/12/2023 14.42, Nicholas Piggin wrote:

This has a known failure on QEMU TCG machines where the decrementer
interrupt is not lowered when the DEC wraps from -ve to +ve.

Signed-off-by: Nicholas Piggin 
---
  lib/powerpc/asm/ppc_asm.h   |   1 +
  lib/powerpc/asm/processor.h |  22 +++
  powerpc/Makefile.common |   1 +
  powerpc/smp.c   |  22 ---
  powerpc/timebase.c  | 328 
  powerpc/unittests.cfg   |   8 +
  6 files changed, 360 insertions(+), 22 deletions(-)
  create mode 100644 powerpc/timebase.c

...

diff --git a/powerpc/timebase.c b/powerpc/timebase.c
new file mode 100644
index ..4d80ea09
--- /dev/null
+++ b/powerpc/timebase.c
@@ -0,0 +1,328 @@
+/*
+ * Test Timebase
+ *
+ * Copyright 2017  Thomas Huth, Red Hat Inc.


No, not really. Please update ;-)

 Thomas

Re: [kvm-unit-tests PATCH v5 25/29] powerpc: Add rtas stop-self support

[Thomas Huth]

On 16/12/2023 14.42, Nicholas Piggin wrote:

In preparation for improved SMP support, add stop-self support to the
harness. This is non-trivial because it requires an unlocked rtas
call: a CPU can't be holding a spin lock when it goes offline or it
will deadlock other CPUs. rtas permits stop-self to be called without
serialising all other rtas operations.

Signed-off-by: Nicholas Piggin 
---
  lib/powerpc/asm/rtas.h |  2 ++
  lib/powerpc/rtas.c | 78 +-
  2 files changed, 64 insertions(+), 16 deletions(-)

...

+void rtas_stop_self(void)
+{
+   struct rtas_args args;
+   uint32_t token;
+   int ret;
+
+   ret = rtas_token("stop-self", );
+   if (ret) {
+   puts("RTAS stop-self not available\n");
+   return;
+   }
+
+   ret = rtas_call_unlocked(, token, 0, 1, NULL);
+   printf("RTAS stop-self returnd %d\n", ret);


s/returnd/returned/


+}


With the typo fixed:

Reviewed-by: Thomas Huth 

Re: [kvm-unit-tests PATCH v5 24/29] powerpc: interrupt tests

[Thomas Huth]

On 16/12/2023 14.42, Nicholas Piggin wrote:

Add basic testing of various kinds of interrupts, machine check,
page fault, illegal, decrementer, trace, syscall, etc.

This has a known failure on QEMU TCG pseries machines where MSR[ME]
can be incorrectly set to 0.

Signed-off-by: Nicholas Piggin 
---
  lib/powerpc/asm/ppc_asm.h |  21 +-
  powerpc/Makefile.common   |   3 +-
  powerpc/interrupts.c  | 422 ++
  powerpc/unittests.cfg |   3 +
  4 files changed, 445 insertions(+), 4 deletions(-)
  create mode 100644 powerpc/interrupts.c

diff --git a/lib/powerpc/asm/ppc_asm.h b/lib/powerpc/asm/ppc_asm.h
index ef2d91dd..778e78ee 100644
--- a/lib/powerpc/asm/ppc_asm.h
+++ b/lib/powerpc/asm/ppc_asm.h
@@ -35,17 +35,32 @@
  
  #endif /* __BYTE_ORDER__ */
  
+#define SPR_DSISR	0x012

+#define SPR_DAR0x013
+#define SPR_DEC0x016
+#define SPR_SRR0   0x01A
+#define SPR_SRR1   0x01B
+#define SPR_FSCR   0x099
+#define   FSCR_PREFIX  0x2000
+#define SPR_HDEC   0x136
  #define SPR_HSRR0 0x13A
  #define SPR_HSRR1 0x13B
+#define SPR_LPCR   0x13E
+#define   LPCR_HDICE   0x1UL
+#define SPR_HEIR   0x153
+#define SPR_SIAR   0x31C
  
  /* Machine State Register definitions: */

  #define MSR_LE_BIT0
  #define MSR_EE_BIT15  /* External Interrupts Enable */
  #define MSR_HV_BIT60  /* Hypervisor mode */
  #define MSR_SF_BIT63  /* 64-bit mode */
-#define MSR_ME 0x1000ULL
  
-#define SPR_HSRR0	0x13A

-#define SPR_HSRR1  0x13B
+#define MSR_DR 0x0010ULL
+#define MSR_IR 0x0020ULL
+#define MSR_BE 0x0200ULL   /* Branch Trace Enable */
+#define MSR_SE 0x0400ULL   /* Single Step Enable */
+#define MSR_EE 0x8000ULL
+#define MSR_ME 0x1000ULL
  
  #endif /* _ASMPOWERPC_PPC_ASM_H */

diff --git a/powerpc/Makefile.common b/powerpc/Makefile.common
index a7af225b..b340a53b 100644
--- a/powerpc/Makefile.common
+++ b/powerpc/Makefile.common
@@ -11,7 +11,8 @@ tests-common = \
$(TEST_DIR)/rtas.elf \
$(TEST_DIR)/emulator.elf \
$(TEST_DIR)/tm.elf \
-   $(TEST_DIR)/sprs.elf
+   $(TEST_DIR)/sprs.elf \
+   $(TEST_DIR)/interrupts.elf
  
  tests-all = $(tests-common) $(tests)

  all: directories $(TEST_DIR)/boot_rom.bin $(tests-all)
diff --git a/powerpc/interrupts.c b/powerpc/interrupts.c
new file mode 100644
index ..3217b15e
--- /dev/null
+++ b/powerpc/interrupts.c
@@ -0,0 +1,422 @@
+/*
+ * Test interrupts
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.
+ */
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define SPR_LPCR   0x13E
+#define LPCR_HDICE 0x1UL
+#define SPR_DEC0x016
+#define SPR_HDEC   0x136
+
+#define MSR_DR 0x0010ULL
+#define MSR_IR 0x0020ULL
+#define MSR_EE 0x8000ULL
+#define MSR_ME 0x1000ULL


Why don't you use the definitions from ppc_asm.h above?


+static bool cpu_has_heir(void)
+{
+   uint32_t pvr = mfspr(287);  /* Processor Version Register */
+
+   if (!machine_is_powernv())
+   return false;
+
+   /* POWER6 has HEIR, but QEMU powernv support does not go that far */
+   switch (pvr >> 16) {
+   case 0x4b:  /* POWER8E */
+   case 0x4c:  /* POWER8NVL */
+   case 0x4d:  /* POWER8 */
+   case 0x4e:  /* POWER9 */
+   case 0x80:  /* POWER10 */


I'd suggest to introduce some #defines for those PVR values instead of using 
magic numbers all over the place?



+   return true;
+   default:
+   return false;
+   }
+}
+
+static bool cpu_has_prefix(void)
+{
+   uint32_t pvr = mfspr(287);  /* Processor Version Register */
+   switch (pvr >> 16) {
+   case 0x80:  /* POWER10 */
+   return true;
+   default:
+   return false;
+   }
+}
+
+static bool cpu_has_lev_in_srr1(void)
+{
+   uint32_t pvr = mfspr(287);  /* Processor Version Register */
+   switch (pvr >> 16) {
+   case 0x80:  /* POWER10 */
+   return true;
+   default:
+   return false;
+   }
+}
+
+static bool regs_is_prefix(volatile struct pt_regs *regs)
+{
+   return (regs->msr >> (63-34)) & 1;


You introduced a bunch of new #define MSR_xx statements ... why not for this 
one, too?



+}
+
+static void regs_advance_insn(struct pt_regs *regs)
+{
+   if (regs_is_prefix(regs))
+   regs->nip += 8;
+   else
+   regs->nip += 4;
+}
+
+static volatile bool got_interrupt;
+static volatile struct pt_regs recorded_regs;
+
+static void mce_handler(struct pt_regs *regs, void *opaque)
+{
+   got_interrupt = true;
+   

Re: [kvm-unit-tests PATCH v5 22/29] powerpc: Fix emulator illegal instruction test for powernv

[Thomas Huth]

On 16/12/2023 14.42, Nicholas Piggin wrote:

Illegal instructions cause 0xe40 (HEAI) interrupts rather
than program interrupts.

Signed-off-by: Nicholas Piggin 
---
  powerpc/emulator.c | 19 ++-
  1 file changed, 18 insertions(+), 1 deletion(-)


Acked-by: Thomas Huth 

Re: [kvm-unit-tests PATCH v5 20/29] scripts: Accommodate powerpc powernv machine differences

[Thomas Huth]

On 16/12/2023 14.42, Nicholas Piggin wrote:

The QEMU powerpc powernv machine has minor differences that must be
accommodated for in output parsing:

- Summary parsing must search more lines of output for the summary
   line, to accommodate OPAL message on shutdown.
- Premature failure testing must tolerate case differences in kernel
   load error message.

Signed-off-by: Nicholas Piggin 
---
  scripts/runtime.bash | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)


Acked-by: Thomas Huth 

Re: [kvm-unit-tests PATCH v5 19/29] scripts: allow machine option to be specified in unittests.cfg

[Thomas Huth]

On 16/12/2023 14.42, Nicholas Piggin wrote:

This allows different machines with different requirements to be
supported by run_tests.sh, similarly to how different accelerators
are handled.

Signed-off-by: Nicholas Piggin 
---
  scripts/common.bash  |  8 ++--
  scripts/runtime.bash | 16 
  2 files changed, 18 insertions(+), 6 deletions(-)


Acked-by: Thomas Huth 

Re: [kvm-unit-tests PATCH v5 18/29] powerpc: Fix stack backtrace termination

[Thomas Huth]

On 16/12/2023 14.42, Nicholas Piggin wrote:

The backtrace handler terminates when it sees a NULL caller address,
but the powerpc stack setup does not keep such a NULL caller frame
at the start of the stack.

This happens to work on pseries because the memory at 0 is mapped and
it contains 0 at the location of the return address pointer if it
were a stack frame. But this is fragile, and does not work with powernv
where address 0 contains firmware instructions.

Use the existing dummy frame on stack as the NULL caller, and create a
new frame on stack for the entry code.

Signed-off-by: Nicholas Piggin 
---
  powerpc/cstart64.S | 12 ++--
  1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/powerpc/cstart64.S b/powerpc/cstart64.S
index e18ae9a2..14ab0c6c 100644
--- a/powerpc/cstart64.S
+++ b/powerpc/cstart64.S
@@ -46,8 +46,16 @@ start:
add r1, r1, r31
add r2, r2, r31
  
+	/* Zero backpointers in initial stack frame so backtrace() stops */

+   li  r0,0
+   std r0,0(r1)
+   std r0,16(r1)
+
+   /* Create entry frame */
+   stdur1,-INT_FRAME_SIZE(r1)


Shouldn't that rather be STACK_FRAME_OVERHEAD instead of INT_FRAME_SIZE...


/* save DTB pointer */
-   std r3, 56(r1)
+   SAVE_GPR(3,r1)


... since SAVE_GPR uses STACK_FRAME_OVERHEAD (via GPR0), too?

 Thomas

Re: [kvm-unit-tests PATCH v5 16/29] powerpc: Set .got section alignment to 256 bytes

[Thomas Huth]

On 16/12/2023 14.42, Nicholas Piggin wrote:

Modern powerpc64 toolchains require the .got section have alignment of
256 bytes. Incorrect alignment ends up causing the .data section ELF
load address to move by 8 bytes from its file offset, relative to
previous sections. This is not a problem for the QEMU bios loader used
by the pseries machine, but it is a problem for the powernv machine
using skiboot as the bios and the test programs as a kernel, because the
skiboot ELF loader is crippled:

   * Note that we execute the kernel in-place, we don't actually
   * obey the load informations in the headers. This is expected
   * to work for the Linux Kernel because it's a fairly dumb ELF
   * but it will not work for any ELF binary.

This causes all references to data to be incorrect. Aligning the .got
to 256 bytes prevents this offset skew and allows the skiboot "flat"
loader to work. [I don't know why the .got alignment can cause this
difference in linking.]

Signed-off-by: Nicholas Piggin 
---
  powerpc/flat.lds | 3 +--
  1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/powerpc/flat.lds b/powerpc/flat.lds
index 5eed368d..e07b91c1 100644
--- a/powerpc/flat.lds
+++ b/powerpc/flat.lds
@@ -41,8 +41,7 @@ SECTIONS
  /*
   * tocptr is tocbase + 32K, allowing toc offsets to be +-32K
   */
-tocptr = . + 32K;
-.got : { *(.toc) *(.got) }
+.got : ALIGN(256) { tocptr = . + 32K; *(.toc .got) }
  . = ALIGN(64K);
  edata = .;
  . += 64K;


Acked-by: Thomas Huth 

Re: [kvm-unit-tests PATCH v5 14/29] powerpc: Expand exception handler vector granularity

[Thomas Huth]

On 16/12/2023 14.42, Nicholas Piggin wrote:

Exception handlers are currently indexed in units of 0x100, but
powerpc can have vectors that are aligned to as little as 0x20
bytes. Increase granularity of the handler functions before
adding support for those vectors.

Signed-off-by: Nicholas Piggin 
---
  lib/powerpc/processor.c | 16 +++-
  1 file changed, 11 insertions(+), 5 deletions(-)


Reviewed-by: Thomas Huth 

Re: [kvm-unit-tests PATCH v5 13/29] powerpc: Make interrupt handler error more readable

[Thomas Huth]

On 16/12/2023 14.42, Nicholas Piggin wrote:

Installing the same handler twice reports a shifted trap vector
address which is hard to decipher. Print the unshifed address.

Signed-off-by: Nicholas Piggin 
---
  lib/powerpc/processor.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/powerpc/processor.c b/lib/powerpc/processor.c
index aaf45b68..b4cd5b4c 100644
--- a/lib/powerpc/processor.c
+++ b/lib/powerpc/processor.c
@@ -26,7 +26,7 @@ void handle_exception(int trap, void (*func)(struct pt_regs 
*, void *),
trap >>= 8;


You only change this to >>= 5 in the next patch...


if (func && handlers[trap].func) {
-   printf("exception handler installed twice %#x\n", trap);
+   printf("exception handler installed twice %#x\n", trap << 5);


... so I think you should move this patch here after the next one.

 Thomas



abort();
}
handlers[trap].func = func;

Re: [kvm-unit-tests PATCH v5 12/29] powerpc/sprs: Avoid taking async interrupts caused by register fuzzing

[Thomas Huth]

On 16/12/2023 14.42, Nicholas Piggin wrote:

Storing certain values in some registers can cause asynchronous
interrupts that can crash the test case, for example decrementer
or PMU interrupts.

Change the msleep to mdelay which does not enable MSR[EE] and so
avoids the problem. This allows removing some of the SPR special
casing.

Signed-off-by: Nicholas Piggin 
---
  powerpc/sprs.c | 14 ++
  1 file changed, 2 insertions(+), 12 deletions(-)

diff --git a/powerpc/sprs.c b/powerpc/sprs.c
index 01041912..313698e0 100644
--- a/powerpc/sprs.c
+++ b/powerpc/sprs.c
@@ -481,12 +481,7 @@ static void set_sprs(uint64_t val)
continue;
if (sprs[i].type & SPR_HARNESS)
continue;
-   if (!strcmp(sprs[i].name, "MMCR0")) {
-   /* XXX: could use a comment or better abstraction! */
-   __mtspr(i, (val & 0xfbab3fffULL) | 0xfa0b2070);
-   } else {
-   __mtspr(i, val);
-   }
+   __mtspr(i, val);
}
  }
  
@@ -536,12 +531,7 @@ int main(int argc, char **argv)

if (pause) {
migrate_once();
} else {
-   msleep(2000);
-
-   /* Taking a dec updates SRR0, SRR1, SPRG1, so don't fail. */
-   sprs[26].type |= SPR_ASYNC;
-   sprs[27].type |= SPR_ASYNC;
-   sprs[273].type |= SPR_ASYNC;
+   mdelay(2000);
}


IIRC I used the H_CEDE stuff here on purpose to increase the possibility 
that the guest gets rescheduled onto another CPU core on the host, and thus 
that it uncovers sprs that are not saved and restored on the host more 
easily. So I'd rather keep the msleep() here.


 Thomas

Re: [kvm-unit-tests PATCH v5 11/29] powerpc/sprs: Don't fail changed SPRs that are used by the test harness

[Thomas Huth]

On 16/12/2023 14.42, Nicholas Piggin wrote:

SPRs annotated with SPR_HARNESS can change between consecutive reads
because the test harness code has changed them. Avoid failing the
test in this case.

[XER was observed to change after the next changeset to use mdelay.]

Signed-off-by: Nicholas Piggin 
---
  powerpc/sprs.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/powerpc/sprs.c b/powerpc/sprs.c
index cd8b472d..01041912 100644
--- a/powerpc/sprs.c
+++ b/powerpc/sprs.c
@@ -557,7 +557,7 @@ int main(int argc, char **argv)
if (before[i] >> 32)
pass = false;
}
-   if (!(sprs[i].type & SPR_ASYNC) && (before[i] != after[i]))
+   if (!(sprs[i].type & (SPR_HARNESS|SPR_ASYNC)) && (before[i] != 
after[i]))
pass = false;


I guess you could even squash this into the previous patch.

Anyway:
Reviewed-by: Thomas Huth 

Re: [kvm-unit-tests PATCH v5 10/29] powerpc/sprs: Specify SPRs with data rather than code

[Thomas Huth]

On 16/12/2023 14.42, Nicholas Piggin wrote:

A significant rework that builds an array of 'struct spr', where each
element describes an SPR. This makes various metadata about the SPR
like name and access type easier to carry and use.

Hypervisor privileged registers are described despite not being used
at the moment for completeness, but also the code might one day be
reused for a hypervisor-privileged test.

Acked-by: Thomas Huth 
Signed-off-by: Nicholas Piggin 
---
  powerpc/sprs.c | 643 ++---
  1 file changed, 450 insertions(+), 193 deletions(-)

diff --git a/powerpc/sprs.c b/powerpc/sprs.c
index 57e487ce..cd8b472d 100644
--- a/powerpc/sprs.c
+++ b/powerpc/sprs.c
@@ -28,231 +28,465 @@
  #include 
  #include 
  
-uint64_t before[1024], after[1024];

-
-/* Common SPRs for all PowerPC CPUs */
-static void set_sprs_common(uint64_t val)
+/* "Indirect" mfspr/mtspr which accept a non-constant spr number */
+static uint64_t __mfspr(unsigned spr)
  {
-   mtspr(9, val);  /* CTR */
-   // mtspr(273, val); /* SPRG1 */  /* Used by our exception handler */
-   mtspr(274, val);/* SPRG2 */
-   mtspr(275, val);/* SPRG3 */
+   uint64_t tmp;
+   uint64_t ret;
+
+   asm volatile(
+" bcl 20, 31, 1f  \n"
+"1:   mflr%0  \n"
+" addi%0, %0, (2f-1b) \n"
+" add %0, %0, %2  \n"
+" mtctr   %0  \n"
+" bctr\n"
+"2:   \n"
+".LSPR=0  \n"
+".rept 1024   \n"
+" mfspr   %1, .LSPR   \n"
+" b   3f  \n"
+" .LSPR=.LSPR+1   \n"
+".endr\n"
+"3:   \n"
+   : "="(tmp),
+ "=r"(ret)
+   : "r"(spr*8) /* 8 bytes per 'mfspr ; b' block */
+   : "lr", "ctr");
+
+   return ret;
  }
  
-/* SPRs from PowerPC Operating Environment Architecture, Book III, Vers. 2.01 */

-static void set_sprs_book3s_201(uint64_t val)
+static void __mtspr(unsigned spr, uint64_t val)
  {
-   mtspr(18, val); /* DSISR */
-   mtspr(19, val); /* DAR */
-   mtspr(152, val);/* CTRL */
-   mtspr(256, val);/* VRSAVE */
-   mtspr(786, val);/* MMCRA */
-   mtspr(795, val);/* MMCR0 */
-   mtspr(798, val);/* MMCR1 */
+   uint64_t tmp;
+
+   asm volatile(
+" bcl 20, 31, 1f  \n"
+"1:   mflr%0  \n"
+" addi%0, %0, (2f-1b) \n"
+" add %0, %0, %2  \n"
+" mtctr   %0  \n"
+" bctr\n"
+"2:   \n"
+".LSPR=0  \n"
+".rept 1024   \n"
+" mtspr   .LSPR, %1   \n"
+" b   3f  \n"
+" .LSPR=.LSPR+1   \n"
+".endr\n"
+"3:   \n"
+   : "="(tmp)
+   : "r"(val),
+ "r"(spr*8) /* 8 bytes per 'mfspr ; b' block */
+   : "lr", "ctr", "xer");
  }
  
+static uint64_t before[1024], after[1024];

+
+#define SPR_PR_READ0x0001
+#define SPR_PR_WRITE   0x0002
+#define SPR_OS_READ0x0010
+#define SPR_OS_WRITE   0x0020
+#define SPR_HV_READ0x0100
+#define SPR_HV_WRITE   0x0200
+
+#define RW 0x333
+#define RO 0x111
+#define WO 0x222
+#define OS_RW  0x330
+#define OS_RO  0x110
+#define OS_WO  0x220
+#define HV_RW  0x300
+#define HV_RO  0x100
+#define HV_WO  0x200
+
+#define SPR_ASYNC  0x1000  /* May be updated asynchronously */
+#define SPR_INT0x2000  /* May be updated by synchronous 
interrupt */
+#define SPR_HARNESS0x4000  /* Test harness uses the register */
+
+struct spr {
+   const char  *name;
+   uint8_t width;
+   uint16_taccess;
+   uint16_ttype;
+};
+
+/* SPRs common denominator back to PowerPC Operating Environment Architecture 
*/
+static const struct spr sprs_common[1024] = {
+  [1] = {"XER",  64, RW, SPR_HARNESS, }, /* 
Compiler */
+  [8] = {"LR",   64, RW, SPR_HARNESS, }, /* 
Compiler, mfspr/mtspr */
+  [9] = {"CTR",  64, RW, SPR_HARNESS, }, /* 
Compiler, mfspr/mtspr */
+ [

Re: [kvm-unit-tests PATCH v5 09/29] powerpc: Fix interrupt stack alignment

[Thomas Huth]

On 16/12/2023 14.42, Nicholas Piggin wrote:

ppc64 requires the stack to be 16-byte aligned but the interrupt
stack frame has 8-byte aligned size. Add padding to fix.

Signed-off-by: Nicholas Piggin 
---
  lib/powerpc/setup.c| 3 +++
  lib/ppc64/asm/ptrace.h | 1 +
  2 files changed, 4 insertions(+)


Reviewed-by: Thomas Huth 

Re: [kvm-unit-tests PATCH v5 08/29] powerpc: Require KVM for the TM test

[Thomas Huth]

On 16/12/2023 14.42, Nicholas Piggin wrote:

QEMU TCG does not support TM. Skip this test gracefully when not on KVM.

Signed-off-by: Nicholas Piggin 
---
  powerpc/unittests.cfg | 1 +
  1 file changed, 1 insertion(+)

diff --git a/powerpc/unittests.cfg b/powerpc/unittests.cfg
index dd5f361c..e71140aa 100644
--- a/powerpc/unittests.cfg
+++ b/powerpc/unittests.cfg
@@ -65,6 +65,7 @@ file = emulator.elf
  
  [h_cede_tm]

  file = tm.elf
+accel = kvm
  smp = 2,threads=2
  extra_params = -machine cap-htm=on -append "h_cede_tm"
  groups = h_cede_tm


Reviewed-by: Thomas Huth 

Re: [kvm-unit-tests PATCH v5 07/29] powerpc: Add a migration stress tester

[Thomas Huth]

On 16/12/2023 14.42, Nicholas Piggin wrote:

This performs 1000 migrations a tight loop to flush out simple issues
in the multiple-migration code.

Signed-off-by: Nicholas Piggin 
---
  powerpc/Makefile.common |  1 +
  powerpc/migrate.c   | 64 +
  2 files changed, 65 insertions(+)
  create mode 100644 powerpc/migrate.c


You should likely add an entry to powerpc/unittests.cfg ...
also, wouldn't it be better to extend the sprs.elf test for this, so that it 
e.g. changes some stuff for each migration?


 Thomas

Re: [kvm-unit-tests PATCH v5 06/29] powerpc: Quiet QEMU TCG pseries capability warnings

[Thomas Huth]

On 16/12/2023 14.42, Nicholas Piggin wrote:

Quieten this console spam.

Signed-off-by: Nicholas Piggin 
---
  powerpc/run | 5 +
  1 file changed, 5 insertions(+)

diff --git a/powerpc/run b/powerpc/run
index b353169f..e469f1eb 100755
--- a/powerpc/run
+++ b/powerpc/run
@@ -21,6 +21,11 @@ fi
  
  M='-machine pseries'

  M+=",accel=$ACCEL$ACCEL_PROPS"
+
+if [[ "$ACCEL" == "tcg" ]] ; then
+   M+=",cap-cfpc=broken,cap-sbbc=broken,cap-ibs=broken,cap-ccf-assist=off"
+fi


This also bugs me when I run QEMU with TCG manually and IMHO it should be 
fixed in QEMU instead (e.g. by providing a different CPU model for TCG mode).

Anyway, for the time being:

Reviewed-by: Thomas Huth 

Re: [PATCH 18/26] KVM: s390: Stop adding virt/kvm to the arch include path

[Thomas Huth]

On 16/09/2023 02.31, Sean Christopherson wrote:

Don't add virt/kvm to KVM s390's include path, the headers in virt/kvm are
intended to be used only by other code in virt/kvm, i.e. are "private" to
the core KVM code.  It's not clear that s390 *ever* included a header from
virt/kvm, i.e. odds are good the "-Ivirt/kvm" was copied from a x86's
Makefile when s390 support was first added.

The only headers in virt/kvm at the time were the x86 specific ioapic.h,
and iodev.h, neither of which shows up as an #include in the diff for the
commit range 37817f2982d0f..e976a2b997fc4.

Signed-off-by: Sean Christopherson 
---
  arch/s390/kvm/Makefile | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/s390/kvm/Makefile b/arch/s390/kvm/Makefile
index 02217fb4ae10..f17249ab2a72 100644
--- a/arch/s390/kvm/Makefile
+++ b/arch/s390/kvm/Makefile
@@ -5,7 +5,7 @@
  
  include $(srctree)/virt/kvm/Makefile.kvm
  
-ccflags-y := -Ivirt/kvm -Iarch/s390/kvm

+ccflags-y := -Iarch/s390/kvm
  
  kvm-y += kvm-s390.o intercept.o interrupt.o priv.o sigp.o

  kvm-y += diag.o gaccess.o guestdbg.o vsie.o pv.o


Reviewed-by: Thomas Huth 

Re: [kvm-unit-tests PATCH 2/2] Link with "-z noexecstack" to avoid warning from newer versions of ld

[Thomas Huth]

On 27/06/2023 00.34, Sean Christopherson wrote:

On Fri, Jun 23, 2023, Thomas Huth wrote:

On 23/06/2023 16.24, Sean Christopherson wrote:

On Fri, Jun 23, 2023, Thomas Huth wrote:

Newer versions of ld (from binutils 2.40) complain on s390x and x86:

   ld: warning: s390x/cpu.o: missing .note.GNU-stack section implies
executable stack
   ld: NOTE: This behaviour is deprecated and will be removed in a
 future version of the linker

We can silence these warnings by using "-z noexecstack" for linking
(which should not have any real influence on the kvm-unit-tests since
the information from the ELF header is not used here anyway, so it's
just cosmetics).

Signed-off-by: Thomas Huth 
---
   Makefile | 2 +-
   1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index 0e5d85a1..20f7137c 100644
--- a/Makefile
+++ b/Makefile
@@ -96,7 +96,7 @@ CFLAGS += -Woverride-init -Wmissing-prototypes 
-Wstrict-prototypes
   autodepend-flags = -MMD -MF $(dir $*).$(notdir $*).d
-LDFLAGS += -nostdlib
+LDFLAGS += -nostdlib -z noexecstack


Drat, the pull request[1] I sent to Paolo yesterday only fixes x86[2].

...

Paolo, want me to redo the pull request to drop the x86-specific patch?


I can also respin my patch on top of your series later ... the problem
currently also only seems to happen on x86 and s390x, on ppc64 and aarch64,
the linker does not complain ... so maybe it's even better to do it
per-architecture only anyway? Opinions?


I don't think it makes sense to do this per-arch, other architectures likely 
aren't
problematic purely because of linker specific behavior, e.g. see

https://patches.linaro.org/project/binutils/patch/1506025575-1559-1-git-send-email-jim.wil...@linaro.org


Ok, I've pushed now my patches since other people were running into this 
issue, too (see 
https://lore.kernel.org/kvm/20230809091717.1549-1-...@linux.ibm.com/ ).


Sean, could you please rebase your series now?

 Thanks,
  Thomas

Re: [kvm-unit-tests v4 00/12] powerpc: updates, P10, PNV support

[Thomas Huth]

On 08/06/2023 09.58, Nicholas Piggin wrote:

Posting again, a couple of patches were merged and accounted for review
comments from last time.


Sorry for not being very responsive ... it's been a busy month.

Anyway, I've now merged the first 5 patches and the VPA test since they look 
fine to me.


As Joel already wrote, there is an issue with the sprs patch, I also get an 
error with the PIR register on the P8 box that I have access to as soon as I 
apply the "Specify SPRs with data rather than code" patch. It would be good 
to get that problem resolved before merging the remaining patches...


 Thomas

Re: [kvm-unit-tests v4 03/12] powerpc: Abstract H_CEDE calls into a sleep functions

[Thomas Huth]

On 08/06/2023 09.58, Nicholas Piggin wrote:

This consolidates several implementations, and it no longer leaves
MSR[EE] enabled after the decrementer interrupt is handled, but
rather disables it on return.

The handler no longer allows a continuous ticking, but rather dec
has to be re-armed and EE re-enabled (e.g., via H_CEDE hcall) each
time.

Signed-off-by: Nicholas Piggin 
---
  lib/powerpc/asm/handlers.h  |  2 +-
  lib/powerpc/asm/ppc_asm.h   |  1 +
  lib/powerpc/asm/processor.h |  7 ++
  lib/powerpc/handlers.c  | 10 -
  lib/powerpc/processor.c | 43 +
  powerpc/sprs.c  |  6 +-
  powerpc/tm.c| 20 +
  7 files changed, 58 insertions(+), 31 deletions(-)

diff --git a/lib/powerpc/asm/handlers.h b/lib/powerpc/asm/handlers.h
index 64ba727a..e4a0cd45 100644
--- a/lib/powerpc/asm/handlers.h
+++ b/lib/powerpc/asm/handlers.h
@@ -3,6 +3,6 @@
  
  #include 
  
-void dec_except_handler(struct pt_regs *regs, void *data);

+void dec_handler_oneshot(struct pt_regs *regs, void *data);
  
  #endif /* _ASMPOWERPC_HANDLERS_H_ */

diff --git a/lib/powerpc/asm/ppc_asm.h b/lib/powerpc/asm/ppc_asm.h
index 1b85f6bb..6299ff53 100644
--- a/lib/powerpc/asm/ppc_asm.h
+++ b/lib/powerpc/asm/ppc_asm.h
@@ -36,6 +36,7 @@
  #endif /* __BYTE_ORDER__ */
  
  /* Machine State Register definitions: */

+#define MSR_EE_BIT 15  /* External Interrupts Enable */
  #define MSR_SF_BIT63  /* 64-bit mode */
  
  #endif /* _ASMPOWERPC_PPC_ASM_H */

diff --git a/lib/powerpc/asm/processor.h b/lib/powerpc/asm/processor.h
index ac001e18..ebfeff2b 100644
--- a/lib/powerpc/asm/processor.h
+++ b/lib/powerpc/asm/processor.h
@@ -20,6 +20,8 @@ static inline uint64_t get_tb(void)
  
  extern void delay(uint64_t cycles);

  extern void udelay(uint64_t us);
+extern void sleep_tb(uint64_t cycles);
+extern void usleep(uint64_t us);
  
  static inline void mdelay(uint64_t ms)

  {
@@ -27,4 +29,9 @@ static inline void mdelay(uint64_t ms)
udelay(1000);
  }
  
+static inline void msleep(uint64_t ms)

+{
+   usleep(ms * 1000);
+}
+
  #endif /* _ASMPOWERPC_PROCESSOR_H_ */
diff --git a/lib/powerpc/handlers.c b/lib/powerpc/handlers.c
index c8721e0a..296f14ff 100644
--- a/lib/powerpc/handlers.c
+++ b/lib/powerpc/handlers.c
@@ -9,15 +9,13 @@
  #include 
  #include 
  #include 
+#include 
  
  /*

   * Generic handler for decrementer exceptions (0x900)
- * Just reset the decrementer back to the value specified when registering the
- * handler
+ * Return with MSR[EE] disabled.
   */
-void dec_except_handler(struct pt_regs *regs __unused, void *data)
+void dec_handler_oneshot(struct pt_regs *regs, void *data)
  {
-   uint64_t dec = *((uint64_t *) data);
-
-   asm volatile ("mtdec %0" : : "r" (dec));
+   regs->msr &= ~(1UL << MSR_EE_BIT);
  }
diff --git a/lib/powerpc/processor.c b/lib/powerpc/processor.c
index 0550e4fc..aaf45b68 100644
--- a/lib/powerpc/processor.c
+++ b/lib/powerpc/processor.c
@@ -10,6 +10,8 @@
  #include 
  #include 
  #include 
+#include 
+#include 
  
  static struct {

void (*func)(struct pt_regs *, void *data);
@@ -58,3 +60,44 @@ void udelay(uint64_t us)
  {
delay((us * tb_hz) / 100);
  }
+
+void sleep_tb(uint64_t cycles)
+{
+   uint64_t start, end, now;
+
+   start = now = get_tb();
+   end = start + cycles;
+
+   while (end > now) {
+   uint64_t left = end - now;
+
+   /* TODO: Could support large decrementer */
+   if (left > 0x7fff)
+   left = 0x7fff;
+
+   /* DEC won't fire until H_CEDE is called because EE=0 */
+   asm volatile ("mtdec %0" : : "r" (left));
+   handle_exception(0x900, _handler_oneshot, NULL);
+   /*
+* H_CEDE is called with MSR[EE] clear and enables it as part
+* of the hcall, returning with EE enabled. The dec interrupt
+* is then taken immediately and the handler disables EE.
+*
+* If H_CEDE returned for any other interrupt than dec
+* expiring, that is considered an unhandled interrupt and
+* the test case would be stopped.
+*/
+   if (hcall(H_CEDE) != H_SUCCESS) {
+   printf("H_CEDE failed\n");
+   abort();
+   }
+   handle_exception(0x900, NULL, NULL);
+
+   now = get_tb();
+   }
+}
+
+void usleep(uint64_t us)
+{
+   sleep_tb((us * tb_hz) / 100);
+}
diff --git a/powerpc/sprs.c b/powerpc/sprs.c
index 5cc1cd16..ba4ddee4 100644
--- a/powerpc/sprs.c
+++ b/powerpc/sprs.c
@@ -254,7 +254,6 @@ int main(int argc, char **argv)
0x1234567890ABCDEFULL, 0xFEDCBA0987654321ULL,
-1ULL,
};
-   static uint64_t decr = 0x7FFF; /* Max value */
  
  	for 

Re: [kvm-unit-tests PATCH 2/2] Link with "-z noexecstack" to avoid warning from newer versions of ld

[Thomas Huth]

On 23/06/2023 16.24, Sean Christopherson wrote:

On Fri, Jun 23, 2023, Thomas Huth wrote:

Newer versions of ld (from binutils 2.40) complain on s390x and x86:

  ld: warning: s390x/cpu.o: missing .note.GNU-stack section implies
   executable stack
  ld: NOTE: This behaviour is deprecated and will be removed in a
future version of the linker

We can silence these warnings by using "-z noexecstack" for linking
(which should not have any real influence on the kvm-unit-tests since
the information from the ELF header is not used here anyway, so it's
just cosmetics).

Signed-off-by: Thomas Huth 
---
  Makefile | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index 0e5d85a1..20f7137c 100644
--- a/Makefile
+++ b/Makefile
@@ -96,7 +96,7 @@ CFLAGS += -Woverride-init -Wmissing-prototypes 
-Wstrict-prototypes
  
  autodepend-flags = -MMD -MF $(dir $*).$(notdir $*).d
  
-LDFLAGS += -nostdlib

+LDFLAGS += -nostdlib -z noexecstack


Drat, the pull request[1] I sent to Paolo yesterday only fixes x86[2].


Oops, sorry, I did not notice that patch in my overcrowded mailboxes (or 
forgot about it during KVM forum...) :-/



Paolo, want me to redo the pull request to drop the x86-specific patch?


I can also respin my patch on top of your series later ... the problem 
currently also only seems to happen on x86 and s390x, on ppc64 and aarch64, 
the linker does not complain ... so maybe it's even better to do it 
per-architecture only anyway? Opinions?


 Thomas

[kvm-unit-tests PATCH 2/2] Link with "-z noexecstack" to avoid warning from newer versions of ld

[Thomas Huth]

Newer versions of ld (from binutils 2.40) complain on s390x and x86:

 ld: warning: s390x/cpu.o: missing .note.GNU-stack section implies
  executable stack
 ld: NOTE: This behaviour is deprecated and will be removed in a
   future version of the linker

We can silence these warnings by using "-z noexecstack" for linking
(which should not have any real influence on the kvm-unit-tests since
the information from the ELF header is not used here anyway, so it's
just cosmetics).

Signed-off-by: Thomas Huth 
---
 Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index 0e5d85a1..20f7137c 100644
--- a/Makefile
+++ b/Makefile
@@ -96,7 +96,7 @@ CFLAGS += -Woverride-init -Wmissing-prototypes 
-Wstrict-prototypes
 
 autodepend-flags = -MMD -MF $(dir $*).$(notdir $*).d
 
-LDFLAGS += -nostdlib
+LDFLAGS += -nostdlib -z noexecstack
 
 $(libcflat): $(cflatobjs)
$(AR) rcs $@ $^
-- 
2.39.3

[kvm-unit-tests PATCH 1/2] Rework the common LDFLAGS to become more useful again

[Thomas Huth]

Currently the LDFLAGS settings from the main Makefile are ignored in
most architecture specific directories (except s390x), which is very
confusing when you try to add a linker switch for all architectures.

Let's change this so that all architectures extend the common LDFLAGS
instead of replacing them. So it is sufficient now to specify the
"-nostdlib" switch in the main Makefile now instead of repeating it
everywhere.

While we're at it, avoid to repeat the whole set of CFLAGS in the
common LDFLAGS - the options that are meant for the C compiler should
not be exposed unconditionally to the linker.

Signed-off-by: Thomas Huth 
---
 Makefile| 2 +-
 arm/Makefile.common | 2 +-
 powerpc/Makefile.common | 2 +-
 s390x/Makefile  | 2 +-
 x86/Makefile.common | 4 ++--
 5 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/Makefile b/Makefile
index 6ed5deac..0e5d85a1 100644
--- a/Makefile
+++ b/Makefile
@@ -96,7 +96,7 @@ CFLAGS += -Woverride-init -Wmissing-prototypes 
-Wstrict-prototypes
 
 autodepend-flags = -MMD -MF $(dir $*).$(notdir $*).d
 
-LDFLAGS += $(CFLAGS)
+LDFLAGS += -nostdlib
 
 $(libcflat): $(cflatobjs)
$(AR) rcs $@ $^
diff --git a/arm/Makefile.common b/arm/Makefile.common
index 1bbec64f..e2cb1a56 100644
--- a/arm/Makefile.common
+++ b/arm/Makefile.common
@@ -60,7 +60,7 @@ libeabi = lib/arm/libeabi.a
 eabiobjs = lib/arm/eabi_compat.o
 
 FLATLIBS = $(libcflat) $(LIBFDT_archive) $(libeabi)
-%.elf: LDFLAGS = -nostdlib $(arch_LDFLAGS)
+%.elf: LDFLAGS += $(arch_LDFLAGS)
 %.elf: %.o $(FLATLIBS) $(SRCDIR)/arm/flat.lds $(cstart.o)
$(CC) $(CFLAGS) -c -o $(@:.elf=.aux.o) $(SRCDIR)/lib/auxinfo.c \
-DPROGNAME=\"$(@:.elf=.flat)\" -DAUXFLAGS=$(AUXFLAGS)
diff --git a/powerpc/Makefile.common b/powerpc/Makefile.common
index 8ce00340..f8f47490 100644
--- a/powerpc/Makefile.common
+++ b/powerpc/Makefile.common
@@ -49,7 +49,7 @@ OBJDIRS += lib/powerpc
 
 FLATLIBS = $(libcflat) $(LIBFDT_archive)
 %.elf: CFLAGS += $(arch_CFLAGS)
-%.elf: LDFLAGS = $(arch_LDFLAGS) -nostdlib -pie -n
+%.elf: LDFLAGS += $(arch_LDFLAGS) -pie -n
 %.elf: %.o $(FLATLIBS) $(SRCDIR)/powerpc/flat.lds $(cstart.o) $(reloc.o)
$(CC) $(CFLAGS) -c -o $(@:.elf=.aux.o) $(SRCDIR)/lib/auxinfo.c \
-DPROGNAME=\"$@\"
diff --git a/s390x/Makefile b/s390x/Makefile
index a80db538..d75e86c2 100644
--- a/s390x/Makefile
+++ b/s390x/Makefile
@@ -76,7 +76,7 @@ CFLAGS += -O2
 CFLAGS += -march=zEC12
 CFLAGS += -mbackchain
 CFLAGS += -fno-delete-null-pointer-checks
-LDFLAGS += -nostdlib -Wl,--build-id=none
+LDFLAGS += -Wl,--build-id=none
 
 # We want to keep intermediate files
 .PRECIOUS: %.o %.lds
diff --git a/x86/Makefile.common b/x86/Makefile.common
index 365e199f..e64aac52 100644
--- a/x86/Makefile.common
+++ b/x86/Makefile.common
@@ -31,7 +31,7 @@ endif
 
 OBJDIRS += lib/x86
 
-$(libcflat): LDFLAGS += -nostdlib $(arch_LDFLAGS)
+$(libcflat): LDFLAGS += $(arch_LDFLAGS)
 $(libcflat): CFLAGS += -ffreestanding -I $(SRCDIR)/lib -I lib
 
 COMMON_CFLAGS += -m$(bits)
@@ -62,7 +62,7 @@ else
 # We want to keep intermediate file: %.elf and %.o
 .PRECIOUS: %.elf %.o
 
-%.elf: LDFLAGS = -nostdlib $(arch_LDFLAGS)
+%.elf: LDFLAGS += $(arch_LDFLAGS)
 %.elf: %.o $(FLATLIBS) $(SRCDIR)/x86/flat.lds $(cstart.o)
$(LD) $(LDFLAGS) -T $(SRCDIR)/x86/flat.lds -o $@ \
$(filter %.o, $^) $(FLATLIBS)
-- 
2.39.3

[kvm-unit-tests PATCH 0/2] Rework LDFLAGS and link with noexecstack

[Thomas Huth]

I noticed that the latest version of ld (in Fedora rawhide) emits
a warning on x86 and s390x, complaining about missing .note.GNU-stack
section that implies an executable stack. It can be silenced by
linking with "-z noexecstack".

While trying to add this switch globally to the kvm-unit-tests, I
had to discover that the common LDFLAGS are hardly used anywhere,
so the first patch cleans up that problem first before adding the
new flag in the second patch.

Thomas Huth (2):
  Rework the common LDFLAGS to become more useful again
  Link with "-z noexecstack" to avoid warning from newer versions of ld

 Makefile| 2 +-
 arm/Makefile.common | 2 +-
 powerpc/Makefile.common | 2 +-
 s390x/Makefile  | 2 +-
 x86/Makefile.common | 4 ++--
 5 files changed, 6 insertions(+), 6 deletions(-)

-- 
2.39.3

Re: [kvm-unit-tests v4 02/12] powerpc: Add some checking to exception handler install

[Thomas Huth]

On 08/06/2023 09.58, Nicholas Piggin wrote:

Check to ensure exception handlers are not being overwritten or
invalid exception numbers are used.

Signed-off-by: Nicholas Piggin 
---
Since v3:
- Simplified code as suggested by Thomas.

  lib/powerpc/processor.c | 10 +++---
  1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/lib/powerpc/processor.c b/lib/powerpc/processor.c
index 05b4b04f..0550e4fc 100644
--- a/lib/powerpc/processor.c
+++ b/lib/powerpc/processor.c
@@ -19,12 +19,16 @@ static struct {
  void handle_exception(int trap, void (*func)(struct pt_regs *, void *),
  void * data)
  {
+   assert(!(trap & ~0xf00));
+
trap >>= 8;
  
-	if (trap < 16) {

-   handlers[trap].func = func;
-   handlers[trap].data = data;
+   if (func && handlers[trap].func) {
+   printf("exception handler installed twice %#x\n", trap);
+   abort();
}
+   handlers[trap].func = func;
+   handlers[trap].data = data;
  }
  
  void do_handle_exception(struct pt_regs *regs)


Reviewed-by: Thomas Huth 

Re: [kvm-unit-tests v4 01/12] powerpc: Report instruction address and MSR in unhandled exception error

[Thomas Huth]

On 08/06/2023 09.58, Nicholas Piggin wrote:

Signed-off-by: Nicholas Piggin 
---
Since v3:
- New patch

  lib/powerpc/processor.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/powerpc/processor.c b/lib/powerpc/processor.c
index ec85b9d8..05b4b04f 100644
--- a/lib/powerpc/processor.c
+++ b/lib/powerpc/processor.c
@@ -38,7 +38,7 @@ void do_handle_exception(struct pt_regs *regs)
return;
}
  
-	printf("unhandled cpu exception %#lx\n", regs->trap);

+   printf("unhandled cpu exception %#lx at NIA:0x%016lx MSR:0x%016lx\n", 
regs->trap, regs->nip, regs->msr);


 Why NIA and not NIP ? 

 Thomas

Re: [PATCH] KVM: PPC: remove unneeded variable

[Thomas Huth]

On 14/06/2023 07.58, baomingtong...@208suo.com wrote:

fix the following coccicheck warning:

arch/powerpc/kvm/book3s_pr.c:424:5-6: Unneeded variable: "r".

Signed-off-by: Mingtong Bao 
---
  arch/powerpc/kvm/book3s_pr.c | 5 ++---
  1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/kvm/book3s_pr.c b/arch/powerpc/kvm/book3s_pr.c
index 9118242063fb..1b68de369b88 100644
--- a/arch/powerpc/kvm/book3s_pr.c
+++ b/arch/powerpc/kvm/book3s_pr.c
@@ -421,14 +421,13 @@ void kvmppc_restore_tm_pr(struct kvm_vcpu *vcpu)

  static int kvmppc_core_check_requests_pr(struct kvm_vcpu *vcpu)
  {
-    int r = 1; /* Indicate we want to get back into the guest */

  /* We misuse TLB_FLUSH to indicate that we want to clear
     all shadow cache entries */
  if (kvm_check_request(KVM_REQ_TLB_FLUSH, vcpu))
  kvmppc_mmu_pte_flush(vcpu, 0, 0);
-
-    return r;
+    /* Indicate we want to get back into the guest */
+    return 1;
  }

  /* MMU Notifiers */



Please make sure to CC: linuxppc-dev@lists.ozlabs.org when sending patches 
for KVM on ppc.


Reviewed-by: Thomas Huth 

Re: [kvm-unit-tests v3 11/13] powerpc: Discover runtime load address dynamically

[Thomas Huth]

On 27/03/2023 14.45, Nicholas Piggin wrote:

The next change will load the kernels at different addresses depending
on test options, so this needs to be reverted back to dynamic
discovery.

Signed-off-by: Nicholas Piggin 
---
  powerpc/cstart64.S | 19 ++-
  1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/powerpc/cstart64.S b/powerpc/cstart64.S
index 1bd0437..0592e03 100644
--- a/powerpc/cstart64.S
+++ b/powerpc/cstart64.S
@@ -33,9 +33,14 @@ start:
 * We were loaded at QEMU's kernel load address, but we're not
 * allowed to link there due to how QEMU deals with linker VMAs,
 * so we just linked at zero. This means the first thing to do is
-* to find our stack and toc, and then do a relocate.
+* to find our stack and toc, and then do a relocate. powernv and
+* pseries load addreses are not the same, so find the address


With s/addreses/addresses/ :

Acked-by: Thomas Huth 

Re: [kvm-unit-tests v3 10/13] powerpc: Add support for more interrupts including HV interrupts

[Thomas Huth]

On 27/03/2023 14.45, Nicholas Piggin wrote:

Interrupt vectors were not being populated for all architected
interrupt types, which could lead to crashes rather than a message for
unhandled interrupts.

0x20 sized vectors require some reworking of the code to fit. This
also adds support for HV / HSRR type interrupts which will be used in
a later change.

Signed-off-by: Nicholas Piggin 
---
  powerpc/cstart64.S | 79 ++
  1 file changed, 65 insertions(+), 14 deletions(-)


Acked-by: Thomas Huth 

Re: [kvm-unit-tests v3 09/13] powerpc: Expand exception handler vector granularity

[Thomas Huth]

On 27/03/2023 14.45, Nicholas Piggin wrote:

Exception handlers are currently indexed in units of 0x100, but
powerpc can have vectors that are aligned to as little as 0x20
bytes. Increase granularity of the handler functions before
adding support for thse vectors.


s/thse/those/

 Thomas

Re: [kvm-unit-tests v3 08/13] powerpc/spapr_vpa: Add basic VPA tests

[Thomas Huth]

On 27/03/2023 14.45, Nicholas Piggin wrote:

The VPA is an optional memory structure shared between the hypervisor
and operating system, defined by PAPR. This test defines the structure
and adds registration, deregistration, and a few simple sanity tests.

[Thanks to Thomas Huth for suggesting many of the test cases.]
Signed-off-by: Nicholas Piggin 
---

...

diff --git a/powerpc/Makefile.ppc64 b/powerpc/Makefile.ppc64
index ea68447..b0ed2b1 100644
--- a/powerpc/Makefile.ppc64
+++ b/powerpc/Makefile.ppc64
@@ -19,7 +19,7 @@ reloc.o  = $(TEST_DIR)/reloc64.o
  OBJDIRS += lib/ppc64
  
  # ppc64 specific tests

-tests =
+tests = $(TEST_DIR)/spapr_vpa.elf
  
  include $(SRCDIR)/$(TEST_DIR)/Makefile.common


That reminds me: We added all other tests to Makefile.common ... without 
ever checking them on 32-bit. Since we removed the early 32-bit code long 
ago already (see commit 2a814baab80af990eaf), it just might not make sense 
anymore to keep the separation for 64-bit and 32-bit Makefiles around here 
anymore --> could be a future cleanup to merge the Makefiles in the powerpc 
folder.


Anyway, that's not a problem of your patch here which looks fine, so:

Reviewed-by: Thomas Huth 

Re: [kvm-unit-tests v3 07/13] powerpc/sprs: Specify SPRs with data rather than code

[Thomas Huth]

On 27/03/2023 14.45, Nicholas Piggin wrote:

A significant rework that builds an array of 'struct spr', where each
element describes an SPR. This makes various metadata about the SPR
like name and access type easier to carry and use.

Hypervisor privileged registers are described despite not being used
at the moment for completeness, but also the code might one day be
reused for a hypervisor-privileged test.

Signed-off-by: Nicholas Piggin 

This ended up a little over-engineered perhaps, but there are lots of
SPRs, lots of access types, lots of changes between processor and ISA
versions, and lots of places they are implemented and used, so lots of
room for mistakes. There is not a good system in place to easily
see that userspace, supervisor, etc., switches perform all the right
SPR context switching so this is a nice test case to have. The sprs test
quickly caught a few QEMU TCG SPR bugs which really motivated me to
improve the SPR coverage.
---
Since v2:
- Merged with "Indirect SPR accessor functions" patch.

  powerpc/sprs.c | 643 ++---
  1 file changed, 450 insertions(+), 193 deletions(-)


Acked-by: Thomas Huth