[Qemu-devel] [PATCH v2 for-2.11 0/2] Improvements for the pxe tester

Thomas Huth posted 2 patches 6 years, 8 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/1502431076-22849-1-git-send-email-thuth@redhat.com
Test FreeBSD passed
Test checkpatch passed
Test docker passed
Test s390x passed
tests/Makefile.include |  1 +
tests/boot-sector.c    | 61 +++++++++++++++++++++++++++++++++++++-------------
tests/pxe-test.c       |  7 ++++++
3 files changed, 54 insertions(+), 15 deletions(-)
[Qemu-devel] [PATCH v2 for-2.11 0/2] Improvements for the pxe tester
Posted by Thomas Huth 6 years, 8 months ago
The first patch improves the buffer handling in the pxe tester a
little bit by allocating a separate buffer on the heap for each
architecture. This also gets rid of the huge pre-initialized
array in the tester, shrinking the size of the executable by
half of a megabyte!
The second patch adds s390x support to the pxe tester. Starting
with QEMU 2.10, the guest firmware on s390x can now net-boot via
TFTP, too, so we can automatically test this code in the pxe tester.

v2: Adressed Cornelia's review feedback from the first version, e.g.:
 - Use g_malloc0() instead of g_malloc()
 - Use sizeof with parentheses

Thomas Huth (2):
  tests/boot-sector: Do not overwrite the x86 buffer on other
    architectures
  tests/pxe: Check virtio-net-ccw on s390x

 tests/Makefile.include |  1 +
 tests/boot-sector.c    | 61 +++++++++++++++++++++++++++++++++++++-------------
 tests/pxe-test.c       |  7 ++++++
 3 files changed, 54 insertions(+), 15 deletions(-)

-- 
1.8.3.1


Re: [Qemu-devel] [PATCH v2 for-2.11 0/2] Improvements for the pxe tester
Posted by Cornelia Huck 6 years, 8 months ago
On Fri, 11 Aug 2017 07:57:54 +0200
Thomas Huth <thuth@redhat.com> wrote:

> The first patch improves the buffer handling in the pxe tester a
> little bit by allocating a separate buffer on the heap for each
> architecture. This also gets rid of the huge pre-initialized
> array in the tester, shrinking the size of the executable by
> half of a megabyte!
> The second patch adds s390x support to the pxe tester. Starting
> with QEMU 2.10, the guest firmware on s390x can now net-boot via
> TFTP, too, so we can automatically test this code in the pxe tester.
> 
> v2: Adressed Cornelia's review feedback from the first version, e.g.:
>  - Use g_malloc0() instead of g_malloc()
>  - Use sizeof with parentheses
> 
> Thomas Huth (2):
>   tests/boot-sector: Do not overwrite the x86 buffer on other
>     architectures
>   tests/pxe: Check virtio-net-ccw on s390x
> 
>  tests/Makefile.include |  1 +
>  tests/boot-sector.c    | 61 +++++++++++++++++++++++++++++++++++++-------------
>  tests/pxe-test.c       |  7 ++++++
>  3 files changed, 54 insertions(+), 15 deletions(-)
> 

Haven't reviewed the updates yet, but tried it out (on my laptop and on
a z/VM guest). Laptop went fine; on the z/VM guest, I forgot to turn on
vm.allocate_pgste at first:

TEST: tests/pxe-test... (pid=56084)
  /s390x/pxe/virtio-ccw:                                               ioctl(KVM_CREATE_VM) failed: 22 Invalid argument
Host kernel setup problem detected. Please verify:
- for kernels supporting the switch_amode or user_mode parameters, whether
  user space is running in primary address space
- for kernels supporting the vm.allocate_pgste sysctl, whether it is enabled
qemu-system-s390x: failed to initialize KVM: Invalid argument
qemu-system-s390x: Back to tcg accelerator
OK
PASS: tests/pxe-test

It does the correct fallback (the need for allocate_pgste will go away
with future kernels), so this is fine. However, it prompted me to test
with --disable-tcg:

TEST: tests/boot-serial-test... (pid=60499)
  /s390x/boot-serial/s390-ccw-virtio:                                  qemu-system-s390x: -machine accel=tcg: No accelerator found
socket_accept failed: Resource temporarily unavailable
**
ERROR:/root/git/qemu/tests/libqtest.c:212:qtest_init_without_qmp_handshake: assertion failed: (s->fd >= 0 && s->qmp_fd >= 0)
FAIL
GTester: last random seed: R02S83c8a1860bbd8c2658647407c7070b7e
(pid=60503)
FAIL: tests/boot-serial-test

