[PATCH 0/3] tests/acceptance: Test virtio-rng and -balloon on s390x

[PATCH 0/3] tests/acceptance: Test virtio-rng and -balloon on s390x

[Thomas Huth]

Add two more simple tests to check that virtio-rng and virtio-balloon
are at least (very) basically working on s390x.

Based-on: 20201204121450.120730-1-cohuck@redhat.com

Thomas Huth (3):
  tests/acceptance: Extract the code to clear dmesg and wait for CRW
    reports
  tests/acceptance/machine_s390_ccw_virtio: Test virtio-rng via
    /dev/hwrng
  tests/acceptance/machine_s390_ccw_virtio: Test the virtio-balloon
    device

 tests/acceptance/machine_s390_ccw_virtio.py | 59 +++++++++++++++------
 1 file changed, 43 insertions(+), 16 deletions(-)

-- 
2.27.0

[PATCH 1/3] tests/acceptance: Extract the code to clear dmesg and wait for CRW reports

[Thomas Huth]

We will use this in more spots soon, so it's easier to put this into
a separate function.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 tests/acceptance/machine_s390_ccw_virtio.py | 30 ++++++++++++---------
 1 file changed, 17 insertions(+), 13 deletions(-)

diff --git a/tests/acceptance/machine_s390_ccw_virtio.py b/tests/acceptance/machine_s390_ccw_virtio.py
index 864ef4ee6e..733a7ca24a 100644
--- a/tests/acceptance/machine_s390_ccw_virtio.py
+++ b/tests/acceptance/machine_s390_ccw_virtio.py
@@ -17,12 +17,24 @@ from avocado_qemu import wait_for_console_pattern
 class S390CCWVirtioMachine(Test):
     KERNEL_COMMON_COMMAND_LINE = 'printk.time=0 '
 
+    timeout = 120
+
     def wait_for_console_pattern(self, success_message, vm=None):
         wait_for_console_pattern(self, success_message,
                                  failure_message='Kernel panic - not syncing',
                                  vm=vm)
 
-    timeout = 120
+    def wait_for_crw_reports(self):
+        exec_command_and_wait_for_pattern(self,
+                        'while ! (dmesg -c | grep CRW) ; do sleep 1 ; done',
+                        'CRW reports')
+
+    dmesg_clear_count = 1
+    def clear_guests_dmesg(self):
+        exec_command_and_wait_for_pattern(self, 'dmesg -c > /dev/null; '
+                    'echo dm_clear\ ' + str(self.dmesg_clear_count),
+                    'dm_clear ' + str(self.dmesg_clear_count))
+        self.dmesg_clear_count += 1
 
     def test_s390x_devices(self):
 
@@ -100,26 +112,18 @@ class S390CCWVirtioMachine(Test):
                         'cat /sys/bus/pci/devices/000a\:00\:00.0/function_id',
                         '0x0000000c')
         # add another device
-        exec_command_and_wait_for_pattern(self,
-                                    'dmesg -c > /dev/null; echo dm_clear\ 1',
-                                    'dm_clear 1')
+        self.clear_guests_dmesg()
         self.vm.command('device_add', driver='virtio-net-ccw',
                         devno='fe.0.4711', id='net_4711')
-        exec_command_and_wait_for_pattern(self,
-                        'while ! (dmesg -c | grep CRW) ; do sleep 1 ; done',
-                        'CRW reports')
+        self.wait_for_crw_reports()
         exec_command_and_wait_for_pattern(self, 'ls /sys/bus/ccw/devices/',
                                           '0.0.4711')
         # and detach it again
-        exec_command_and_wait_for_pattern(self,
-                                    'dmesg -c > /dev/null; echo dm_clear\ 2',
-                                    'dm_clear 2')
+        self.clear_guests_dmesg()
         self.vm.command('device_del', id='net_4711')
         self.vm.event_wait(name='DEVICE_DELETED',
                            match={'data': {'device': 'net_4711'}})
-        exec_command_and_wait_for_pattern(self,
-                        'while ! (dmesg -c | grep CRW) ; do sleep 1 ; done',
-                        'CRW reports')
+        self.wait_for_crw_reports()
         exec_command_and_wait_for_pattern(self,
                                           'ls /sys/bus/ccw/devices/0.0.4711',
                                           'No such file or directory')
-- 
2.27.0

[PATCH 2/3] tests/acceptance/machine_s390_ccw_virtio: Test virtio-rng via /dev/hwrng

[Thomas Huth]

/dev/hwrng is only functional if virtio-rng is working right, so let's
add a sanity check for this device node.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 tests/acceptance/machine_s390_ccw_virtio.py | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/tests/acceptance/machine_s390_ccw_virtio.py b/tests/acceptance/machine_s390_ccw_virtio.py
index 733a7ca24a..7d0a78139b 100644
--- a/tests/acceptance/machine_s390_ccw_virtio.py
+++ b/tests/acceptance/machine_s390_ccw_virtio.py
@@ -64,9 +64,9 @@ class S390CCWVirtioMachine(Test):
                          '-append', kernel_command_line,
                          '-device', 'virtio-net-ccw,devno=fe.1.1111',
                          '-device',
