[PATCH 0/6] tests/boot_linux_console: add extra boot acceptance tests

[PATCH 0/6] tests/boot_linux_console: add extra boot acceptance tests

[Liam Merwick]

Add acceptance tests for the microvm machine class, PVH, and the
new qboot BIOS.

In the case of the test to boot an uncompressed kernel there didn't
seem to be any suitable uncompressed kernel on https://archives.fedoraproject.org/
(there is a vmlinux in kernel-debuginfo but that RPM is 575M and
caused timeouts when populating the Avocado cache when first run)
so I chose an RPM with kernels for Kata that is 14M.
(there was a discussion in [1] regarding testing PVH boot but it focussed
more around building a vmlinux binary during testing).

[ What prompted these patches was the discovery that a 'pc' guest booting an
uncompressed kernel (PVH) with a PCI netdev hangs (before we even get guest
console output) when bios-microvm.bin (qboot) is supplied via -bios
(no issue when using 'q35' or 'microvm' machine classes).

E.g. adding the following line to test_x86_64_pc_qboot_pvh() is enough to
trigger a guest hang during startup:
self.vm.add_args('-netdev', 'user,id=n1', '-device', 'virtio-net-pci,netdev=n1')

I bisected that issue to 176d2cda0dee [2] in 4.1 but haven't worked out yet
how/why the "die-id" changes impact the qboot/PVH combination
(the boot succeeds with any subset of those boot variables).

Is booting the 'pc' machine class with bios-microvm.bin something that QEMU
officially supports or is qboot intended for microvm only? ]

Each test added here adds about 1.5s to the overall runtime.
I have run them through the Travis QEMU CI [3] and those acceptance tests pass.

My modifications to test_x86_64_pc() in Patch1 will conflict with Wainer's
patch in [4] - I'll rebase on top of that once that series is pulled and
and apply feedback for this series, etc.

[1] https://patchew.org/QEMU/20191206140012.15517-1-wainersm@redhat.com/
[2] 176d2cda0dee ("i386/cpu: Consolidate die-id validity in smp context")
[3] https://travis-ci.org/merwick/qemu/jobs/641505543
[4] https://github.com/wainersm/qemu/commit/8f705e98df90b436b0f4946331d441309c437f7b


Liam Merwick (6):
  tests/boot_linux_console: add microvm acceptance test
  tests/boot_linux_console: add BIOS acceptance test
  tests/boot_linux_console: fix extract_from_deb() comment
  travis.yml: install rpm2cpio for acceptance tests
  tests/boot_linux_console: add extract_from_rpm method
  tests/boot_linux_console: add PVH acceptance tests

 .travis.yml                            |  1 +
 tests/acceptance/boot_linux_console.py | 91 +++++++++++++++++++++++++++++++---
 2 files changed, 84 insertions(+), 8 deletions(-)

-- 
1.8.3.1

[PATCH 1/6] tests/boot_linux_console: add microvm acceptance test

[Liam Merwick]

Refactor test_x86_64_pc() to test_x86_64_machine() so that separate
functions which specify the Avocado tag of ':avocado: tags=machine:'
as being either 'pc' or 'microvm' can be used to test booting a
compressed kernel using either machine class.

Signed-off-by: Liam Merwick <liam.merwick@oracle.com>
---
 tests/acceptance/boot_linux_console.py | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/tests/acceptance/boot_linux_console.py b/tests/acceptance/boot_linux_console.py
index e40b84651b0b..aa5b07b1c609 100644
--- a/tests/acceptance/boot_linux_console.py
+++ b/tests/acceptance/boot_linux_console.py
@@ -51,10 +51,9 @@ class BootLinuxConsole(Test):
         os.chdir(cwd)
         return self.workdir + path
 
