[Qemu-devel] [PATCH v2 0/4] pc-bios/s390-ccw: Allow network booting via pxelinux.cfg

[Qemu-devel] [PATCH v2 0/4] pc-bios/s390-ccw: Allow network booting via pxelinux.cfg

[Thomas Huth]

This patch series adds pxelinux.cfg-style network booting to the s390-ccw
firmware. The core pxelinux.cfg loading and parsing logic has recently
been merged to SLOF, so these patches now just have to make sure to call
the right functions to get the config file loaded and parsed. Once this is
done, the kernel and initrd are loaded separately, and are then glued
together in RAM.

v2:
 - Update SLOF submodule now that the git mirror is in sync again
 - Last parameter to tftp_get_error_info() must not be NULL
 - Check CC when calling STSI, and use a #define for the UUID offset
 - Only support files with the magic "# pxelinux" string comment when
   trying to guess the contents of a file that has been downloaded via
   the "bootfile" DHCP parameter. This is just for developers' convenience,
   the official way to specify pxelinux.cfg files is to use the DHCP
   options 209 and 210 instead.

Thomas Huth (4):
  roms: Update SLOF submodule to current status
  pc-bios/s390-ccw/net: Update code for the latest changes in SLOF
  pc-bios/s390-ccw/net: Add support for pxelinux-style config files
  pc-bios/s390-ccw/net: Try to load pxelinux.cfg file accoring to the
    UUID

 pc-bios/s390-ccw/netboot.mak |   9 +-
 pc-bios/s390-ccw/netmain.c   | 226 +++++++++++++++++++++++++++++--------------
 roms/SLOF                    |   2 +-
 3 files changed, 162 insertions(+), 75 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH v2 1/4] roms: Update SLOF submodule to current status

[Thomas Huth]

We need the latest version of SLOF's libnet for adding pxelinux.cfg
support in the s390-ccw bios, too.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 I assume that Alexey will send a SLOF update soon, too, which
 supplies the SLOF.bin file for these changes.

 roms/SLOF | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/roms/SLOF b/roms/SLOF
index fa98132..2317427 160000
--- a/roms/SLOF
+++ b/roms/SLOF
@@ -1 +1 @@
-Subproject commit fa981320a1e0968d6fc1b8de319723ff8212b337
+Subproject commit 2317427ce76006723f7ae103a6998ab41dd79c68
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 2/4] pc-bios/s390-ccw/net: Update code for the latest changes in SLOF

[Thomas Huth]

The ip_version information now has to be stored in the filename_ip_t
structure, and there is now a common function called tftp_get_error_info()
which can be used to get the error string for a TFTP error code.
We can also get rid of some superfluous "(char *)" casts now.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 pc-bios/s390-ccw/netboot.mak |  2 +-
 pc-bios/s390-ccw/netmain.c   | 86 +++++++++-----------------------------------
 2 files changed, 18 insertions(+), 70 deletions(-)

diff --git a/pc-bios/s390-ccw/netboot.mak b/pc-bios/s390-ccw/netboot.mak
index 4f64128..a73be36 100644
--- a/pc-bios/s390-ccw/netboot.mak
+++ b/pc-bios/s390-ccw/netboot.mak
@@ -34,7 +34,7 @@ STDLIB_OBJS = atoi.o atol.o strtoul.o strtol.o rand.o malloc.o free.o
 %.o : $(SLOF_DIR)/lib/libc/stdlib/%.c
 	$(call quiet-command,$(CC) $(LIBC_CFLAGS) -c -o $@ $<,"CC","$(TARGET_DIR)$@")
 
-STDIO_OBJS = sprintf.o vfprintf.o vsnprintf.o vsprintf.o fprintf.o \
+STDIO_OBJS = sprintf.o snprintf.o vfprintf.o vsnprintf.o vsprintf.o fprintf.o \
 	     printf.o putc.o puts.o putchar.o stdchnls.o fileno.o
 %.o : $(SLOF_DIR)/lib/libc/stdio/%.c
 	$(call quiet-command,$(CC) $(LIBC_CFLAGS) -c -o $@ $<,"CC","$(TARGET_DIR)$@")
diff --git a/pc-bios/s390-ccw/netmain.c b/pc-bios/s390-ccw/netmain.c
index 6000241..d007fb7 100644
--- a/pc-bios/s390-ccw/netmain.c
+++ b/pc-bios/s390-ccw/netmain.c
@@ -47,7 +47,6 @@ IplParameterBlock iplb __attribute__((aligned(PAGE_SIZE)));
 static char cfgbuf[2048];
 
 static SubChannelId net_schid = { .one = 1 };
-static int ip_version = 4;
 static uint64_t dest_timer;
 
 static uint64_t get_timer_ms(void)
@@ -100,10 +99,10 @@ static int dhcp(struct filename_ip *fn_ip, int retries)
             printf("\nGiving up after %d DHCP requests\n", retries);
             return -1;
         }