-                         'virtio-rng-ccw,devno=fe.2.0000,max_revision=0',
+                         'virtio-rng-ccw,devno=fe.2.0000,max_revision=0,id=rn1',
                          '-device',
-                         'virtio-rng-ccw,devno=fe.3.1234,max_revision=2',
+                         'virtio-rng-ccw,devno=fe.3.1234,max_revision=2,id=rn2',
                          '-device', 'zpci,uid=5,target=zzz',
                          '-device', 'virtio-net-pci,id=zzz',
                          '-device', 'zpci,uid=0xa,fid=12,target=serial',
@@ -96,6 +96,19 @@ class S390CCWVirtioMachine(Test):
         exec_command_and_wait_for_pattern(self,
                         'cat /sys/bus/ccw/devices/0.3.1234/virtio?/features',
                         virtio_rng_features)
+        # check that /dev/hwrng works - and that it's gone after ejecting
+        exec_command_and_wait_for_pattern(self,
+                        'dd if=/dev/hwrng of=/tmp/out.dat bs=1k count=10',
+                        '10+0 records out')
+        self.clear_guests_dmesg()
+        self.vm.command('device_del', id='rn1')
+        self.wait_for_crw_reports()
+        self.clear_guests_dmesg()
+        self.vm.command('device_del', id='rn2')
+        self.wait_for_crw_reports()
+        exec_command_and_wait_for_pattern(self,
+                        'dd if=/dev/hwrng of=/tmp/out.dat bs=1k count=10',
+                        'dd: /dev/hwrng: No such device')
         # verify that we indeed have virtio-net devices (without having the
         # virtio-net driver handy)
         exec_command_and_wait_for_pattern(self,
-- 
2.27.0

[PATCH 3/3] tests/acceptance/machine_s390_ccw_virtio: Test the virtio-balloon device

[Thomas Huth]

Inflate the balloon and check whether the size of the memory changes.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 tests/acceptance/machine_s390_ccw_virtio.py | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/tests/acceptance/machine_s390_ccw_virtio.py b/tests/acceptance/machine_s390_ccw_virtio.py
index 7d0a78139b..81f6c066c7 100644
--- a/tests/acceptance/machine_s390_ccw_virtio.py
+++ b/tests/acceptance/machine_s390_ccw_virtio.py
@@ -70,7 +70,8 @@ class S390CCWVirtioMachine(Test):
                          '-device', 'zpci,uid=5,target=zzz',
                          '-device', 'virtio-net-pci,id=zzz',
                          '-device', 'zpci,uid=0xa,fid=12,target=serial',
-                         '-device', 'virtio-serial-pci,id=serial')
+                         '-device', 'virtio-serial-pci,id=serial',
+                         '-device', 'virtio-balloon-ccw')
         self.vm.launch()
 
         shell_ready = "sh: can't access tty; job control turned off"
@@ -140,3 +141,12 @@ class S390CCWVirtioMachine(Test):
         exec_command_and_wait_for_pattern(self,
                                           'ls /sys/bus/ccw/devices/0.0.4711',
                                           'No such file or directory')
+        # test the virtio-balloon device
+        exec_command_and_wait_for_pattern(self, 'head -n 1 /proc/meminfo',
+                                          'MemTotal:         115640 kB')
+        self.vm.command('human-monitor-command', command_line='balloon 96')
+        exec_command_and_wait_for_pattern(self, 'head -n 1 /proc/meminfo',
+                                          'MemTotal:          82872 kB')
+        self.vm.command('human-monitor-command', command_line='balloon 128')
+        exec_command_and_wait_for_pattern(self, 'head -n 1 /proc/meminfo',
+                                          'MemTotal:         115640 kB')
-- 
2.27.0

Re: [PATCH 1/3] tests/acceptance: Extract the code to clear dmesg and wait for CRW reports

[Wainer dos Santos Moschetta]

Hi,

On 12/11/20 2:31 PM, Thomas Huth wrote:
> We will use this in more spots soon, so it's easier to put this into
> a separate function.
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>   tests/acceptance/machine_s390_ccw_virtio.py | 30 ++++++++++++---------
>   1 file changed, 17 insertions(+), 13 deletions(-)


Reviewed-by: Wainer dos Santos Moschetta <wainersm@redhat.com>