There are probably more tests that rely on tcg always being available,
which is no longer true. (Also, the boot-serial test should probably
check if qemu even started rather than run into in a timeout.)

[Your patches are fine, but I think we really need to consider tests
and accelerators.]

Re: [Qemu-devel] [PATCH v2 for-2.11 0/2] Improvements for the pxe tester
Posted by Cornelia Huck 6 years, 8 months ago
On Fri, 11 Aug 2017 11:49:22 +0200
Cornelia Huck <cohuck@redhat.com> wrote:

> On Fri, 11 Aug 2017 07:57:54 +0200
> Thomas Huth <thuth@redhat.com> wrote:
> 
> > The first patch improves the buffer handling in the pxe tester a
> > little bit by allocating a separate buffer on the heap for each
> > architecture. This also gets rid of the huge pre-initialized
> > array in the tester, shrinking the size of the executable by
> > half of a megabyte!
> > The second patch adds s390x support to the pxe tester. Starting
> > with QEMU 2.10, the guest firmware on s390x can now net-boot via
> > TFTP, too, so we can automatically test this code in the pxe tester.
> > 
> > v2: Adressed Cornelia's review feedback from the first version, e.g.:
> >  - Use g_malloc0() instead of g_malloc()
> >  - Use sizeof with parentheses
> > 
> > Thomas Huth (2):
> >   tests/boot-sector: Do not overwrite the x86 buffer on other
> >     architectures
> >   tests/pxe: Check virtio-net-ccw on s390x
> > 
> >  tests/Makefile.include |  1 +
> >  tests/boot-sector.c    | 61 +++++++++++++++++++++++++++++++++++++-------------
> >  tests/pxe-test.c       |  7 ++++++
> >  3 files changed, 54 insertions(+), 15 deletions(-)
> >   
> 
> Haven't reviewed the updates yet, but tried it out (on my laptop and on
> a z/VM guest). Laptop went fine; on the z/VM guest, I forgot to turn on
> vm.allocate_pgste at first:
> 
> TEST: tests/pxe-test... (pid=56084)
>   /s390x/pxe/virtio-ccw:                                               ioctl(KVM_CREATE_VM) failed: 22 Invalid argument
> Host kernel setup problem detected. Please verify:
> - for kernels supporting the switch_amode or user_mode parameters, whether
>   user space is running in primary address space
> - for kernels supporting the vm.allocate_pgste sysctl, whether it is enabled
> qemu-system-s390x: failed to initialize KVM: Invalid argument
> qemu-system-s390x: Back to tcg accelerator
> OK
> PASS: tests/pxe-test
> 
> It does the correct fallback (the need for allocate_pgste will go away
> with future kernels), so this is fine. However, it prompted me to test
> with --disable-tcg:
> 
> TEST: tests/boot-serial-test... (pid=60499)
>   /s390x/boot-serial/s390-ccw-virtio:                                  qemu-system-s390x: -machine accel=tcg: No accelerator found
> socket_accept failed: Resource temporarily unavailable
> **
> ERROR:/root/git/qemu/tests/libqtest.c:212:qtest_init_without_qmp_handshake: assertion failed: (s->fd >= 0 && s->qmp_fd >= 0)
> FAIL
> GTester: last random seed: R02S83c8a1860bbd8c2658647407c7070b7e
> (pid=60503)
> FAIL: tests/boot-serial-test
> 
> There are probably more tests that rely on tcg always being available,
> which is no longer true. (Also, the boot-serial test should probably
> check if qemu even started rather than run into in a timeout.)
> 
> [Your patches are fine, but I think we really need to consider tests
> and accelerators.]

A quick test on x86_64 with --disable-tcg showed no further problems
than boot-serial-test, so at least it's only the one for now (and it
might make sense to simply use accel=tcg:kvm for it).

