Re: [Qemu-devel] [PATCH] tests/pxe: Check virtio-net-ccw on s390x

[Thomas Huth]

On 07.08.2017 22:35, Michael S. Tsirkin wrote:
> On Thu, Aug 03, 2017 at 03:30:19PM +0200, Thomas Huth wrote:
>> Now that we've got a firmware that can do TFTP booting on s390x (i.e.
>> the pc-bios/s390-netboot.img), we can enable the PXE tester for this
>> architecture, too.
>>
>> Signed-off-by: Thomas Huth <address@hidden>
>> ---
>>  Since this only adds a test, I guess this could still be included
>>  for 2.10 ... otherwise, I think it's also OK to simply postpone this
>>  to 2.11 instead
> 
> I think 2.11 is preferable.
> 
>>  tests/Makefile.include |  1 +
>>  tests/boot-sector.c    | 25 ++++++++++++++++++++++---
>>  tests/pxe-test.c       |  7 +++++++
>>  3 files changed, 30 insertions(+), 3 deletions(-)
>>
>> diff --git a/tests/Makefile.include b/tests/Makefile.include
>> index 59e536b..f9af5e3 100644
>> --- a/tests/Makefile.include
>> +++ b/tests/Makefile.include
>> @@ -337,6 +337,7 @@ check-qtest-microblazeel-y = $(check-qtest-microblaze-y)
>>  check-qtest-xtensaeb-y = $(check-qtest-xtensa-y)
>>  
>>  check-qtest-s390x-y = tests/boot-serial-test$(EXESUF)
>> +check-qtest-s390x-y += tests/pxe-test$(EXESUF)
>>  
>>  check-qtest-generic-y += tests/qom-test$(EXESUF)
>>  check-qtest-generic-y += tests/test-hmp$(EXESUF)
>> diff --git a/tests/boot-sector.c b/tests/boot-sector.c
>> index e3880f4..87a8fb5 100644
>> --- a/tests/boot-sector.c
>> +++ b/tests/boot-sector.c
>> @@ -21,6 +21,7 @@
>>  #define SIGNATURE 0xdead
>>  #define SIGNATURE_OFFSET 0x10
>>  #define BOOT_SECTOR_ADDRESS 0x7c00
>> +#define SIGNATURE_ADDR (BOOT_SECTOR_ADDRESS + SIGNATURE_OFFSET)
>>  
>>  /* Boot sector code: write SIGNATURE into memory,
>>   * then halt.
>> @@ -73,6 +74,7 @@ int boot_sector_init(char *fname)
>>  {
>>      int fd, ret;
>>      size_t len = sizeof boot_sector;
>> +    const char *arch = qtest_get_arch();
>>  
>>      fd = mkstemp(fname);
>>      if (fd < 0) {
>> @@ -81,10 +83,27 @@ int boot_sector_init(char *fname)
>>      }
>>  
>>      /* For Open Firmware based system, we can use a Forth script instead */
>> -    if (strcmp(qtest_get_arch(), "ppc64") == 0) {
>> +    if (g_str_equal(arch, "ppc64")) {
>>          len = sprintf((char *)boot_sector, "\\ Bootscript\n%x %x c! %x %x 
>> c!\n",
>> -                LOW(SIGNATURE), BOOT_SECTOR_ADDRESS + SIGNATURE_OFFSET,
>> -                HIGH(SIGNATURE), BOOT_SECTOR_ADDRESS + SIGNATURE_OFFSET + 
>> 1);
>> +                LOW(SIGNATURE), SIGNATURE_ADDR,
>> +                HIGH(SIGNATURE), SIGNATURE_ADDR + 1);
>> +    } else if (g_str_equal(arch, "s390x")) {
>> +        /* For s390x, fake a kernel signature */
>> +        const uint8_t psw[] = {
>> +            0x00, 0x08, 0x00, 0x00, 0x80, 0x01, 0x00, 0x00
>> +        };
>> +        const uint8_t code[] = {
>> +            0xa7, 0xf4, 0x00, 0x0a,     /* j 0x10010 */
>> +            0x00, 0x00, 0x00, 0x00,
>> +            'S', '3', '9', '0',
>> +            'E', 'P', 0x00, 0x01,
>> +            0xa7, 0x38, HIGH(SIGNATURE_ADDR), LOW(SIGNATURE_ADDR), /* lhi 
>> r3 */
>> +            0xa7, 0x48, LOW(SIGNATURE), HIGH(SIGNATURE),    /* lhi 
>> r4,0xadde */
>> +            0x40, 0x40, 0x30, 0x00,     /* sth r4,0(r3) */
>> +            0xa7, 0xf4, 0xff, 0xfa      /* j 0x10010 */
>> +        };
>> +        memcpy(boot_sector, psw, 8);
>> +        memcpy(&boot_sector[0x10000], code, sizeof(code));
> 
> 
> Could we avoid overwriting boot sector?
> We really should have
> x86_boot_sector
> ppc64_boot_sector
> s390_boot_sector
> 
> And write out the correct thing.

Ok, I don't mind either way ... I can try to come up with a patch.

> Also:
>  * Q35 machine requires a minimum 0x7e000 bytes disk.
>  * (bug or feature?)
> 
> probably does not apply to s390x, does it?

We can also just load 0x10000 + sizeof(code) bytes here. I'll fix it.

Cornelia, please unqueue the patch, I'll send a v2...

 Thomas