>
> diff --git a/tests/acceptance/machine_s390_ccw_virtio.py b/tests/acceptance/machine_s390_ccw_virtio.py
> index 864ef4ee6e..733a7ca24a 100644
> --- a/tests/acceptance/machine_s390_ccw_virtio.py
> +++ b/tests/acceptance/machine_s390_ccw_virtio.py
> @@ -17,12 +17,24 @@ from avocado_qemu import wait_for_console_pattern
>   class S390CCWVirtioMachine(Test):
>       KERNEL_COMMON_COMMAND_LINE = 'printk.time=0 '
>   
> +    timeout = 120
> +
>       def wait_for_console_pattern(self, success_message, vm=None):
>           wait_for_console_pattern(self, success_message,
>                                    failure_message='Kernel panic - not syncing',
>                                    vm=vm)
>   
> -    timeout = 120
> +    def wait_for_crw_reports(self):
> +        exec_command_and_wait_for_pattern(self,
> +                        'while ! (dmesg -c | grep CRW) ; do sleep 1 ; done',
> +                        'CRW reports')
> +
> +    dmesg_clear_count = 1
> +    def clear_guests_dmesg(self):
> +        exec_command_and_wait_for_pattern(self, 'dmesg -c > /dev/null; '
> +                    'echo dm_clear\ ' + str(self.dmesg_clear_count),
> +                    'dm_clear ' + str(self.dmesg_clear_count))
> +        self.dmesg_clear_count += 1
>   
>       def test_s390x_devices(self):
>   
> @@ -100,26 +112,18 @@ class S390CCWVirtioMachine(Test):
>                           'cat /sys/bus/pci/devices/000a\:00\:00.0/function_id',
>                           '0x0000000c')
>           # add another device
> -        exec_command_and_wait_for_pattern(self,
> -                                    'dmesg -c > /dev/null; echo dm_clear\ 1',
> -                                    'dm_clear 1')
> +        self.clear_guests_dmesg()
>           self.vm.command('device_add', driver='virtio-net-ccw',
>                           devno='fe.0.4711', id='net_4711')
> -        exec_command_and_wait_for_pattern(self,
> -                        'while ! (dmesg -c | grep CRW) ; do sleep 1 ; done',
> -                        'CRW reports')
> +        self.wait_for_crw_reports()
>           exec_command_and_wait_for_pattern(self, 'ls /sys/bus/ccw/devices/',
>                                             '0.0.4711')
>           # and detach it again
> -        exec_command_and_wait_for_pattern(self,
> -                                    'dmesg -c > /dev/null; echo dm_clear\ 2',
> -                                    'dm_clear 2')
> +        self.clear_guests_dmesg()
>           self.vm.command('device_del', id='net_4711')
>           self.vm.event_wait(name='DEVICE_DELETED',
>                              match={'data': {'device': 'net_4711'}})
> -        exec_command_and_wait_for_pattern(self,
> -                        'while ! (dmesg -c | grep CRW) ; do sleep 1 ; done',
> -                        'CRW reports')
> +        self.wait_for_crw_reports()
>           exec_command_and_wait_for_pattern(self,
>                                             'ls /sys/bus/ccw/devices/0.0.4711',
>                                             'No such file or directory')

Re: [PATCH 0/3] tests/acceptance: Test virtio-rng and -balloon on s390x

[Willian Rampazzo]

On 12/11/20 2:31 PM, Thomas Huth wrote:
> Add two more simple tests to check that virtio-rng and virtio-balloon
> are at least (very) basically working on s390x.
> 
> Based-on: 20201204121450.120730-1-cohuck@redhat.com
> 
> Thomas Huth (3):
>    tests/acceptance: Extract the code to clear dmesg and wait for CRW
>      reports
>    tests/acceptance/machine_s390_ccw_virtio: Test virtio-rng via
>      /dev/hwrng
>    tests/acceptance/machine_s390_ccw_virtio: Test the virtio-balloon
>      device
> 
>   tests/acceptance/machine_s390_ccw_virtio.py | 59 +++++++++++++++------
>   1 file changed, 43 insertions(+), 16 deletions(-)
> 

One observation, test_s390x_devices tends to get longer and difficult to 
debug in case of problems. If a test covers one specific device type, It 
will improve readability, flexibility, and debugging. In case you don't 
want to spend time breaking this into multiple tests, I'll be glad to do 
that after the whole series is merged.

As far as code concerned,

Reviewed-by: Willian Rampazzo <willianr@redhat.com>
Tested-by: Willian Rampazzo <willianr@redhat.com>

Fetching asset from 
tests/acceptance/machine_s390_ccw_virtio.py:S390CCWVirtioMachine.test_s390x_devices
Fetching asset from 
tests/acceptance/machine_s390_ccw_virtio.py:S390CCWVirtioMachine.test_s390x_devices
JOB ID     : 8ba8e572f2582f9a48f2542423342e51e257db97
JOB LOG    : 
/home/linux1/src/qemu.dev/build/tests/results/job-2020-12-11T15.01-8ba8e57/job.log
  (1/1) 
tests/acceptance/machine_s390_ccw_virtio.py:S390CCWVirtioMachine.test_s390x_devices: 
PASS (7.89 s)
RESULTS    : PASS 1 | ERROR 0 | FAIL 0 | SKIP 0 | WARN 0 | INTERRUPT 0 | 
CANCEL 0
JOB TIME   : 8.03 s

Re: [PATCH 2/3] tests/acceptance/machine_s390_ccw_virtio: Test virtio-rng via /dev/hwrng

[Wainer dos Santos Moschetta]

Hi,

On 12/11/20 2:31 PM, Thomas Huth wrote:
> /dev/hwrng is only functional if virtio-rng is working right, so let's
> add a sanity check for this device node.

Good idea.