Re: [Qemu-devel] [PATCH v2 for-2.11 0/2] Improvements for the pxe tester
Posted by Thomas Huth 6 years, 8 months ago
On 11.08.2017 12:19, Cornelia Huck wrote:
> On Fri, 11 Aug 2017 11:49:22 +0200
> Cornelia Huck <cohuck@redhat.com> wrote:
> 
>> On Fri, 11 Aug 2017 07:57:54 +0200
>> Thomas Huth <thuth@redhat.com> wrote:
>>
>>> The first patch improves the buffer handling in the pxe tester a
>>> little bit by allocating a separate buffer on the heap for each
>>> architecture. This also gets rid of the huge pre-initialized
>>> array in the tester, shrinking the size of the executable by
>>> half of a megabyte!
>>> The second patch adds s390x support to the pxe tester. Starting
>>> with QEMU 2.10, the guest firmware on s390x can now net-boot via
>>> TFTP, too, so we can automatically test this code in the pxe tester.
>>>
>>> v2: Adressed Cornelia's review feedback from the first version, e.g.:
>>>  - Use g_malloc0() instead of g_malloc()
>>>  - Use sizeof with parentheses
>>>
>>> Thomas Huth (2):
>>>   tests/boot-sector: Do not overwrite the x86 buffer on other
>>>     architectures
>>>   tests/pxe: Check virtio-net-ccw on s390x
>>>
>>>  tests/Makefile.include |  1 +
>>>  tests/boot-sector.c    | 61 +++++++++++++++++++++++++++++++++++++-------------
>>>  tests/pxe-test.c       |  7 ++++++
>>>  3 files changed, 54 insertions(+), 15 deletions(-)
>>>   
>>
>> Haven't reviewed the updates yet, but tried it out (on my laptop and on
>> a z/VM guest). Laptop went fine; on the z/VM guest, I forgot to turn on
>> vm.allocate_pgste at first:
>>
>> TEST: tests/pxe-test... (pid=56084)
>>   /s390x/pxe/virtio-ccw:                                               ioctl(KVM_CREATE_VM) failed: 22 Invalid argument
>> Host kernel setup problem detected. Please verify:
>> - for kernels supporting the switch_amode or user_mode parameters, whether
>>   user space is running in primary address space
>> - for kernels supporting the vm.allocate_pgste sysctl, whether it is enabled
>> qemu-system-s390x: failed to initialize KVM: Invalid argument
>> qemu-system-s390x: Back to tcg accelerator
>> OK
>> PASS: tests/pxe-test
>>
>> It does the correct fallback (the need for allocate_pgste will go away
>> with future kernels), so this is fine. However, it prompted me to test
>> with --disable-tcg:
>>
>> TEST: tests/boot-serial-test... (pid=60499)
>>   /s390x/boot-serial/s390-ccw-virtio:                                  qemu-system-s390x: -machine accel=tcg: No accelerator found
>> socket_accept failed: Resource temporarily unavailable
>> **
>> ERROR:/root/git/qemu/tests/libqtest.c:212:qtest_init_without_qmp_handshake: assertion failed: (s->fd >= 0 && s->qmp_fd >= 0)
>> FAIL
>> GTester: last random seed: R02S83c8a1860bbd8c2658647407c7070b7e
>> (pid=60503)
>> FAIL: tests/boot-serial-test
>>
>> There are probably more tests that rely on tcg always being available,
>> which is no longer true. (Also, the boot-serial test should probably
>> check if qemu even started rather than run into in a timeout.)
>>
>> [Your patches are fine, but I think we really need to consider tests
>> and accelerators.]
> 
> A quick test on x86_64 with --disable-tcg showed no further problems
> than boot-serial-test, so at least it's only the one for now (and it
> might make sense to simply use accel=tcg:kvm for it).

$ grep -rl accel=tcg tests/
tests/pnv-xscom-test.c
tests/boot-serial-test.c
tests/prom-env-test.c

... so the other problematic tests are for ppc only - which does not
support --disable-tcg yet.

 Thomas

Re: [Qemu-devel] [PATCH v2 for-2.11 0/2] Improvements for the pxe tester
Posted by Cornelia Huck 6 years, 8 months ago
On Fri, 11 Aug 2017 12:52:54 +0200
Thomas Huth <thuth@redhat.com> wrote:

> On 11.08.2017 12:19, Cornelia Huck wrote:

> > A quick test on x86_64 with --disable-tcg showed no further problems
> > than boot-serial-test, so at least it's only the one for now (and it
> > might make sense to simply use accel=tcg:kvm for it).  
> 
> $ grep -rl accel=tcg tests/
> tests/pnv-xscom-test.c
> tests/boot-serial-test.c
> tests/prom-env-test.c
> 
> ... so the other problematic tests are for ppc only - which does not
> support --disable-tcg yet.

I'll send a patch for boot serial, then. You probably know more about
whether the ppc tests would make sense with kvm as well than I do :)

