[Qemu-devel] [PATCH v1 for-2.13 0/4] pc-bios/s390-ccw: Network boot improvements

[Qemu-devel] [PATCH v1 for-2.13 0/4] pc-bios/s390-ccw: Network boot improvements

[Thomas Huth]

Some patches to improve the network boot experience on s390x:

First, make sure that we shut down the virtio-net device before jumping
into the kernel. Otherwise some incoming packets might destroy some of
the kernel's data if it has not taken over the device yet.

Then the last two patches add support for loading kernels via
configuration files - pxelinux-style and .INS-file style. This way
you don't have to manually glue your ramdisk to your kernel anymore,
so this should be quite a relieve for all users who want to boot
Linux via the network.

The config file parsers have been completely written by myself from
scratch and only tested with some config files that I came up with
on my own. So if anybody has some pre-existing pxelinux config files
already for booting a s390x, I'd appreciate some testing to see whether
this works as expected for you, too!

Thomas Huth (4):
  pc-bios/s390-ccw/net: Split up net_load() into init, load and uninit
    parts
  pc-bios/s390-ccw/net: Stop virtio-net device before jumping into the
    OS
  pc-bios/s390-ccw/net: Add support for pxelinux-style config files
  pc-bios/s390-ccw/net: Add support for .INS config files

 pc-bios/s390-ccw/netboot.mak  |   5 +-
 pc-bios/s390-ccw/netmain.c    | 312 ++++++++++++++++++++++++++++++++++++++----
 pc-bios/s390-ccw/virtio-net.c |   8 ++
 pc-bios/s390-ccw/virtio.c     |  19 ++-
 pc-bios/s390-ccw/virtio.h     |   3 +
 5 files changed, 312 insertions(+), 35 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH v1 for-2.13 1/4] pc-bios/s390-ccw/net: Split up net_load() into init, load and uninit parts

[Thomas Huth]

When we want to support pxelinux-style network booting later, we've got
to do several TFTP transfers - and we do not want to apply for a new IP
address via DHCP each time. So split up net_load into three parts:

1. net_init(), which initializes virtio-net, gets an IP address via DHCP
   and prints out the related information.

2. The tftp_load call is now moved directly into the main() function

3. A new net_uninit() function which should tear down the network stack
   before we are done in the firmware.

This will make it easier to extend the code in the next patches.

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

diff --git a/pc-bios/s390-ccw/netmain.c b/pc-bios/s390-ccw/netmain.c
index d86d46b..ebdc440 100644
--- a/pc-bios/s390-ccw/netmain.c
+++ b/pc-bios/s390-ccw/netmain.c
@@ -128,13 +128,13 @@ static void seed_rng(uint8_t mac[])
     srand(seed);
 }
 