-        ip_version = 4;
+        fn_ip->ip_version = 4;
         rc = dhcpv4(NULL, fn_ip);
         if (rc == -1) {
-            ip_version = 6;
+            fn_ip->ip_version = 6;
             set_ipv6_address(fn_ip->fd, 0);
             rc = dhcpv6(NULL, fn_ip);
             if (rc == 0) {
@@ -137,8 +136,7 @@ static int tftp_load(filename_ip_t *fnip, void *buffer, int len)
     tftp_err_t tftp_err;
     int rc;
 
-    rc = tftp(fnip, buffer, len, DEFAULT_TFTP_RETRIES, &tftp_err, 1, 1428,
-              ip_version);
+    rc = tftp(fnip, buffer, len, DEFAULT_TFTP_RETRIES, &tftp_err);
 
     if (rc < 0) {
         /* Make sure that error messages are put into a new line */
@@ -149,61 +147,11 @@ static int tftp_load(filename_ip_t *fnip, void *buffer, int len)
         printf("  TFTP: Received %s (%d KBytes)\n", fnip->filename, rc / 1024);
     } else if (rc > 0) {
         printf("  TFTP: Received %s (%d Bytes)\n", fnip->filename, rc);
-    } else if (rc == -1) {
-        puts("unknown TFTP error");
-    } else if (rc == -2) {
-        printf("TFTP buffer of %d bytes is too small for %s\n",
-            len, fnip->filename);
-    } else if (rc == -3) {
-        printf("file not found: %s\n", fnip->filename);
-    } else if (rc == -4) {
-        puts("TFTP access violation");
-    } else if (rc == -5) {
-        puts("illegal TFTP operation");
-    } else if (rc == -6) {
-        puts("unknown TFTP transfer ID");
-    } else if (rc == -7) {
-        puts("no such TFTP user");
-    } else if (rc == -8) {
-        puts("TFTP blocksize negotiation failed");
-    } else if (rc == -9) {
-        puts("file exceeds maximum TFTP transfer size");
-    } else if (rc <= -10 && rc >= -15) {
-        const char *icmp_err_str;
-        switch (rc) {
-        case -ICMP_NET_UNREACHABLE - 10:
-            icmp_err_str = "net unreachable";
-            break;
-        case -ICMP_HOST_UNREACHABLE - 10:
-            icmp_err_str = "host unreachable";
-            break;
-        case -ICMP_PROTOCOL_UNREACHABLE - 10:
-            icmp_err_str = "protocol unreachable";
-            break;
-        case -ICMP_PORT_UNREACHABLE - 10:
-            icmp_err_str = "port unreachable";
-            break;
-        case -ICMP_FRAGMENTATION_NEEDED - 10:
-            icmp_err_str = "fragmentation needed and DF set";
-            break;
-        case -ICMP_SOURCE_ROUTE_FAILED - 10:
-            icmp_err_str = "source route failed";
-            break;
-        default:
-            icmp_err_str = " UNKNOWN";
-            break;
-        }
-        printf("ICMP ERROR \"%s\"\n", icmp_err_str);
-    } else if (rc == -40) {
-        printf("TFTP error occurred after %d bad packets received",
-            tftp_err.bad_tftp_packets);
-    } else if (rc == -41) {
-        printf("TFTP error occurred after missing %d responses",
-            tftp_err.no_packets);
-    } else if (rc == -42) {
-        printf("TFTP error missing block %d, expected block was %d",
-            tftp_err.blocks_missed,
-            tftp_err.blocks_received);
+    } else {
+        const char *errstr = NULL;
+        int ecode;
+        tftp_get_error_info(fnip, &tftp_err, rc, &errstr, &ecode);
+        printf("TFTP error: %s\n", errstr ? errstr : "unknown error");
     }
 
     return rc;
@@ -231,7 +179,7 @@ static int net_init(filename_ip_t *fn_ip)
 
     rc = dhcp(fn_ip, DEFAULT_BOOT_RETRIES);
     if (rc >= 0) {
-        if (ip_version == 4) {
+        if (fn_ip->ip_version == 4) {
             set_ipv4_address(fn_ip->own_ip);
         }
     } else {
@@ -239,11 +187,11 @@ static int net_init(filename_ip_t *fn_ip)
         return -101;
     }
 
-    if (ip_version == 4) {
+    if (fn_ip->ip_version == 4) {
         printf("  Using IPv4 address: %d.%d.%d.%d\n",
               (fn_ip->own_ip >> 24) & 0xFF, (fn_ip->own_ip >> 16) & 0xFF,
               (fn_ip->own_ip >>  8) & 0xFF, fn_ip->own_ip & 0xFF);
-    } else if (ip_version == 6) {
+    } else if (fn_ip->ip_version == 6) {
         char ip6_str[40];
         ipv6_to_str(fn_ip->own_ip6.addr, ip6_str);
         printf("  Using IPv6 address: %s\n", ip6_str);
@@ -261,17 +209,17 @@ static int net_init(filename_ip_t *fn_ip)
     }
 
     printf("  Using TFTP server: ");
-    if (ip_version == 4) {
+    if (fn_ip->ip_version == 4) {
         printf("%d.%d.%d.%d\n",
                (fn_ip->server_ip >> 24) & 0xFF, (fn_ip->server_ip >> 16) & 0xFF,
                (fn_ip->server_ip >>  8) & 0xFF, fn_ip->server_ip & 0xFF);
-    } else if (ip_version == 6) {
+    } else if (fn_ip->ip_version == 6) {
         char ip6_str[40];
         ipv6_to_str(fn_ip->server_ip6.addr, ip6_str);
         printf("%s\n", ip6_str);
     }
 
-    if (strlen((char *)fn_ip->filename) > 0) {
+    if (strlen(fn_ip->filename) > 0) {
         printf("  Bootfile name: '%s'\n", fn_ip->filename);
     }
 
@@ -280,7 +228,7 @@ static int net_init(filename_ip_t *fn_ip)
 
 static void net_release(filename_ip_t *fn_ip)
 {
-    if (ip_version == 4) {
+    if (fn_ip->ip_version == 4) {
         dhcp_send_release(fn_ip->fd);
     }
 }
@@ -322,7 +270,7 @@ static int handle_ins_cfg(filename_ip_t *fn_ip, char *cfg, int cfgsize)
             return -1;
         }
         *ptr = 0;
-        strncpy((char *)fn_ip->filename, insbuf, sizeof(fn_ip->filename));
+        strncpy(fn_ip->filename, insbuf, sizeof(fn_ip->filename));
         destaddr = (char *)atol(ptr + 1);
         rc = tftp_load(fn_ip, destaddr, (long)_start - (long)destaddr);
         if (rc <= 0) {
@@ -455,7 +403,7 @@ void main(void)
         panic("Network initialization failed. Halting.\n");
     }
 
-    fnlen = strlen((char *)fn_ip.filename);
+    fnlen = strlen(fn_ip.filename);
     if (fnlen > 0 && fn_ip.filename[fnlen - 1] != '/') {
         rc = net_try_direct_tftp_load(&fn_ip);
     }
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 3/4] pc-bios/s390-ccw/net: Add support for pxelinux-style config files

[Thomas Huth]

Since it is quite cumbersome to manually create a combined kernel with
initrd image for network booting, we now support loading via pxelinux
configuration files, too. In these files, the kernel, initrd and command
line parameters can be specified seperately, and the firmware then takes
care of glueing everything together in memory after the files have been
downloaded. See this URL for details about the config file layout:
https://www.syslinux.org/wiki/index.php?title=PXELINUX

The user can either specify a config file directly as bootfile via DHCP
(but in this case, the file has to start with the magic "# pxelinux"
comment so we can distinguish it from binary kernels), or a folder (i.e.
the bootfile name must end with "/") where the firmware should look for
the typical pxelinux.cfg file names, e.g. based on MAC or IP address.
We also support the pxelinux.cfg DHCP options 209 and 210 from RFC 5071.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 pc-bios/s390-ccw/netboot.mak |  7 ++--
 pc-bios/s390-ccw/netmain.c   | 86 +++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 89 insertions(+), 4 deletions(-)

diff --git a/pc-bios/s390-ccw/netboot.mak b/pc-bios/s390-ccw/netboot.mak
index a73be36..8af0cfd 100644
--- a/pc-bios/s390-ccw/netboot.mak
+++ b/pc-bios/s390-ccw/netboot.mak
@@ -25,8 +25,9 @@ CTYPE_OBJS = isdigit.o isxdigit.o toupper.o
 %.o : $(SLOF_DIR)/lib/libc/ctype/%.c
 	$(call quiet-command,$(CC) $(LIBC_CFLAGS) -c -o $@ $<,"CC","$(TARGET_DIR)$@")
 
-STRING_OBJS = strcat.o strchr.o strcmp.o strcpy.o strlen.o strncmp.o strncpy.o \
-	      strstr.o memset.o memcpy.o memmove.o memcmp.o
+STRING_OBJS = strcat.o strchr.o strrchr.o strcpy.o strlen.o strncpy.o \
+	      strcmp.o strncmp.o strcasecmp.o strncasecmp.o strstr.o \
+	      memset.o memcpy.o memmove.o memcmp.o
 %.o : $(SLOF_DIR)/lib/libc/string/%.c
 	$(call quiet-command,$(CC) $(LIBC_CFLAGS) -c -o $@ $<,"CC","$(TARGET_DIR)$@")
 
@@ -50,7 +51,7 @@ libc.a: $(LIBCOBJS)
 # libnet files:
 
 LIBNETOBJS := args.o dhcp.o dns.o icmpv6.o ipv6.o tcp.o udp.o bootp.o \
-	      dhcpv6.o ethernet.o ipv4.o ndp.o tftp.o
+	      dhcpv6.o ethernet.o ipv4.o ndp.o tftp.o pxelinux.o
 LIBNETCFLAGS := $(QEMU_CFLAGS) -DDHCPARCH=0x1F $(LIBC_INC) $(LIBNET_INC)
 
 %.o : $(SLOF_DIR)/lib/libnet/%.c
diff --git a/pc-bios/s390-ccw/netmain.c b/pc-bios/s390-ccw/netmain.c
index d007fb7..c059546 100644
--- a/pc-bios/s390-ccw/netmain.c
+++ b/pc-bios/s390-ccw/netmain.c
@@ -30,6 +30,7 @@
 #include <ipv6.h>
 #include <dns.h>
 #include <time.h>
+#include <pxelinux.h>
 
 #include "s390-ccw.h"
 #include "virtio.h"
@@ -41,12 +42,14 @@ extern char _start[];
 
 #define KERNEL_ADDR             ((void *)0L)
 #define KERNEL_MAX_SIZE         ((long)_start)
+#define ARCH_COMMAND_LINE_SIZE  896              /* Taken from Linux kernel */
 
 char stack[PAGE_SIZE * 8] __attribute__((aligned(PAGE_SIZE)));
 IplParameterBlock iplb __attribute__((aligned(PAGE_SIZE)));
 static char cfgbuf[2048];
 
 static SubChannelId net_schid = { .one = 1 };
+static uint8_t mac[6];
 static uint64_t dest_timer;
 
 static uint64_t get_timer_ms(void)
@@ -159,7 +162,6 @@ static int tftp_load(filename_ip_t *fnip, void *buffer, int len)
 
 static int net_init(filename_ip_t *fn_ip)
 {
-    uint8_t mac[6];
     int rc;
 
     memset(fn_ip, 0, sizeof(filename_ip_t));
@@ -234,6 +236,66 @@ static void net_release(filename_ip_t *fn_ip)
 }
 
 /**
+ * Load a kernel with initrd (i.e. with the information that we've got from
+ * a pxelinux.cfg config file)
+ */
+static int load_kernel_with_initrd(filename_ip_t *fn_ip,
+                                   struct pl_cfg_entry *entry)
+{
+    int rc;
+
+    printf("Loading pxelinux.cfg entry '%s'\n", entry->label);
+
+    if (!entry->kernel) {
+        printf("Kernel entry is missing!\n");
+        return -1;
+    }
+
+    strncpy(fn_ip->filename, entry->kernel, sizeof(fn_ip->filename));
+    rc = tftp_load(fn_ip, KERNEL_ADDR, KERNEL_MAX_SIZE);
+    if (rc < 0) {
+        return rc;
+    }
+
+    if (entry->initrd) {
+        uint64_t iaddr = (rc + 0xfff) & ~0xfffUL;
+
+        strncpy(fn_ip->filename, entry->initrd, sizeof(fn_ip->filename));
+        rc = tftp_load(fn_ip, (void *)iaddr, KERNEL_MAX_SIZE - iaddr);
+        if (rc < 0) {
+            return rc;
+        }
+        /* Patch location and size: */
+        *(uint64_t *)0x10408 = iaddr;
+        *(uint64_t *)0x10410 = rc;
+        rc += iaddr;
+    }
+
+    if (entry->append) {
+        strncpy((char *)0x10480, entry->append, ARCH_COMMAND_LINE_SIZE);
+    }
+
+    return rc;
+}
+
+#define MAX_PXELINUX_ENTRIES 16
+
+static int net_try_pxelinux_cfg(filename_ip_t *fn_ip)
+{
+    struct pl_cfg_entry entries[MAX_PXELINUX_ENTRIES];
+    int num_ent, def_ent = 0;
+
+    num_ent = pxelinux_load_parse_cfg(fn_ip, mac, NULL, DEFAULT_TFTP_RETRIES,
+                                      cfgbuf, sizeof(cfgbuf),
+                                      entries, MAX_PXELINUX_ENTRIES, &def_ent);
+    if (num_ent > 0) {
+        return load_kernel_with_initrd(fn_ip, &entries[def_ent]);
+    }
+
+    return -1;
+}
+
+/**
  * Load via information from a .INS file (which can be found on CD-ROMs
  * for example)
  */
@@ -302,6 +364,25 @@ static int net_try_direct_tftp_load(filename_ip_t *fn_ip)
         if (!strncmp("* ", cfgbuf, 2)) {
             return handle_ins_cfg(fn_ip, cfgbuf, rc);
         }
+        /*
+         * pxelinux.cfg support via bootfile name is just here for developers'
+         * convenience (it eases testing with the built-in DHCP server of QEMU
+         * that does not support RFC 5071). The official way to configure a
+         * pxelinux.cfg file name is to use DHCP options 209 and 210 instead.
+         * So only use the pxelinux.cfg parser here for files that start with
+         * a magic comment string.
+         */
+        if (!strncasecmp("# pxelinux", cfgbuf, 10)) {
+            struct pl_cfg_entry entries[MAX_PXELINUX_ENTRIES];
+            int num_ent, def_ent = 0;
+
+            num_ent = pxelinux_parse_cfg(cfgbuf, sizeof(cfgbuf), entries,
+                                         MAX_PXELINUX_ENTRIES, &def_ent);
+            if (num_ent <= 0) {
+                return -1;
+            }
+            return load_kernel_with_initrd(fn_ip, &entries[def_ent]);
+        }
     }
 
     /* Move kernel to right location */
@@ -407,6 +488,9 @@ void main(void)
     if (fnlen > 0 && fn_ip.filename[fnlen - 1] != '/') {
         rc = net_try_direct_tftp_load(&fn_ip);
     }
+    if (rc <= 0) {
+        rc = net_try_pxelinux_cfg(&fn_ip);
+    }
 
     net_release(&fn_ip);
 
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 4/4] pc-bios/s390-ccw/net: Try to load pxelinux.cfg file accoring to the UUID

[Thomas Huth]

With the STSI instruction, we can get the UUID of the current VM instance,
so we can support loading pxelinux config files via UUID in the file name,
too.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 pc-bios/s390-ccw/netmain.c | 56 +++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 55 insertions(+), 1 deletion(-)

diff --git a/pc-bios/s390-ccw/netmain.c b/pc-bios/s390-ccw/netmain.c
index c059546..0392131 100644
--- a/pc-bios/s390-ccw/netmain.c
+++ b/pc-bios/s390-ccw/netmain.c
@@ -44,6 +44,9 @@ extern char _start[];
 #define KERNEL_MAX_SIZE         ((long)_start)
 #define ARCH_COMMAND_LINE_SIZE  896              /* Taken from Linux kernel */
 
+/* STSI 3.2.2 offset of first vmdb + offset of uuid inside vmdb */
+#define STSI322_VMDB_UUID_OFFSET ((8 + 12) * 4)
+
 char stack[PAGE_SIZE * 8] __attribute__((aligned(PAGE_SIZE)));
 IplParameterBlock iplb __attribute__((aligned(PAGE_SIZE)));
 static char cfgbuf[2048];
@@ -236,6 +239,56 @@ static void net_release(filename_ip_t *fn_ip)
 }
 
 /**
+ * Retrieve the Universally Unique Identifier of the VM.
+ * @return UUID string, or NULL in case of errors
+ */
+static const char *get_uuid(void)
+{
+    register int r0 asm("0");
+    register int r1 asm("1");
+    uint8_t *mem, *buf, uuid[16];
+    int i, cc, chk = 0;
+    static char uuid_str[37];
+
+    mem = malloc(2 * PAGE_SIZE);
+    if (!mem) {
+        puts("Out of memory ... can not get UUID.");
+        return NULL;
+    }
+    buf = (uint8_t *)(((uint64_t)mem + PAGE_SIZE - 1) & ~(PAGE_SIZE - 1));
+    memset(buf, 0, PAGE_SIZE);
+
+    /* Get SYSIB 3.2.2 */
+    r0 = (3 << 28) | 2;
+    r1 = 2;
+    asm volatile(" stsi 0(%[addr])\n"
+                 " ipm  %[cc]\n"
+                 " srl  %[cc],28\n"
+                 : [cc] "=d" (cc)
+                 : "d" (r0), "d" (r1), [addr] "a" (buf)
+                 : "cc", "memory");
+    if (cc) {
+        return NULL;
+    }
+
+    for (i = 0; i < 16; i++) {
+        uuid[i] = buf[STSI322_VMDB_UUID_OFFSET + i];
+        chk |= uuid[i];
+    }
+    free(mem);
+    if (!chk) {
+        return NULL;
+    }
+
+    sprintf(uuid_str, "%02x%02x%02x%02x-%02x%02x-%02x%02x-%02x%02x-"
+            "%02x%02x%02x%02x%02x%02x", uuid[0], uuid[1], uuid[2], uuid[3],
+            uuid[4], uuid[5], uuid[6], uuid[7], uuid[8], uuid[9], uuid[10],
+            uuid[11], uuid[12], uuid[13], uuid[14], uuid[15]);
+
+    return uuid_str;
+}
+
+/**
  * Load a kernel with initrd (i.e. with the information that we've got from
  * a pxelinux.cfg config file)
  */
@@ -285,7 +338,8 @@ static int net_try_pxelinux_cfg(filename_ip_t *fn_ip)
     struct pl_cfg_entry entries[MAX_PXELINUX_ENTRIES];
     int num_ent, def_ent = 0;
 
-    num_ent = pxelinux_load_parse_cfg(fn_ip, mac, NULL, DEFAULT_TFTP_RETRIES,
+    num_ent = pxelinux_load_parse_cfg(fn_ip, mac, get_uuid(),
+                                      DEFAULT_TFTP_RETRIES,
                                       cfgbuf, sizeof(cfgbuf),
                                       entries, MAX_PXELINUX_ENTRIES, &def_ent);
     if (num_ent > 0) {
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v2 0/4] pc-bios/s390-ccw: Allow network booting via pxelinux.cfg

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 1528374154-14680-1-git-send-email-thuth@redhat.com
Subject: [Qemu-devel] [PATCH v2 0/4] pc-bios/s390-ccw: Allow network booting via pxelinux.cfg

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
   5d328d7d2f..8e36d27c5a  master     -> master
 * [new tag]               patchew/1528374154-14680-1-git-send-email-thuth@redhat.com -> patchew/1528374154-14680-1-git-send-email-thuth@redhat.com
 * [new tag]               patchew/1528403460-30526-1-git-send-email-gengdongjiu@huawei.com -> patchew/1528403460-30526-1-git-send-email-gengdongjiu@huawei.com
 t [tag update]            patchew/20180530180120.13355-1-richard.henderson@linaro.org -> patchew/20180530180120.13355-1-richard.henderson@linaro.org
Switched to a new branch 'test'
132ffa3587 pc-bios/s390-ccw/net: Try to load pxelinux.cfg file accoring to the UUID
b3b73613ba pc-bios/s390-ccw/net: Add support for pxelinux-style config files
699020402a pc-bios/s390-ccw/net: Update code for the latest changes in SLOF
5dd76a9c89 roms: Update SLOF submodule to current status

=== OUTPUT BEGIN ===
Checking PATCH 1/4: roms: Update SLOF submodule to current status...
Checking PATCH 2/4: pc-bios/s390-ccw/net: Update code for the latest changes in SLOF...
Checking PATCH 3/4: pc-bios/s390-ccw/net: Add support for pxelinux-style config files...
Checking PATCH 4/4: pc-bios/s390-ccw/net: Try to load pxelinux.cfg file accoring to the UUID...
ERROR: space prohibited before open square bracket '['
#57: FILE: pc-bios/s390-ccw/netmain.c:267:
+                 : [cc] "=d" (cc)

total: 1 errors, 0 warnings, 74 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [Qemu-devel] [PATCH v2 0/4] pc-bios/s390-ccw: Allow network booting via pxelinux.cfg

[Thomas Huth]

On 07.06.2018 14:31, no-reply@patchew.org wrote:
> Hi,
> 
> This series seems to have some coding style problems. See output below for
> more information:
> 
> Type: series
> Message-id: 1528374154-14680-1-git-send-email-thuth@redhat.com
> Subject: [Qemu-devel] [PATCH v2 0/4] pc-bios/s390-ccw: Allow network booting via pxelinux.cfg
> 
> === TEST SCRIPT BEGIN ===
> #!/bin/bash
> 
> BASE=base
> n=1
> total=$(git log --oneline $BASE.. | wc -l)
> failed=0
> 
> git config --local diff.renamelimit 0
> git config --local diff.renames True
> git config --local diff.algorithm histogram
> 
> commits="$(git log --format=%H --reverse $BASE..)"
> for c in $commits; do
>     echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
>     if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
>         failed=1
>         echo
>     fi
>     n=$((n+1))
> done
> 
> exit $failed
> === TEST SCRIPT END ===
> 
> Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
> From https://github.com/patchew-project/qemu
>    5d328d7d2f..8e36d27c5a  master     -> master
>  * [new tag]               patchew/1528374154-14680-1-git-send-email-thuth@redhat.com -> patchew/1528374154-14680-1-git-send-email-thuth@redhat.com
>  * [new tag]               patchew/1528403460-30526-1-git-send-email-gengdongjiu@huawei.com -> patchew/1528403460-30526-1-git-send-email-gengdongjiu@huawei.com
>  t [tag update]            patchew/20180530180120.13355-1-richard.henderson@linaro.org -> patchew/20180530180120.13355-1-richard.henderson@linaro.org
> Switched to a new branch 'test'
> 132ffa3587 pc-bios/s390-ccw/net: Try to load pxelinux.cfg file accoring to the UUID
> b3b73613ba pc-bios/s390-ccw/net: Add support for pxelinux-style config files
> 699020402a pc-bios/s390-ccw/net: Update code for the latest changes in SLOF
> 5dd76a9c89 roms: Update SLOF submodule to current status
> 
> === OUTPUT BEGIN ===
> Checking PATCH 1/4: roms: Update SLOF submodule to current status...
> Checking PATCH 2/4: pc-bios/s390-ccw/net: Update code for the latest changes in SLOF...
> Checking PATCH 3/4: pc-bios/s390-ccw/net: Add support for pxelinux-style config files...
> Checking PATCH 4/4: pc-bios/s390-ccw/net: Try to load pxelinux.cfg file accoring to the UUID...
> ERROR: space prohibited before open square bracket '['
> #57: FILE: pc-bios/s390-ccw/netmain.c:267:
> +                 : [cc] "=d" (cc)

Patchew obviously does not know about inline assembly ... I think I
prefer to keep the space here.

 Thomas

Re: [Qemu-devel] [qemu-s390x] [PATCH v2 2/4] pc-bios/s390-ccw/net: Update code for the latest changes in SLOF

[Christian Borntraeger]

On 06/07/2018 02:22 PM, Thomas Huth wrote:
> The ip_version information now has to be stored in the filename_ip_t
> structure, and there is now a common function called tftp_get_error_info()
> which can be used to get the error string for a TFTP error code.
> We can also get rid of some superfluous "(char *)" casts now.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>



> ---
>  pc-bios/s390-ccw/netboot.mak |  2 +-
>  pc-bios/s390-ccw/netmain.c   | 86 +++++++++-----------------------------------
>  2 files changed, 18 insertions(+), 70 deletions(-)
> 
> diff --git a/pc-bios/s390-ccw/netboot.mak b/pc-bios/s390-ccw/netboot.mak
> index 4f64128..a73be36 100644
> --- a/pc-bios/s390-ccw/netboot.mak
> +++ b/pc-bios/s390-ccw/netboot.mak
> @@ -34,7 +34,7 @@ STDLIB_OBJS = atoi.o atol.o strtoul.o strtol.o rand.o malloc.o free.o
>  %.o : $(SLOF_DIR)/lib/libc/stdlib/%.c
>  	$(call quiet-command,$(CC) $(LIBC_CFLAGS) -c -o $@ $<,"CC","$(TARGET_DIR)$@")
>  
> -STDIO_OBJS = sprintf.o vfprintf.o vsnprintf.o vsprintf.o fprintf.o \
> +STDIO_OBJS = sprintf.o snprintf.o vfprintf.o vsnprintf.o vsprintf.o fprintf.o \
>  	     printf.o putc.o puts.o putchar.o stdchnls.o fileno.o
>  %.o : $(SLOF_DIR)/lib/libc/stdio/%.c
>  	$(call quiet-command,$(CC) $(LIBC_CFLAGS) -c -o $@ $<,"CC","$(TARGET_DIR)$@")
> diff --git a/pc-bios/s390-ccw/netmain.c b/pc-bios/s390-ccw/netmain.c
> index 6000241..d007fb7 100644
> --- a/pc-bios/s390-ccw/netmain.c
> +++ b/pc-bios/s390-ccw/netmain.c
> @@ -47,7 +47,6 @@ IplParameterBlock iplb __attribute__((aligned(PAGE_SIZE)));
>  static char cfgbuf[2048];
>  
>  static SubChannelId net_schid = { .one = 1 };
> -static int ip_version = 4;
>  static uint64_t dest_timer;
>  
>  static uint64_t get_timer_ms(void)
> @@ -100,10 +99,10 @@ static int dhcp(struct filename_ip *fn_ip, int retries)
>              printf("\nGiving up after %d DHCP requests\n", retries);
>              return -1;
>          }
> -        ip_version = 4;
> +        fn_ip->ip_version = 4;
>          rc = dhcpv4(NULL, fn_ip);
>          if (rc == -1) {
> -            ip_version = 6;
> +            fn_ip->ip_version = 6;
>              set_ipv6_address(fn_ip->fd, 0);
>              rc = dhcpv6(NULL, fn_ip);
>              if (rc == 0) {
> @@ -137,8 +136,7 @@ static int tftp_load(filename_ip_t *fnip, void *buffer, int len)
>      tftp_err_t tftp_err;
>      int rc;
>  
> -    rc = tftp(fnip, buffer, len, DEFAULT_TFTP_RETRIES, &tftp_err, 1, 1428,
> -              ip_version);
> +    rc = tftp(fnip, buffer, len, DEFAULT_TFTP_RETRIES, &tftp_err);
>  
>      if (rc < 0) {
>          /* Make sure that error messages are put into a new line */
> @@ -149,61 +147,11 @@ static int tftp_load(filename_ip_t *fnip, void *buffer, int len)
>          printf("  TFTP: Received %s (%d KBytes)\n", fnip->filename, rc / 1024);
>      } else if (rc > 0) {
>          printf("  TFTP: Received %s (%d Bytes)\n", fnip->filename, rc);
> -    } else if (rc == -1) {
> -        puts("unknown TFTP error");
> -    } else if (rc == -2) {
> -        printf("TFTP buffer of %d bytes is too small for %s\n",
> -            len, fnip->filename);
> -    } else if (rc == -3) {
> -        printf("file not found: %s\n", fnip->filename);
> -    } else if (rc == -4) {
> -        puts("TFTP access violation");
> -    } else if (rc == -5) {
> -        puts("illegal TFTP operation");
> -    } else if (rc == -6) {
> -        puts("unknown TFTP transfer ID");
> -    } else if (rc == -7) {
> -        puts("no such TFTP user");
> -    } else if (rc == -8) {
> -        puts("TFTP blocksize negotiation failed");
> -    } else if (rc == -9) {
> -        puts("file exceeds maximum TFTP transfer size");
> -    } else if (rc <= -10 && rc >= -15) {
> -        const char *icmp_err_str;
> -        switch (rc) {
> -        case -ICMP_NET_UNREACHABLE - 10:
> -            icmp_err_str = "net unreachable";
> -            break;
> -        case -ICMP_HOST_UNREACHABLE - 10:
> -            icmp_err_str = "host unreachable";
> -            break;
> -        case -ICMP_PROTOCOL_UNREACHABLE - 10:
> -            icmp_err_str = "protocol unreachable";
> -            break;
> -        case -ICMP_PORT_UNREACHABLE - 10:
> -            icmp_err_str = "port unreachable";
> -            break;
> -        case -ICMP_FRAGMENTATION_NEEDED - 10:
> -            icmp_err_str = "fragmentation needed and DF set";
> -            break;
> -        case -ICMP_SOURCE_ROUTE_FAILED - 10:
> -            icmp_err_str = "source route failed";
> -            break;
> -        default:
> -            icmp_err_str = " UNKNOWN";
> -            break;
> -        }
> -        printf("ICMP ERROR \"%s\"\n", icmp_err_str);
> -    } else if (rc == -40) {
> -        printf("TFTP error occurred after %d bad packets received",
> -            tftp_err.bad_tftp_packets);
> -    } else if (rc == -41) {
> -        printf("TFTP error occurred after missing %d responses",
> -            tftp_err.no_packets);
> -    } else if (rc == -42) {
> -        printf("TFTP error missing block %d, expected block was %d",
> -            tftp_err.blocks_missed,
> -            tftp_err.blocks_received);
> +    } else {
> +        const char *errstr = NULL;
> +        int ecode;
> +        tftp_get_error_info(fnip, &tftp_err, rc, &errstr, &ecode);
> +        printf("TFTP error: %s\n", errstr ? errstr : "unknown error");
>      }
>  
>      return rc;
> @@ -231,7 +179,7 @@ static int net_init(filename_ip_t *fn_ip)
>  
>      rc = dhcp(fn_ip, DEFAULT_BOOT_RETRIES);
>      if (rc >= 0) {
> -        if (ip_version == 4) {
> +        if (fn_ip->ip_version == 4) {
>              set_ipv4_address(fn_ip->own_ip);
>          }
>      } else {
> @@ -239,11 +187,11 @@ static int net_init(filename_ip_t *fn_ip)
>          return -101;
>      }
>  
> -    if (ip_version == 4) {
> +    if (fn_ip->ip_version == 4) {
>          printf("  Using IPv4 address: %d.%d.%d.%d\n",
>                (fn_ip->own_ip >> 24) & 0xFF, (fn_ip->own_ip >> 16) & 0xFF,
>                (fn_ip->own_ip >>  8) & 0xFF, fn_ip->own_ip & 0xFF);
> -    } else if (ip_version == 6) {
> +    } else if (fn_ip->ip_version == 6) {
>          char ip6_str[40];
>          ipv6_to_str(fn_ip->own_ip6.addr, ip6_str);
>          printf("  Using IPv6 address: %s\n", ip6_str);
> @@ -261,17 +209,17 @@ static int net_init(filename_ip_t *fn_ip)
>      }
>  
>      printf("  Using TFTP server: ");
> -    if (ip_version == 4) {
> +    if (fn_ip->ip_version == 4) {
>          printf("%d.%d.%d.%d\n",
>                 (fn_ip->server_ip >> 24) & 0xFF, (fn_ip->server_ip >> 16) & 0xFF,
>                 (fn_ip->server_ip >>  8) & 0xFF, fn_ip->server_ip & 0xFF);
> -    } else if (ip_version == 6) {
> +    } else if (fn_ip->ip_version == 6) {
>          char ip6_str[40];
>          ipv6_to_str(fn_ip->server_ip6.addr, ip6_str);
>          printf("%s\n", ip6_str);
>      }
>  
> -    if (strlen((char *)fn_ip->filename) > 0) {
> +    if (strlen(fn_ip->filename) > 0) {
>          printf("  Bootfile name: '%s'\n", fn_ip->filename);
>      }
>  
> @@ -280,7 +228,7 @@ static int net_init(filename_ip_t *fn_ip)
>  
>  static void net_release(filename_ip_t *fn_ip)
>  {
> -    if (ip_version == 4) {
> +    if (fn_ip->ip_version == 4) {
>          dhcp_send_release(fn_ip->fd);
>      }
>  }
> @@ -322,7 +270,7 @@ static int handle_ins_cfg(filename_ip_t *fn_ip, char *cfg, int cfgsize)
>              return -1;
>          }
>          *ptr = 0;
> -        strncpy((char *)fn_ip->filename, insbuf, sizeof(fn_ip->filename));
> +        strncpy(fn_ip->filename, insbuf, sizeof(fn_ip->filename));
>          destaddr = (char *)atol(ptr + 1);
>          rc = tftp_load(fn_ip, destaddr, (long)_start - (long)destaddr);
>          if (rc <= 0) {
> @@ -455,7 +403,7 @@ void main(void)
>          panic("Network initialization failed. Halting.\n");
>      }
>  
> -    fnlen = strlen((char *)fn_ip.filename);
> +    fnlen = strlen(fn_ip.filename);
>      if (fnlen > 0 && fn_ip.filename[fnlen - 1] != '/') {
>          rc = net_try_direct_tftp_load(&fn_ip);
>      }
> 

Re: [Qemu-devel] [PATCH v2 0/4] pc-bios/s390-ccw: Allow network booting via pxelinux.cfg

[Viktor VM Mihajlovski]

On 07.06.2018 14:22, Thomas Huth wrote:
> This patch series adds pxelinux.cfg-style network booting to the s390-ccw
> firmware. The core pxelinux.cfg loading and parsing logic has recently
> been merged to SLOF, so these patches now just have to make sure to call
> the right functions to get the config file loaded and parsed. Once this is
> done, the kernel and initrd are loaded separately, and are then glued
> together in RAM.
> 
> v2:
>  - Update SLOF submodule now that the git mirror is in sync again
>  - Last parameter to tftp_get_error_info() must not be NULL
>  - Check CC when calling STSI, and use a #define for the UUID offset
>  - Only support files with the magic "# pxelinux" string comment when
>    trying to guess the contents of a file that has been downloaded via
>    the "bootfile" DHCP parameter. This is just for developers' convenience,
>    the official way to specify pxelinux.cfg files is to use the DHCP
>    options 209 and 210 instead.
> 
> Thomas Huth (4):
>   roms: Update SLOF submodule to current status
>   pc-bios/s390-ccw/net: Update code for the latest changes in SLOF
>   pc-bios/s390-ccw/net: Add support for pxelinux-style config files
>   pc-bios/s390-ccw/net: Try to load pxelinux.cfg file accoring to the
>     UUID
> 
>  pc-bios/s390-ccw/netboot.mak |   9 +-
>  pc-bios/s390-ccw/netmain.c   | 226 +++++++++++++++++++++++++++++--------------
>  roms/SLOF                    |   2 +-
>  3 files changed, 162 insertions(+), 75 deletions(-)
> 
I tested the series both with a self-created pxelinux.0 blob (to verify
backward compatibility) and without an existing pxelinux.0 file (to
force the standard pxelinux pattern). Both worked as expected, although
the built-in search took significantly longer (timeout?).

Tested-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com>

-- 
Regards,
  Viktor Mihajlovski

Re: [Qemu-devel] [PATCH v2 0/4] pc-bios/s390-ccw: Allow network booting via pxelinux.cfg

[Christian Borntraeger]

On 06/07/2018 02:22 PM, Thomas Huth wrote:
> This patch series adds pxelinux.cfg-style network booting to the s390-ccw
> firmware. The core pxelinux.cfg loading and parsing logic has recently
> been merged to SLOF, so these patches now just have to make sure to call
> the right functions to get the config file loaded and parsed. Once this is
> done, the kernel and initrd are loaded separately, and are then glued
> together in RAM.
> 
> v2:
>  - Update SLOF submodule now that the git mirror is in sync again
>  - Last parameter to tftp_get_error_info() must not be NULL
>  - Check CC when calling STSI, and use a #define for the UUID offset
>  - Only support files with the magic "# pxelinux" string comment when
>    trying to guess the contents of a file that has been downloaded via
>    the "bootfile" DHCP parameter. This is just for developers' convenience,
>    the official way to specify pxelinux.cfg files is to use the DHCP
>    options 209 and 210 instead.
> 
> Thomas Huth (4):
>   roms: Update SLOF submodule to current status
>   pc-bios/s390-ccw/net: Update code for the latest changes in SLOF
>   pc-bios/s390-ccw/net: Add support for pxelinux-style config files
>   pc-bios/s390-ccw/net: Try to load pxelinux.cfg file accoring to the
>     UUID
> 
>  pc-bios/s390-ccw/netboot.mak |   9 +-
>  pc-bios/s390-ccw/netmain.c   | 226 +++++++++++++++++++++++++++++--------------
>  roms/SLOF                    |   2 +-
>  3 files changed, 162 insertions(+), 75 deletions(-)
> 

Series
Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>

Re: [Qemu-devel] [PATCH v2 0/4] pc-bios/s390-ccw: Allow network booting via pxelinux.cfg

[Thomas Huth]

On 11.06.2018 11:08, Viktor VM Mihajlovski wrote:
> On 07.06.2018 14:22, Thomas Huth wrote:
>> This patch series adds pxelinux.cfg-style network booting to the s390-ccw
>> firmware. The core pxelinux.cfg loading and parsing logic has recently
>> been merged to SLOF, so these patches now just have to make sure to call
>> the right functions to get the config file loaded and parsed. Once this is
>> done, the kernel and initrd are loaded separately, and are then glued
>> together in RAM.
>>
>> v2:
>>  - Update SLOF submodule now that the git mirror is in sync again
>>  - Last parameter to tftp_get_error_info() must not be NULL
>>  - Check CC when calling STSI, and use a #define for the UUID offset
>>  - Only support files with the magic "# pxelinux" string comment when
>>    trying to guess the contents of a file that has been downloaded via
>>    the "bootfile" DHCP parameter. This is just for developers' convenience,
>>    the official way to specify pxelinux.cfg files is to use the DHCP
>>    options 209 and 210 instead.
>>
>> Thomas Huth (4):
>>   roms: Update SLOF submodule to current status
>>   pc-bios/s390-ccw/net: Update code for the latest changes in SLOF
>>   pc-bios/s390-ccw/net: Add support for pxelinux-style config files
>>   pc-bios/s390-ccw/net: Try to load pxelinux.cfg file accoring to the
>>     UUID
>>
>>  pc-bios/s390-ccw/netboot.mak |   9 +-
>>  pc-bios/s390-ccw/netmain.c   | 226 +++++++++++++++++++++++++++++--------------
>>  roms/SLOF                    |   2 +-
>>  3 files changed, 162 insertions(+), 75 deletions(-)
>>
> I tested the series both with a self-created pxelinux.0 blob (to verify
> backward compatibility) and without an existing pxelinux.0 file (to
> force the standard pxelinux pattern). Both worked as expected, although
> the built-in search took significantly longer (timeout?).
> 
> Tested-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com>

Thanks a lot for the testing!

Hmm, I've got no real clue why there should be a big difference in the
amount of time here ... is there maybe a lot of unrelated broadcast
network traffic from other hosts going on in that network where you've
tested it? It could be that the virtio-net driver of the s390-ccw bios
can not deal with that situation very well yet. You could try to
increase the "64" in virtio_net_init() in pc-bios/s390-ccw/virtio-net.c
to see whether it makes a difference. Or if you've got some spare time,
could you maybe run Wireshark on the server side to have a look at the
time-stamps of the related packets, and to see whether there are
duplicated TFTP read requests? This could indicate that the firmware
missed a packet and thus ran into a timeout.

 Thomas

PS: After I posted v1 of the patch set, you reported on IRC that you saw
a problem with a corrupted initrd download or something similar. Is that
problem now fixed for you?

Re: [Qemu-devel] [PATCH v2 0/4] pc-bios/s390-ccw: Allow network booting via pxelinux.cfg

[Viktor VM Mihajlovski]

On 11.06.2018 13:12, Thomas Huth wrote:
> On 11.06.2018 11:08, Viktor VM Mihajlovski wrote:
>> On 07.06.2018 14:22, Thomas Huth wrote:
>>> This patch series adds pxelinux.cfg-style network booting to the s390-ccw
>>> firmware. The core pxelinux.cfg loading and parsing logic has recently
>>> been merged to SLOF, so these patches now just have to make sure to call
>>> the right functions to get the config file loaded and parsed. Once this is
>>> done, the kernel and initrd are loaded separately, and are then glued
>>> together in RAM.
>>>
>>> v2:
>>>  - Update SLOF submodule now that the git mirror is in sync again
>>>  - Last parameter to tftp_get_error_info() must not be NULL
>>>  - Check CC when calling STSI, and use a #define for the UUID offset
>>>  - Only support files with the magic "# pxelinux" string comment when
>>>    trying to guess the contents of a file that has been downloaded via
>>>    the "bootfile" DHCP parameter. This is just for developers' convenience,
>>>    the official way to specify pxelinux.cfg files is to use the DHCP
>>>    options 209 and 210 instead.
>>>
>>> Thomas Huth (4):
>>>   roms: Update SLOF submodule to current status
>>>   pc-bios/s390-ccw/net: Update code for the latest changes in SLOF
>>>   pc-bios/s390-ccw/net: Add support for pxelinux-style config files
>>>   pc-bios/s390-ccw/net: Try to load pxelinux.cfg file accoring to the
>>>     UUID
>>>
>>>  pc-bios/s390-ccw/netboot.mak |   9 +-
>>>  pc-bios/s390-ccw/netmain.c   | 226 +++++++++++++++++++++++++++++--------------
>>>  roms/SLOF                    |   2 +-
>>>  3 files changed, 162 insertions(+), 75 deletions(-)
>>>
>> I tested the series both with a self-created pxelinux.0 blob (to verify
>> backward compatibility) and without an existing pxelinux.0 file (to
>> force the standard pxelinux pattern). Both worked as expected, although
>> the built-in search took significantly longer (timeout?).
>>
>> Tested-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com>
> 
> Thanks a lot for the testing!
> 
> Hmm, I've got no real clue why there should be a big difference in the
> amount of time here ... is there maybe a lot of unrelated broadcast
> network traffic from other hosts going on in that network where you've
> tested it? It could be that the virtio-net driver of the s390-ccw bios
> can not deal with that situation very well yet. You could try to
> increase the "64" in virtio_net_init() in pc-bios/s390-ccw/virtio-net.c
> to see whether it makes a difference. Or if you've got some spare time,
> could you maybe run Wireshark on the server side to have a look at the
> time-stamps of the related packets, and to see whether there are
> duplicated TFTP read requests? This could indicate that the firmware
> missed a packet and thus ran into a timeout.
FWIW: I'm using the isolated libvirt network on an otherwise idle host,
so it's unlikely that there's interference. If I find the time I'll have
a look at the traffic.
> 
>  Thomas
> 
> PS: After I posted v1 of the patch set, you reported on IRC that you saw
> a problem with a corrupted initrd download or something similar. Is that
> problem now fixed for you?
> 
Yes, it was my bad. I used an older kernel with the following problem
https://lkml.org/lkml/2017/3/13/683.

-- 
Regards,
  Viktor Mihajlovski

Re: [Qemu-devel] [qemu-s390x] [PATCH v2 0/4] pc-bios/s390-ccw: Allow network booting via pxelinux.cfg

[Thomas Huth]

On 11.06.2018 14:03, Viktor VM Mihajlovski wrote:
> On 11.06.2018 13:12, Thomas Huth wrote:
>> On 11.06.2018 11:08, Viktor VM Mihajlovski wrote:
>>> On 07.06.2018 14:22, Thomas Huth wrote:
>>>> This patch series adds pxelinux.cfg-style network booting to the s390-ccw
>>>> firmware. The core pxelinux.cfg loading and parsing logic has recently
>>>> been merged to SLOF, so these patches now just have to make sure to call
>>>> the right functions to get the config file loaded and parsed. Once this is
>>>> done, the kernel and initrd are loaded separately, and are then glued
>>>> together in RAM.
>>>>
>>>> v2:
>>>>  - Update SLOF submodule now that the git mirror is in sync again
>>>>  - Last parameter to tftp_get_error_info() must not be NULL
>>>>  - Check CC when calling STSI, and use a #define for the UUID offset
>>>>  - Only support files with the magic "# pxelinux" string comment when
>>>>    trying to guess the contents of a file that has been downloaded via
>>>>    the "bootfile" DHCP parameter. This is just for developers' convenience,
>>>>    the official way to specify pxelinux.cfg files is to use the DHCP
>>>>    options 209 and 210 instead.
>>>>
>>>> Thomas Huth (4):
>>>>   roms: Update SLOF submodule to current status
>>>>   pc-bios/s390-ccw/net: Update code for the latest changes in SLOF
>>>>   pc-bios/s390-ccw/net: Add support for pxelinux-style config files
>>>>   pc-bios/s390-ccw/net: Try to load pxelinux.cfg file accoring to the
>>>>     UUID
>>>>
>>>>  pc-bios/s390-ccw/netboot.mak |   9 +-
>>>>  pc-bios/s390-ccw/netmain.c   | 226 +++++++++++++++++++++++++++++--------------
>>>>  roms/SLOF                    |   2 +-
>>>>  3 files changed, 162 insertions(+), 75 deletions(-)
>>>>
>>> I tested the series both with a self-created pxelinux.0 blob (to verify
>>> backward compatibility) and without an existing pxelinux.0 file (to
>>> force the standard pxelinux pattern). Both worked as expected, although
>>> the built-in search took significantly longer (timeout?).
>>>
>>> Tested-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com>
>>
>> Thanks a lot for the testing!
>>
>> Hmm, I've got no real clue why there should be a big difference in the
>> amount of time here ... is there maybe a lot of unrelated broadcast
>> network traffic from other hosts going on in that network where you've
>> tested it? It could be that the virtio-net driver of the s390-ccw bios
>> can not deal with that situation very well yet. You could try to
>> increase the "64" in virtio_net_init() in pc-bios/s390-ccw/virtio-net.c
>> to see whether it makes a difference. Or if you've got some spare time,
>> could you maybe run Wireshark on the server side to have a look at the
>> time-stamps of the related packets, and to see whether there are
>> duplicated TFTP read requests? This could indicate that the firmware
>> missed a packet and thus ran into a timeout.
> FWIW: I'm using the isolated libvirt network on an otherwise idle host,
> so it's unlikely that there's interference. If I find the time I'll have
> a look at the traffic.

If you have the time to look at the traffic, could you please also check
the TFTP block size option that is negotiated at the beginning of the
TFTP transfer? If this other client is negotiating a transfer block size
that is bigger than the one from the s390-ccw firmware, this could
explain the differences in the downloading time, too.

libnet from SLOF currently uses a block size of 1428. This is the size
where all TFTP data should still fit nicely into one ethernet packet -
and this is also the size which is still supported by all TFTP servers
that support the blksize option. But theoretically it's also possible to
use a bigger block sizes if both, the server and the client support
fragmented UDP packets. Unfortunately, as far as I can see, SLOF's
libnet does not support fragmented UDP packets, so we can't increase the
block size here anymore so easily.

Can you tell which tftp client you are using in your other pxelinux.0
blob? (busybox tftp? HPA-tftp ?)

 Thomas

Re: [Qemu-devel] [qemu-s390x] [PATCH v2 0/4] pc-bios/s390-ccw: Allow network booting via pxelinux.cfg

[Viktor VM Mihajlovski]

On 12.06.2018 08:17, Thomas Huth wrote:
> On 11.06.2018 14:03, Viktor VM Mihajlovski wrote:
[...]
> 
> If you have the time to look at the traffic, could you please also check
> the TFTP block size option that is negotiated at the beginning of the
> TFTP transfer? If this other client is negotiating a transfer block size
> that is bigger than the one from the s390-ccw firmware, this could
> explain the differences in the downloading time, too.
> 
> libnet from SLOF currently uses a block size of 1428. This is the size
> where all TFTP data should still fit nicely into one ethernet packet -
> and this is also the size which is still supported by all TFTP servers
> that support the blksize option. But theoretically it's also possible to
> use a bigger block sizes if both, the server and the client support
> fragmented UDP packets. Unfortunately, as far as I can see, SLOF's
> libnet does not support fragmented UDP packets, so we can't increase the
> block size here anymore so easily.
You will be pleased to hear that the SLOF TFTP client outperforms the
busybox version (which uses 544-byte packets) by 30%. There's some
randomness introduced by the differences in DHCP response times which is
clearly not the client's fault. All is good...
[...]
-- 
Regards,
  Viktor Mihajlovski

Re: [Qemu-devel] [qemu-s390x] [PATCH v2 0/4] pc-bios/s390-ccw: Allow network booting via pxelinux.cfg

[Thomas Huth]

[-- Attachment #1: Type: text/plain, Size: 1462 bytes --]

On 13.06.2018 12:56, Viktor VM Mihajlovski wrote:
> On 12.06.2018 08:17, Thomas Huth wrote:
>> On 11.06.2018 14:03, Viktor VM Mihajlovski wrote:
> [...]
>>
>> If you have the time to look at the traffic, could you please also check
>> the TFTP block size option that is negotiated at the beginning of the
>> TFTP transfer? If this other client is negotiating a transfer block size
>> that is bigger than the one from the s390-ccw firmware, this could
>> explain the differences in the downloading time, too.
>>
>> libnet from SLOF currently uses a block size of 1428. This is the size
>> where all TFTP data should still fit nicely into one ethernet packet -
>> and this is also the size which is still supported by all TFTP servers
>> that support the blksize option. But theoretically it's also possible to
>> use a bigger block sizes if both, the server and the client support
>> fragmented UDP packets. Unfortunately, as far as I can see, SLOF's
>> libnet does not support fragmented UDP packets, so we can't increase the
>> block size here anymore so easily.
>
> You will be pleased to hear that the SLOF TFTP client outperforms the
> busybox version (which uses 544-byte packets) by 30%. There's some
> randomness introduced by the differences in DHCP response times which is
> clearly not the client's fault. All is good...

Ah, great, thanks for checking!

So I'm going to prepare a pull request for this next...

 Thomas


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]