>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>   tests/acceptance/machine_s390_ccw_virtio.py | 17 +++++++++++++++--
>   1 file changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/tests/acceptance/machine_s390_ccw_virtio.py b/tests/acceptance/machine_s390_ccw_virtio.py
> index 733a7ca24a..7d0a78139b 100644
> --- a/tests/acceptance/machine_s390_ccw_virtio.py
> +++ b/tests/acceptance/machine_s390_ccw_virtio.py
> @@ -64,9 +64,9 @@ class S390CCWVirtioMachine(Test):
>                            '-append', kernel_command_line,
>                            '-device', 'virtio-net-ccw,devno=fe.1.1111',
>                            '-device',
> -                         'virtio-rng-ccw,devno=fe.2.0000,max_revision=0',
> +                         'virtio-rng-ccw,devno=fe.2.0000,max_revision=0,id=rn1',
>                            '-device',
> -                         'virtio-rng-ccw,devno=fe.3.1234,max_revision=2',
> +                         'virtio-rng-ccw,devno=fe.3.1234,max_revision=2,id=rn2',
>                            '-device', 'zpci,uid=5,target=zzz',
>                            '-device', 'virtio-net-pci,id=zzz',
>                            '-device', 'zpci,uid=0xa,fid=12,target=serial',
> @@ -96,6 +96,19 @@ class S390CCWVirtioMachine(Test):
>           exec_command_and_wait_for_pattern(self,
>                           'cat /sys/bus/ccw/devices/0.3.1234/virtio?/features',
>                           virtio_rng_features)
> +        # check that /dev/hwrng works - and that it's gone after ejecting
> +        exec_command_and_wait_for_pattern(self,
> +                        'dd if=/dev/hwrng of=/tmp/out.dat bs=1k count=10',
> +                        '10+0 records out')
> +        self.clear_guests_dmesg()
> +        self.vm.command('device_del', id='rn1')
> +        self.wait_for_crw_reports()
> +        self.clear_guests_dmesg()
> +        self.vm.command('device_del', id='rn2')
> +        self.wait_for_crw_reports()
> +        exec_command_and_wait_for_pattern(self,
> +                        'dd if=/dev/hwrng of=/tmp/out.dat bs=1k count=10',
> +                        'dd: /dev/hwrng: No such device')

Maybe the expected pattern is too fragile. On my Fedora 33 system, 'dd' 
will print a different message.

What if it checks for the presence of the device file, e.g:

... self, 'test -c /dev/hwrng; echo $?', '1')

- Wainer

>           # verify that we indeed have virtio-net devices (without having the
>           # virtio-net driver handy)
>           exec_command_and_wait_for_pattern(self,

Re: [PATCH 3/3] tests/acceptance/machine_s390_ccw_virtio: Test the virtio-balloon device

[Wainer dos Santos Moschetta]

On 12/11/20 2:31 PM, Thomas Huth wrote:
> Inflate the balloon and check whether the size of the memory changes.
Yeah, because a true balloon should inflate :D
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>   tests/acceptance/machine_s390_ccw_virtio.py | 12 +++++++++++-
>   1 file changed, 11 insertions(+), 1 deletion(-)

Reviewed-by: Wainer dos Santos Moschetta <wainersm@redhat.com>

>
> diff --git a/tests/acceptance/machine_s390_ccw_virtio.py b/tests/acceptance/machine_s390_ccw_virtio.py
> index 7d0a78139b..81f6c066c7 100644
> --- a/tests/acceptance/machine_s390_ccw_virtio.py
> +++ b/tests/acceptance/machine_s390_ccw_virtio.py
> @@ -70,7 +70,8 @@ class S390CCWVirtioMachine(Test):
>                            '-device', 'zpci,uid=5,target=zzz',
>                            '-device', 'virtio-net-pci,id=zzz',
>                            '-device', 'zpci,uid=0xa,fid=12,target=serial',
> -                         '-device', 'virtio-serial-pci,id=serial')
> +                         '-device', 'virtio-serial-pci,id=serial',
> +                         '-device', 'virtio-balloon-ccw')
>           self.vm.launch()
>   
>           shell_ready = "sh: can't access tty; job control turned off"
> @@ -140,3 +141,12 @@ class S390CCWVirtioMachine(Test):
>           exec_command_and_wait_for_pattern(self,
>                                             'ls /sys/bus/ccw/devices/0.0.4711',
>                                             'No such file or directory')
> +        # test the virtio-balloon device
> +        exec_command_and_wait_for_pattern(self, 'head -n 1 /proc/meminfo',
> +                                          'MemTotal:         115640 kB')
> +        self.vm.command('human-monitor-command', command_line='balloon 96')
> +        exec_command_and_wait_for_pattern(self, 'head -n 1 /proc/meminfo',
> +                                          'MemTotal:          82872 kB')
> +        self.vm.command('human-monitor-command', command_line='balloon 128')
> +        exec_command_and_wait_for_pattern(self, 'head -n 1 /proc/meminfo',
> +                                          'MemTotal:         115640 kB')

Re: [PATCH 2/3] tests/acceptance/machine_s390_ccw_virtio: Test virtio-rng via /dev/hwrng

[Thomas Huth]