-static int tftp_load(filename_ip_t *fnip, void *buffer, int len,
-                     unsigned int retries, int ip_vers)
+static int tftp_load(filename_ip_t *fnip, void *buffer, int len)
 {
     tftp_err_t tftp_err;
     int rc;
 
-    rc = tftp(fnip, buffer, len, retries, &tftp_err, 1, 1428, ip_vers);
+    rc = tftp(fnip, buffer, len, DEFAULT_TFTP_RETRIES, &tftp_err, 1, 1428,
+              ip_version);
 
     if (rc > 0) {
         printf("  TFTP: Received %s (%d KBytes)\n", fnip->filename,
@@ -199,20 +199,19 @@ static int tftp_load(filename_ip_t *fnip, void *buffer, int len,
     return rc;
 }
 
-static int net_load(char *buffer, int len)
+static int net_init(filename_ip_t *fn_ip)
 {
-    filename_ip_t fn_ip;
     uint8_t mac[6];
     int rc;
 
-    memset(&fn_ip, 0, sizeof(filename_ip_t));
+    memset(fn_ip, 0, sizeof(filename_ip_t));
 
     rc = virtio_net_init(mac);
     if (rc < 0) {
         puts("Could not initialize network device");
         return -101;
     }
-    fn_ip.fd = rc;
+    fn_ip->fd = rc;
 
     printf("  Using MAC address: %02x:%02x:%02x:%02x:%02x:%02x\n",
            mac[0], mac[1], mac[2], mac[3], mac[4], mac[5]);
@@ -220,10 +219,10 @@ static int net_load(char *buffer, int len)
     set_mac_address(mac);    /* init ethernet layer */
     seed_rng(mac);
 
-    rc = dhcp(&fn_ip, DEFAULT_BOOT_RETRIES);
+    rc = dhcp(fn_ip, DEFAULT_BOOT_RETRIES);
     if (rc >= 0) {
         if (ip_version == 4) {
-            set_ipv4_address(fn_ip.own_ip);
+            set_ipv4_address(fn_ip->own_ip);
         }
     } else {
         puts("Could not get IP address");
@@ -232,18 +231,18 @@ static int net_load(char *buffer, int len)
 
     if (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);
+              (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) {
         char ip6_str[40];
-        ipv6_to_str(fn_ip.own_ip6.addr, ip6_str);
+        ipv6_to_str(fn_ip->own_ip6.addr, ip6_str);
         printf("  Using IPv6 address: %s\n", ip6_str);
     }
 
     if (rc == -2) {
         printf("ARP request to TFTP server (%d.%d.%d.%d) failed\n",
-               (fn_ip.server_ip >> 24) & 0xFF, (fn_ip.server_ip >> 16) & 0xFF,
-               (fn_ip.server_ip >>  8) & 0xFF, fn_ip.server_ip & 0xFF);
+               (fn_ip->server_ip >> 24) & 0xFF, (fn_ip->server_ip >> 16) & 0xFF,
+               (fn_ip->server_ip >>  8) & 0xFF, fn_ip->server_ip & 0xFF);
         return -102;
     }
     if (rc == -4 || rc == -3) {
@@ -251,28 +250,31 @@ static int net_load(char *buffer, int len)
         return -107;
     }
 
+    printf("  Using TFTP server: ");
     if (ip_version == 4) {
-        printf("  Requesting file \"%s\" via TFTP from %d.%d.%d.%d\n",
-               fn_ip.filename,
-               (fn_ip.server_ip >> 24) & 0xFF, (fn_ip.server_ip >> 16) & 0xFF,
-               (fn_ip.server_ip >>  8) & 0xFF, fn_ip.server_ip & 0xFF);
+        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) {
         char ip6_str[40];
-        printf("  Requesting file \"%s\" via TFTP from ", fn_ip.filename);
-        ipv6_to_str(fn_ip.server_ip6.addr, ip6_str);
+        ipv6_to_str(fn_ip->server_ip6.addr, ip6_str);
         printf("%s\n", ip6_str);
     }
 
-    /* Do the TFTP load and print error message if necessary */
-    rc = tftp_load(&fn_ip, buffer, len, DEFAULT_TFTP_RETRIES, ip_version);
-
-    if (ip_version == 4) {
-        dhcp_send_release(fn_ip.fd);
+    if (strlen((char *)fn_ip->filename) > 0) {
+        printf("  Bootfile name: '%s'\n", fn_ip->filename);
     }
 
     return rc;
 }
 
+static void net_uninit(filename_ip_t *fn_ip)
+{
+    if (ip_version == 4) {
+        dhcp_send_release(fn_ip->fd);
+    }
+}
+
 void panic(const char *string)
 {
     sclp_print(string);
@@ -344,6 +346,7 @@ static void virtio_setup(void)
 
 void main(void)
 {
+    filename_ip_t fn_ip;
     int rc;
 
     sclp_setup();
@@ -351,7 +354,15 @@ void main(void)
 
     virtio_setup();
 
-    rc = net_load(NULL, (long)_start);
+    rc = net_init(&fn_ip);
+    if (rc) {
+        panic("Network initialization failed. Halting.\n");
+    }
+
+    rc = tftp_load(&fn_ip, NULL, (long)_start);
+
+    net_uninit(&fn_ip);
+
     if (rc > 0) {
         sclp_print("Network loading done, starting kernel...\n");
         asm volatile (" lpsw 0(%0) " : : "r"(0) : "memory");
-- 
1.8.3.1

[Qemu-devel] [PATCH v1 for-2.13 2/4] pc-bios/s390-ccw/net: Stop virtio-net device before jumping into the OS

[Thomas Huth]

The virtio-net receive buffers are filled asynchronously, so we should
make sure to properly shut down the virtio-net device before we jump into
the loaded kernel. Otherwise an incoming packet could destroy memory of
the OS kernel if it did not re-initialize the virtio-net device fast
enough yet.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 pc-bios/s390-ccw/netmain.c    |  1 +
 pc-bios/s390-ccw/virtio-net.c |  8 ++++++++
 pc-bios/s390-ccw/virtio.c     | 19 ++++++++++++++-----
 pc-bios/s390-ccw/virtio.h     |  3 +++
 4 files changed, 26 insertions(+), 5 deletions(-)

diff --git a/pc-bios/s390-ccw/netmain.c b/pc-bios/s390-ccw/netmain.c
index ebdc440..e11ed4e 100644
--- a/pc-bios/s390-ccw/netmain.c
+++ b/pc-bios/s390-ccw/netmain.c
@@ -273,6 +273,7 @@ static void net_uninit(filename_ip_t *fn_ip)
     if (ip_version == 4) {
         dhcp_send_release(fn_ip->fd);
     }
+    virtio_net_uninit();
 }
 
 void panic(const char *string)
diff --git a/pc-bios/s390-ccw/virtio-net.c b/pc-bios/s390-ccw/virtio-net.c
index ff7f4da..339fe53 100644
--- a/pc-bios/s390-ccw/virtio-net.c
+++ b/pc-bios/s390-ccw/virtio-net.c
@@ -67,6 +67,14 @@ int virtio_net_init(void *mac_addr)
     return 0;
 }
 
+void virtio_net_uninit(void)
+{
+    VDev *vdev = virtio_get_device();
+
+    virtio_status_set(vdev, VIRTIO_CONFIG_S_FAILED);
+    virtio_reset_dev(vdev);
+}
+
 int send(int fd, const void *buf, int len, int flags)
 {
     VirtioNetHdr tx_hdr;
diff --git a/pc-bios/s390-ccw/virtio.c b/pc-bios/s390-ccw/virtio.c
index cdb66f4..1d15b03 100644
--- a/pc-bios/s390-ccw/virtio.c
+++ b/pc-bios/s390-ccw/virtio.c
@@ -248,10 +248,21 @@ int virtio_run(VDev *vdev, int vqid, VirtioCmd *cmd)
     return 0;
 }
 
+void virtio_status_set(VDev *vdev, uint8_t status)
+{
+    IPL_assert(
+        run_ccw(vdev, CCW_CMD_WRITE_STATUS, &status, sizeof(status)) == 0,
+        "Could not write status to host");
+}
+
+void virtio_reset_dev(VDev *vdev)
+{
+    run_ccw(vdev, CCW_CMD_VDEV_RESET, NULL, 0);
+}
+
 void virtio_setup_ccw(VDev *vdev)
 {
     int i, rc, cfg_size = 0;
-    unsigned char status = VIRTIO_CONFIG_S_DRIVER_OK;
     struct VirtioFeatureDesc {
         uint32_t features;
         uint8_t index;
@@ -263,7 +274,7 @@ void virtio_setup_ccw(VDev *vdev)
     vdev->config.blk.blk_size = 0; /* mark "illegal" - setup started... */
     vdev->guessed_disk_nature = VIRTIO_GDN_NONE;
 
-    run_ccw(vdev, CCW_CMD_VDEV_RESET, NULL, 0);
+    virtio_reset_dev(vdev);
 
     switch (vdev->senseid.cu_model) {
     case VIRTIO_ID_NET:
@@ -320,9 +331,7 @@ void virtio_setup_ccw(VDev *vdev)
         IPL_assert(run_ccw(vdev, CCW_CMD_SET_VQ, &info, sizeof(info)) == 0,
                    "Cannot set VQ info");
     }
-    IPL_assert(
-        run_ccw(vdev, CCW_CMD_WRITE_STATUS, &status, sizeof(status)) == 0,
-        "Could not write status to host");
+    virtio_status_set(vdev, VIRTIO_CONFIG_S_DRIVER_OK);
 }
 
 bool virtio_is_supported(SubChannelId schid)
diff --git a/pc-bios/s390-ccw/virtio.h b/pc-bios/s390-ccw/virtio.h
index 19fceb6..c2479be 100644
--- a/pc-bios/s390-ccw/virtio.h
+++ b/pc-bios/s390-ccw/virtio.h
@@ -277,8 +277,11 @@ void vring_send_buf(VRing *vr, void *p, int len, int flags);
 int vr_poll(VRing *vr);
 int vring_wait_reply(void);
 int virtio_run(VDev *vdev, int vqid, VirtioCmd *cmd);
+void virtio_status_set(VDev *vdev, uint8_t status);
+void virtio_reset_dev(VDev *vdev);
 void virtio_setup_ccw(VDev *vdev);
 
 int virtio_net_init(void *mac_addr);
+void virtio_net_uninit(void);
 
 #endif /* VIRTIO_H */
-- 
1.8.3.1

[Qemu-devel] [PATCH v1 for-2.13 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.

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

diff --git a/pc-bios/s390-ccw/netboot.mak b/pc-bios/s390-ccw/netboot.mak
index a25d238..8db9573 100644
--- a/pc-bios/s390-ccw/netboot.mak
+++ b/pc-bios/s390-ccw/netboot.mak
@@ -24,8 +24,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 = strcasecmp.o strcat.o strchr.o strcmp.o strcpy.o strlen.o \
+	      strncasecmp.o strncmp.o strncpy.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)$@")
 
diff --git a/pc-bios/s390-ccw/netmain.c b/pc-bios/s390-ccw/netmain.c
index e11ed4e..fa62bfe 100644
--- a/pc-bios/s390-ccw/netmain.c
+++ b/pc-bios/s390-ccw/netmain.c
@@ -39,11 +39,17 @@
 
 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 int ip_version = 4;
+static uint8_t mac[6];
 static uint64_t dest_timer;
 
 static uint64_t get_timer_ms(void)
@@ -136,9 +142,15 @@ static int tftp_load(filename_ip_t *fnip, void *buffer, int len)
     rc = tftp(fnip, buffer, len, DEFAULT_TFTP_RETRIES, &tftp_err, 1, 1428,
               ip_version);
 
-    if (rc > 0) {
-        printf("  TFTP: Received %s (%d KBytes)\n", fnip->filename,
-               rc / 1024);
+    if (rc < 0) {
+        /* Make sure that error messages are put into a new line */
+        printf("\n  ");
+    }
+
+    if (rc > 1024) {
+        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) {
@@ -201,7 +213,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));
@@ -276,6 +287,183 @@ static void net_uninit(filename_ip_t *fn_ip)
     virtio_net_uninit();
 }
 
+/* This structure holds the data from one pxelinux.cfg file entry */
+struct lkia {
+    const char *label;
+    const char *kernel;
+    const char *initrd;
+    const char *append;
+};
+
+static int load_kernel_with_initrd(filename_ip_t *fn_ip, struct lkia *kia)
+{
+    int rc;
+
+    printf("Loading pxelinux.cfg entry '%s'\n", kia->label);
+
+    if (!kia->kernel) {
+        printf("Kernel entry is missing!\n");
+        return -1;
+    }
+
+    strncpy((char *)&fn_ip->filename, kia->kernel, sizeof(fn_ip->filename));
+    rc = tftp_load(fn_ip, KERNEL_ADDR, KERNEL_MAX_SIZE);
+    if (rc < 0) {
+        return rc;
+    }
+
+    if (kia->initrd) {
+        uint64_t iaddr = (rc + 0xfff) & ~0xfffUL;
+
+        strncpy((char *)&fn_ip->filename, kia->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 (kia->append) {
+        strncpy((char *)0x10480, kia->append, ARCH_COMMAND_LINE_SIZE);
+    }
+
+    return rc;
+}
+
+#define MAX_PXELINUX_ENTRIES 16
+
+/**
+ * Parse a pxelinux-style configuration file.
+ * See the following URL for more inforation about the config file syntax:
+ * https://www.syslinux.org/wiki/index.php?title=PXELINUX
+ */
+static int handle_pxelinux_cfg(filename_ip_t *fn_ip, char *cfg, int cfgsize)
+{
+    struct lkia entries[MAX_PXELINUX_ENTRIES];
+    int num_entries = 0;
+    char *ptr = cfg, *eol, *arg;
+    char *defaultlabel = NULL;
+    int def_ent = 0;
+
+    while (ptr < cfg + cfgsize && num_entries < MAX_PXELINUX_ENTRIES) {
+        eol = strchr(ptr, '\n');
+        if (!eol) {
+            eol = cfg + cfgsize;
+        }
+        if (eol > ptr && *(eol - 1) == '\r') {
+            *(eol - 1) = 0;
+        }
+        *eol = '\0';
+        while (*ptr == ' ' || *ptr == '\t') {
+            ptr++;
+        }
+        if (*ptr == 0 || *ptr == '#') {   /* Ignore comments and empty lines */
+            goto nextline;
+        }
+        arg = strchr(ptr, ' ');    /* Look for space between command and arg */
+        if (!arg) {
+            arg = strchr(ptr, '\t');
+        }
+        if (!arg) {
+            printf("Failed to parse the following line:\n %s\n", ptr);
+            goto nextline;
+        }
+        *arg++ = 0;
+        while (*arg == ' ' || *arg == '\t') {
+            arg++;
+        }
+        if (!strcasecmp("default", ptr)) {
+            defaultlabel = arg;
+        } else if (!strcasecmp("label", ptr)) {
+            entries[num_entries].label = arg;
+            if (defaultlabel && !strcmp(arg, defaultlabel)) {
+                def_ent = num_entries;
+            }
+            num_entries++;
+        } else if (!strcasecmp("kernel", ptr)) {
+            entries[num_entries - 1].kernel = arg;
+        } else if (!strcasecmp("initrd", ptr)) {
+            entries[num_entries - 1].initrd = arg;
+        } else if (!strcasecmp("append", ptr)) {
+            entries[num_entries - 1].append = arg;
+        } else {
+            printf("Command '%s' is not supported.\n", ptr);
+        }
+nextline:
+        ptr = eol + 1;
+    }
+
+    return load_kernel_with_initrd(fn_ip, &entries[def_ent]);
+}
+
+static int net_try_pxelinux_cfgs(filename_ip_t *fn_ip)
+{
+    int rc, idx;
+
+    cfgbuf[sizeof(cfgbuf) - 1] = 0;   /* Make sure that it is NUL-terminated */
+
+    printf("Trying pxelinux.cfg files...\n");
+
+    /* Look for config file with MAC address in its name */
+    sprintf((char *)fn_ip->filename,
+            "pxelinux.cfg/%02x-%02x-%02x-%02x-%02x-%02x",
+            mac[0], mac[1], mac[2], mac[3], mac[4], mac[5]);
+    rc = tftp_load(fn_ip, cfgbuf, sizeof(cfgbuf) - 1);
+    if (rc > 0) {
+        return handle_pxelinux_cfg(fn_ip, cfgbuf, sizeof(cfgbuf));
+    }
+
+    /* Look for config file with IP address in its name */
+    if (ip_version == 4) {
+        for (idx = 0; idx <= 7; idx++) {
+            sprintf((char *)fn_ip->filename,
+                    "pxelinux.cfg/%02X%02X%02X%02X",
+                    (fn_ip->own_ip >> 24) & 0xff, (fn_ip->own_ip >> 16) & 0xff,
+                    (fn_ip->own_ip >> 8) & 0xff, fn_ip->own_ip & 0xff);
+            fn_ip->filename[strlen((char *)fn_ip->filename) - idx] = 0;
+            rc = tftp_load(fn_ip, cfgbuf, sizeof(cfgbuf) - 1);
+            if (rc > 0) {
+                return handle_pxelinux_cfg(fn_ip, cfgbuf, sizeof(cfgbuf));
+            }
+        }
+    }
+
+    /* Try "default" config file */
+    strcpy((char *)fn_ip->filename, "pxelinux.cfg/default");
+    rc = tftp_load(fn_ip, cfgbuf, sizeof(cfgbuf) - 1);
+    if (rc > 0) {
+        return handle_pxelinux_cfg(fn_ip, cfgbuf, sizeof(cfgbuf));
+    }
+
+    return -1;
+}
+
+static int net_try_direct_tftp_load(filename_ip_t *fn_ip)
+{
+    int rc;
+    void *baseaddr = (void *)0x2000;  /* Load right after the low-core */
+
+    rc = tftp_load(fn_ip, baseaddr, KERNEL_MAX_SIZE - (long)baseaddr);
+
+    if (rc > 0 && rc < sizeof(cfgbuf) - 1) {
+        /* Check whether it is a configuration file instead of a kernel */
+        memcpy(cfgbuf, baseaddr, rc);
+        cfgbuf[rc] = 0;    /* Make sure that it is NUL-terminated */
+        if (!strncasecmp("default", cfgbuf, 7) || !strncmp("# ", cfgbuf, 2)) {
+            /* Looks like it is a pxelinux.cfg */
+            return handle_pxelinux_cfg(fn_ip, cfgbuf, rc);
+        }
+    }
+
+    /* Move kernel to right location */
+    memmove(KERNEL_ADDR, baseaddr, rc);
+
+    return rc;
+}
+
 void panic(const char *string)
 {
     sclp_print(string);
@@ -360,7 +548,12 @@ void main(void)
         panic("Network initialization failed. Halting.\n");
     }
 
-    rc = tftp_load(&fn_ip, NULL, (long)_start);
+    if (strlen((char *)fn_ip.filename) > 0) {
+        rc = net_try_direct_tftp_load(&fn_ip);
+    }
+    if (rc <= 0) {
+        rc = net_try_pxelinux_cfgs(&fn_ip);
+    }
 
     net_uninit(&fn_ip);
 
-- 
1.8.3.1

[Qemu-devel] [PATCH v1 for-2.13 4/4] pc-bios/s390-ccw/net: Add support for .INS config files

[Thomas Huth]

The .INS config files can normally be found on CD-ROM ISO images,
so by supporting these files, it is now possible to boot directly
when the TFTP server is set up with the contents of such an CD-ROM
image.

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

diff --git a/pc-bios/s390-ccw/netmain.c b/pc-bios/s390-ccw/netmain.c
index fa62bfe..e52e17d 100644
--- a/pc-bios/s390-ccw/netmain.c
+++ b/pc-bios/s390-ccw/netmain.c
@@ -441,6 +441,55 @@ static int net_try_pxelinux_cfgs(filename_ip_t *fn_ip)
     return -1;
 }
 
+/**
+ * Load via information from a .INS file (which can be found on CD-ROMs
+ * for example)
+ */
+static int handle_ins_cfg(filename_ip_t *fn_ip, char *cfg, int cfgsize)
+{
+    char *ptr;
+    int rc = -1, llen;
+    void *destaddr;
+    char *insbuf = cfg;
+
+    ptr = strchr(insbuf, '\n');
+    if (!ptr) {
+        puts("Does not seem to be a valid .INS file");
+        return -1;
+    }
+
+    *ptr = 0;
+    printf("\nParsing .INS file:\n %s\n", &insbuf[2]);
+
+    insbuf = ptr + 1;
+    while (*insbuf && insbuf < cfg + cfgsize) {
+        ptr = strchr(insbuf, '\n');
+        if (ptr) {
+            *ptr = 0;
+        }
+        llen = strlen(insbuf);
+        if (!llen) {
+            insbuf = ptr + 1;
+            continue;
+        }
+        ptr = strchr(insbuf, ' ');
+        if (!ptr) {
+            puts("Missing space separator in .INS file");
+            return -1;
+        }
+        *ptr = 0;
+        strncpy((char *)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) {
+            break;
+        }
+        insbuf += llen + 1;
+    }
+
+    return rc;
+}
+
 static int net_try_direct_tftp_load(filename_ip_t *fn_ip)
 {
     int rc;
@@ -455,6 +504,8 @@ static int net_try_direct_tftp_load(filename_ip_t *fn_ip)
         if (!strncasecmp("default", cfgbuf, 7) || !strncmp("# ", cfgbuf, 2)) {
             /* Looks like it is a pxelinux.cfg */
             return handle_pxelinux_cfg(fn_ip, cfgbuf, rc);
+        } else if (!strncmp("* ", cfgbuf, 2)) {
+            return handle_ins_cfg(fn_ip, cfgbuf, rc);
         }
     }
 
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v1 for-2.13 1/4] pc-bios/s390-ccw/net: Split up net_load() into init, load and uninit parts

[Farhan Ali]

On 04/18/2018 08:31 AM, Thomas Huth wrote:
> When we want to support pxelinux-style network booting later, we've got
> to do several TFTP transfers - and we do not want to apply for a new IP
> address via DHCP each time. So split up net_load into three parts:
> 
> 1. net_init(), which initializes virtio-net, gets an IP address via DHCP
>     and prints out the related information.
> 
> 2. The tftp_load call is now moved directly into the main() function
> 
> 3. A new net_uninit() function which should tear down the network stack
>     before we are done in the firmware.
> 
> This will make it easier to extend the code in the next patches.
> 
> Signed-off-by: Thomas Huth<thuth@redhat.com>


Just a minor nit, if we could rename *_uninit functions to 
destroy/release? I think it's just easier to read

Re: [Qemu-devel] [PATCH v1 for-2.13 0/4] pc-bios/s390-ccw: Network boot improvements

[Farhan Ali]

On 04/18/2018 08:31 AM, Thomas Huth wrote:
> Some patches to improve the network boot experience on s390x:
> 
> First, make sure that we shut down the virtio-net device before jumping
> into the kernel. Otherwise some incoming packets might destroy some of
> the kernel's data if it has not taken over the device yet.
> 
> Then the last two patches add support for loading kernels via
> configuration files - pxelinux-style and .INS-file style. This way
> you don't have to manually glue your ramdisk to your kernel anymore,
> so this should be quite a relieve for all users who want to boot
> Linux via the network.
> 
> The config file parsers have been completely written by myself from
> scratch and only tested with some config files that I came up with
> on my own. So if anybody has some pre-existing pxelinux config files
> already for booting a s390x, I'd appreciate some testing to see whether
> this works as expected for you, too!
> 
> Thomas Huth (4):
>    pc-bios/s390-ccw/net: Split up net_load() into init, load and uninit
>      parts
>    pc-bios/s390-ccw/net: Stop virtio-net device before jumping into the
>      OS
>    pc-bios/s390-ccw/net: Add support for pxelinux-style config files
>    pc-bios/s390-ccw/net: Add support for .INS config files
> 
>   pc-bios/s390-ccw/netboot.mak  |   5 +-
>   pc-bios/s390-ccw/netmain.c    | 312 ++++++++++++++++++++++++++++++++++++++----
>   pc-bios/s390-ccw/virtio-net.c |   8 ++
>   pc-bios/s390-ccw/virtio.c     |  19 ++-
>   pc-bios/s390-ccw/virtio.h     |   3 +
>   5 files changed, 312 insertions(+), 35 deletions(-)
> 


I have tried with pxelinux default config file I had and it worked fine. 
I will try a couple more tests.

Also while going through the code again, I noticed a memory leak in the 
function virtio_net_init, and fixed it. I could send a formal patch for 
it, if you want to queue it as part of this series as I think we might 
have missed 2.12 window?

diff --git a/pc-bios/s390-ccw/virtio-net.c b/pc-bios/s390-ccw/virtio-net.c
index ff7f4da..e83ba08 100644
--- a/pc-bios/s390-ccw/virtio-net.c
+++ b/pc-bios/s390-ccw/virtio-net.c
@@ -61,6 +61,7 @@ int virtio_net_init(void *mac_addr)
          IPL_assert(buf != NULL, "Can not allocate memory for receive 
buffers");
          vring_send_buf(rxvq, buf, ETH_MTU_SIZE + sizeof(VirtioNetHdr),
                         VRING_DESC_F_WRITE);
+        free(buf);
      }
      vring_notify(rxvq);

Re: [Qemu-devel] [qemu-s390x] [PATCH v1 for-2.13 1/4] pc-bios/s390-ccw/net: Split up net_load() into init, load and uninit parts

[Thomas Huth]

On 18.04.2018 20:11, Farhan Ali wrote:
> 
> 
> On 04/18/2018 08:31 AM, Thomas Huth wrote:
>> When we want to support pxelinux-style network booting later, we've got
>> to do several TFTP transfers - and we do not want to apply for a new IP
>> address via DHCP each time. So split up net_load into three parts:
>>
>> 1. net_init(), which initializes virtio-net, gets an IP address via DHCP
>>     and prints out the related information.
>>
>> 2. The tftp_load call is now moved directly into the main() function
>>
>> 3. A new net_uninit() function which should tear down the network stack
>>     before we are done in the firmware.
>>
>> This will make it easier to extend the code in the next patches.
>>
>> Signed-off-by: Thomas Huth<thuth@redhat.com>
> 
> 
> Just a minor nit, if we could rename *_uninit functions to
> destroy/release? I think it's just easier to read

Sure. Completely unrepresentative statistics from my QEMU repository:

$ grep -r uninit . | wc -l
212
$ grep -r destroy . | wc -l
904
$ grep -r release . | wc -l
1133

So I think I'll go with "release".

 Thomas

Re: [Qemu-devel] [qemu-s390x] [PATCH v1 for-2.13 0/4] pc-bios/s390-ccw: Network boot improvements

[Thomas Huth]

On 18.04.2018 20:21, Farhan Ali wrote:
> On 04/18/2018 08:31 AM, Thomas Huth wrote:
>> Some patches to improve the network boot experience on s390x:
>>
>> First, make sure that we shut down the virtio-net device before jumping
>> into the kernel. Otherwise some incoming packets might destroy some of
>> the kernel's data if it has not taken over the device yet.
>>
>> Then the last two patches add support for loading kernels via
>> configuration files - pxelinux-style and .INS-file style. This way
>> you don't have to manually glue your ramdisk to your kernel anymore,
>> so this should be quite a relieve for all users who want to boot
>> Linux via the network.
>>
>> The config file parsers have been completely written by myself from
>> scratch and only tested with some config files that I came up with
>> on my own. So if anybody has some pre-existing pxelinux config files
>> already for booting a s390x, I'd appreciate some testing to see whether
>> this works as expected for you, too!
[...]
>
> I have tried with pxelinux default config file I had and it worked fine.
> I will try a couple more tests.

Thanks!

> Also while going through the code again, I noticed a memory leak in the
> function virtio_net_init, and fixed it. I could send a formal patch for
> it, if you want to queue it as part of this series as I think we might
> have missed 2.12 window?
> 
> diff --git a/pc-bios/s390-ccw/virtio-net.c b/pc-bios/s390-ccw/virtio-net.c
> index ff7f4da..e83ba08 100644
> --- a/pc-bios/s390-ccw/virtio-net.c
> +++ b/pc-bios/s390-ccw/virtio-net.c
> @@ -61,6 +61,7 @@ int virtio_net_init(void *mac_addr)
>          IPL_assert(buf != NULL, "Can not allocate memory for receive
> buffers");
>          vring_send_buf(rxvq, buf, ETH_MTU_SIZE + sizeof(VirtioNetHdr),
>                         VRING_DESC_F_WRITE);
> +        free(buf);
>      }
>      vring_notify(rxvq);

Ah, that's not a memory leak, though it might look like one if you don't
know what the vring_send_buf function is doing: vring_send_buf adds a
buffer to a virtio ring for later use. So the buffer is not unused after
this function, but gets filled in later by the virtio-net device. Thus
we must not free the buffer here.

We could free it in the "uninit" function after shutting down the
device, but OTOH we are then leaving the firmware code anyway by jumping
into the OS kernel, so it does not really matter whether we free()ed the
buffers or not.

 Thomas

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

[Viktor VM Mihajlovski]

On 18.04.2018 14:31, Thomas Huth wrote:
> 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.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  pc-bios/s390-ccw/netboot.mak |   5 +-
>  pc-bios/s390-ccw/netmain.c   | 203 +++++++++++++++++++++++++++++++++++++++++--
>  2 files changed, 201 insertions(+), 7 deletions(-)
> 
> diff --git a/pc-bios/s390-ccw/netboot.mak b/pc-bios/s390-ccw/netboot.mak
> index a25d238..8db9573 100644
> --- a/pc-bios/s390-ccw/netboot.mak
> +++ b/pc-bios/s390-ccw/netboot.mak
> @@ -24,8 +24,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 = strcasecmp.o strcat.o strchr.o strcmp.o strcpy.o strlen.o \
> +	      strncasecmp.o strncmp.o strncpy.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)$@")
> 
> diff --git a/pc-bios/s390-ccw/netmain.c b/pc-bios/s390-ccw/netmain.c
> index e11ed4e..fa62bfe 100644
> --- a/pc-bios/s390-ccw/netmain.c
> +++ b/pc-bios/s390-ccw/netmain.c
> @@ -39,11 +39,17 @@
> 
>  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 int ip_version = 4;
> +static uint8_t mac[6];
>  static uint64_t dest_timer;
> 
>  static uint64_t get_timer_ms(void)
> @@ -136,9 +142,15 @@ static int tftp_load(filename_ip_t *fnip, void *buffer, int len)
>      rc = tftp(fnip, buffer, len, DEFAULT_TFTP_RETRIES, &tftp_err, 1, 1428,
>                ip_version);
> 
> -    if (rc > 0) {
> -        printf("  TFTP: Received %s (%d KBytes)\n", fnip->filename,
> -               rc / 1024);
> +    if (rc < 0) {
> +        /* Make sure that error messages are put into a new line */
> +        printf("\n  ");
> +    }
> +
> +    if (rc > 1024) {
> +        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) {
> @@ -201,7 +213,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));
> @@ -276,6 +287,183 @@ static void net_uninit(filename_ip_t *fn_ip)
>      virtio_net_uninit();
>  }
> 
> +/* This structure holds the data from one pxelinux.cfg file entry */
> +struct lkia {
> +    const char *label;
> +    const char *kernel;
> +    const char *initrd;
> +    const char *append;
> +};
> +
> +static int load_kernel_with_initrd(filename_ip_t *fn_ip, struct lkia *kia)
> +{
> +    int rc;
> +
> +    printf("Loading pxelinux.cfg entry '%s'\n", kia->label);
> +
> +    if (!kia->kernel) {
> +        printf("Kernel entry is missing!\n");
> +        return -1;
> +    }
> +
> +    strncpy((char *)&fn_ip->filename, kia->kernel, sizeof(fn_ip->filename));
> +    rc = tftp_load(fn_ip, KERNEL_ADDR, KERNEL_MAX_SIZE);
> +    if (rc < 0) {
> +        return rc;
> +    }
> +
> +    if (kia->initrd) {
> +        uint64_t iaddr = (rc + 0xfff) & ~0xfffUL;
> +
> +        strncpy((char *)&fn_ip->filename, kia->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 (kia->append) {
> +        strncpy((char *)0x10480, kia->append, ARCH_COMMAND_LINE_SIZE);
> +    }
> +
> +    return rc;
> +}
> +
> +#define MAX_PXELINUX_ENTRIES 16
> +
> +/**
> + * Parse a pxelinux-style configuration file.
> + * See the following URL for more inforation about the config file syntax:
> + * https://www.syslinux.org/wiki/index.php?title=PXELINUX
> + */
> +static int handle_pxelinux_cfg(filename_ip_t *fn_ip, char *cfg, int cfgsize)
> +{
> +    struct lkia entries[MAX_PXELINUX_ENTRIES];
> +    int num_entries = 0;
> +    char *ptr = cfg, *eol, *arg;
> +    char *defaultlabel = NULL;
> +    int def_ent = 0;
> +
> +    while (ptr < cfg + cfgsize && num_entries < MAX_PXELINUX_ENTRIES) {
> +        eol = strchr(ptr, '\n');
> +        if (!eol) {
> +            eol = cfg + cfgsize;
> +        }
> +        if (eol > ptr && *(eol - 1) == '\r') {
> +            *(eol - 1) = 0;
> +        }
> +        *eol = '\0';
> +        while (*ptr == ' ' || *ptr == '\t') {
> +            ptr++;
> +        }
> +        if (*ptr == 0 || *ptr == '#') {   /* Ignore comments and empty lines */
> +            goto nextline;
> +        }
> +        arg = strchr(ptr, ' ');    /* Look for space between command and arg */
> +        if (!arg) {
> +            arg = strchr(ptr, '\t');
> +        }
> +        if (!arg) {
> +            printf("Failed to parse the following line:\n %s\n", ptr);
> +            goto nextline;
> +        }
> +        *arg++ = 0;
> +        while (*arg == ' ' || *arg == '\t') {
> +            arg++;
> +        }
> +        if (!strcasecmp("default", ptr)) {
> +            defaultlabel = arg;
> +        } else if (!strcasecmp("label", ptr)) {
> +            entries[num_entries].label = arg;
> +            if (defaultlabel && !strcmp(arg, defaultlabel)) {
> +                def_ent = num_entries;
> +            }
> +            num_entries++;
> +        } else if (!strcasecmp("kernel", ptr)) {
> +            entries[num_entries - 1].kernel = arg;
> +        } else if (!strcasecmp("initrd", ptr)) {
> +            entries[num_entries - 1].initrd = arg;
> +        } else if (!strcasecmp("append", ptr)) {
> +            entries[num_entries - 1].append = arg;
> +        } else {
> +            printf("Command '%s' is not supported.\n", ptr);
> +        }
> +nextline:
> +        ptr = eol + 1;
> +    }
> +
> +    return load_kernel_with_initrd(fn_ip, &entries[def_ent]);
> +}
> +
> +static int net_try_pxelinux_cfgs(filename_ip_t *fn_ip)
> +{
> +    int rc, idx;
> +
> +    cfgbuf[sizeof(cfgbuf) - 1] = 0;   /* Make sure that it is NUL-terminated */
> +
> +    printf("Trying pxelinux.cfg files...\n");
> +
> +    /* Look for config file with MAC address in its name */
> +    sprintf((char *)fn_ip->filename,
> +            "pxelinux.cfg/%02x-%02x-%02x-%02x-%02x-%02x",
> +            mac[0], mac[1], mac[2], mac[3], mac[4], mac[5]);
> +    rc = tftp_load(fn_ip, cfgbuf, sizeof(cfgbuf) - 1);
> +    if (rc > 0) {
> +        return handle_pxelinux_cfg(fn_ip, cfgbuf, sizeof(cfgbuf));
> +    }
> +
> +    /* Look for config file with IP address in its name */
> +    if (ip_version == 4) {
> +        for (idx = 0; idx <= 7; idx++) {
> +            sprintf((char *)fn_ip->filename,
> +                    "pxelinux.cfg/%02X%02X%02X%02X",
> +                    (fn_ip->own_ip >> 24) & 0xff, (fn_ip->own_ip >> 16) & 0xff,
> +                    (fn_ip->own_ip >> 8) & 0xff, fn_ip->own_ip & 0xff);
> +            fn_ip->filename[strlen((char *)fn_ip->filename) - idx] = 0;
> +            rc = tftp_load(fn_ip, cfgbuf, sizeof(cfgbuf) - 1);
> +            if (rc > 0) {
> +                return handle_pxelinux_cfg(fn_ip, cfgbuf, sizeof(cfgbuf));
> +            }
> +        }
> +    }
> +
> +    /* Try "default" config file */
> +    strcpy((char *)fn_ip->filename, "pxelinux.cfg/default");
> +    rc = tftp_load(fn_ip, cfgbuf, sizeof(cfgbuf) - 1);
> +    if (rc > 0) {
> +        return handle_pxelinux_cfg(fn_ip, cfgbuf, sizeof(cfgbuf));
> +    }
> +
> +    return -1;
> +}
> +
> +static int net_try_direct_tftp_load(filename_ip_t *fn_ip)
> +{
> +    int rc;
> +    void *baseaddr = (void *)0x2000;  /* Load right after the low-core */
> +
> +    rc = tftp_load(fn_ip, baseaddr, KERNEL_MAX_SIZE - (long)baseaddr);
> +
> +    if (rc > 0 && rc < sizeof(cfgbuf) - 1) {
> +        /* Check whether it is a configuration file instead of a kernel */
That's interesting because treating the bootfile as pxe-ish config is
what DPM does. Which means that with this change the processor
architecture type 0x1f (Basic) will turn into a superset of 0x20
(Extended). I guess this should be documented somewhere(TM) to avoid
confusion.
> +        memcpy(cfgbuf, baseaddr, rc);
> +        cfgbuf[rc] = 0;    /* Make sure that it is NUL-terminated */
> +        if (!strncasecmp("default", cfgbuf, 7) || !strncmp("# ", cfgbuf, 2)) {
> +            /* Looks like it is a pxelinux.cfg */
> +            return handle_pxelinux_cfg(fn_ip, cfgbuf, rc);> +        }
> +    }
> +
> +    /* Move kernel to right location */
> +    memmove(KERNEL_ADDR, baseaddr, rc);
Move this into the if block above. If the tftp_load fails with rc < 0
bad things will happen...
> +
> +    return rc;
> +}
> +
>  void panic(const char *string)
>  {
>      sclp_print(string);
> @@ -360,7 +548,12 @@ void main(void)
>          panic("Network initialization failed. Halting.\n");
>      }
> 
> -    rc = tftp_load(&fn_ip, NULL, (long)_start);
> +    if (strlen((char *)fn_ip.filename) > 0) {
> +        rc = net_try_direct_tftp_load(&fn_ip);
> +    }
> +    if (rc <= 0) {
> +        rc = net_try_pxelinux_cfgs(&fn_ip);
> +    }
> 
>      net_uninit(&fn_ip);
> 


-- 
Regards,
  Viktor Mihajlovski

Re: [Qemu-devel] [PATCH v1 for-2.13 4/4] pc-bios/s390-ccw/net: Add support for .INS config files

[Viktor VM Mihajlovski]

On 18.04.2018 14:31, Thomas Huth wrote:
> The .INS config files can normally be found on CD-ROM ISO images,
> so by supporting these files, it is now possible to boot directly
> when the TFTP server is set up with the contents of such an CD-ROM
> image.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  pc-bios/s390-ccw/netmain.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 51 insertions(+)
> 
> diff --git a/pc-bios/s390-ccw/netmain.c b/pc-bios/s390-ccw/netmain.c
> index fa62bfe..e52e17d 100644
> --- a/pc-bios/s390-ccw/netmain.c
> +++ b/pc-bios/s390-ccw/netmain.c
> @@ -441,6 +441,55 @@ static int net_try_pxelinux_cfgs(filename_ip_t *fn_ip)
>      return -1;
>  }
> 
> +/**
> + * Load via information from a .INS file (which can be found on CD-ROMs
> + * for example)
> + */
> +static int handle_ins_cfg(filename_ip_t *fn_ip, char *cfg, int cfgsize)
> +{
> +    char *ptr;
> +    int rc = -1, llen;
> +    void *destaddr;
> +    char *insbuf = cfg;
> +
> +    ptr = strchr(insbuf, '\n');
> +    if (!ptr) {
> +        puts("Does not seem to be a valid .INS file");
> +        return -1;
> +    }
> +
> +    *ptr = 0;
> +    printf("\nParsing .INS file:\n %s\n", &insbuf[2]);
> +
> +    insbuf = ptr + 1;
> +    while (*insbuf && insbuf < cfg + cfgsize) {
> +        ptr = strchr(insbuf, '\n');
> +        if (ptr) {
> +            *ptr = 0;
> +        }
> +        llen = strlen(insbuf);
> +        if (!llen) {
> +            insbuf = ptr + 1;
> +            continue;
> +        }
> +        ptr = strchr(insbuf, ' ');
> +        if (!ptr) {
> +            puts("Missing space separator in .INS file");
> +            return -1;
> +        }
> +        *ptr = 0;
> +        strncpy((char *)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) {
> +            break;
> +        }
> +        insbuf += llen + 1;
> +    }
> +
> +    return rc;
> +}
> +
>  static int net_try_direct_tftp_load(filename_ip_t *fn_ip)
>  {
>      int rc;
> @@ -455,6 +504,8 @@ static int net_try_direct_tftp_load(filename_ip_t *fn_ip)
>          if (!strncasecmp("default", cfgbuf, 7) || !strncmp("# ", cfgbuf, 2)) {
>              /* Looks like it is a pxelinux.cfg */
>              return handle_pxelinux_cfg(fn_ip, cfgbuf, rc);
> +        } else if (!strncmp("* ", cfgbuf, 2)) {
> +            return handle_ins_cfg(fn_ip, cfgbuf, rc);
>          }
>      }
> 
You could consider to move the magic matching code into a helper
function, I could imagine that the detection might grow more complex
over time and then could clutter the try-load code.
I.e. something like

typedef enum {
	FT_UNKOWN,
	FT_PXECFG,
	FT_INSFILE,
} FileType;

static FileType probe_file_type(const char * cfgbuf, size_t cfgbuflen)
{
        ...
        if (!strncasecmp("default", cfgbuf, 7) || !strncmp("# ",
                          cfgbuf, 2)) {
            /* Looks like it is a pxelinux.cfg */
            return FT_PXECFG;
        } else if (!strncmp("* ", cfgbuf, 2)) {
            return FT_INSFILE;
        } else {
	    return FT_UNKOWN;
        }
}

...
  FileType ft = probe_file_type(cfgbuf, ...);
  switch (ft)
  {
     case FT_PXECFG:
       return handle_pxelinux_cfg(fn_ip, cfgbuf, rc);
       break;
     ...
     case FT_UNKNOWN:
       /* got to be a kernel */
       memmove(...);
       return rc;
  }
...



-- 
Regards,
  Viktor Mihajlovski

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

[Thomas Huth]

On 19.04.2018 09:41, Viktor VM Mihajlovski wrote:
> On 18.04.2018 14:31, Thomas Huth wrote:
>> 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.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>  pc-bios/s390-ccw/netboot.mak |   5 +-
>>  pc-bios/s390-ccw/netmain.c   | 203 +++++++++++++++++++++++++++++++++++++++++--
>>  2 files changed, 201 insertions(+), 7 deletions(-)
>>
>> diff --git a/pc-bios/s390-ccw/netboot.mak b/pc-bios/s390-ccw/netboot.mak
>> index a25d238..8db9573 100644
>> --- a/pc-bios/s390-ccw/netboot.mak
>> +++ b/pc-bios/s390-ccw/netboot.mak
>> @@ -24,8 +24,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 = strcasecmp.o strcat.o strchr.o strcmp.o strcpy.o strlen.o \
>> +	      strncasecmp.o strncmp.o strncpy.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)$@")
>>
>> diff --git a/pc-bios/s390-ccw/netmain.c b/pc-bios/s390-ccw/netmain.c
>> index e11ed4e..fa62bfe 100644
>> --- a/pc-bios/s390-ccw/netmain.c
>> +++ b/pc-bios/s390-ccw/netmain.c
>> @@ -39,11 +39,17 @@
>>
>>  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 int ip_version = 4;
>> +static uint8_t mac[6];
>>  static uint64_t dest_timer;
>>
>>  static uint64_t get_timer_ms(void)
>> @@ -136,9 +142,15 @@ static int tftp_load(filename_ip_t *fnip, void *buffer, int len)
>>      rc = tftp(fnip, buffer, len, DEFAULT_TFTP_RETRIES, &tftp_err, 1, 1428,
>>                ip_version);
>>
>> -    if (rc > 0) {
>> -        printf("  TFTP: Received %s (%d KBytes)\n", fnip->filename,
>> -               rc / 1024);
>> +    if (rc < 0) {
>> +        /* Make sure that error messages are put into a new line */
>> +        printf("\n  ");
>> +    }
>> +
>> +    if (rc > 1024) {
>> +        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) {
>> @@ -201,7 +213,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));
>> @@ -276,6 +287,183 @@ static void net_uninit(filename_ip_t *fn_ip)
>>      virtio_net_uninit();
>>  }
>>
>> +/* This structure holds the data from one pxelinux.cfg file entry */
>> +struct lkia {
>> +    const char *label;
>> +    const char *kernel;
>> +    const char *initrd;
>> +    const char *append;
>> +};
>> +
>> +static int load_kernel_with_initrd(filename_ip_t *fn_ip, struct lkia *kia)
>> +{
>> +    int rc;
>> +
>> +    printf("Loading pxelinux.cfg entry '%s'\n", kia->label);
>> +
>> +    if (!kia->kernel) {
>> +        printf("Kernel entry is missing!\n");
>> +        return -1;
>> +    }
>> +
>> +    strncpy((char *)&fn_ip->filename, kia->kernel, sizeof(fn_ip->filename));
>> +    rc = tftp_load(fn_ip, KERNEL_ADDR, KERNEL_MAX_SIZE);
>> +    if (rc < 0) {
>> +        return rc;
>> +    }
>> +
>> +    if (kia->initrd) {
>> +        uint64_t iaddr = (rc + 0xfff) & ~0xfffUL;
>> +
>> +        strncpy((char *)&fn_ip->filename, kia->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 (kia->append) {
>> +        strncpy((char *)0x10480, kia->append, ARCH_COMMAND_LINE_SIZE);
>> +    }
>> +
>> +    return rc;
>> +}
>> +
>> +#define MAX_PXELINUX_ENTRIES 16
>> +
>> +/**
>> + * Parse a pxelinux-style configuration file.
>> + * See the following URL for more inforation about the config file syntax:
>> + * https://www.syslinux.org/wiki/index.php?title=PXELINUX
>> + */
>> +static int handle_pxelinux_cfg(filename_ip_t *fn_ip, char *cfg, int cfgsize)
>> +{
>> +    struct lkia entries[MAX_PXELINUX_ENTRIES];
>> +    int num_entries = 0;
>> +    char *ptr = cfg, *eol, *arg;
>> +    char *defaultlabel = NULL;
>> +    int def_ent = 0;
>> +
>> +    while (ptr < cfg + cfgsize && num_entries < MAX_PXELINUX_ENTRIES) {
>> +        eol = strchr(ptr, '\n');
>> +        if (!eol) {
>> +            eol = cfg + cfgsize;
>> +        }
>> +        if (eol > ptr && *(eol - 1) == '\r') {
>> +            *(eol - 1) = 0;
>> +        }
>> +        *eol = '\0';
>> +        while (*ptr == ' ' || *ptr == '\t') {
>> +            ptr++;
>> +        }
>> +        if (*ptr == 0 || *ptr == '#') {   /* Ignore comments and empty lines */
>> +            goto nextline;
>> +        }
>> +        arg = strchr(ptr, ' ');    /* Look for space between command and arg */
>> +        if (!arg) {
>> +            arg = strchr(ptr, '\t');
>> +        }
>> +        if (!arg) {
>> +            printf("Failed to parse the following line:\n %s\n", ptr);
>> +            goto nextline;
>> +        }
>> +        *arg++ = 0;
>> +        while (*arg == ' ' || *arg == '\t') {
>> +            arg++;
>> +        }
>> +        if (!strcasecmp("default", ptr)) {
>> +            defaultlabel = arg;
>> +        } else if (!strcasecmp("label", ptr)) {
>> +            entries[num_entries].label = arg;
>> +            if (defaultlabel && !strcmp(arg, defaultlabel)) {
>> +                def_ent = num_entries;
>> +            }
>> +            num_entries++;
>> +        } else if (!strcasecmp("kernel", ptr)) {
>> +            entries[num_entries - 1].kernel = arg;
>> +        } else if (!strcasecmp("initrd", ptr)) {
>> +            entries[num_entries - 1].initrd = arg;
>> +        } else if (!strcasecmp("append", ptr)) {
>> +            entries[num_entries - 1].append = arg;
>> +        } else {
>> +            printf("Command '%s' is not supported.\n", ptr);
>> +        }
>> +nextline:
>> +        ptr = eol + 1;
>> +    }
>> +
>> +    return load_kernel_with_initrd(fn_ip, &entries[def_ent]);
>> +}
>> +
>> +static int net_try_pxelinux_cfgs(filename_ip_t *fn_ip)
>> +{
>> +    int rc, idx;
>> +
>> +    cfgbuf[sizeof(cfgbuf) - 1] = 0;   /* Make sure that it is NUL-terminated */
>> +
>> +    printf("Trying pxelinux.cfg files...\n");
>> +
>> +    /* Look for config file with MAC address in its name */
>> +    sprintf((char *)fn_ip->filename,
>> +            "pxelinux.cfg/%02x-%02x-%02x-%02x-%02x-%02x",
>> +            mac[0], mac[1], mac[2], mac[3], mac[4], mac[5]);
>> +    rc = tftp_load(fn_ip, cfgbuf, sizeof(cfgbuf) - 1);
>> +    if (rc > 0) {
>> +        return handle_pxelinux_cfg(fn_ip, cfgbuf, sizeof(cfgbuf));
>> +    }
>> +
>> +    /* Look for config file with IP address in its name */
>> +    if (ip_version == 4) {
>> +        for (idx = 0; idx <= 7; idx++) {
>> +            sprintf((char *)fn_ip->filename,
>> +                    "pxelinux.cfg/%02X%02X%02X%02X",
>> +                    (fn_ip->own_ip >> 24) & 0xff, (fn_ip->own_ip >> 16) & 0xff,
>> +                    (fn_ip->own_ip >> 8) & 0xff, fn_ip->own_ip & 0xff);
>> +            fn_ip->filename[strlen((char *)fn_ip->filename) - idx] = 0;
>> +            rc = tftp_load(fn_ip, cfgbuf, sizeof(cfgbuf) - 1);
>> +            if (rc > 0) {
>> +                return handle_pxelinux_cfg(fn_ip, cfgbuf, sizeof(cfgbuf));
>> +            }
>> +        }
>> +    }
>> +
>> +    /* Try "default" config file */
>> +    strcpy((char *)fn_ip->filename, "pxelinux.cfg/default");
>> +    rc = tftp_load(fn_ip, cfgbuf, sizeof(cfgbuf) - 1);
>> +    if (rc > 0) {
>> +        return handle_pxelinux_cfg(fn_ip, cfgbuf, sizeof(cfgbuf));
>> +    }
>> +
>> +    return -1;
>> +}
>> +
>> +static int net_try_direct_tftp_load(filename_ip_t *fn_ip)
>> +{
>> +    int rc;
>> +    void *baseaddr = (void *)0x2000;  /* Load right after the low-core */
>> +
>> +    rc = tftp_load(fn_ip, baseaddr, KERNEL_MAX_SIZE - (long)baseaddr);
>> +
>> +    if (rc > 0 && rc < sizeof(cfgbuf) - 1) {
>> +        /* Check whether it is a configuration file instead of a kernel */
> That's interesting because treating the bootfile as pxe-ish config is
> what DPM does. Which means that with this change the processor
> architecture type 0x1f (Basic) will turn into a superset of 0x20
> (Extended).

Is there any reference available what "basic" and "extended" exactly
mean? I just know that there are these two values registered by you at
the IANA:

https://www.iana.org/assignments/dhcpv6-parameters/dhcpv6-parameters.xhtml#processor-architecture

... but I haven't seen a description of the differences of these two
values yet.

> I guess this should be documented somewhere(TM) to avoid
> confusion.

I plan to update https://wiki.qemu.org/Features/S390xNetworkBoot once
the patches have been merged upstream.

>> +        memcpy(cfgbuf, baseaddr, rc);
>> +        cfgbuf[rc] = 0;    /* Make sure that it is NUL-terminated */
>> +        if (!strncasecmp("default", cfgbuf, 7) || !strncmp("# ", cfgbuf, 2)) {
>> +            /* Looks like it is a pxelinux.cfg */
>> +            return handle_pxelinux_cfg(fn_ip, cfgbuf, rc);> +        }
>> +    }
>> +
>> +    /* Move kernel to right location */
>> +    memmove(KERNEL_ADDR, baseaddr, rc);
> Move this into the if block above. If the tftp_load fails with rc < 0
> bad things will happen...

Oops, good catch, this should of course only be done if rc > 0 ... will
fix it in v2.

 Thomas

Re: [Qemu-devel] [PATCH v1 for-2.13 4/4] pc-bios/s390-ccw/net: Add support for .INS config files

[Thomas Huth]

On 19.04.2018 10:02, Viktor VM Mihajlovski wrote:
> On 18.04.2018 14:31, Thomas Huth wrote:
>> The .INS config files can normally be found on CD-ROM ISO images,
>> so by supporting these files, it is now possible to boot directly
>> when the TFTP server is set up with the contents of such an CD-ROM
>> image.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>  pc-bios/s390-ccw/netmain.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 51 insertions(+)
>>
>> diff --git a/pc-bios/s390-ccw/netmain.c b/pc-bios/s390-ccw/netmain.c
>> index fa62bfe..e52e17d 100644
>> --- a/pc-bios/s390-ccw/netmain.c
>> +++ b/pc-bios/s390-ccw/netmain.c
>> @@ -441,6 +441,55 @@ static int net_try_pxelinux_cfgs(filename_ip_t *fn_ip)
>>      return -1;
>>  }
>>
>> +/**
>> + * Load via information from a .INS file (which can be found on CD-ROMs
>> + * for example)
>> + */
>> +static int handle_ins_cfg(filename_ip_t *fn_ip, char *cfg, int cfgsize)
>> +{
>> +    char *ptr;
>> +    int rc = -1, llen;
>> +    void *destaddr;
>> +    char *insbuf = cfg;
>> +
>> +    ptr = strchr(insbuf, '\n');
>> +    if (!ptr) {
>> +        puts("Does not seem to be a valid .INS file");
>> +        return -1;
>> +    }
>> +
>> +    *ptr = 0;
>> +    printf("\nParsing .INS file:\n %s\n", &insbuf[2]);
>> +
>> +    insbuf = ptr + 1;
>> +    while (*insbuf && insbuf < cfg + cfgsize) {
>> +        ptr = strchr(insbuf, '\n');
>> +        if (ptr) {
>> +            *ptr = 0;
>> +        }
>> +        llen = strlen(insbuf);
>> +        if (!llen) {
>> +            insbuf = ptr + 1;
>> +            continue;
>> +        }
>> +        ptr = strchr(insbuf, ' ');
>> +        if (!ptr) {
>> +            puts("Missing space separator in .INS file");
>> +            return -1;
>> +        }
>> +        *ptr = 0;
>> +        strncpy((char *)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) {
>> +            break;
>> +        }
>> +        insbuf += llen + 1;
>> +    }
>> +
>> +    return rc;
>> +}
>> +
>>  static int net_try_direct_tftp_load(filename_ip_t *fn_ip)
>>  {
>>      int rc;
>> @@ -455,6 +504,8 @@ static int net_try_direct_tftp_load(filename_ip_t *fn_ip)
>>          if (!strncasecmp("default", cfgbuf, 7) || !strncmp("# ", cfgbuf, 2)) {
>>              /* Looks like it is a pxelinux.cfg */
>>              return handle_pxelinux_cfg(fn_ip, cfgbuf, rc);
>> +        } else if (!strncmp("* ", cfgbuf, 2)) {
>> +            return handle_ins_cfg(fn_ip, cfgbuf, rc);
>>          }
>>      }
>>
> You could consider to move the magic matching code into a helper
> function, I could imagine that the detection might grow more complex
> over time and then could clutter the try-load code.
> I.e. something like
> 
> typedef enum {
> 	FT_UNKOWN,
> 	FT_PXECFG,
> 	FT_INSFILE,
> } FileType;
> 
> static FileType probe_file_type(const char * cfgbuf, size_t cfgbuflen)
> {
>         ...
>         if (!strncasecmp("default", cfgbuf, 7) || !strncmp("# ",
>                           cfgbuf, 2)) {
>             /* Looks like it is a pxelinux.cfg */
>             return FT_PXECFG;
>         } else if (!strncmp("* ", cfgbuf, 2)) {
>             return FT_INSFILE;
>         } else {
> 	    return FT_UNKOWN;
>         }
> }
> 
> ...
>   FileType ft = probe_file_type(cfgbuf, ...);
>   switch (ft)
>   {
>      case FT_PXECFG:
>        return handle_pxelinux_cfg(fn_ip, cfgbuf, rc);
>        break;
>      ...
>      case FT_UNKNOWN:
>        /* got to be a kernel */
>        memmove(...);
>        return rc;
>   }
> ...

I agree that this might be a good refactoring if the list grows bigger,
but as long as we only have these few entries here, it looks a little
bit over-engineered to me, so I'd rather prefer to keep the code in its
current shape right now.

 Thomas

Re: [Qemu-devel] [qemu-s390x] [PATCH v1 for-2.13 0/4] pc-bios/s390-ccw: Network boot improvements

[Farhan Ali]

On 04/19/2018 01:27 AM, Thomas Huth wrote:
> Ah, that's not a memory leak, though it might look like one if you don't
> know what the vring_send_buf function is doing: vring_send_buf adds a
> buffer to a virtio ring for later use. So the buffer is not unused after
> this function, but gets filled in later by the virtio-net device. Thus
> we must not free the buffer here.
> 
> We could free it in the "uninit" function after shutting down the
> device, but OTOH we are then leaving the firmware code anyway by jumping
> into the OS kernel, so it does not really matter whether we free()ed the
> buffers or not.
> 
>   Thomas

That makes a lot of sense now :) thanks a lot for that explanation and 
sorry for my ignorance.

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

[Viktor VM Mihajlovski]

On 19.04.2018 10:17, Thomas Huth wrote:
> On 19.04.2018 09:41, Viktor VM Mihajlovski wrote:
>> On 18.04.2018 14:31, Thomas Huth wrote:
>>> 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.
[...]
>>> +static int net_try_direct_tftp_load(filename_ip_t *fn_ip)
>>> +{
>>> +    int rc;
>>> +    void *baseaddr = (void *)0x2000;  /* Load right after the low-core */
>>> +
>>> +    rc = tftp_load(fn_ip, baseaddr, KERNEL_MAX_SIZE - (long)baseaddr);
>>> +
>>> +    if (rc > 0 && rc < sizeof(cfgbuf) - 1) {
>>> +        /* Check whether it is a configuration file instead of a kernel */
>> That's interesting because treating the bootfile as pxe-ish config is
>> what DPM does. Which means that with this change the processor
>> architecture type 0x1f (Basic) will turn into a superset of 0x20
>> (Extended).
> 
> Is there any reference available what "basic" and "extended" exactly
> mean? I just know that there are these two values registered by you at
> the IANA:
> 
> https://www.iana.org/assignments/dhcpv6-parameters/dhcpv6-parameters.xhtml#processor-architecture
> 
> ... but I haven't seen a description of the differences of these two
> values yet.
> 
Yep, the IANA stuff is pretty terse for all architectures. In a
nutshell: 0x1f is standard DHCP-based boot (bootfile is a single
executable binary) and 0x20 serves a pxe-style config file instead.
In fact, "processor architecture identifier" is a misnomer, as it
describes a pair of processor architecture and firmware.
Since the QEMU firmware has two significant extensions now (insfile and
direct config file loading), it might be worthwhile to add a new
identifier value (say s390 QEMU PC-BIOS or similar) announcing the new
capabilities, so that a boot server admin can setup her system accordingly.
>> I guess this should be documented somewhere(TM) to avoid
>> confusion.
> 
> I plan to update https://wiki.qemu.org/Features/S390xNetworkBoot once
> the patches have been merged upstream.
> 
sounds good.
[...]

-- 
Regards,
  Viktor Mihajlovski

Re: [Qemu-devel] [PATCH v1 for-2.13 2/4] pc-bios/s390-ccw/net: Stop virtio-net device before jumping into the OS

[Christian Borntraeger]

On 04/18/2018 02:31 PM, Thomas Huth wrote:
> The virtio-net receive buffers are filled asynchronously, so we should
> make sure to properly shut down the virtio-net device before we jump into
> the loaded kernel. Otherwise an incoming packet could destroy memory of
> the OS kernel if it did not re-initialize the virtio-net device fast
> enough yet.
> 

The normal bios does a full subsystem reset before we enter the OS.
(see jump_to_IPL_code the diag 308). That should reset all virtio 
devices on the qemu level. Shouldnt we rather do the same for
the net bios?



> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  pc-bios/s390-ccw/netmain.c    |  1 +
>  pc-bios/s390-ccw/virtio-net.c |  8 ++++++++
>  pc-bios/s390-ccw/virtio.c     | 19 ++++++++++++++-----
>  pc-bios/s390-ccw/virtio.h     |  3 +++
>  4 files changed, 26 insertions(+), 5 deletions(-)
> 
> diff --git a/pc-bios/s390-ccw/netmain.c b/pc-bios/s390-ccw/netmain.c
> index ebdc440..e11ed4e 100644
> --- a/pc-bios/s390-ccw/netmain.c
> +++ b/pc-bios/s390-ccw/netmain.c
> @@ -273,6 +273,7 @@ static void net_uninit(filename_ip_t *fn_ip)
>      if (ip_version == 4) {
>          dhcp_send_release(fn_ip->fd);
>      }
> +    virtio_net_uninit();
>  }
> 
>  void panic(const char *string)
> diff --git a/pc-bios/s390-ccw/virtio-net.c b/pc-bios/s390-ccw/virtio-net.c
> index ff7f4da..339fe53 100644
> --- a/pc-bios/s390-ccw/virtio-net.c
> +++ b/pc-bios/s390-ccw/virtio-net.c
> @@ -67,6 +67,14 @@ int virtio_net_init(void *mac_addr)
>      return 0;
>  }
> 
> +void virtio_net_uninit(void)
> +{
> +    VDev *vdev = virtio_get_device();
> +
> +    virtio_status_set(vdev, VIRTIO_CONFIG_S_FAILED);
> +    virtio_reset_dev(vdev);
> +}
> +
>  int send(int fd, const void *buf, int len, int flags)
>  {
>      VirtioNetHdr tx_hdr;
> diff --git a/pc-bios/s390-ccw/virtio.c b/pc-bios/s390-ccw/virtio.c
> index cdb66f4..1d15b03 100644
> --- a/pc-bios/s390-ccw/virtio.c
> +++ b/pc-bios/s390-ccw/virtio.c
> @@ -248,10 +248,21 @@ int virtio_run(VDev *vdev, int vqid, VirtioCmd *cmd)
>      return 0;
>  }
> 
> +void virtio_status_set(VDev *vdev, uint8_t status)
> +{
> +    IPL_assert(
> +        run_ccw(vdev, CCW_CMD_WRITE_STATUS, &status, sizeof(status)) == 0,
> +        "Could not write status to host");
> +}
> +
> +void virtio_reset_dev(VDev *vdev)
> +{
> +    run_ccw(vdev, CCW_CMD_VDEV_RESET, NULL, 0);
> +}
> +
>  void virtio_setup_ccw(VDev *vdev)
>  {
>      int i, rc, cfg_size = 0;
> -    unsigned char status = VIRTIO_CONFIG_S_DRIVER_OK;
>      struct VirtioFeatureDesc {
>          uint32_t features;
>          uint8_t index;
> @@ -263,7 +274,7 @@ void virtio_setup_ccw(VDev *vdev)
>      vdev->config.blk.blk_size = 0; /* mark "illegal" - setup started... */
>      vdev->guessed_disk_nature = VIRTIO_GDN_NONE;
> 
> -    run_ccw(vdev, CCW_CMD_VDEV_RESET, NULL, 0);
> +    virtio_reset_dev(vdev);
> 
>      switch (vdev->senseid.cu_model) {
>      case VIRTIO_ID_NET:
> @@ -320,9 +331,7 @@ void virtio_setup_ccw(VDev *vdev)
>          IPL_assert(run_ccw(vdev, CCW_CMD_SET_VQ, &info, sizeof(info)) == 0,
>                     "Cannot set VQ info");
>      }
> -    IPL_assert(
> -        run_ccw(vdev, CCW_CMD_WRITE_STATUS, &status, sizeof(status)) == 0,
> -        "Could not write status to host");
> +    virtio_status_set(vdev, VIRTIO_CONFIG_S_DRIVER_OK);
>  }
> 
>  bool virtio_is_supported(SubChannelId schid)
> diff --git a/pc-bios/s390-ccw/virtio.h b/pc-bios/s390-ccw/virtio.h
> index 19fceb6..c2479be 100644
> --- a/pc-bios/s390-ccw/virtio.h
> +++ b/pc-bios/s390-ccw/virtio.h
> @@ -277,8 +277,11 @@ void vring_send_buf(VRing *vr, void *p, int len, int flags);
>  int vr_poll(VRing *vr);
>  int vring_wait_reply(void);
>  int virtio_run(VDev *vdev, int vqid, VirtioCmd *cmd);
> +void virtio_status_set(VDev *vdev, uint8_t status);
> +void virtio_reset_dev(VDev *vdev);
>  void virtio_setup_ccw(VDev *vdev);
> 
>  int virtio_net_init(void *mac_addr);
> +void virtio_net_uninit(void);
> 
>  #endif /* VIRTIO_H */
> 

Re: [Qemu-devel] [qemu-s390x] [PATCH v1 for-2.13 3/4] pc-bios/s390-ccw/net: Add support for pxelinux-style config files

[Thomas Huth]

On 19.04.2018 14:40, Viktor VM Mihajlovski wrote:
> On 19.04.2018 10:17, Thomas Huth wrote:
>> On 19.04.2018 09:41, Viktor VM Mihajlovski wrote:
>>> On 18.04.2018 14:31, Thomas Huth wrote:
>>>> 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.
> [...]
>>>> +static int net_try_direct_tftp_load(filename_ip_t *fn_ip)
>>>> +{
>>>> +    int rc;
>>>> +    void *baseaddr = (void *)0x2000;  /* Load right after the low-core */
>>>> +
>>>> +    rc = tftp_load(fn_ip, baseaddr, KERNEL_MAX_SIZE - (long)baseaddr);
>>>> +
>>>> +    if (rc > 0 && rc < sizeof(cfgbuf) - 1) {
>>>> +        /* Check whether it is a configuration file instead of a kernel */
>>> That's interesting because treating the bootfile as pxe-ish config is
>>> what DPM does. Which means that with this change the processor
>>> architecture type 0x1f (Basic) will turn into a superset of 0x20
>>> (Extended).
>>
>> Is there any reference available what "basic" and "extended" exactly
>> mean? I just know that there are these two values registered by you at
>> the IANA:
>>
>> https://www.iana.org/assignments/dhcpv6-parameters/dhcpv6-parameters.xhtml#processor-architecture
>>
>> ... but I haven't seen a description of the differences of these two
>> values yet.
>>
> Yep, the IANA stuff is pretty terse for all architectures. In a
> nutshell: 0x1f is standard DHCP-based boot (bootfile is a single
> executable binary) and 0x20 serves a pxe-style config file instead.

Can 0x20 also still handle binary files, or only pxe-style config files?

> In fact, "processor architecture identifier" is a misnomer, as it
> describes a pair of processor architecture and firmware.

Yeah, I know ... it's even worse nowadays, it's processor + firmware +
TCP/IP protocol (HTTP vs. TFTP) ... we should have properly untangled
that when doing RFC 5970, but it was already hard enough to get that
accepted in the current shape, so we missed to clean up the mess from
RFC 4578 / PXE :-(

> Since the QEMU firmware has two significant extensions now (insfile and
> direct config file loading), it might be worthwhile to add a new
> identifier value (say s390 QEMU PC-BIOS or similar) announcing the new
> capabilities, so that a boot server admin can setup her system accordingly.

Not sure ... too many processory-architecture-type entries might also
rather be too confusing for the users? ... so if 0x20 can also handle
binary files like 0x1f, I'd maybe simply go with 0x20 and mainly promote
pxelinux config files in the documentation?

 Thomas

Re: [Qemu-devel] [PATCH v1 for-2.13 2/4] pc-bios/s390-ccw/net: Stop virtio-net device before jumping into the OS

[Thomas Huth]

On 19.04.2018 17:49, Christian Borntraeger wrote:
> On 04/18/2018 02:31 PM, Thomas Huth wrote:
>> The virtio-net receive buffers are filled asynchronously, so we should
>> make sure to properly shut down the virtio-net device before we jump into
>> the loaded kernel. Otherwise an incoming packet could destroy memory of
>> the OS kernel if it did not re-initialize the virtio-net device fast
>> enough yet.
> 
> The normal bios does a full subsystem reset before we enter the OS.
> (see jump_to_IPL_code the diag 308). That should reset all virtio 
> devices on the qemu level. Shouldnt we rather do the same for
> the net bios?

That should fix this issue, too, right. I'll give it a try and include
it in v2 if I can make it work...

 Thomas

Re: [Qemu-devel] [qemu-s390x] [PATCH v1 for-2.13 3/4] pc-bios/s390-ccw/net: Add support for pxelinux-style config files

[Viktor VM Mihajlovski]

On 19.04.2018 18:55, Thomas Huth wrote:
> On 19.04.2018 14:40, Viktor VM Mihajlovski wrote:
>> On 19.04.2018 10:17, Thomas Huth wrote:
>>> On 19.04.2018 09:41, Viktor VM Mihajlovski wrote:
>>>> On 18.04.2018 14:31, Thomas Huth wrote:
>>>>> 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.
>> [...]
>>>>> +static int net_try_direct_tftp_load(filename_ip_t *fn_ip)
>>>>> +{
>>>>> +    int rc;
>>>>> +    void *baseaddr = (void *)0x2000;  /* Load right after the low-core */
>>>>> +
>>>>> +    rc = tftp_load(fn_ip, baseaddr, KERNEL_MAX_SIZE - (long)baseaddr);
>>>>> +
>>>>> +    if (rc > 0 && rc < sizeof(cfgbuf) - 1) {
>>>>> +        /* Check whether it is a configuration file instead of a kernel */
>>>> That's interesting because treating the bootfile as pxe-ish config is
>>>> what DPM does. Which means that with this change the processor
>>>> architecture type 0x1f (Basic) will turn into a superset of 0x20
>>>> (Extended).
>>>
>>> Is there any reference available what "basic" and "extended" exactly
>>> mean? I just know that there are these two values registered by you at
>>> the IANA:
>>>
>>> https://www.iana.org/assignments/dhcpv6-parameters/dhcpv6-parameters.xhtml#processor-architecture
>>>
>>> ... but I haven't seen a description of the differences of these two
>>> values yet.
>>>
>> Yep, the IANA stuff is pretty terse for all architectures. In a
>> nutshell: 0x1f is standard DHCP-based boot (bootfile is a single
>> executable binary) and 0x20 serves a pxe-style config file instead.
> 
> Can 0x20 also still handle binary files, or only pxe-style config files?
> 
Unfortunately, DPM has no support for binaries which was the reason we
ended up with two different identifiers
>> In fact, "processor architecture identifier" is a misnomer, as it
>> describes a pair of processor architecture and firmware.
> 
> Yeah, I know ... it's even worse nowadays, it's processor + firmware +
> TCP/IP protocol (HTTP vs. TFTP) ... we should have properly untangled
> that when doing RFC 5970, but it was already hard enough to get that
> accepted in the current shape, so we missed to clean up the mess from
> RFC 4578 / PXE :-(
> 
>> Since the QEMU firmware has two significant extensions now (insfile and
>> direct config file loading), it might be worthwhile to add a new
>> identifier value (say s390 QEMU PC-BIOS or similar) announcing the new
>> capabilities, so that a boot server admin can setup her system accordingly.
> 
> Not sure ... too many processory-architecture-type entries might also
> rather be too confusing for the users? ... so if 0x20 can also handle
> binary files like 0x1f, I'd maybe simply go with 0x20 and mainly promote
> pxelinux config files in the documentation?
> 
Well, if we don't add another identifier, and I feel reluctant about it
as well for the reasons you wrote, we may stick with 0x1f and consider
the additional capabilities as being optional (for special needs only).
Remember that a client-specific pxe-style config or insfile have to be
tied to a client identifier that can be referenced in the DHCP server
configuration (usually the MAC address). I.e., if MAC equals so-and-so
send back bootfile name xyz.
But this is exactly what the pxelinux config file probing offers: having
client (or client-group) specific configs without the need to
reconfigure the DHCP server for every new (virtual) machine. My
assumption is that server admins will go with the pxelinux config
approach they're used from other architectures (which becomes now even
easier with your changes).
>  Thomas
> 


-- 
Regards,
  Viktor Mihajlovski

Re: [Qemu-devel] [PATCH v1 for-2.13 2/4] pc-bios/s390-ccw/net: Stop virtio-net device before jumping into the OS

[Christian Borntraeger]

On 04/20/2018 08:31 AM, Thomas Huth wrote:
> On 19.04.2018 17:49, Christian Borntraeger wrote:
>> On 04/18/2018 02:31 PM, Thomas Huth wrote:
>>> The virtio-net receive buffers are filled asynchronously, so we should
>>> make sure to properly shut down the virtio-net device before we jump into
>>> the loaded kernel. Otherwise an incoming packet could destroy memory of
>>> the OS kernel if it did not re-initialize the virtio-net device fast
>>> enough yet.
>>
>> The normal bios does a full subsystem reset before we enter the OS.
>> (see jump_to_IPL_code the diag 308). That should reset all virtio 
>> devices on the qemu level. Shouldnt we rather do the same for
>> the net bios?
> 
> That should fix this issue, too, right. I'll give it a try and include
> it in v2 if I can make it work...

The advantage is that it also put the sclp console into a default state.
> 
>  Thomas
> 

Re: [Qemu-devel] [qemu-s390x] [PATCH v1 for-2.13 3/4] pc-bios/s390-ccw/net: Add support for pxelinux-style config files

[Thomas Huth]

On 20.04.2018 08:53, Viktor VM Mihajlovski wrote:
> On 19.04.2018 18:55, Thomas Huth wrote:
>> On 19.04.2018 14:40, Viktor VM Mihajlovski wrote:
[...]
>>>>> That's interesting because treating the bootfile as pxe-ish config is
>>>>> what DPM does. Which means that with this change the processor
>>>>> architecture type 0x1f (Basic) will turn into a superset of 0x20
>>>>> (Extended).
>>>>
>>>> Is there any reference available what "basic" and "extended" exactly
>>>> mean? I just know that there are these two values registered by you at
>>>> the IANA:
>>>>
>>>> https://www.iana.org/assignments/dhcpv6-parameters/dhcpv6-parameters.xhtml#processor-architecture
>>>>
>>>> ... but I haven't seen a description of the differences of these two
>>>> values yet.
>>>>
>>> Yep, the IANA stuff is pretty terse for all architectures. In a
>>> nutshell: 0x1f is standard DHCP-based boot (bootfile is a single
>>> executable binary) and 0x20 serves a pxe-style config file instead.
>>
>> Can 0x20 also still handle binary files, or only pxe-style config files?
>>
> Unfortunately, DPM has no support for binaries which was the reason we
> ended up with two different identifiers

Ok, so switching to 0x20 is not an option for us here.

>>> In fact, "processor architecture identifier" is a misnomer, as it
>>> describes a pair of processor architecture and firmware.
>>
>> Yeah, I know ... it's even worse nowadays, it's processor + firmware +
>> TCP/IP protocol (HTTP vs. TFTP) ... we should have properly untangled
>> that when doing RFC 5970, but it was already hard enough to get that
>> accepted in the current shape, so we missed to clean up the mess from
>> RFC 4578 / PXE :-(
>>
>>> Since the QEMU firmware has two significant extensions now (insfile and
>>> direct config file loading), it might be worthwhile to add a new
>>> identifier value (say s390 QEMU PC-BIOS or similar) announcing the new
>>> capabilities, so that a boot server admin can setup her system accordingly.
>>
>> Not sure ... too many processory-architecture-type entries might also
>> rather be too confusing for the users? ... so if 0x20 can also handle
>> binary files like 0x1f, I'd maybe simply go with 0x20 and mainly promote
>> pxelinux config files in the documentation?
>>
> Well, if we don't add another identifier, and I feel reluctant about it
> as well for the reasons you wrote, we may stick with 0x1f and consider
> the additional capabilities as being optional (for special needs only).

I tend to stay with 0x1f, since booting binaries is still the "primary"
interface (the first check for the "default" or "# " magic keywords is
not 100% reliable, since the config file could also start with another
command, and the probing of pxelinux.cfg/* files is only tried afterwards).

 Thomas

Re: [Qemu-devel] [qemu-s390x] [PATCH v1 for-2.13 3/4] pc-bios/s390-ccw/net: Add support for pxelinux-style config files

[Viktor VM Mihajlovski]

On 20.04.2018 09:36, Thomas Huth wrote:
> On 20.04.2018 08:53, Viktor VM Mihajlovski wrote:
>> On 19.04.2018 18:55, Thomas Huth wrote:
[...]
>> Well, if we don't add another identifier, and I feel reluctant about it
>> as well for the reasons you wrote, we may stick with 0x1f and consider
>> the additional capabilities as being optional (for special needs only).
> 
> I tend to stay with 0x1f, since booting binaries is still the "primary"
> interface (the first check for the "default" or "# " magic keywords is
> not 100% reliable, since the config file could also start with another
> command, and the probing of pxelinux.cfg/* files is only tried afterwards).
> 
Good. It would be important though to define and document the best way
to configure a pxelinux setup: empty bootfile name, bootfile empty (or
not existent) or a special file format for the bootfile content (like
'PXE0' in EBCDIC). The former two carry the danger of accidentally
enabling the pxe-style boot process.
>  Thomas
> 


-- 
Regards,
  Viktor Mihajlovski

Re: [Qemu-devel] [qemu-s390x] [PATCH v1 for-2.13 3/4] pc-bios/s390-ccw/net: Add support for pxelinux-style config files

[Thomas Huth]

On 20.04.2018 09:54, Viktor VM Mihajlovski wrote:
> On 20.04.2018 09:36, Thomas Huth wrote:
>> On 20.04.2018 08:53, Viktor VM Mihajlovski wrote:
>>> On 19.04.2018 18:55, Thomas Huth wrote:
> [...]
>>> Well, if we don't add another identifier, and I feel reluctant about it
>>> as well for the reasons you wrote, we may stick with 0x1f and consider
>>> the additional capabilities as being optional (for special needs only).
>>
>> I tend to stay with 0x1f, since booting binaries is still the "primary"
>> interface (the first check for the "default" or "# " magic keywords is
>> not 100% reliable, since the config file could also start with another
>> command, and the probing of pxelinux.cfg/* files is only tried afterwards).
>>
> Good. It would be important though to define and document the best way
> to configure a pxelinux setup: empty bootfile name, bootfile empty (or
> not existent) or a special file format for the bootfile content (like
> 'PXE0' in EBCDIC). The former two carry the danger of accidentally
> enabling the pxe-style boot process.

I don't like the idea of providing a bootfile with a magic content ...
if you have to provide a bootfile, you could also go with specifying
pxelinux.cfg/default (or whatever) directly, you just have to make sure
that the file starts with the right magic ("default" or "# ").

But I agree that starting to guess the non-MAC-based filenames in case
the boot file name is empty carries some danger, indeed. If we load
pxelinux.cfg/default on s390x, who guarantees that this was not the
default file for x86 instead? Maybe we should rather use "pzelinux.cfg"
as directory ;-) ?

Or maybe it would rather make sense to force the users to specify the
path to the pxelinux config files instead, ending with a slash, so they
have to put "pxelinux.cfg/" or whatever location into the bootfile
parameter?

 Thomas

Re: [Qemu-devel] [qemu-s390x] [PATCH v1 for-2.13 3/4] pc-bios/s390-ccw/net: Add support for pxelinux-style config files

[Viktor VM Mihajlovski]

On 20.04.2018 10:40, Thomas Huth wrote:
> On 20.04.2018 09:54, Viktor VM Mihajlovski wrote:
>> On 20.04.2018 09:36, Thomas Huth wrote:
>>> On 20.04.2018 08:53, Viktor VM Mihajlovski wrote:
>>>> On 19.04.2018 18:55, Thomas Huth wrote:
>> [...]
>>>> Well, if we don't add another identifier, and I feel reluctant about it
>>>> as well for the reasons you wrote, we may stick with 0x1f and consider
>>>> the additional capabilities as being optional (for special needs only).
>>>
>>> I tend to stay with 0x1f, since booting binaries is still the "primary"
>>> interface (the first check for the "default" or "# " magic keywords is
>>> not 100% reliable, since the config file could also start with another
>>> command, and the probing of pxelinux.cfg/* files is only tried afterwards).
>>>
>> Good. It would be important though to define and document the best way
>> to configure a pxelinux setup: empty bootfile name, bootfile empty (or
>> not existent) or a special file format for the bootfile content (like
>> 'PXE0' in EBCDIC). The former two carry the danger of accidentally
>> enabling the pxe-style boot process.
> 
> I don't like the idea of providing a bootfile with a magic content ...
> if you have to provide a bootfile, you could also go with specifying
> pxelinux.cfg/default (or whatever) directly, you just have to make sure
> that the file starts with the right magic ("default" or "# ").
> 
> But I agree that starting to guess the non-MAC-based filenames in case
> the boot file name is empty carries some danger, indeed. If we load
> pxelinux.cfg/default on s390x, who guarantees that this was not the
> default file for x86 instead? Maybe we should rather use "pzelinux.cfg"
> as directory ;-) ?
Yep. Sounds as if a null bootfile name should not be allowed at all and
should be rejected by the netboot bios.
> 
> Or maybe it would rather make sense to force the users to specify the
> path to the pxelinux config files instead, ending with a slash, so they
> have to put "pxelinux.cfg/" or whatever location into the bootfile
> parameter?
Since the bootfile has to be architecture specific anyway, it would  a
good idea to put it into a separate directory, e.g. "/tftpboot/s390".
If the bootfile reported would be "/s390/" or a (nonexisting)
"/s390/pxelinux.0", the new pxe loader could start its
pxelinux.cfg/whatever probing from there. "/" or
"/some-not-existing-file" would be a special case thereof.
> 
>  Thomas
> 


-- 
Regards,
  Viktor Mihajlovski