-    def test_x86_64_pc(self):
+    def do_test_x86_64_machine(self):
         """
         :avocado: tags=arch:x86_64
-        :avocado: tags=machine:pc
         """
         kernel_url = ('https://archives.fedoraproject.org/pub/archive/fedora'
                       '/linux/releases/29/Everything/x86_64/os/images/pxeboot'
@@ -70,6 +69,18 @@ class BootLinuxConsole(Test):
         console_pattern = 'Kernel command line: %s' % kernel_command_line
         self.wait_for_console_pattern(console_pattern)
 
+    def test_x86_64_pc(self):
+        """
+        :avocado: tags=machine:pc
+        """
+        self.do_test_x86_64_machine()
+
+    def test_x86_64_microvm(self):
+        """
+        :avocado: tags=machine:microvm
+        """
+        self.do_test_x86_64_machine()
+
     def test_mips_malta(self):
         """
         :avocado: tags=arch:mips
-- 
1.8.3.1

Re: [PATCH 1/6] tests/boot_linux_console: add microvm acceptance test

[Stefano Garzarella]

On Mon, Jan 27, 2020 at 04:36:29PM +0000, Liam Merwick wrote:
> Refactor test_x86_64_pc() to test_x86_64_machine() so that separate
> functions which specify the Avocado tag of ':avocado: tags=machine:'
> as being either 'pc' or 'microvm' can be used to test booting a
> compressed kernel using either machine class.
> 
> Signed-off-by: Liam Merwick <liam.merwick@oracle.com>
> ---
>  tests/acceptance/boot_linux_console.py | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)

Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>

> 
> diff --git a/tests/acceptance/boot_linux_console.py b/tests/acceptance/boot_linux_console.py
> index e40b84651b0b..aa5b07b1c609 100644
> --- a/tests/acceptance/boot_linux_console.py
> +++ b/tests/acceptance/boot_linux_console.py
> @@ -51,10 +51,9 @@ class BootLinuxConsole(Test):
>          os.chdir(cwd)
>          return self.workdir + path
>  
> -    def test_x86_64_pc(self):
> +    def do_test_x86_64_machine(self):
>          """
>          :avocado: tags=arch:x86_64
> -        :avocado: tags=machine:pc
>          """
>          kernel_url = ('https://archives.fedoraproject.org/pub/archive/fedora'
>                        '/linux/releases/29/Everything/x86_64/os/images/pxeboot'
> @@ -70,6 +69,18 @@ class BootLinuxConsole(Test):
>          console_pattern = 'Kernel command line: %s' % kernel_command_line
>          self.wait_for_console_pattern(console_pattern)
>  
> +    def test_x86_64_pc(self):
> +        """
> +        :avocado: tags=machine:pc
> +        """
> +        self.do_test_x86_64_machine()
> +
> +    def test_x86_64_microvm(self):
> +        """
> +        :avocado: tags=machine:microvm
> +        """
> +        self.do_test_x86_64_machine()
> +
>      def test_mips_malta(self):
>          """
>          :avocado: tags=arch:mips
> -- 
> 1.8.3.1
> 

-- 

Re: [PATCH 1/6] tests/boot_linux_console: add microvm acceptance test

[Wainer dos Santos Moschetta]

On 1/27/20 2:36 PM, Liam Merwick wrote:
> Refactor test_x86_64_pc() to test_x86_64_machine() so that separate
> functions which specify the Avocado tag of ':avocado: tags=machine:'
> as being either 'pc' or 'microvm' can be used to test booting a
> compressed kernel using either machine class.
>
> Signed-off-by: Liam Merwick <liam.merwick@oracle.com>
> ---
>   tests/acceptance/boot_linux_console.py | 15 +++++++++++++--
>   1 file changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/tests/acceptance/boot_linux_console.py b/tests/acceptance/boot_linux_console.py
> index e40b84651b0b..aa5b07b1c609 100644
> --- a/tests/acceptance/boot_linux_console.py
> +++ b/tests/acceptance/boot_linux_console.py
> @@ -51,10 +51,9 @@ class BootLinuxConsole(Test):
>           os.chdir(cwd)
>           return self.workdir + path
>   
> -    def test_x86_64_pc(self):
> +    def do_test_x86_64_machine(self):
>           """
>           :avocado: tags=arch:x86_64
> -        :avocado: tags=machine:pc
>           """
>           kernel_url = ('https://archives.fedoraproject.org/pub/archive/fedora'
>                         '/linux/releases/29/Everything/x86_64/os/images/pxeboot'
> @@ -70,6 +69,18 @@ class BootLinuxConsole(Test):
>           console_pattern = 'Kernel command line: %s' % kernel_command_line
>           self.wait_for_console_pattern(console_pattern)
>   
> +    def test_x86_64_pc(self):
> +        """
> +        :avocado: tags=machine:pc
> +        """

The test method won't inherit the 'arch' tag from 
`do_test_x86_64_machine()`, so you need to explicitly 'arch' tag each 
test you created in this series. If you don't do so, Avocado won't 
filter out those x86_64 tests in case QEMU is built with non-x86_64 targets.

Follows an example, I built QEMU with '--target-list=arm-softmmu'. I got:

```

(02/18) 
tests/acceptance/boot_linux_console.py:BootLinuxConsole.test_x86_64_pc: 
CANCEL: No QEMU binary defined or found in the source tree (0.00 s)
  (03/18) 
tests/acceptance/boot_linux_console.py:BootLinuxConsole.test_x86_64_microvm: 
CANCEL: No QEMU binary defined or found in the source tree (0.00 s)
  (04/18) 
tests/acceptance/boot_linux_console.py:BootLinuxConsole.test_arm_virt: 
PASS (1.25 s)

```

OK, avocado_qemu was smart enough to skip the tests, but ideally it 
should not even consider running them in the first place.

Thanks!

- Wainer

> +        self.do_test_x86_64_machine()
> +
> +    def test_x86_64_microvm(self):
> +        """
> +        :avocado: tags=machine:microvm
> +        """
> +        self.do_test_x86_64_machine()
> +
>       def test_mips_malta(self):
>           """
>           :avocado: tags=arch:mips

Re: [PATCH 1/6] tests/boot_linux_console: add microvm acceptance test

[Philippe Mathieu-Daudé]

On 1/30/20 6:41 PM, Wainer dos Santos Moschetta wrote:
> On 1/27/20 2:36 PM, Liam Merwick wrote:
>> Refactor test_x86_64_pc() to test_x86_64_machine() so that separate
>> functions which specify the Avocado tag of ':avocado: tags=machine:'
>> as being either 'pc' or 'microvm' can be used to test booting a
>> compressed kernel using either machine class.
>>
>> Signed-off-by: Liam Merwick <liam.merwick@oracle.com>
>> ---
>>   tests/acceptance/boot_linux_console.py | 15 +++++++++++++--
>>   1 file changed, 13 insertions(+), 2 deletions(-)
>>
>> diff --git a/tests/acceptance/boot_linux_console.py 
>> b/tests/acceptance/boot_linux_console.py
>> index e40b84651b0b..aa5b07b1c609 100644
>> --- a/tests/acceptance/boot_linux_console.py
>> +++ b/tests/acceptance/boot_linux_console.py
>> @@ -51,10 +51,9 @@ class BootLinuxConsole(Test):
>>           os.chdir(cwd)
>>           return self.workdir + path
>> -    def test_x86_64_pc(self):
>> +    def do_test_x86_64_machine(self):
>>           """
>>           :avocado: tags=arch:x86_64
>> -        :avocado: tags=machine:pc
>>           """
>>           kernel_url = 
>> ('https://archives.fedoraproject.org/pub/archive/fedora'
>>                         
>> '/linux/releases/29/Everything/x86_64/os/images/pxeboot'
>> @@ -70,6 +69,18 @@ class BootLinuxConsole(Test):
>>           console_pattern = 'Kernel command line: %s' % 
>> kernel_command_line
>>           self.wait_for_console_pattern(console_pattern)
>> +    def test_x86_64_pc(self):
>> +        """
>> +        :avocado: tags=machine:pc
>> +        """
> 
> The test method won't inherit the 'arch' tag from 
> `do_test_x86_64_machine()`, so you need to explicitly 'arch' tag each 
> test you created in this series. If you don't do so, Avocado won't 
> filter out those x86_64 tests in case QEMU is built with non-x86_64 
> targets.
> 
> Follows an example, I built QEMU with '--target-list=arm-softmmu'. I got:
> 
> ```
> 
> (02/18) 
> tests/acceptance/boot_linux_console.py:BootLinuxConsole.test_x86_64_pc: 
> CANCEL: No QEMU binary defined or found in the source tree (0.00 s)
>   (03/18) 
> tests/acceptance/boot_linux_console.py:BootLinuxConsole.test_x86_64_microvm: 
> CANCEL: No QEMU binary defined or found in the source tree (0.00 s)
>   (04/18) 
> tests/acceptance/boot_linux_console.py:BootLinuxConsole.test_arm_virt: 
> PASS (1.25 s)
> 
> ```
> 
> OK, avocado_qemu was smart enough to skip the tests, but ideally it 
> should not even consider running them in the first place.

This should be solved by this patch:
https://lists.gnu.org/archive/html/qemu-devel/2020-01/msg07311.html
"tests/acceptance: Use 'machine' tag to check if available in QEMU binary"

> 
> Thanks!
> 
> - Wainer
> 
>> +        self.do_test_x86_64_machine()
>> +
>> +    def test_x86_64_microvm(self):
>> +        """
>> +        :avocado: tags=machine:microvm
>> +        """
>> +        self.do_test_x86_64_machine()
>> +
>>       def test_mips_malta(self):
>>           """
>>           :avocado: tags=arch:mips
> 

Re: [PATCH 1/6] tests/boot_linux_console: add microvm acceptance test

[Wainer dos Santos Moschetta]

On 1/30/20 9:51 PM, Philippe Mathieu-Daudé wrote:
> On 1/30/20 6:41 PM, Wainer dos Santos Moschetta wrote:
>> On 1/27/20 2:36 PM, Liam Merwick wrote:
>>> Refactor test_x86_64_pc() to test_x86_64_machine() so that separate
>>> functions which specify the Avocado tag of ':avocado: tags=machine:'
>>> as being either 'pc' or 'microvm' can be used to test booting a
>>> compressed kernel using either machine class.
>>>
>>> Signed-off-by: Liam Merwick <liam.merwick@oracle.com>
>>> ---
>>>   tests/acceptance/boot_linux_console.py | 15 +++++++++++++--
>>>   1 file changed, 13 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/tests/acceptance/boot_linux_console.py 
>>> b/tests/acceptance/boot_linux_console.py
>>> index e40b84651b0b..aa5b07b1c609 100644
>>> --- a/tests/acceptance/boot_linux_console.py
>>> +++ b/tests/acceptance/boot_linux_console.py
>>> @@ -51,10 +51,9 @@ class BootLinuxConsole(Test):
>>>           os.chdir(cwd)
>>>           return self.workdir + path
>>> -    def test_x86_64_pc(self):
>>> +    def do_test_x86_64_machine(self):
>>>           """
>>>           :avocado: tags=arch:x86_64
>>> -        :avocado: tags=machine:pc
>>>           """
>>>           kernel_url = 
>>> ('https://archives.fedoraproject.org/pub/archive/fedora'
>>> '/linux/releases/29/Everything/x86_64/os/images/pxeboot'
>>> @@ -70,6 +69,18 @@ class BootLinuxConsole(Test):
>>>           console_pattern = 'Kernel command line: %s' % 
>>> kernel_command_line
>>>           self.wait_for_console_pattern(console_pattern)
>>> +    def test_x86_64_pc(self):
>>> +        """
>>> +        :avocado: tags=machine:pc
>>> +        """
>>
>> The test method won't inherit the 'arch' tag from 
>> `do_test_x86_64_machine()`, so you need to explicitly 'arch' tag each 
>> test you created in this series. If you don't do so, Avocado won't 
>> filter out those x86_64 tests in case QEMU is built with non-x86_64 
>> targets.
>>
>> Follows an example, I built QEMU with '--target-list=arm-softmmu'. I 
>> got:
>>
>> ```
>>
>> (02/18) 
>> tests/acceptance/boot_linux_console.py:BootLinuxConsole.test_x86_64_pc: 
>> CANCEL: No QEMU binary defined or found in the source tree (0.00 s)
>>   (03/18) 
>> tests/acceptance/boot_linux_console.py:BootLinuxConsole.test_x86_64_microvm: 
>> CANCEL: No QEMU binary defined or found in the source tree (0.00 s)
>>   (04/18) 
>> tests/acceptance/boot_linux_console.py:BootLinuxConsole.test_arm_virt: 
>> PASS (1.25 s)
>>
>> ```
>>
>> OK, avocado_qemu was smart enough to skip the tests, but ideally it 
>> should not even consider running them in the first place.
>
> This should be solved by this patch:
> https://lists.gnu.org/archive/html/qemu-devel/2020-01/msg07311.html
> "tests/acceptance: Use 'machine' tag to check if available in QEMU binary"


That patch is useful**  but does not solve the problem that I point out 
here. If you have a test case which is arch-specific and the arch-QEMU 
was not built then it should not be executed. Tagging the test with 
'arch' ensures that.

** BTW, I'm going to review it soon

- Wainer


>>
>> Thanks!
>>
>> - Wainer
>>
>>> +        self.do_test_x86_64_machine()
>>> +
>>> +    def test_x86_64_microvm(self):
>>> +        """
>>> +        :avocado: tags=machine:microvm
>>> +        """
>>> +        self.do_test_x86_64_machine()
>>> +
>>>       def test_mips_malta(self):
>>>           """
>>>           :avocado: tags=arch:mips
>>
>
>

[PATCH 2/6] tests/boot_linux_console: add BIOS acceptance test

[Liam Merwick]

Add tests to use qboot with the 'pc' and 'microvm' machine classes
by adding the '-bios' option via self.vm.add_args() before calling
do_test_x86_64_machine().

Signed-off-by: Liam Merwick <liam.merwick@oracle.com>
---
 tests/acceptance/boot_linux_console.py | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/tests/acceptance/boot_linux_console.py b/tests/acceptance/boot_linux_console.py
index aa5b07b1c609..8daf6461ffac 100644
--- a/tests/acceptance/boot_linux_console.py
+++ b/tests/acceptance/boot_linux_console.py
@@ -60,7 +60,6 @@ class BootLinuxConsole(Test):
                       '/vmlinuz')
         kernel_hash = '23bebd2680757891cf7adedb033532163a792495'
         kernel_path = self.fetch_asset(kernel_url, asset_hash=kernel_hash)
-
         self.vm.set_console()
         kernel_command_line = self.KERNEL_COMMON_COMMAND_LINE + 'console=ttyS0'
         self.vm.add_args('-kernel', kernel_path,
@@ -75,12 +74,26 @@ class BootLinuxConsole(Test):
         """
         self.do_test_x86_64_machine()
 
+    def test_x86_64_pc_qboot(self):
+        """
+        :avocado: tags=machine:pc
+        """
+        self.vm.add_args('-bios', 'pc-bios/bios-microvm.bin')
+        self.do_test_x86_64_machine()
+
     def test_x86_64_microvm(self):
         """
         :avocado: tags=machine:microvm
         """
         self.do_test_x86_64_machine()
 
+    def test_x86_64_microvm_qboot(self):
+        """
+        :avocado: tags=machine:microvm
+        """
+        self.vm.add_args('-bios', 'pc-bios/bios-microvm.bin')
+        self.do_test_x86_64_machine()
+
     def test_mips_malta(self):
         """
         :avocado: tags=arch:mips
-- 
1.8.3.1

Re: [PATCH 2/6] tests/boot_linux_console: add BIOS acceptance test

[Stefano Garzarella]

Hi Liam,

On Mon, Jan 27, 2020 at 04:36:30PM +0000, Liam Merwick wrote:
> Add tests to use qboot with the 'pc' and 'microvm' machine classes
> by adding the '-bios' option via self.vm.add_args() before calling
> do_test_x86_64_machine().
> 
> Signed-off-by: Liam Merwick <liam.merwick@oracle.com>
> ---
>  tests/acceptance/boot_linux_console.py | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/acceptance/boot_linux_console.py b/tests/acceptance/boot_linux_console.py
> index aa5b07b1c609..8daf6461ffac 100644
> --- a/tests/acceptance/boot_linux_console.py
> +++ b/tests/acceptance/boot_linux_console.py
> @@ -60,7 +60,6 @@ class BootLinuxConsole(Test):
>                        '/vmlinuz')
>          kernel_hash = '23bebd2680757891cf7adedb033532163a792495'
>          kernel_path = self.fetch_asset(kernel_url, asset_hash=kernel_hash)
> -
>          self.vm.set_console()
>          kernel_command_line = self.KERNEL_COMMON_COMMAND_LINE + 'console=ttyS0'
>          self.vm.add_args('-kernel', kernel_path,
> @@ -75,12 +74,26 @@ class BootLinuxConsole(Test):
>          """
>          self.do_test_x86_64_machine()
>  
> +    def test_x86_64_pc_qboot(self):
> +        """
> +        :avocado: tags=machine:pc
> +        """
> +        self.vm.add_args('-bios', 'pc-bios/bios-microvm.bin')
> +        self.do_test_x86_64_machine()
> +
>      def test_x86_64_microvm(self):
>          """
>          :avocado: tags=machine:microvm
>          """
>          self.do_test_x86_64_machine()
>  
> +    def test_x86_64_microvm_qboot(self):
> +        """
> +        :avocado: tags=machine:microvm
> +        """
> +        self.vm.add_args('-bios', 'pc-bios/bios-microvm.bin')
> +        self.do_test_x86_64_machine()
> +

Reading the docs/microvm.rst, microvm should use qboot as default, so
the test_x86_64_microvm() and test_x86_64_microvm_qboot() maybe are the
same (I didn't test them).

>      def test_mips_malta(self):
>          """
>          :avocado: tags=arch:mips

Thanks for doing these tests!
Stefano

Re: [PATCH 2/6] tests/boot_linux_console: add BIOS acceptance test

[Liam Merwick]

On 30/01/2020 11:27, Stefano Garzarella wrote:
> Hi Liam,
> 
> On Mon, Jan 27, 2020 at 04:36:30PM +0000, Liam Merwick wrote:
>> Add tests to use qboot with the 'pc' and 'microvm' machine classes
>> by adding the '-bios' option via self.vm.add_args() before calling
>> do_test_x86_64_machine().
>>
>> Signed-off-by: Liam Merwick <liam.merwick@oracle.com>
>> ---
>>   tests/acceptance/boot_linux_console.py | 15 ++++++++++++++-
>>   1 file changed, 14 insertions(+), 1 deletion(-)
>>
>> diff --git a/tests/acceptance/boot_linux_console.py b/tests/acceptance/boot_linux_console.py
>> index aa5b07b1c609..8daf6461ffac 100644
>> --- a/tests/acceptance/boot_linux_console.py
>> +++ b/tests/acceptance/boot_linux_console.py

...

>>   
>> +    def test_x86_64_microvm_qboot(self):
>> +        """
>> +        :avocado: tags=machine:microvm
>> +        """
>> +        self.vm.add_args('-bios', 'pc-bios/bios-microvm.bin')
>> +        self.do_test_x86_64_machine()
>> +
> 
> Reading the docs/microvm.rst, microvm should use qboot as default, so
> the test_x86_64_microvm() and test_x86_64_microvm_qboot() maybe are the
> same (I didn't test them).

I traced loader_write_rom() and in both cases bios-microvm.bin got
loaded. While there may be a slight benefit in verifying that usage of
an explicit -bios works, I think I'll just drop the unnecessary test
case in patches 2 and 6 in v2.

> 
>>       def test_mips_malta(self):
>>           """
>>           :avocado: tags=arch:mips
> 
> Thanks for doing these tests!

And thanks for reviewing the series.

Regards,
Liam

Re: [PATCH 2/6] tests/boot_linux_console: add BIOS acceptance test

[Liam Merwick]

On 30/01/2020 15:34, Liam Merwick wrote:
> On 30/01/2020 11:27, Stefano Garzarella wrote:
>> Hi Liam,
>>
>> On Mon, Jan 27, 2020 at 04:36:30PM +0000, Liam Merwick wrote:
>>> Add tests to use qboot with the 'pc' and 'microvm' machine classes
>>> by adding the '-bios' option via self.vm.add_args() before calling
>>> do_test_x86_64_machine().
>>>
>>> Signed-off-by: Liam Merwick <liam.merwick@oracle.com>
>>> ---
>>>   tests/acceptance/boot_linux_console.py | 15 ++++++++++++++-
>>>   1 file changed, 14 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/tests/acceptance/boot_linux_console.py 
>>> b/tests/acceptance/boot_linux_console.py
>>> index aa5b07b1c609..8daf6461ffac 100644
>>> --- a/tests/acceptance/boot_linux_console.py
>>> +++ b/tests/acceptance/boot_linux_console.py
> 
> ...
> 
>>> +    def test_x86_64_microvm_qboot(self):
>>> +        """
>>> +        :avocado: tags=machine:microvm
>>> +        """
>>> +        self.vm.add_args('-bios', 'pc-bios/bios-microvm.bin')
>>> +        self.do_test_x86_64_machine()
>>> +
>>
>> Reading the docs/microvm.rst, microvm should use qboot as default, so
>> the test_x86_64_microvm() and test_x86_64_microvm_qboot() maybe are the
>> same (I didn't test them).
> 
> I traced loader_write_rom() and in both cases bios-microvm.bin got
> loaded. While there may be a slight benefit in verifying that usage of
> an explicit -bios works, I think I'll just drop the unnecessary test
> case in patches 2 and 6 in v2.
> 

When making that change to remove the test case from Patch2, it dawned
on me that it might be worth testing microvm with a different bios 
instead...

--- a/tests/acceptance/boot_linux_console.py
+++ b/tests/acceptance/boot_linux_console.py
@@ -87,6 +87,13 @@ class BootLinuxConsole(Test):
          """
          self.do_test_x86_64_machine()

+    def test_x86_64_microvm_seabios(self):
+        """
+        :avocado: tags=machine:microvm
+        """
+        self.vm.add_args('-bios', 'pc-bios/bios.bin')
+        self.do_test_x86_64_machine()
+


>>
>>>       def test_mips_malta(self):
>>>           """
>>>           :avocado: tags=arch:mips
>>
>> Thanks for doing these tests!
> 
> And thanks for reviewing the series.
> 
> Regards,
> Liam
> 

Re: [PATCH 2/6] tests/boot_linux_console: add BIOS acceptance test

[Stefano Garzarella]

On Thu, Jan 30, 2020 at 04:28:52PM +0000, Liam Merwick wrote:
> On 30/01/2020 15:34, Liam Merwick wrote:
> > On 30/01/2020 11:27, Stefano Garzarella wrote:
> > > Hi Liam,
> > > 
> > > On Mon, Jan 27, 2020 at 04:36:30PM +0000, Liam Merwick wrote:
> > > > Add tests to use qboot with the 'pc' and 'microvm' machine classes
> > > > by adding the '-bios' option via self.vm.add_args() before calling
> > > > do_test_x86_64_machine().
> > > > 
> > > > Signed-off-by: Liam Merwick <liam.merwick@oracle.com>
> > > > ---
> > > >   tests/acceptance/boot_linux_console.py | 15 ++++++++++++++-
> > > >   1 file changed, 14 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/tests/acceptance/boot_linux_console.py
> > > > b/tests/acceptance/boot_linux_console.py
> > > > index aa5b07b1c609..8daf6461ffac 100644
> > > > --- a/tests/acceptance/boot_linux_console.py
> > > > +++ b/tests/acceptance/boot_linux_console.py
> > 
> > ...
> > 
> > > > +    def test_x86_64_microvm_qboot(self):
> > > > +        """
> > > > +        :avocado: tags=machine:microvm
> > > > +        """
> > > > +        self.vm.add_args('-bios', 'pc-bios/bios-microvm.bin')
> > > > +        self.do_test_x86_64_machine()
> > > > +
> > > 
> > > Reading the docs/microvm.rst, microvm should use qboot as default, so
> > > the test_x86_64_microvm() and test_x86_64_microvm_qboot() maybe are the
> > > same (I didn't test them).
> > 
> > I traced loader_write_rom() and in both cases bios-microvm.bin got
> > loaded. While there may be a slight benefit in verifying that usage of
> > an explicit -bios works, I think I'll just drop the unnecessary test
> > case in patches 2 and 6 in v2.
> > 
> 
> When making that change to remove the test case from Patch2, it dawned
> on me that it might be worth testing microvm with a different bios
> instead...

Make sense! In docs/microvm.rst we say we support both, so it seems
reasonable to test even seabios.

Thanks,
Stefano

> 
> --- a/tests/acceptance/boot_linux_console.py
> +++ b/tests/acceptance/boot_linux_console.py
> @@ -87,6 +87,13 @@ class BootLinuxConsole(Test):
>          """
>          self.do_test_x86_64_machine()
> 
> +    def test_x86_64_microvm_seabios(self):
> +        """
> +        :avocado: tags=machine:microvm
> +        """
> +        self.vm.add_args('-bios', 'pc-bios/bios.bin')
> +        self.do_test_x86_64_machine()
> +

[PATCH 3/6] tests/boot_linux_console: fix extract_from_deb() comment

[Liam Merwick]

The second param in extract_from_deb() is 'path' not 'file'

Signed-off-by: Liam Merwick <liam.merwick@oracle.com>
---
 tests/acceptance/boot_linux_console.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/acceptance/boot_linux_console.py b/tests/acceptance/boot_linux_console.py
index 8daf6461ffac..43bc928b03a2 100644
--- a/tests/acceptance/boot_linux_console.py
+++ b/tests/acceptance/boot_linux_console.py
@@ -40,7 +40,7 @@ class BootLinuxConsole(Test):
         Extracts a file from a deb package into the test workdir
 
         :param deb: path to the deb archive
-        :param file: path within the deb archive of the file to be extracted
+        :param path: path within the deb archive of the file to be extracted
         :returns: path of the extracted file
         """
         cwd = os.getcwd()
-- 
1.8.3.1

Re: [PATCH 3/6] tests/boot_linux_console: fix extract_from_deb() comment

[Stefano Garzarella]

On Mon, Jan 27, 2020 at 04:36:31PM +0000, Liam Merwick wrote:
> The second param in extract_from_deb() is 'path' not 'file'
> 
> Signed-off-by: Liam Merwick <liam.merwick@oracle.com>
> ---
>  tests/acceptance/boot_linux_console.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>

> 
> diff --git a/tests/acceptance/boot_linux_console.py b/tests/acceptance/boot_linux_console.py
> index 8daf6461ffac..43bc928b03a2 100644
> --- a/tests/acceptance/boot_linux_console.py
> +++ b/tests/acceptance/boot_linux_console.py
> @@ -40,7 +40,7 @@ class BootLinuxConsole(Test):
>          Extracts a file from a deb package into the test workdir
>  
>          :param deb: path to the deb archive
> -        :param file: path within the deb archive of the file to be extracted
> +        :param path: path within the deb archive of the file to be extracted
>          :returns: path of the extracted file
>          """
>          cwd = os.getcwd()
> -- 
> 1.8.3.1
> 

-- 

Re: [PATCH 3/6] tests/boot_linux_console: fix extract_from_deb() comment

[Wainer dos Santos Moschetta]

On 1/27/20 2:36 PM, Liam Merwick wrote:
> The second param in extract_from_deb() is 'path' not 'file'
>
> Signed-off-by: Liam Merwick <liam.merwick@oracle.com>
> ---
>   tests/acceptance/boot_linux_console.py | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tests/acceptance/boot_linux_console.py b/tests/acceptance/boot_linux_console.py
> index 8daf6461ffac..43bc928b03a2 100644
> --- a/tests/acceptance/boot_linux_console.py
> +++ b/tests/acceptance/boot_linux_console.py
> @@ -40,7 +40,7 @@ class BootLinuxConsole(Test):
>           Extracts a file from a deb package into the test workdir
>   
>           :param deb: path to the deb archive
> -        :param file: path within the deb archive of the file to be extracted
> +        :param path: path within the deb archive of the file to be extracted

Well noted!

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

>           :returns: path of the extracted file
>           """
>           cwd = os.getcwd()

Re: [PATCH 3/6] tests/boot_linux_console: fix extract_from_deb() comment

[Philippe Mathieu-Daudé]

On 1/27/20 5:36 PM, Liam Merwick wrote:
> The second param in extract_from_deb() is 'path' not 'file'
> 
> Signed-off-by: Liam Merwick <liam.merwick@oracle.com>
> ---
>   tests/acceptance/boot_linux_console.py | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/acceptance/boot_linux_console.py b/tests/acceptance/boot_linux_console.py
> index 8daf6461ffac..43bc928b03a2 100644
> --- a/tests/acceptance/boot_linux_console.py
> +++ b/tests/acceptance/boot_linux_console.py
> @@ -40,7 +40,7 @@ class BootLinuxConsole(Test):
>           Extracts a file from a deb package into the test workdir
>   
>           :param deb: path to the deb archive
> -        :param file: path within the deb archive of the file to be extracted
> +        :param path: path within the deb archive of the file to be extracted
>           :returns: path of the extracted file
>           """
>           cwd = os.getcwd()
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Also, applied to my python-next tree:
https://gitlab.com/philmd/qemu/commits/python-next

[PATCH 4/6] travis.yml: install rpm2cpio for acceptance tests

[Liam Merwick]

The extract_from_rpm() method added for the PVH acceptance tests needs
rpm2cpio to extract a vmlinux binary from an RPM.

Signed-off-by: Liam Merwick <liam.merwick@oracle.com>
---
 .travis.yml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/.travis.yml b/.travis.yml
index 1ae645e9fcec..3d8c2a38e679 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -278,6 +278,7 @@ matrix:
             - python3-pil
             - python3-pip
             - python3.5-venv
+            - rpm2cpio
             - tesseract-ocr
             - tesseract-ocr-eng
 
-- 
1.8.3.1

Re: [PATCH 4/6] travis.yml: install rpm2cpio for acceptance tests

[Stefano Garzarella]

On Mon, Jan 27, 2020 at 04:36:32PM +0000, Liam Merwick wrote:
> The extract_from_rpm() method added for the PVH acceptance tests needs
> rpm2cpio to extract a vmlinux binary from an RPM.
> 
> Signed-off-by: Liam Merwick <liam.merwick@oracle.com>
> ---
>  .travis.yml | 1 +
>  1 file changed, 1 insertion(+)

Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>

> 
> diff --git a/.travis.yml b/.travis.yml
> index 1ae645e9fcec..3d8c2a38e679 100644
> --- a/.travis.yml
> +++ b/.travis.yml
> @@ -278,6 +278,7 @@ matrix:
>              - python3-pil
>              - python3-pip
>              - python3.5-venv
> +            - rpm2cpio
>              - tesseract-ocr
>              - tesseract-ocr-eng
>  
> -- 
> 1.8.3.1
> 

[PATCH 5/6] tests/boot_linux_console: add extract_from_rpm method

[Liam Merwick]

Add a method to extract a specified file from an RPM to the test's
working directory and return the path to the extracted file.

Signed-off-by: Liam Merwick <liam.merwick@oracle.com>
---
 tests/acceptance/boot_linux_console.py | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/tests/acceptance/boot_linux_console.py b/tests/acceptance/boot_linux_console.py
index 43bc928b03a2..6af19ae3b14a 100644
--- a/tests/acceptance/boot_linux_console.py
+++ b/tests/acceptance/boot_linux_console.py
@@ -51,6 +51,20 @@ class BootLinuxConsole(Test):
         os.chdir(cwd)
         return self.workdir + path
 
+    def extract_from_rpm(self, rpm, path):
+        """
+        Extracts a file from a rpm package into the test workdir
+
+        :param rpm: path to the rpm archive
+        :param path: path within the rpm archive of the file to be extracted
+        :returns: path of the extracted file
+        """
+        cwd = os.getcwd()
+        os.chdir(self.workdir)
+        process.run("rpm2cpio %s | cpio -id %s" % (rpm, path), shell=True)
+        os.chdir(cwd)
+        return self.workdir + '/' + path
+
     def do_test_x86_64_machine(self):
         """
         :avocado: tags=arch:x86_64
-- 
1.8.3.1

Re: [PATCH 5/6] tests/boot_linux_console: add extract_from_rpm method

[Stefano Garzarella]

On Mon, Jan 27, 2020 at 04:36:33PM +0000, Liam Merwick wrote:
> Add a method to extract a specified file from an RPM to the test's
> working directory and return the path to the extracted file.
> 
> Signed-off-by: Liam Merwick <liam.merwick@oracle.com>
> ---
>  tests/acceptance/boot_linux_console.py | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/tests/acceptance/boot_linux_console.py b/tests/acceptance/boot_linux_console.py
> index 43bc928b03a2..6af19ae3b14a 100644
> --- a/tests/acceptance/boot_linux_console.py
> +++ b/tests/acceptance/boot_linux_console.py
> @@ -51,6 +51,20 @@ class BootLinuxConsole(Test):
>          os.chdir(cwd)
>          return self.workdir + path
>  
> +    def extract_from_rpm(self, rpm, path):
> +        """
> +        Extracts a file from a rpm package into the test workdir
> +
> +        :param rpm: path to the rpm archive
> +        :param path: path within the rpm archive of the file to be extracted
> +        :returns: path of the extracted file
> +        """
> +        cwd = os.getcwd()
> +        os.chdir(self.workdir)
> +        process.run("rpm2cpio %s | cpio -id %s" % (rpm, path), shell=True)
> +        os.chdir(cwd)
> +        return self.workdir + '/' + path
                                  ^
    Is the extra slash needed? (just because the extract_from_deb()
    doesn't put it)

Anyway this patch LGTM:

Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>

> +
>      def do_test_x86_64_machine(self):
>          """
>          :avocado: tags=arch:x86_64
> -- 
> 1.8.3.1
> 

Re: [PATCH 5/6] tests/boot_linux_console: add extract_from_rpm method

[Liam Merwick]

On 30/01/2020 12:05, Stefano Garzarella wrote:
> On Mon, Jan 27, 2020 at 04:36:33PM +0000, Liam Merwick wrote:
>> Add a method to extract a specified file from an RPM to the test's
>> working directory and return the path to the extracted file.
>>
>> Signed-off-by: Liam Merwick <liam.merwick@oracle.com>
>> ---
>>   tests/acceptance/boot_linux_console.py | 14 ++++++++++++++
>>   1 file changed, 14 insertions(+)
>>
>> diff --git a/tests/acceptance/boot_linux_console.py b/tests/acceptance/boot_linux_console.py
>> index 43bc928b03a2..6af19ae3b14a 100644
>> --- a/tests/acceptance/boot_linux_console.py
>> +++ b/tests/acceptance/boot_linux_console.py
>> @@ -51,6 +51,20 @@ class BootLinuxConsole(Test):
>>           os.chdir(cwd)
>>           return self.workdir + path
>>   
>> +    def extract_from_rpm(self, rpm, path):
>> +        """
>> +        Extracts a file from a rpm package into the test workdir
>> +
>> +        :param rpm: path to the rpm archive
>> +        :param path: path within the rpm archive of the file to be extracted
>> +        :returns: path of the extracted file
>> +        """
>> +        cwd = os.getcwd()
>> +        os.chdir(self.workdir)
>> +        process.run("rpm2cpio %s | cpio -id %s" % (rpm, path), shell=True)
>> +        os.chdir(cwd)
>> +        return self.workdir + '/' + path
>                                    ^
>      Is the extra slash needed? (just because the extract_from_deb()
>      doesn't put it)
> 


Yes, I needed to put it in there because the 'path' passed in for
processing by cpio is a relative patch unlike the deb arg so it
couldn't be just appended to 'self.workdir' which doesn't end in a '/'.

Regards,
Liam


> Anyway this patch LGTM:
> 
> Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
> 
>> +
>>       def do_test_x86_64_machine(self):
>>           """
>>           :avocado: tags=arch:x86_64
>> -- 
>> 1.8.3.1
>>
> 

Re: [PATCH 5/6] tests/boot_linux_console: add extract_from_rpm method

[Wainer dos Santos Moschetta]

On 1/30/20 1:34 PM, Liam Merwick wrote:
> On 30/01/2020 12:05, Stefano Garzarella wrote:
>> On Mon, Jan 27, 2020 at 04:36:33PM +0000, Liam Merwick wrote:
>>> Add a method to extract a specified file from an RPM to the test's
>>> working directory and return the path to the extracted file.
>>>
>>> Signed-off-by: Liam Merwick <liam.merwick@oracle.com>
>>> ---
>>>   tests/acceptance/boot_linux_console.py | 14 ++++++++++++++
>>>   1 file changed, 14 insertions(+)
>>>
>>> diff --git a/tests/acceptance/boot_linux_console.py 
>>> b/tests/acceptance/boot_linux_console.py
>>> index 43bc928b03a2..6af19ae3b14a 100644
>>> --- a/tests/acceptance/boot_linux_console.py
>>> +++ b/tests/acceptance/boot_linux_console.py
>>> @@ -51,6 +51,20 @@ class BootLinuxConsole(Test):
>>>           os.chdir(cwd)
>>>           return self.workdir + path
>>>   +    def extract_from_rpm(self, rpm, path):
>>> +        """
>>> +        Extracts a file from a rpm package into the test workdir
>>> +
>>> +        :param rpm: path to the rpm archive
>>> +        :param path: path within the rpm archive of the file to be 
>>> extracted


Might not be obvious to users that `path` should start with '.', and if 
he/she doesn't do that then extract_from_rpm() will silently fail to 
extract the file. So could you document that?

>>>
>>> +        :returns: path of the extracted file
>>> +        """
>>> +        cwd = os.getcwd()
>>> +        os.chdir(self.workdir)
>>> +        process.run("rpm2cpio %s | cpio -id %s" % (rpm, path), 
>>> shell=True)
>>> +        os.chdir(cwd)
>>> +        return self.workdir + '/' + path
>>                                    ^
>>      Is the extra slash needed? (just because the extract_from_deb()
>>      doesn't put it)
>>
>
>
> Yes, I needed to put it in there because the 'path' passed in for
> processing by cpio is a relative patch unlike the deb arg so it
> couldn't be just appended to 'self.workdir' which doesn't end in a '/'.


It is a good practice use the `os.path` module methods when dealing with 
filesystem paths. So that can be replaced with:

 >>> os.path.normpath(os.path.join('/path/to/workdir', './file/in/rpm'))
'/path/to/workdir/file/in/rpm'

Thanks,

Wainer


>
>
> Regards,
> Liam
>
>
>> Anyway this patch LGTM:
>>
>> Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
>>
>>> +
>>>       def do_test_x86_64_machine(self):
>>>           """
>>>           :avocado: tags=arch:x86_64
>>> -- 
>>> 1.8.3.1
>>>
>>
>

Re: [PATCH 5/6] tests/boot_linux_console: add extract_from_rpm method

[Philippe Mathieu-Daudé]

On 1/30/20 8:19 PM, Wainer dos Santos Moschetta wrote:
> On 1/30/20 1:34 PM, Liam Merwick wrote:
>> On 30/01/2020 12:05, Stefano Garzarella wrote:
>>> On Mon, Jan 27, 2020 at 04:36:33PM +0000, Liam Merwick wrote:
>>>> Add a method to extract a specified file from an RPM to the test's
>>>> working directory and return the path to the extracted file.
>>>>
>>>> Signed-off-by: Liam Merwick <liam.merwick@oracle.com>
>>>> ---
>>>>   tests/acceptance/boot_linux_console.py | 14 ++++++++++++++
>>>>   1 file changed, 14 insertions(+)
>>>>
>>>> diff --git a/tests/acceptance/boot_linux_console.py 
>>>> b/tests/acceptance/boot_linux_console.py
>>>> index 43bc928b03a2..6af19ae3b14a 100644
>>>> --- a/tests/acceptance/boot_linux_console.py
>>>> +++ b/tests/acceptance/boot_linux_console.py
>>>> @@ -51,6 +51,20 @@ class BootLinuxConsole(Test):
>>>>           os.chdir(cwd)
>>>>           return self.workdir + path
>>>>   +    def extract_from_rpm(self, rpm, path):
>>>> +        """
>>>> +        Extracts a file from a rpm package into the test workdir
>>>> +
>>>> +        :param rpm: path to the rpm archive
>>>> +        :param path: path within the rpm archive of the file to be 
>>>> extracted
> 
> 
> Might not be obvious to users that `path` should start with '.', and if 
> he/she doesn't do that then extract_from_rpm() will silently fail to 
> extract the file. So could you document that?
> 
>>>>
>>>> +        :returns: path of the extracted file
>>>> +        """
>>>> +        cwd = os.getcwd()
>>>> +        os.chdir(self.workdir)
>>>> +        process.run("rpm2cpio %s | cpio -id %s" % (rpm, path), 
>>>> shell=True)
>>>> +        os.chdir(cwd)
>>>> +        return self.workdir + '/' + path
>>>                                    ^
>>>      Is the extra slash needed? (just because the extract_from_deb()
>>>      doesn't put it)
>>>
>>
>>
>> Yes, I needed to put it in there because the 'path' passed in for
>> processing by cpio is a relative patch unlike the deb arg so it
>> couldn't be just appended to 'self.workdir' which doesn't end in a '/'.
> 
> 
> It is a good practice use the `os.path` module methods when dealing with 
> filesystem paths. So that can be replaced with:
> 
>  >>> os.path.normpath(os.path.join('/path/to/workdir', './file/in/rpm'))
> '/path/to/workdir/file/in/rpm'

With Wainer suggestion addressed:
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> 
> Thanks,
> 
> Wainer
> 
> 
>>
>>
>> Regards,
>> Liam
>>
>>
>>> Anyway this patch LGTM:
>>>
>>> Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
>>>
>>>> +
>>>>       def do_test_x86_64_machine(self):
>>>>           """
>>>>           :avocado: tags=arch:x86_64
>>>> -- 
>>>> 1.8.3.1
>>>>
>>>
>>
> 

Re: [PATCH 5/6] tests/boot_linux_console: add extract_from_rpm method

[Liam Merwick]

On 30/01/2020 19:19, Wainer dos Santos Moschetta wrote:
> 
> On 1/30/20 1:34 PM, Liam Merwick wrote:
>> On 30/01/2020 12:05, Stefano Garzarella wrote:
>>> On Mon, Jan 27, 2020 at 04:36:33PM +0000, Liam Merwick wrote:
>>>> Add a method to extract a specified file from an RPM to the test's
>>>> working directory and return the path to the extracted file.
>>>>
>>>> Signed-off-by: Liam Merwick <liam.merwick@oracle.com>
>>>> ---
>>>>   tests/acceptance/boot_linux_console.py | 14 ++++++++++++++
>>>>   1 file changed, 14 insertions(+)
>>>>
>>>> diff --git a/tests/acceptance/boot_linux_console.py 
>>>> b/tests/acceptance/boot_linux_console.py
>>>> index 43bc928b03a2..6af19ae3b14a 100644
>>>> --- a/tests/acceptance/boot_linux_console.py
>>>> +++ b/tests/acceptance/boot_linux_console.py
>>>> @@ -51,6 +51,20 @@ class BootLinuxConsole(Test):
>>>>           os.chdir(cwd)
>>>>           return self.workdir + path
>>>>   +    def extract_from_rpm(self, rpm, path):
>>>> +        """
>>>> +        Extracts a file from a rpm package into the test workdir
>>>> +
>>>> +        :param rpm: path to the rpm archive
>>>> +        :param path: path within the rpm archive of the file to be 
>>>> extracted
> 
> 
> Might not be obvious to users that `path` should start with '.', and if 
> he/she doesn't do that then extract_from_rpm() will silently fail to 
> extract the file. So could you document that?


Sure.

> 
>>>>
>>>> +        :returns: path of the extracted file
>>>> +        """
>>>> +        cwd = os.getcwd()
>>>> +        os.chdir(self.workdir)
>>>> +        process.run("rpm2cpio %s | cpio -id %s" % (rpm, path), 
>>>> shell=True)
>>>> +        os.chdir(cwd)
>>>> +        return self.workdir + '/' + path
>>>                                    ^
>>>      Is the extra slash needed? (just because the extract_from_deb()
>>>      doesn't put it)
>>>
>>
>>
>> Yes, I needed to put it in there because the 'path' passed in for
>> processing by cpio is a relative patch unlike the deb arg so it
>> couldn't be just appended to 'self.workdir' which doesn't end in a '/'.
> 
> 
> It is a good practice use the `os.path` module methods when dealing with 
> filesystem paths. So that can be replaced with:
> 
>  >>> os.path.normpath(os.path.join('/path/to/workdir', './file/in/rpm'))
> '/path/to/workdir/file/in/rpm'
> 


Will do.  I'll add a patch to fix extract_from_deb() too.

Regards,
Liam


> Thanks,
> 
> Wainer
> 
> 
>>
>>
>> Regards,
>> Liam
>>
>>
>>> Anyway this patch LGTM:
>>>
>>> Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
>>>
>>>> +
>>>>       def do_test_x86_64_machine(self):
>>>>           """
>>>>           :avocado: tags=arch:x86_64
>>>> -- 
>>>> 1.8.3.1
>>>>
>>>
>>
> 

Re: [PATCH 5/6] tests/boot_linux_console: add extract_from_rpm method

[Liam Merwick]

On 31/01/2020 15:02, Liam Merwick wrote:

[... deleted ...]

>>
>>>>>
>>>>> +        :returns: path of the extracted file
>>>>> +        """
>>>>> +        cwd = os.getcwd()
>>>>> +        os.chdir(self.workdir)
>>>>> +        process.run("rpm2cpio %s | cpio -id %s" % (rpm, path), 
>>>>> shell=True)
>>>>> +        os.chdir(cwd)
>>>>> +        return self.workdir + '/' + path
>>>>                                    ^
>>>>      Is the extra slash needed? (just because the extract_from_deb()
>>>>      doesn't put it)
>>>>
>>>
>>>
>>> Yes, I needed to put it in there because the 'path' passed in for
>>> processing by cpio is a relative patch unlike the deb arg so it
>>> couldn't be just appended to 'self.workdir' which doesn't end in a '/'.
>>
>>
>> It is a good practice use the `os.path` module methods when dealing 
>> with filesystem paths. So that can be replaced with:
>>
>>  >>> os.path.normpath(os.path.join('/path/to/workdir', './file/in/rpm'))
>> '/path/to/workdir/file/in/rpm'
>>
> 
> 
> Will do.  I'll add a patch to fix extract_from_deb() too.

Using the exact same code didn't work with extract_from_deb() because 
the callers set 'path' to an absolute path (so os.path.join() drops the 
self.workdir part).  I'll include a patch with the following change and 
it can be dropped if people think using os.path.relpath() is too much of 
a hack.

--- a/tests/acceptance/boot_linux_console.py
+++ b/tests/acceptance/boot_linux_console.py
@@ -49,7 +49,12 @@ class BootLinuxConsole(Test):
          process.run("ar x %s %s" % (deb, file_path))
          archive.extract(file_path, self.workdir)
          os.chdir(cwd)
-        return self.workdir + path
+        # Return complete path to extracted file.  We need to use
+        # os.path.relpath() because callers specify 'path' with a leading
+        # slash which causes os.path.join() to interpret it as an absolute
+        # path and to drop self.workdir part.
+        return os.path.normpath(os.path.join(self.workdir,
+                                             os.path.relpath(path, '/')))

      def extract_from_rpm(self, rpm, path):
          """

Regards,
Liam

Re: [PATCH 5/6] tests/boot_linux_console: add extract_from_rpm method

[Philippe Mathieu-Daudé]

On Tue, Feb 4, 2020 at 2:34 PM Liam Merwick <liam.merwick@oracle.com> wrote:
> On 31/01/2020 15:02, Liam Merwick wrote:
>
> [... deleted ...]
>
> >>
> >>>>>
> >>>>> +        :returns: path of the extracted file
> >>>>> +        """
> >>>>> +        cwd = os.getcwd()
> >>>>> +        os.chdir(self.workdir)
> >>>>> +        process.run("rpm2cpio %s | cpio -id %s" % (rpm, path),
> >>>>> shell=True)
> >>>>> +        os.chdir(cwd)
> >>>>> +        return self.workdir + '/' + path
> >>>>                                    ^
> >>>>      Is the extra slash needed? (just because the extract_from_deb()
> >>>>      doesn't put it)
> >>>>
> >>>
> >>>
> >>> Yes, I needed to put it in there because the 'path' passed in for
> >>> processing by cpio is a relative patch unlike the deb arg so it
> >>> couldn't be just appended to 'self.workdir' which doesn't end in a '/'.
> >>
> >>
> >> It is a good practice use the `os.path` module methods when dealing
> >> with filesystem paths. So that can be replaced with:
> >>
> >>  >>> os.path.normpath(os.path.join('/path/to/workdir', './file/in/rpm'))
> >> '/path/to/workdir/file/in/rpm'
> >>
> >
> >
> > Will do.  I'll add a patch to fix extract_from_deb() too.
>
> Using the exact same code didn't work with extract_from_deb() because
> the callers set 'path' to an absolute path (so os.path.join() drops the
> self.workdir part).  I'll include a patch with the following change and
> it can be dropped if people think using os.path.relpath() is too much of
> a hack.
>
> --- a/tests/acceptance/boot_linux_console.py
> +++ b/tests/acceptance/boot_linux_console.py
> @@ -49,7 +49,12 @@ class BootLinuxConsole(Test):
>           process.run("ar x %s %s" % (deb, file_path))
>           archive.extract(file_path, self.workdir)
>           os.chdir(cwd)
> -        return self.workdir + path
> +        # Return complete path to extracted file.  We need to use
> +        # os.path.relpath() because callers specify 'path' with a leading
> +        # slash which causes os.path.join() to interpret it as an absolute
> +        # path and to drop self.workdir part.
> +        return os.path.normpath(os.path.join(self.workdir,
> +                                             os.path.relpath(path, '/')))

LGTM, so the next one modifying this code won't make a mistake.

>
>       def extract_from_rpm(self, rpm, path):
>           """
>
> Regards,
> Liam
>

[PATCH 6/6] tests/boot_linux_console: add PVH acceptance tests

[Liam Merwick]

Add tests to boot an uncompressed kernel using the x86/HVM direct boot ABI.
The vmlinux binary is obtained from a small RPM for Kata containers and
extracted using the new extract_from_rpm() method.

Signed-off-by: Liam Merwick <liam.merwick@oracle.com>
---
 tests/acceptance/boot_linux_console.py | 49 +++++++++++++++++++++++++++++-----
 1 file changed, 43 insertions(+), 6 deletions(-)

diff --git a/tests/acceptance/boot_linux_console.py b/tests/acceptance/boot_linux_console.py
index 6af19ae3b14a..ab2200aa0e47 100644
--- a/tests/acceptance/boot_linux_console.py
+++ b/tests/acceptance/boot_linux_console.py
@@ -65,15 +65,26 @@ class BootLinuxConsole(Test):
         os.chdir(cwd)
         return self.workdir + '/' + path
 
-    def do_test_x86_64_machine(self):
+    def do_test_x86_64_machine(self, pvh=False):
         """
         :avocado: tags=arch:x86_64
         """
-        kernel_url = ('https://archives.fedoraproject.org/pub/archive/fedora'
-                      '/linux/releases/29/Everything/x86_64/os/images/pxeboot'
-                      '/vmlinuz')
-        kernel_hash = '23bebd2680757891cf7adedb033532163a792495'
-        kernel_path = self.fetch_asset(kernel_url, asset_hash=kernel_hash)
+        if pvh:
+            rpm_url = ('https://yum.oracle.com/repo/OracleLinux/'
+                       'OL7/olcne/x86_64/getPackage/'
+                       'kernel-uek-container-4.14.35-1902.6.6.1.el7.x86_64.rpm')
+            rpm_hash = '4c781711a9d32dcb8e81da2b397cb98926744e23'
+            rpm_path = self.fetch_asset(rpm_url, asset_hash=rpm_hash)
+            kernel_path = self.extract_from_rpm(rpm_path,
+                                                './usr/share/kata-containers/'
+                                    'vmlinux-4.14.35-1902.6.6.1.el7.container')
+        else:
+            kernel_url = ('https://archives.fedoraproject.org/pub/archive/'
+                          'fedora/linux/releases/29/Everything/x86_64/os/'
+                          'images/pxeboot/vmlinuz')
+            kernel_hash = '23bebd2680757891cf7adedb033532163a792495'
+            kernel_path = self.fetch_asset(kernel_url, asset_hash=kernel_hash)
+
         self.vm.set_console()
         kernel_command_line = self.KERNEL_COMMON_COMMAND_LINE + 'console=ttyS0'
         self.vm.add_args('-kernel', kernel_path,
@@ -95,6 +106,19 @@ class BootLinuxConsole(Test):
         self.vm.add_args('-bios', 'pc-bios/bios-microvm.bin')
         self.do_test_x86_64_machine()
 
+    def test_x86_64_pc_pvh(self):
+        """
+        :avocado: tags=machine:pc
+        """
+        self.do_test_x86_64_machine(pvh=True)
+
+    def test_x86_64_pc_qboot_pvh(self):
+        """
+        :avocado: tags=machine:pc
+        """
+        self.vm.add_args('-bios', 'pc-bios/bios-microvm.bin')
+        self.do_test_x86_64_machine(pvh=True)
+
     def test_x86_64_microvm(self):
         """
         :avocado: tags=machine:microvm
@@ -108,6 +132,19 @@ class BootLinuxConsole(Test):
         self.vm.add_args('-bios', 'pc-bios/bios-microvm.bin')
         self.do_test_x86_64_machine()
 
+    def test_x86_64_microvm_pvh(self):
+        """
+        :avocado: tags=machine:microvm
+        """
+        self.do_test_x86_64_machine(pvh=True)
+
+    def test_x86_64_microvm_qboot_pvh(self):
+        """
+        :avocado: tags=machine:microvm
+        """
+        self.vm.add_args('-bios', 'pc-bios/bios-microvm.bin')
+        self.do_test_x86_64_machine(pvh=True)
+
     def test_mips_malta(self):
         """
         :avocado: tags=arch:mips
-- 
1.8.3.1

Re: [PATCH 6/6] tests/boot_linux_console: add PVH acceptance tests

[Stefano Garzarella]

On Mon, Jan 27, 2020 at 04:36:34PM +0000, Liam Merwick wrote:
> Add tests to boot an uncompressed kernel using the x86/HVM direct boot ABI.
> The vmlinux binary is obtained from a small RPM for Kata containers and
> extracted using the new extract_from_rpm() method.
> 
> Signed-off-by: Liam Merwick <liam.merwick@oracle.com>
> ---
>  tests/acceptance/boot_linux_console.py | 49 +++++++++++++++++++++++++++++-----
>  1 file changed, 43 insertions(+), 6 deletions(-)
> 
> diff --git a/tests/acceptance/boot_linux_console.py b/tests/acceptance/boot_linux_console.py
> index 6af19ae3b14a..ab2200aa0e47 100644
> --- a/tests/acceptance/boot_linux_console.py
> +++ b/tests/acceptance/boot_linux_console.py
> @@ -65,15 +65,26 @@ class BootLinuxConsole(Test):
>          os.chdir(cwd)
>          return self.workdir + '/' + path
>  
> -    def do_test_x86_64_machine(self):
> +    def do_test_x86_64_machine(self, pvh=False):
>          """
>          :avocado: tags=arch:x86_64
>          """
> -        kernel_url = ('https://archives.fedoraproject.org/pub/archive/fedora'
> -                      '/linux/releases/29/Everything/x86_64/os/images/pxeboot'
> -                      '/vmlinuz')
> -        kernel_hash = '23bebd2680757891cf7adedb033532163a792495'
> -        kernel_path = self.fetch_asset(kernel_url, asset_hash=kernel_hash)
> +        if pvh:
> +            rpm_url = ('https://yum.oracle.com/repo/OracleLinux/'
> +                       'OL7/olcne/x86_64/getPackage/'
> +                       'kernel-uek-container-4.14.35-1902.6.6.1.el7.x86_64.rpm')
> +            rpm_hash = '4c781711a9d32dcb8e81da2b397cb98926744e23'
> +            rpm_path = self.fetch_asset(rpm_url, asset_hash=rpm_hash)
> +            kernel_path = self.extract_from_rpm(rpm_path,
> +                                                './usr/share/kata-containers/'
> +                                    'vmlinux-4.14.35-1902.6.6.1.el7.container')
> +        else:
> +            kernel_url = ('https://archives.fedoraproject.org/pub/archive/'
> +                          'fedora/linux/releases/29/Everything/x86_64/os/'
> +                          'images/pxeboot/vmlinuz')
> +            kernel_hash = '23bebd2680757891cf7adedb033532163a792495'
> +            kernel_path = self.fetch_asset(kernel_url, asset_hash=kernel_hash)
> +
>          self.vm.set_console()
>          kernel_command_line = self.KERNEL_COMMON_COMMAND_LINE + 'console=ttyS0'
>          self.vm.add_args('-kernel', kernel_path,
> @@ -95,6 +106,19 @@ class BootLinuxConsole(Test):
>          self.vm.add_args('-bios', 'pc-bios/bios-microvm.bin')
>          self.do_test_x86_64_machine()
>  
> +    def test_x86_64_pc_pvh(self):
> +        """
> +        :avocado: tags=machine:pc
> +        """
> +        self.do_test_x86_64_machine(pvh=True)
> +
> +    def test_x86_64_pc_qboot_pvh(self):
> +        """
> +        :avocado: tags=machine:pc
> +        """
> +        self.vm.add_args('-bios', 'pc-bios/bios-microvm.bin')
> +        self.do_test_x86_64_machine(pvh=True)
> +
>      def test_x86_64_microvm(self):
>          """
>          :avocado: tags=machine:microvm
> @@ -108,6 +132,19 @@ class BootLinuxConsole(Test):
>          self.vm.add_args('-bios', 'pc-bios/bios-microvm.bin')
>          self.do_test_x86_64_machine()
>  
> +    def test_x86_64_microvm_pvh(self):
> +        """
> +        :avocado: tags=machine:microvm
> +        """
> +        self.do_test_x86_64_machine(pvh=True)
> +
> +    def test_x86_64_microvm_qboot_pvh(self):
> +        """
> +        :avocado: tags=machine:microvm
> +        """
> +        self.vm.add_args('-bios', 'pc-bios/bios-microvm.bin')
> +        self.do_test_x86_64_machine(pvh=True)

Also in this case I think we are using qboot in both tests.
Maybe we can remove one of them.

Thanks,
Stefano

Re: [PATCH 6/6] tests/boot_linux_console: add PVH acceptance tests

[Philippe Mathieu-Daudé]

On 1/27/20 5:36 PM, Liam Merwick wrote:
> Add tests to boot an uncompressed kernel using the x86/HVM direct boot ABI.
> The vmlinux binary is obtained from a small RPM for Kata containers and
> extracted using the new extract_from_rpm() method.
> 
> Signed-off-by: Liam Merwick <liam.merwick@oracle.com>
> ---
>   tests/acceptance/boot_linux_console.py | 49 +++++++++++++++++++++++++++++-----
>   1 file changed, 43 insertions(+), 6 deletions(-)
> 
> diff --git a/tests/acceptance/boot_linux_console.py b/tests/acceptance/boot_linux_console.py
> index 6af19ae3b14a..ab2200aa0e47 100644
> --- a/tests/acceptance/boot_linux_console.py
> +++ b/tests/acceptance/boot_linux_console.py
> @@ -65,15 +65,26 @@ class BootLinuxConsole(Test):
>           os.chdir(cwd)
>           return self.workdir + '/' + path
>   
> -    def do_test_x86_64_machine(self):
> +    def do_test_x86_64_machine(self, pvh=False):
>           """
>           :avocado: tags=arch:x86_64
>           """
> -        kernel_url = ('https://archives.fedoraproject.org/pub/archive/fedora'
> -                      '/linux/releases/29/Everything/x86_64/os/images/pxeboot'
> -                      '/vmlinuz')
> -        kernel_hash = '23bebd2680757891cf7adedb033532163a792495'
> -        kernel_path = self.fetch_asset(kernel_url, asset_hash=kernel_hash)
> +        if pvh:
> +            rpm_url = ('https://yum.oracle.com/repo/OracleLinux/'
> +                       'OL7/olcne/x86_64/getPackage/'
> +                       'kernel-uek-container-4.14.35-1902.6.6.1.el7.x86_64.rpm')
> +            rpm_hash = '4c781711a9d32dcb8e81da2b397cb98926744e23'
> +            rpm_path = self.fetch_asset(rpm_url, asset_hash=rpm_hash)
> +            kernel_path = self.extract_from_rpm(rpm_path,
> +                                                './usr/share/kata-containers/'
> +                                    'vmlinux-4.14.35-1902.6.6.1.el7.container')
> +        else:
> +            kernel_url = ('https://archives.fedoraproject.org/pub/archive/'
> +                          'fedora/linux/releases/29/Everything/x86_64/os/'
> +                          'images/pxeboot/vmlinuz')
> +            kernel_hash = '23bebd2680757891cf7adedb033532163a792495'
> +            kernel_path = self.fetch_asset(kernel_url, asset_hash=kernel_hash)
> +

Can you try using a dictionaries instead? This way we can add more 
images easily.
See IMAGE_INFO in tests/acceptance/linux_ssh_mips_malta.py.

>           self.vm.set_console()
>           kernel_command_line = self.KERNEL_COMMON_COMMAND_LINE + 'console=ttyS0'
>           self.vm.add_args('-kernel', kernel_path,
> @@ -95,6 +106,19 @@ class BootLinuxConsole(Test):
>           self.vm.add_args('-bios', 'pc-bios/bios-microvm.bin')
>           self.do_test_x86_64_machine()
>   
> +    def test_x86_64_pc_pvh(self):
> +        """
> +        :avocado: tags=machine:pc
> +        """
> +        self.do_test_x86_64_machine(pvh=True)
> +
> +    def test_x86_64_pc_qboot_pvh(self):
> +        """
> +        :avocado: tags=machine:pc
> +        """
> +        self.vm.add_args('-bios', 'pc-bios/bios-microvm.bin')
> +        self.do_test_x86_64_machine(pvh=True)
> +
>       def test_x86_64_microvm(self):
>           """
>           :avocado: tags=machine:microvm
> @@ -108,6 +132,19 @@ class BootLinuxConsole(Test):
>           self.vm.add_args('-bios', 'pc-bios/bios-microvm.bin')
>           self.do_test_x86_64_machine()
>   
> +    def test_x86_64_microvm_pvh(self):
> +        """
> +        :avocado: tags=machine:microvm
> +        """
> +        self.do_test_x86_64_machine(pvh=True)
> +
> +    def test_x86_64_microvm_qboot_pvh(self):
> +        """
> +        :avocado: tags=machine:microvm
> +        """
> +        self.vm.add_args('-bios', 'pc-bios/bios-microvm.bin')
> +        self.do_test_x86_64_machine(pvh=True)
> +
>       def test_mips_malta(self):
>           """
>           :avocado: tags=arch:mips
> 

Re: [PATCH 6/6] tests/boot_linux_console: add PVH acceptance tests

[Liam Merwick]

On 30/01/2020 23:57, Philippe Mathieu-Daudé wrote:
> On 1/27/20 5:36 PM, Liam Merwick wrote:
>> Add tests to boot an uncompressed kernel using the x86/HVM direct boot 
>> ABI.
>> The vmlinux binary is obtained from a small RPM for Kata containers and
>> extracted using the new extract_from_rpm() method.
>>
>> Signed-off-by: Liam Merwick <liam.merwick@oracle.com>
>> ---
>>   tests/acceptance/boot_linux_console.py | 49 
>> +++++++++++++++++++++++++++++-----
>>   1 file changed, 43 insertions(+), 6 deletions(-)
>>
>> diff --git a/tests/acceptance/boot_linux_console.py 
>> b/tests/acceptance/boot_linux_console.py
>> index 6af19ae3b14a..ab2200aa0e47 100644
>> --- a/tests/acceptance/boot_linux_console.py
>> +++ b/tests/acceptance/boot_linux_console.py
>> @@ -65,15 +65,26 @@ class BootLinuxConsole(Test):
>>           os.chdir(cwd)
>>           return self.workdir + '/' + path
>> -    def do_test_x86_64_machine(self):
>> +    def do_test_x86_64_machine(self, pvh=False):
>>           """
>>           :avocado: tags=arch:x86_64
>>           """
>> -        kernel_url = 
>> ('https://archives.fedoraproject.org/pub/archive/fedora'
>> -                      
>> '/linux/releases/29/Everything/x86_64/os/images/pxeboot'
>> -                      '/vmlinuz')
>> -        kernel_hash = '23bebd2680757891cf7adedb033532163a792495'
>> -        kernel_path = self.fetch_asset(kernel_url, 
>> asset_hash=kernel_hash)
>> +        if pvh:
>> +            rpm_url = ('https://yum.oracle.com/repo/OracleLinux/'
>> +                       'OL7/olcne/x86_64/getPackage/'
>> +                       
>> 'kernel-uek-container-4.14.35-1902.6.6.1.el7.x86_64.rpm')
>> +            rpm_hash = '4c781711a9d32dcb8e81da2b397cb98926744e23'
>> +            rpm_path = self.fetch_asset(rpm_url, asset_hash=rpm_hash)
>> +            kernel_path = self.extract_from_rpm(rpm_path,
>> +                                                
>> './usr/share/kata-containers/'
>> +                                    
>> 'vmlinux-4.14.35-1902.6.6.1.el7.container')
>> +        else:
>> +            kernel_url = 
>> ('https://archives.fedoraproject.org/pub/archive/'
>> +                          
>> 'fedora/linux/releases/29/Everything/x86_64/os/'
>> +                          'images/pxeboot/vmlinuz')
>> +            kernel_hash = '23bebd2680757891cf7adedb033532163a792495'
>> +            kernel_path = self.fetch_asset(kernel_url, 
>> asset_hash=kernel_hash)
>> +
> 
> Can you try using a dictionaries instead? This way we can add more 
> images easily.
> See IMAGE_INFO in tests/acceptance/linux_ssh_mips_malta.py.

I can. I won't convert the users of extract_from_deb() but will try make 
it easily extendable.


--- a/tests/acceptance/boot_linux_console.py
+++ b/tests/acceptance/boot_linux_console.py
@@ -31,6 +31,29 @@ class BootLinuxConsole(Test):

      KERNEL_COMMON_COMMAND_LINE = 'printk.time=0 '

+    KERNEL_PATH_INFO = {
+        ('x86_64', 'bzImage'): {
+            'type': 'file',
+            'url': 'https://archives.fedoraproject.org/'
+                   'pub/archive/fedora/linux/releases/29/Everything/'
+                   'x86_64/os/images/pxeboot/vmlinuz',
+            'hash': '23bebd2680757891cf7adedb033532163a792495'
+        }
+    }
+
+    def get_kernel_path(self, key):
+        """
+        For the provided key, download (and extract, if necessary) the 
kernel
+        and return the path the the kernel binary.
+
+        :param key: index into KERNEL_PATH_INFO dict containing kernel 
location
+        :returns: path of the extracted file
+        """
+        dinfo = self.KERNEL_PATH_INFO[(self.arch, key)]
+
+        if dinfo['type'] is 'file':
+            return self.fetch_asset(dinfo['url'], asset_hash=dinfo['hash'])
+
      def wait_for_console_pattern(self, success_message):
          wait_for_console_pattern(self, success_message,
                                   failure_message='Kernel panic - not 
syncing')
@@ -72,11 +95,7 @@ class BootLinuxConsole(Test):
          Common routine to boot an x86_64 guest.
          Caller must specify tags=arch and tags=machine
          """
-        kernel_url = 
('https://archives.fedoraproject.org/pub/archive/fedora'
- 
'/linux/releases/29/Everything/x86_64/os/images/pxeboot'
-                      '/vmlinuz')
-        kernel_hash = '23bebd2680757891cf7adedb033532163a792495'
-        kernel_path = self.fetch_asset(kernel_url, asset_hash=kernel_hash)
+        kernel_path = self.get_kernel_path('bzImage')
          self.vm.set_console()
          kernel_command_line = self.KERNEL_COMMON_COMMAND_LINE + 
'console=ttyS0'
          self.vm.add_args('-kernel', kernel_path,


Regards,
Liam

Re: [PATCH 6/6] tests/boot_linux_console: add PVH acceptance tests

[Philippe Mathieu-Daudé]

On 1/31/20 4:03 PM, Liam Merwick wrote:
> On 30/01/2020 23:57, Philippe Mathieu-Daudé wrote:
>> On 1/27/20 5:36 PM, Liam Merwick wrote:
>>> Add tests to boot an uncompressed kernel using the x86/HVM direct 
>>> boot ABI.
>>> The vmlinux binary is obtained from a small RPM for Kata containers and
>>> extracted using the new extract_from_rpm() method.
>>>
>>> Signed-off-by: Liam Merwick <liam.merwick@oracle.com>
>>> ---
>>>   tests/acceptance/boot_linux_console.py | 49 
>>> +++++++++++++++++++++++++++++-----
>>>   1 file changed, 43 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/tests/acceptance/boot_linux_console.py 
>>> b/tests/acceptance/boot_linux_console.py
>>> index 6af19ae3b14a..ab2200aa0e47 100644
>>> --- a/tests/acceptance/boot_linux_console.py
>>> +++ b/tests/acceptance/boot_linux_console.py
>>> @@ -65,15 +65,26 @@ class BootLinuxConsole(Test):
>>>           os.chdir(cwd)
>>>           return self.workdir + '/' + path
>>> -    def do_test_x86_64_machine(self):
>>> +    def do_test_x86_64_machine(self, pvh=False):
>>>           """
>>>           :avocado: tags=arch:x86_64
>>>           """
>>> -        kernel_url = 
>>> ('https://archives.fedoraproject.org/pub/archive/fedora'
>>> - '/linux/releases/29/Everything/x86_64/os/images/pxeboot'
>>> -                      '/vmlinuz')
>>> -        kernel_hash = '23bebd2680757891cf7adedb033532163a792495'
>>> -        kernel_path = self.fetch_asset(kernel_url, 
>>> asset_hash=kernel_hash)
>>> +        if pvh:
>>> +            rpm_url = ('https://yum.oracle.com/repo/OracleLinux/'
>>> +                       'OL7/olcne/x86_64/getPackage/'
>>> + 'kernel-uek-container-4.14.35-1902.6.6.1.el7.x86_64.rpm')
>>> +            rpm_hash = '4c781711a9d32dcb8e81da2b397cb98926744e23'
>>> +            rpm_path = self.fetch_asset(rpm_url, asset_hash=rpm_hash)
>>> +            kernel_path = self.extract_from_rpm(rpm_path,
>>> + './usr/share/kata-containers/'
>>> + 'vmlinux-4.14.35-1902.6.6.1.el7.container')
>>> +        else:
>>> +            kernel_url = 
>>> ('https://archives.fedoraproject.org/pub/archive/'
>>> + 'fedora/linux/releases/29/Everything/x86_64/os/'
>>> +                          'images/pxeboot/vmlinuz')
>>> +            kernel_hash = '23bebd2680757891cf7adedb033532163a792495'
>>> +            kernel_path = self.fetch_asset(kernel_url, 
>>> asset_hash=kernel_hash)
>>> +
>>
>> Can you try using a dictionaries instead? This way we can add more 
>> images easily.
>> See IMAGE_INFO in tests/acceptance/linux_ssh_mips_malta.py.
> 
> I can. I won't convert the users of extract_from_deb() but will try make 
> it easily extendable.
> 
> 
> --- a/tests/acceptance/boot_linux_console.py
> +++ b/tests/acceptance/boot_linux_console.py
> @@ -31,6 +31,29 @@ class BootLinuxConsole(Test):
> 
>       KERNEL_COMMON_COMMAND_LINE = 'printk.time=0 '
> 
> +    KERNEL_PATH_INFO = {
> +        ('x86_64', 'bzImage'): {
> +            'type': 'file',
> +            'url': 'https://archives.fedoraproject.org/'
> +                   'pub/archive/fedora/linux/releases/29/Everything/'
> +                   'x86_64/os/images/pxeboot/vmlinuz',
> +            'hash': '23bebd2680757891cf7adedb033532163a792495'
> +        }
> +    }

I was thinking of something simpler, adding the dictionary local to 
do_test_x86_64_machine().

  def do_test_x86_64_machine(self, test_type):
    d = {
      'pvh_disabled': {
         'url': 'https://archives.fedoraproject.org/'
                'pub/archive/fedora/linux/releases/29/Everything/'
                'x86_64/os/images/pxeboot/vmlinuz',
          'hash': '23bebd2680757891cf7adedb033532163a792495'
      },
      'pvh_enabled': {...}
    }
    ...

  def test_x86_64_pc_pvh(self):
         """
         :avocado: tags=machine:pc
         """
         self.do_test_x86_64_machine('pvh_enabled')

> +
> +    def get_kernel_path(self, key):
> +        """
> +        For the provided key, download (and extract, if necessary) the 
> kernel
> +        and return the path the the kernel binary.
> +
> +        :param key: index into KERNEL_PATH_INFO dict containing kernel 
> location
> +        :returns: path of the extracted file
> +        """
> +        dinfo = self.KERNEL_PATH_INFO[(self.arch, key)]
> +
> +        if dinfo['type'] is 'file':
> +            return self.fetch_asset(dinfo['url'], 
> asset_hash=dinfo['hash'])
> +
>       def wait_for_console_pattern(self, success_message):
>           wait_for_console_pattern(self, success_message,
>                                    failure_message='Kernel panic - not 
> syncing')
> @@ -72,11 +95,7 @@ class BootLinuxConsole(Test):
>           Common routine to boot an x86_64 guest.
>           Caller must specify tags=arch and tags=machine
>           """
> -        kernel_url = 
> ('https://archives.fedoraproject.org/pub/archive/fedora'
> - '/linux/releases/29/Everything/x86_64/os/images/pxeboot'
> -                      '/vmlinuz')
> -        kernel_hash = '23bebd2680757891cf7adedb033532163a792495'
> -        kernel_path = self.fetch_asset(kernel_url, asset_hash=kernel_hash)
> +        kernel_path = self.get_kernel_path('bzImage')
>           self.vm.set_console()
>           kernel_command_line = self.KERNEL_COMMON_COMMAND_LINE + 
> 'console=ttyS0'
>           self.vm.add_args('-kernel', kernel_path,
> 
> 
> Regards,
> Liam
> 
> 

Re: [PATCH 0/6] tests/boot_linux_console: add extra boot acceptance tests

[Wainer dos Santos Moschetta]

Hi Liam,

On 1/27/20 2:36 PM, Liam Merwick wrote:
> Add acceptance tests for the microvm machine class, PVH, and the
> new qboot BIOS.
>
> In the case of the test to boot an uncompressed kernel there didn't
> seem to be any suitable uncompressed kernel on https://archives.fedoraproject.org/
> (there is a vmlinux in kernel-debuginfo but that RPM is 575M and
> caused timeouts when populating the Avocado cache when first run)
> so I chose an RPM with kernels for Kata that is 14M.
> (there was a discussion in [1] regarding testing PVH boot but it focussed
> more around building a vmlinux binary during testing).

Yeah, my proposal [1] for building the vmlinux with PVH at test time was 
gently rejected. I was going to send a v2 where I would use a built 
kernel from somewhere. I'm glad you send this series before, so I 
discard mine in favor of yours.

>
> [ What prompted these patches was the discovery that a 'pc' guest booting an
> uncompressed kernel (PVH) with a PCI netdev hangs (before we even get guest
> console output) when bios-microvm.bin (qboot) is supplied via -bios
> (no issue when using 'q35' or 'microvm' machine classes).
>
> E.g. adding the following line to test_x86_64_pc_qboot_pvh() is enough to
> trigger a guest hang during startup:
> self.vm.add_args('-netdev', 'user,id=n1', '-device', 'virtio-net-pci,netdev=n1')
>
> I bisected that issue to 176d2cda0dee [2] in 4.1 but haven't worked out yet
> how/why the "die-id" changes impact the qboot/PVH combination
> (the boot succeeds with any subset of those boot variables).
>
> Is booting the 'pc' machine class with bios-microvm.bin something that QEMU
> officially supports or is qboot intended for microvm only? ]
>
> Each test added here adds about 1.5s to the overall runtime.
> I have run them through the Travis QEMU CI [3] and those acceptance tests pass.

Thanks for using Travis CI to check it. This way I've some minutes 
saved, otherwise I would have to test it manually. :)

>
> My modifications to test_x86_64_pc() in Patch1 will conflict with Wainer's
> patch in [4] - I'll rebase on top of that once that series is pulled and
> and apply feedback for this series, etc.

No problem, I can rebase mine patches in case this get in first.

>
> [1] https://patchew.org/QEMU/20191206140012.15517-1-wainersm@redhat.com/
> [2] 176d2cda0dee ("i386/cpu: Consolidate die-id validity in smp context")
> [3] https://travis-ci.org/merwick/qemu/jobs/641505543
> [4] https://github.com/wainersm/qemu/commit/8f705e98df90b436b0f4946331d441309c437f7b
>
>
> Liam Merwick (6):
>    tests/boot_linux_console: add microvm acceptance test
>    tests/boot_linux_console: add BIOS acceptance test
>    tests/boot_linux_console: fix extract_from_deb() comment
>    travis.yml: install rpm2cpio for acceptance tests
>    tests/boot_linux_console: add extract_from_rpm method
>    tests/boot_linux_console: add PVH acceptance tests
>
>   .travis.yml                            |  1 +
>   tests/acceptance/boot_linux_console.py | 91 +++++++++++++++++++++++++++++++---
>   2 files changed, 84 insertions(+), 8 deletions(-)
>