On 11/12/2020 21.30, Wainer dos Santos Moschetta wrote:
> Hi,
> 
> On 12/11/20 2:31 PM, Thomas Huth wrote:
>> /dev/hwrng is only functional if virtio-rng is working right, so let's
>> add a sanity check for this device node.
> 
> Good idea.
> 
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>   tests/acceptance/machine_s390_ccw_virtio.py | 17 +++++++++++++++--
>>   1 file changed, 15 insertions(+), 2 deletions(-)
>>
>> diff --git a/tests/acceptance/machine_s390_ccw_virtio.py
>> b/tests/acceptance/machine_s390_ccw_virtio.py
>> index 733a7ca24a..7d0a78139b 100644
>> --- a/tests/acceptance/machine_s390_ccw_virtio.py
>> +++ b/tests/acceptance/machine_s390_ccw_virtio.py
>> @@ -64,9 +64,9 @@ class S390CCWVirtioMachine(Test):
>>                            '-append', kernel_command_line,
>>                            '-device', 'virtio-net-ccw,devno=fe.1.1111',
>>                            '-device',
>> -                         'virtio-rng-ccw,devno=fe.2.0000,max_revision=0',
>> +                        
>> 'virtio-rng-ccw,devno=fe.2.0000,max_revision=0,id=rn1',
>>                            '-device',
>> -                         'virtio-rng-ccw,devno=fe.3.1234,max_revision=2',
>> +                        
>> 'virtio-rng-ccw,devno=fe.3.1234,max_revision=2,id=rn2',
>>                            '-device', 'zpci,uid=5,target=zzz',
>>                            '-device', 'virtio-net-pci,id=zzz',
>>                            '-device', 'zpci,uid=0xa,fid=12,target=serial',
>> @@ -96,6 +96,19 @@ class S390CCWVirtioMachine(Test):
>>           exec_command_and_wait_for_pattern(self,
>>                           'cat
>> /sys/bus/ccw/devices/0.3.1234/virtio?/features',
>>                           virtio_rng_features)
>> +        # check that /dev/hwrng works - and that it's gone after ejecting
>> +        exec_command_and_wait_for_pattern(self,
>> +                        'dd if=/dev/hwrng of=/tmp/out.dat bs=1k count=10',
>> +                        '10+0 records out')
>> +        self.clear_guests_dmesg()
>> +        self.vm.command('device_del', id='rn1')
>> +        self.wait_for_crw_reports()
>> +        self.clear_guests_dmesg()
>> +        self.vm.command('device_del', id='rn2')
>> +        self.wait_for_crw_reports()
>> +        exec_command_and_wait_for_pattern(self,
>> +                        'dd if=/dev/hwrng of=/tmp/out.dat bs=1k count=10',
>> +                        'dd: /dev/hwrng: No such device')
> 
> Maybe the expected pattern is too fragile. On my Fedora 33 system, 'dd' will
> print a different message.

We are running this test with a well-defined kernel + initrd, so I don't
think we have to care of other versions of dd here.

> What if it checks for the presence of the device file, e.g:
> 
> ... self, 'test -c /dev/hwrng; echo $?', '1')

That doesn't work, the /dev/hwrng is still there (so test -c succeeds),
since this initrd uses static device nodes for this in /dev. /dev/hwrng just
can not be opened anymore after the device has been removed.

 Thomas

Re: [PATCH 0/3] tests/acceptance: Test virtio-rng and -balloon on s390x

[Thomas Huth]

On 11/12/2020 21.10, Willian Rampazzo wrote:
> On 12/11/20 2:31 PM, Thomas Huth wrote:
>> Add two more simple tests to check that virtio-rng and virtio-balloon
>> are at least (very) basically working on s390x.
>>
>> Based-on: 20201204121450.120730-1-cohuck@redhat.com
>>
>> Thomas Huth (3):
>>    tests/acceptance: Extract the code to clear dmesg and wait for CRW
>>      reports
>>    tests/acceptance/machine_s390_ccw_virtio: Test virtio-rng via
>>      /dev/hwrng
>>    tests/acceptance/machine_s390_ccw_virtio: Test the virtio-balloon
>>      device
>>
>>   tests/acceptance/machine_s390_ccw_virtio.py | 59 +++++++++++++++------
>>   1 file changed, 43 insertions(+), 16 deletions(-)
>>
> 
> One observation, test_s390x_devices tends to get longer and difficult to
> debug in case of problems. If a test covers one specific device type, It
> will improve readability, flexibility, and debugging. In case you don't want
> to spend time breaking this into multiple tests, I'll be glad to do that
> after the whole series is merged.

Theoretically yes, but practically we also want to run the tests as fast as
possible. Quite a bit of time is used to boot the kernel, so if we add a new
test for each and every device, that would increase the test time quite a
bit. Thus I'd rather prefer to keep everything in one single test instead
for now.

> As far as code concerned,
> 
> Reviewed-by: Willian Rampazzo <willianr@redhat.com>
> Tested-by: Willian Rampazzo <willianr@redhat.com>

Thanks!

 Thomas

Re: [PATCH 0/3] tests/acceptance: Test virtio-rng and -balloon on s390x

[Cornelia Huck]

On Sat, 12 Dec 2020 08:15:13 +0100
Thomas Huth <thuth@redhat.com> wrote:

> On 11/12/2020 21.10, Willian Rampazzo wrote:
> > On 12/11/20 2:31 PM, Thomas Huth wrote:  
> >> Add two more simple tests to check that virtio-rng and virtio-balloon
> >> are at least (very) basically working on s390x.
> >>
> >> Based-on: 20201204121450.120730-1-cohuck@redhat.com
> >>
> >> Thomas Huth (3):
> >>    tests/acceptance: Extract the code to clear dmesg and wait for CRW
> >>      reports
> >>    tests/acceptance/machine_s390_ccw_virtio: Test virtio-rng via
> >>      /dev/hwrng
> >>    tests/acceptance/machine_s390_ccw_virtio: Test the virtio-balloon
> >>      device
> >>
> >>   tests/acceptance/machine_s390_ccw_virtio.py | 59 +++++++++++++++------
> >>   1 file changed, 43 insertions(+), 16 deletions(-)
> >>  
> > 
> > One observation, test_s390x_devices tends to get longer and difficult to
> > debug in case of problems. If a test covers one specific device type, It
> > will improve readability, flexibility, and debugging. In case you don't want
> > to spend time breaking this into multiple tests, I'll be glad to do that
> > after the whole series is merged.  
> 
> Theoretically yes, but practically we also want to run the tests as fast as
> possible. Quite a bit of time is used to boot the kernel, so if we add a new
> test for each and every device, that would increase the test time quite a
> bit. Thus I'd rather prefer to keep everything in one single test instead
> for now.

I agree.

Is there any other way to make this more debuggable?

Re: [PATCH 1/3] tests/acceptance: Extract the code to clear dmesg and wait for CRW reports

[Cornelia Huck]

On Fri, 11 Dec 2020 18:31:32 +0100
Thomas Huth <thuth@redhat.com> wrote:

> We will use this in more spots soon, so it's easier to put this into
> a separate function.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  tests/acceptance/machine_s390_ccw_virtio.py | 30 ++++++++++++---------
>  1 file changed, 17 insertions(+), 13 deletions(-)
> 
> diff --git a/tests/acceptance/machine_s390_ccw_virtio.py b/tests/acceptance/machine_s390_ccw_virtio.py
> index 864ef4ee6e..733a7ca24a 100644
> --- a/tests/acceptance/machine_s390_ccw_virtio.py
> +++ b/tests/acceptance/machine_s390_ccw_virtio.py
> @@ -17,12 +17,24 @@ from avocado_qemu import wait_for_console_pattern
>  class S390CCWVirtioMachine(Test):
>      KERNEL_COMMON_COMMAND_LINE = 'printk.time=0 '
>  
> +    timeout = 120
> +
>      def wait_for_console_pattern(self, success_message, vm=None):
>          wait_for_console_pattern(self, success_message,
>                                   failure_message='Kernel panic - not syncing',
>                                   vm=vm)
>  
> -    timeout = 120
> +    def wait_for_crw_reports(self):
> +        exec_command_and_wait_for_pattern(self,
> +                        'while ! (dmesg -c | grep CRW) ; do sleep 1 ; done',
> +                        'CRW reports')
> +
> +    dmesg_clear_count = 1
> +    def clear_guests_dmesg(self):

<nitpick> Maybe clear_guest_dmesg? </nitpick>

> +        exec_command_and_wait_for_pattern(self, 'dmesg -c > /dev/null; '
> +                    'echo dm_clear\ ' + str(self.dmesg_clear_count),
> +                    'dm_clear ' + str(self.dmesg_clear_count))
> +        self.dmesg_clear_count += 1
>  
>      def test_s390x_devices(self):
>  

Re: [PATCH 2/3] tests/acceptance/machine_s390_ccw_virtio: Test virtio-rng via /dev/hwrng

[Cornelia Huck]

On Sat, 12 Dec 2020 08:10:01 +0100
Thomas Huth <thuth@redhat.com> wrote:

Minor nit: I think that the subject is a bit unwieldy. What about

"tests/acceptance: Test virtio-rng on s390 via /dev/hwrng"

?