Re: [Qemu-devel] [PATCH v2 for-2.11 0/2] Improvements for the pxe tester
Posted by Michael S. Tsirkin 6 years, 8 months ago
On Fri, Aug 11, 2017 at 01:43:55PM +0200, Cornelia Huck wrote:
> On Fri, 11 Aug 2017 12:52:54 +0200
> Thomas Huth <thuth@redhat.com> wrote:
> 
> > On 11.08.2017 12:19, Cornelia Huck wrote:
> 
> > > A quick test on x86_64 with --disable-tcg showed no further problems
> > > than boot-serial-test, so at least it's only the one for now (and it
> > > might make sense to simply use accel=tcg:kvm for it).  
> > 
> > $ grep -rl accel=tcg tests/
> > tests/pnv-xscom-test.c
> > tests/boot-serial-test.c
> > tests/prom-env-test.c
> > 
> > ... so the other problematic tests are for ppc only - which does not
> > support --disable-tcg yet.
> 
> I'll send a patch for boot serial, then. You probably know more about
> whether the ppc tests would make sense with kvm as well than I do :)

Weird I thought I fixed that one.  Tweaking that is trivial though.
Posted - could someone try on ppc pls? If it helps pls merge through the
ppc tree.

-- 
MST

Re: [Qemu-devel] [PATCH v2 for-2.11 0/2] Improvements for the pxe tester
Posted by Cornelia Huck 6 years, 8 months ago
On Fri, 11 Aug 2017 07:57:54 +0200
Thomas Huth <thuth@redhat.com> wrote:

> The first patch improves the buffer handling in the pxe tester a
> little bit by allocating a separate buffer on the heap for each
> architecture. This also gets rid of the huge pre-initialized
> array in the tester, shrinking the size of the executable by
> half of a megabyte!
> The second patch adds s390x support to the pxe tester. Starting
> with QEMU 2.10, the guest firmware on s390x can now net-boot via
> TFTP, too, so we can automatically test this code in the pxe tester.
> 
> v2: Adressed Cornelia's review feedback from the first version, e.g.:
>  - Use g_malloc0() instead of g_malloc()
>  - Use sizeof with parentheses
> 
> Thomas Huth (2):
>   tests/boot-sector: Do not overwrite the x86 buffer on other
>     architectures
>   tests/pxe: Check virtio-net-ccw on s390x
> 
>  tests/Makefile.include |  1 +
>  tests/boot-sector.c    | 61 +++++++++++++++++++++++++++++++++++++-------------
>  tests/pxe-test.c       |  7 ++++++
>  3 files changed, 54 insertions(+), 15 deletions(-)
> 

It's that question again: Who picks this up? :)

I can take it through the s390 tree if nobody else wants it.

Re: [Qemu-devel] [PATCH v2 for-2.11 0/2] Improvements for the pxe tester
Posted by Cornelia Huck 6 years, 8 months ago
On Tue, 15 Aug 2017 17:27:55 +0200
Cornelia Huck <cohuck@redhat.com> wrote:

> On Fri, 11 Aug 2017 07:57:54 +0200
> Thomas Huth <thuth@redhat.com> wrote:
> 
> > The first patch improves the buffer handling in the pxe tester a
> > little bit by allocating a separate buffer on the heap for each
> > architecture. This also gets rid of the huge pre-initialized
> > array in the tester, shrinking the size of the executable by
> > half of a megabyte!
> > The second patch adds s390x support to the pxe tester. Starting
> > with QEMU 2.10, the guest firmware on s390x can now net-boot via
> > TFTP, too, so we can automatically test this code in the pxe tester.
> > 
> > v2: Adressed Cornelia's review feedback from the first version, e.g.:
> >  - Use g_malloc0() instead of g_malloc()
> >  - Use sizeof with parentheses
> > 
> > Thomas Huth (2):
> >   tests/boot-sector: Do not overwrite the x86 buffer on other
> >     architectures
> >   tests/pxe: Check virtio-net-ccw on s390x
> > 
> >  tests/Makefile.include |  1 +
> >  tests/boot-sector.c    | 61 +++++++++++++++++++++++++++++++++++++-------------
> >  tests/pxe-test.c       |  7 ++++++
> >  3 files changed, 54 insertions(+), 15 deletions(-)
> >   
> 
> It's that question again: Who picks this up? :)
> 
> I can take it through the s390 tree if nobody else wants it.

OK, I just went ahead and queued it to my s390-next branch.