> On 11/12/2020 21.30, Wainer dos Santos Moschetta wrote:
> > Hi,
> > 
> > On 12/11/20 2:31 PM, Thomas Huth wrote:  
> >> /dev/hwrng is only functional if virtio-rng is working right, so let's
> >> add a sanity check for this device node.  
> > 
> > Good idea.
> >   
> >>
> >> Signed-off-by: Thomas Huth <thuth@redhat.com>
> >> ---
> >>   tests/acceptance/machine_s390_ccw_virtio.py | 17 +++++++++++++++--
> >>   1 file changed, 15 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/tests/acceptance/machine_s390_ccw_virtio.py
> >> b/tests/acceptance/machine_s390_ccw_virtio.py
> >> index 733a7ca24a..7d0a78139b 100644
> >> --- a/tests/acceptance/machine_s390_ccw_virtio.py
> >> +++ b/tests/acceptance/machine_s390_ccw_virtio.py
> >> @@ -64,9 +64,9 @@ class S390CCWVirtioMachine(Test):
> >>                            '-append', kernel_command_line,
> >>                            '-device', 'virtio-net-ccw,devno=fe.1.1111',
> >>                            '-device',
> >> -                         'virtio-rng-ccw,devno=fe.2.0000,max_revision=0',
> >> +                        
> >> 'virtio-rng-ccw,devno=fe.2.0000,max_revision=0,id=rn1',
> >>                            '-device',
> >> -                         'virtio-rng-ccw,devno=fe.3.1234,max_revision=2',
> >> +                        
> >> 'virtio-rng-ccw,devno=fe.3.1234,max_revision=2,id=rn2',
> >>                            '-device', 'zpci,uid=5,target=zzz',
> >>                            '-device', 'virtio-net-pci,id=zzz',
> >>                            '-device', 'zpci,uid=0xa,fid=12,target=serial',
> >> @@ -96,6 +96,19 @@ class S390CCWVirtioMachine(Test):
> >>           exec_command_and_wait_for_pattern(self,
> >>                           'cat
> >> /sys/bus/ccw/devices/0.3.1234/virtio?/features',
> >>                           virtio_rng_features)
> >> +        # check that /dev/hwrng works - and that it's gone after ejecting
> >> +        exec_command_and_wait_for_pattern(self,
> >> +                        'dd if=/dev/hwrng of=/tmp/out.dat bs=1k count=10',
> >> +                        '10+0 records out')
> >> +        self.clear_guests_dmesg()
> >> +        self.vm.command('device_del', id='rn1')
> >> +        self.wait_for_crw_reports()
> >> +        self.clear_guests_dmesg()
> >> +        self.vm.command('device_del', id='rn2')
> >> +        self.wait_for_crw_reports()
> >> +        exec_command_and_wait_for_pattern(self,
> >> +                        'dd if=/dev/hwrng of=/tmp/out.dat bs=1k count=10',

Does this work if you direct the output to /dev/null?

> >> +                        'dd: /dev/hwrng: No such device')  
> > 
> > Maybe the expected pattern is too fragile. On my Fedora 33 system, 'dd' will
> > print a different message.  
> 
> We are running this test with a well-defined kernel + initrd, so I don't
> think we have to care of other versions of dd here.
> 
> > What if it checks for the presence of the device file, e.g:
> > 
> > ... self, 'test -c /dev/hwrng; echo $?', '1')  
> 
> That doesn't work, the /dev/hwrng is still there (so test -c succeeds),
> since this initrd uses static device nodes for this in /dev. /dev/hwrng just
> can not be opened anymore after the device has been removed.

I had been thinking about a different approach to check that, but dd
really looks like the easiest way.

Re: [PATCH 0/3] tests/acceptance: Test virtio-rng and -balloon on s390x

[Willian Rampazzo]

On Sat, Dec 12, 2020 at 4:15 AM Thomas Huth <thuth@redhat.com> wrote:
>
> On 11/12/2020 21.10, Willian Rampazzo wrote:
> > On 12/11/20 2:31 PM, Thomas Huth wrote:
> >> Add two more simple tests to check that virtio-rng and virtio-balloon
> >> are at least (very) basically working on s390x.
> >>
> >> Based-on: 20201204121450.120730-1-cohuck@redhat.com
> >>
> >> Thomas Huth (3):
> >>    tests/acceptance: Extract the code to clear dmesg and wait for CRW
> >>      reports
> >>    tests/acceptance/machine_s390_ccw_virtio: Test virtio-rng via
> >>      /dev/hwrng
> >>    tests/acceptance/machine_s390_ccw_virtio: Test the virtio-balloon
> >>      device
> >>
> >>   tests/acceptance/machine_s390_ccw_virtio.py | 59 +++++++++++++++------
> >>   1 file changed, 43 insertions(+), 16 deletions(-)
> >>
> >
> > One observation, test_s390x_devices tends to get longer and difficult to
> > debug in case of problems. If a test covers one specific device type, It
> > will improve readability, flexibility, and debugging. In case you don't want
> > to spend time breaking this into multiple tests, I'll be glad to do that
> > after the whole series is merged.
>
> Theoretically yes, but practically we also want to run the tests as fast as
> possible. Quite a bit of time is used to boot the kernel, so if we add a new
> test for each and every device, that would increase the test time quite a
> bit. Thus I'd rather prefer to keep everything in one single test instead
> for now.

That is, indeed, a good point!


>
> > As far as code concerned,
> >
> > Reviewed-by: Willian Rampazzo <willianr@redhat.com>
> > Tested-by: Willian Rampazzo <willianr@redhat.com>
>
> Thanks!
>
>  Thomas
>

Re: [PATCH 0/3] tests/acceptance: Test virtio-rng and -balloon on s390x

[Willian Rampazzo]

On Mon, Dec 14, 2020 at 8:39 AM Cornelia Huck <cohuck@redhat.com> wrote:
>
> On Sat, 12 Dec 2020 08:15:13 +0100
> Thomas Huth <thuth@redhat.com> wrote:
>
> > On 11/12/2020 21.10, Willian Rampazzo wrote:
> > > On 12/11/20 2:31 PM, Thomas Huth wrote:
> > >> Add two more simple tests to check that virtio-rng and virtio-balloon
> > >> are at least (very) basically working on s390x.
> > >>
> > >> Based-on: 20201204121450.120730-1-cohuck@redhat.com
> > >>
> > >> Thomas Huth (3):
> > >>    tests/acceptance: Extract the code to clear dmesg and wait for CRW
> > >>      reports
> > >>    tests/acceptance/machine_s390_ccw_virtio: Test virtio-rng via
> > >>      /dev/hwrng
> > >>    tests/acceptance/machine_s390_ccw_virtio: Test the virtio-balloon
> > >>      device
> > >>
> > >>   tests/acceptance/machine_s390_ccw_virtio.py | 59 +++++++++++++++------
> > >>   1 file changed, 43 insertions(+), 16 deletions(-)
> > >>
> > >
> > > One observation, test_s390x_devices tends to get longer and difficult to
> > > debug in case of problems. If a test covers one specific device type, It
> > > will improve readability, flexibility, and debugging. In case you don't want
> > > to spend time breaking this into multiple tests, I'll be glad to do that
> > > after the whole series is merged.
> >
> > Theoretically yes, but practically we also want to run the tests as fast as
> > possible. Quite a bit of time is used to boot the kernel, so if we add a new
> > test for each and every device, that would increase the test time quite a
> > bit. Thus I'd rather prefer to keep everything in one single test instead
> > for now.
>
> I agree.
>
> Is there any other way to make this more debuggable?

I have some ideas in mind but I need to run some tests. I'm fine
merging the current code and improving it later.

Re: [PATCH 2/3] tests/acceptance/machine_s390_ccw_virtio: Test virtio-rng via /dev/hwrng

[Wainer dos Santos Moschetta]

On 12/12/20 4:10 AM, Thomas Huth wrote:
> On 11/12/2020 21.30, Wainer dos Santos Moschetta wrote:
>> Hi,
>>
>> On 12/11/20 2:31 PM, Thomas Huth wrote:
>>> /dev/hwrng is only functional if virtio-rng is working right, so let's
>>> add a sanity check for this device node.
>> Good idea.
>>
>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>> ---
>>>    tests/acceptance/machine_s390_ccw_virtio.py | 17 +++++++++++++++--
>>>    1 file changed, 15 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/tests/acceptance/machine_s390_ccw_virtio.py
>>> b/tests/acceptance/machine_s390_ccw_virtio.py
>>> index 733a7ca24a..7d0a78139b 100644
>>> --- a/tests/acceptance/machine_s390_ccw_virtio.py
>>> +++ b/tests/acceptance/machine_s390_ccw_virtio.py
>>> @@ -64,9 +64,9 @@ class S390CCWVirtioMachine(Test):
>>>                             '-append', kernel_command_line,
>>>                             '-device', 'virtio-net-ccw,devno=fe.1.1111',
>>>                             '-device',
>>> -                         'virtio-rng-ccw,devno=fe.2.0000,max_revision=0',
>>> +
>>> 'virtio-rng-ccw,devno=fe.2.0000,max_revision=0,id=rn1',
>>>                             '-device',
>>> -                         'virtio-rng-ccw,devno=fe.3.1234,max_revision=2',
>>> +
>>> 'virtio-rng-ccw,devno=fe.3.1234,max_revision=2,id=rn2',
>>>                             '-device', 'zpci,uid=5,target=zzz',
>>>                             '-device', 'virtio-net-pci,id=zzz',
>>>                             '-device', 'zpci,uid=0xa,fid=12,target=serial',
>>> @@ -96,6 +96,19 @@ class S390CCWVirtioMachine(Test):
>>>            exec_command_and_wait_for_pattern(self,
>>>                            'cat
>>> /sys/bus/ccw/devices/0.3.1234/virtio?/features',
>>>                            virtio_rng_features)
>>> +        # check that /dev/hwrng works - and that it's gone after ejecting
>>> +        exec_command_and_wait_for_pattern(self,
>>> +                        'dd if=/dev/hwrng of=/tmp/out.dat bs=1k count=10',
>>> +                        '10+0 records out')
>>> +        self.clear_guests_dmesg()
>>> +        self.vm.command('device_del', id='rn1')
>>> +        self.wait_for_crw_reports()
>>> +        self.clear_guests_dmesg()
>>> +        self.vm.command('device_del', id='rn2')
>>> +        self.wait_for_crw_reports()
>>> +        exec_command_and_wait_for_pattern(self,
>>> +                        'dd if=/dev/hwrng of=/tmp/out.dat bs=1k count=10',
>>> +                        'dd: /dev/hwrng: No such device')
>> Maybe the expected pattern is too fragile. On my Fedora 33 system, 'dd' will
>> print a different message.
> We are running this test with a well-defined kernel + initrd, so I don't
> think we have to care of other versions of dd here.

I was thinking in the case someone change the kernel+initrd for this 
test. Well, in that case the same person will have to make the 
adjustments. So,

Reviewed-by: Wainer dos Santos Moschetta <wainersm@redhat.com>

>
>> What if it checks for the presence of the device file, e.g:
>>
>> ... self, 'test -c /dev/hwrng; echo $?', '1')
> That doesn't work, the /dev/hwrng is still there (so test -c succeeds),
> since this initrd uses static device nodes for this in /dev. /dev/hwrng just
> can not be opened anymore after the device has been removed.
>
>   Thomas