[Qemu-devel] [PATCH 0/6] S390 April patch round

[Qemu-devel] [PATCH 0/6] S390 April patch round

[Alexander Graf]

While improving KVM on S390x I stumbled across some more issues. The most
annoying ones were in the character layer, where -nographic would just fail
on most users. This set fixes these issues.

The set also adds support for my new zipl bootloader. Using this firmware
and a patches version of zipl you can finally boot from virtio disks without
the need to pass -kernel and -initrd directly!

Alexander Graf (6):
  S390: Add stub for cpu_get_phys_page_debug
  S390: Tell user why VM creation failed
  Make char muxer more robust wrt small FIFOs
  Always notify consumers of char devices if they're open
  [S390] Implement virtio reset
  [S390] Add firmware code

 hw/s390-virtio-bus.c  |    3 +--
 hw/s390-virtio-bus.h  |    1 +
 hw/s390-virtio.c      |   29 +++++++++++++++++++++++++----
 kvm-all.c             |    7 ++++++-
 pc-bios/s390-zipl.rom |  Bin 0 -> 2448 bytes
 qemu-char.c           |   30 ++++++++++++++++++++++++++++++
 qemu-char.h           |    1 +
 target-s390x/helper.c |    5 +++++
 8 files changed, 69 insertions(+), 7 deletions(-)
 create mode 100755 pc-bios/s390-zipl.rom

[Qemu-devel] [PATCH 1/6] S390: Add stub for cpu_get_phys_page_debug

[Alexander Graf]

We don't implement any virtual memory in the S390 target so far, so let's
add a stub for this now mandatory function.

Fixes building of S390 target.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 target-s390x/helper.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/target-s390x/helper.c b/target-s390x/helper.c
index ba0c052..4a5297b 100644
--- a/target-s390x/helper.c
+++ b/target-s390x/helper.c
@@ -58,6 +58,11 @@ void cpu_reset(CPUS390XState *env)
     tlb_flush(env, 1);
 }
 
+target_phys_addr_t cpu_get_phys_page_debug(CPUState *env, target_ulong addr)
+{
+    return 0;
+}
+
 #ifndef CONFIG_USER_ONLY
 
 int cpu_s390x_handle_mmu_fault (CPUState *env, target_ulong address, int rw,
-- 
1.6.0.2

[Qemu-devel] [PATCH 2/6] S390: Tell user why VM creation failed

[Alexander Graf]

The KVM kernel module on S390 refuses to create a VM when the switch_amode
kernel parameter is not used.

Since that is not exactly obvious, let's give the user a nice warning.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 kvm-all.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/kvm-all.c b/kvm-all.c
index 40b5a51..9de66c9 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -607,8 +607,13 @@ int kvm_init(int smp_cpus)
     }
 
     s->vmfd = kvm_ioctl(s, KVM_CREATE_VM, 0);
-    if (s->vmfd < 0)
+    if (s->vmfd < 0) {
+#ifdef TARGET_S390X
+        fprintf(stderr, "Please add the 'switch_amode' kernel parameter to "
+                        "your host kernel command line\n");
+#endif
         goto err;
+    }
 
     /* initially, KVM allocated its own memory and we had to jump through
      * hooks to make phys_ram_base point to this.  Modern versions of KVM
-- 
1.6.0.2

[Qemu-devel] [PATCH 3/6] Make char muxer more robust wrt small FIFOs

[Alexander Graf]

Virtio-Console can only process one character at a time. Using it on S390
gave me strage "lags" where I got the character I pressed before when
pressing one. So I typed in "abc" and only received "a", then pressed "d"
but the guest received "b" and so on.

While the stdio driver calls a poll function that just processes on its
queue in case virtio-console can't take multiple characters at once, the
muxer does not have such callbacks, so it can't empty its queue.

To work around that limitation, I introduced a new timer that only gets
active when the guest can not receive any more characters. In that case
it polls again after a while to check if the guest is now receiving input.

This patch fixes input when using -nographic on s390 for me.

---

Please consider for stable.
---
 qemu-char.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/qemu-char.c b/qemu-char.c
index 048da3f..6ad6609 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -219,6 +219,7 @@ typedef struct {
     IOEventHandler *chr_event[MAX_MUX];
     void *ext_opaque[MAX_MUX];
     CharDriverState *drv;
+    QEMUTimer *accept_timer;
     int focus;
     int mux_cnt;
     int term_got_escape;
@@ -380,6 +381,13 @@ static void mux_chr_accept_input(CharDriverState *chr)
         d->chr_read[m](d->ext_opaque[m],
                        &d->buffer[m][d->cons[m]++ & MUX_BUFFER_MASK], 1);
     }
+
+    /* We're still not able to sync producer and consumer, so let's wait a bit
+       and try again by then. */
+    if (d->prod[m] != d->cons[m]) {
+        qemu_mod_timer(d->accept_timer, qemu_get_clock(vm_clock)
+                                        + (int64_t)100000);
+    }
 }
 
 static int mux_chr_can_read(void *opaque)
@@ -462,6 +470,8 @@ static CharDriverState *qemu_chr_open_mux(CharDriverState *drv)
     chr->opaque = d;
     d->drv = drv;
     d->focus = -1;
+    d->accept_timer = qemu_new_timer(vm_clock,
+                                     (QEMUTimerCB*)mux_chr_accept_input, chr);
     chr->chr_write = mux_chr_write;
     chr->chr_update_read_handler = mux_chr_update_read_handler;
     chr->chr_accept_input = mux_chr_accept_input;
-- 
1.6.0.2

[Qemu-devel] [PATCH 4/6] Always notify consumers of char devices if they're open

[Alexander Graf]

When using virtio-console on s390, the input doesn't work.

The root of the problem is rather simple. What happens is the following:

 1) create character device for stdio
 2) char device is done creating, sends OPENED event
 3) virtio-console adds handlers
 4) no event comes because the char device is open already
 5) virtio-console doesn't accept input because it didn't
    receive an OPENED event

To make that sure virtio-console gets notified that the character device
is open even when it's been open from the beginning, this patch introduces
a variable that keeps track of the opened state. If the device is open when
the event handlers get installed, we just notify the handler.

This fixes input with virtio-console on s390.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 qemu-char.c |   20 ++++++++++++++++++++
 qemu-char.h |    1 +
 2 files changed, 21 insertions(+), 0 deletions(-)

diff --git a/qemu-char.c b/qemu-char.c
index 6ad6609..a9d9442 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -109,6 +109,16 @@ static QTAILQ_HEAD(CharDriverStateHead, CharDriverState) chardevs =
 
 static void qemu_chr_event(CharDriverState *s, int event)
 {
+    /* Keep track if the char device is open */
+    switch (event) {
+        case CHR_EVENT_OPENED:
+            s->opened = 1;
+            break;
+        case CHR_EVENT_CLOSED:
+            s->opened = 0;
+            break;
+    }
+
     if (!s->chr_event)
         return;
     s->chr_event(s->handler_opaque, event);
@@ -193,6 +203,12 @@ void qemu_chr_add_handlers(CharDriverState *s,
     s->handler_opaque = opaque;
     if (s->chr_update_read_handler)
         s->chr_update_read_handler(s);
+
+    /* We're connecting to an already opened device, so let's make sure we
+       also get the open event */
+    if (s->opened) {
+        qemu_chr_generic_open(s);
+    }
 }
 
 static int null_chr_write(CharDriverState *chr, const uint8_t *buf, int len)
@@ -475,6 +491,10 @@ static CharDriverState *qemu_chr_open_mux(CharDriverState *drv)
     chr->chr_write = mux_chr_write;
     chr->chr_update_read_handler = mux_chr_update_read_handler;
     chr->chr_accept_input = mux_chr_accept_input;
+
+    /* Muxes are always open on creation */
+    qemu_chr_generic_open(chr);
+
     return chr;
 }
 
diff --git a/qemu-char.h b/qemu-char.h
index 3a9427b..e3a0783 100644
--- a/qemu-char.h
+++ b/qemu-char.h
@@ -67,6 +67,7 @@ struct CharDriverState {
     QEMUBH *bh;
     char *label;
     char *filename;
+    int opened;
     QTAILQ_ENTRY(CharDriverState) next;
 };
 
-- 
1.6.0.2

[Qemu-devel] [PATCH 5/6] [S390] Implement virtio reset

[Alexander Graf]

The guest may issue a RESET command for virtio. So far we didn't bother
to implement it, but with my new bootloader we actually need it for Linux
to get back to a safe state.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 hw/s390-virtio-bus.c |    3 +--
 hw/s390-virtio-bus.h |    1 +
 hw/s390-virtio.c     |    9 +++++----
 3 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/hw/s390-virtio-bus.c b/hw/s390-virtio-bus.c
index 9fc01e9..b52d08d 100644
--- a/hw/s390-virtio-bus.c
+++ b/hw/s390-virtio-bus.c
@@ -56,7 +56,6 @@ typedef struct {
 static const VirtIOBindings virtio_s390_bindings;
 
 static ram_addr_t s390_virtio_device_num_vq(VirtIOS390Device *dev);
-static void s390_virtio_device_sync(VirtIOS390Device *dev);
 
 VirtIOS390Bus *s390_virtio_bus_init(ram_addr_t *ram_size)
 {
@@ -185,7 +184,7 @@ static ram_addr_t s390_virtio_next_ring(VirtIOS390Bus *bus)
     return r;
 }
 
-static void s390_virtio_device_sync(VirtIOS390Device *dev)
+void s390_virtio_device_sync(VirtIOS390Device *dev)
 {
     VirtIOS390Bus *bus = DO_UPCAST(VirtIOS390Bus, bus, dev->qdev.parent_bus);
     ram_addr_t cur_offs;
diff --git a/hw/s390-virtio-bus.h b/hw/s390-virtio-bus.h
index 0ea8f54..333fea8 100644
--- a/hw/s390-virtio-bus.h
+++ b/hw/s390-virtio-bus.h
@@ -65,3 +65,4 @@ extern VirtIOS390Device *s390_virtio_bus_find_vring(VirtIOS390Bus *bus,
                                                     int *vq_num);
 extern VirtIOS390Device *s390_virtio_bus_find_mem(VirtIOS390Bus *bus,
                                                   ram_addr_t mem);
+extern void s390_virtio_device_sync(VirtIOS390Device *dev);
diff --git a/hw/s390-virtio.c b/hw/s390-virtio.c
index ad3386f..c36a8b2 100644
--- a/hw/s390-virtio.c
+++ b/hw/s390-virtio.c
@@ -99,10 +99,11 @@ int s390_virtio_hypercall(CPUState *env)
         break;
     case KVM_S390_VIRTIO_RESET:
     {
-        /* Virtio_reset resets the internal addresses, so we'd have to sync
-           them up again. We don't want to reallocate a vring though, so let's
-           just not reset. */
-        /* virtio_reset(dev->vdev); */
+        VirtIOS390Device *dev;
+
+        dev = s390_virtio_bus_find_mem(s390_bus, mem);
+        virtio_reset(dev->vdev);
+        s390_virtio_device_sync(dev);
         break;
     }
     case KVM_S390_VIRTIO_SET_STATUS:
-- 
1.6.0.2

[Qemu-devel] [PATCH 6/6] [S390] Add firmware code

[Alexander Graf]

This patch adds a firmware blob to the S390 target. The blob is a simple
implementation of a virtio client that tries to read the second stage
bootloader from sectors described as of offset 0x20 in the MBR.

In combination with an updated zipl this allows for booting from virtio
block devices. This firmware is built from the same sources as the second
stage bootloader. You can find the zipl patch to build both here:

http://alex.csgraf.de/qemu/0001-Zipl-VirtIO-bootloader-code.patch

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 hw/s390-virtio.c      |   20 ++++++++++++++++++++
 pc-bios/s390-zipl.rom |  Bin 0 -> 2448 bytes
 2 files changed, 20 insertions(+), 0 deletions(-)
 create mode 100755 pc-bios/s390-zipl.rom

diff --git a/hw/s390-virtio.c b/hw/s390-virtio.c
index c36a8b2..91a3065 100644
--- a/hw/s390-virtio.c
+++ b/hw/s390-virtio.c
@@ -52,6 +52,10 @@
 #define INITRD_PARM_SIZE                0x010410UL
 #define PARMFILE_START                  0x001000UL
 
+#define ZIPL_START			0x001000UL
+#define ZIPL_LOAD_ADDR			0x001000UL
+#define ZIPL_FILENAME			"s390-zipl.rom"
+
 #define MAX_BLK_DEVS                    10
 
 static VirtIOS390Bus *s390_bus;
@@ -140,6 +144,8 @@ static void s390_init(ram_addr_t ram_size,
     ram_addr_t kernel_size = 0;
     ram_addr_t initrd_offset;
     ram_addr_t initrd_size = 0;
+    ram_addr_t bios_size = 0;
+    char *bios_filename;
     int i;
 
     /* XXX we only work on KVM for now */
@@ -178,6 +184,20 @@ static void s390_init(ram_addr_t ram_size,
     env->halted = 0;
     env->exception_index = 0;
 
+    /* Load zipl bootloader */
+    if (bios_name == NULL)
+        bios_name = ZIPL_FILENAME;
+
+    bios_filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
+    bios_size = load_image(bios_filename, qemu_get_ram_ptr(ZIPL_LOAD_ADDR));
+
+    if ((long)bios_size < 0) {
+        hw_error("could not load bootloader '%s'\n", bios_name);
+    }
+
+    env->psw.addr = ZIPL_START;
+    env->psw.mask = 0x0000000180000000ULL;
+
     if (kernel_filename) {
         kernel_size = load_image(kernel_filename, qemu_get_ram_ptr(0));
 
diff --git a/pc-bios/s390-zipl.rom b/pc-bios/s390-zipl.rom
new file mode 100755
index 0000000000000000000000000000000000000000..0f1b26afd5ad530035cdc6fbe285bae278ad5983
GIT binary patch
literal 2448
zcmZuyX>1%t7Jl9Cqz1=MDvoU?BtY32h)oXKjv<!`-A(`p7Pb=%IGpBOj)j<AvxIv9
zbMC>|5ZGN{GmwmH6&7LzAs7Twb4f`2NQ49u%AbMeSTYi6vqKaSaPz&g7uXFW^>o#%
zcYN=w_g?D*w~($>eHz8_Cdz~Xk<*|L?(6eqoG&`mrdwrxm?Y^rWckf86&2O#fn@)1
zHhxy8C3(<`B%47}*r{mN&Sp{%Dw?YoQ<Wa1nRwfJfEsj#rbROTCk;I$slHfGO^`Uc
zHA(bAnK$trG5&!Sj|3@1_(oTfy+V{<i+d@f*I5KKZ{m3oj{|?{NGeHA<SC*?-AWX>
z(x#!}1C%K?QggQ(PAxZ+MYUeLbb4jza70KPs?|r36{#>qsq_hjrQ=E3*Xzg%NTocf
zrbnajnDJay6Tk|Qra>2xx}~z&5;}Gy!!$Vto8x)X7UShzaUb~GO;mM<KA7|ZWfz{a
zcTQn`;yh<cRBodhs);1j93X9i#uPkn6jR`;LPtl+OObPLW(pN;ItT1Evc1X9_?WPV
z<-%eUEHX4zw~z%(2}>Z-v%!N>;9-OE96gwPgmcnqJj;jz=`jn<k>n_mH^w>a8pGOi
zWsT8rWlB`w4>bX_`{I7&Bth^#<kYvB>tK+M0pdEGJHC`w+Mm;9<O9jKkhjDCnvkz#
zTyG!>KTDFN$?W&*<?L|`OokOmubo(naon-t4iUN;DZIR;*OG)iZVC00@!dEdAb;cM
z6X;xy-8S|;{vT*}8u^EMtxbA^Rb}K~z~*ImvCP@KklRMS$LvGKVzA8z3oYP6#V=D1
zT!0Z#;V!WMQJzPNYRXH<r5e3}u|S+H*3dl0Y;bRhyC?44{$qI-Ycoq+BNJAIMw3mK
zUIk<WRGWSVXx{-tkLm5WwO0(wt;{rJ3vjkR@=zJISY_&(VUc3A_Ll}}C#0=-I?e7H
z{UkgzQG)w$%zeFw7_UM$lI%fFU>Uj#%Uj}qb4K8JW9&|1UCt;`Ux;933@Z-bal<$)
zJ_$#$`Y!Cp4K51%u>LyCVl8+E7_Gfw4*MCrkEleHtK*s|MXF>`&JRjl&s~u$*_6{~
zQrce=ke*Jq-yzB0YU<}E?5mL7>ybN}rp#vQqsSYTl!;0Ro{92kz)Td3rqK5TwEgv_
z-GsWf_1m-!cOYpf3IfzDy1YFKvd}88<~|4K+}mf&Nc^yLWT3KKNsIj@0Yw3N-HF^R
z#|Uz;@H<?mur$WZ$O3`xMpt_-P$0vhz{?SQcO2rCJ-Ipf_{lo=GwWfeWIA`)>f`P?
z%dpimk>jEHanJmi<1&-u0CixJa(-$uzqUa3)Bod$R6jEVM*Jnp<QTdA|AVUk7f=~A
zwZj&o%2E~Oz|G!bHGQmmlA{VIXNhNwsfEtM(b*QRyM~i`UFl#a16P3?iJ666I`VAz
zj}-4CW%FH`dNcEFW4wE^-gd+RP)<z3cC9ew2WSOmHY$d_TlaJ;MV^XBct_v(O^P-P
z#kDja*xm~BknZ(I;}LK<ZFO6o{4lh-T<D|kzkdl67@nZH0F^uw-;3CI?J=h#j0JW)
zbFIlaw<A8l$jSy6&uJ<$I_Aal;pa_be<D^}-?!res-5H94Fm-~slOj5D|w<|E}2>K
z-Am|NGkfYc8C?rKJBW$gidwB;LIazTUlO{AHi#<2jmHJ_MT<qNrAvAxSv~j`%x>Ub
z@AC5I7GOJFpN=@kCi@IGUl}i*hvtGBuneOa&iCivAlhRl(SPsMDeTHB-3oRqREw_}
z-<-=mVLWdEv+@J6n?kOp^~e#^D*4L9ex5awxq7xrVLHN2j0u5kCVn>h4*dQ2o6+S>
ztdnF?c7-*$z$+okt`didyD&Yc)znU(A!pXj64fb@o(+ll%@v~h@Hu8VWh!2cyma`l
zzCj$Jl6wd5%ZvLT*ml9<&XxsD$KLbcLk~amC-LWxFUlO(y7J;h$BVx}^^t5rEIj6t
zRVRxaTME(Nw_=_*|D<Md>1CH+aphINx%!$Hc0fFK;M(hcd;JX?`)~Z+h#nHd;`2Ly
zKXB6@Zr*gut<OKlc27R_^tX8Z>p8LgcHZCScdcG?%BiPyuN99zw)t6c$LVMM>ddo#
z9jsd~9>4pEt$)36|Jgm~oO@pHhV#D{e}lf;*1n|UgcFx8TQ0=kv1ck4Q*AZTqE(7c
QS)s0MzqFbZ_CNi800lxdZvX%Q

literal 0
HcmV?d00001

-- 
1.6.0.2

[Qemu-devel] Re: [PATCH 6/6] [S390] Add firmware code

[Bastian Blank]

On Thu, Apr 01, 2010 at 06:42:41PM +0200, Alexander Graf wrote:
> This patch adds a firmware blob to the S390 target. The blob is a simple
> implementation of a virtio client that tries to read the second stage
> bootloader from sectors described as of offset 0x20 in the MBR.

How do you intend to fullfill the source requirement for this firmware?
I see no explicit license, so I assume GPL.

Bastian

-- 
Intuition, however illogical, is recognized as a command prerogative.
		-- Kirk, "Obsession", stardate 3620.7

[Qemu-devel] Re: [PATCH 6/6] [S390] Add firmware code

[Alexander Graf]

Am 01.04.2010 um 23:18 schrieb Bastian Blank <waldi@debian.org>:

> On Thu, Apr 01, 2010 at 06:42:41PM +0200, Alexander Graf wrote:
>> This patch adds a firmware blob to the S390 target. The blob is a  
>> simple
>> implementation of a virtio client that tries to read the second stage
>> bootloader from sectors described as of offset 0x20 in the MBR.
>
> How do you intend to fullfill the source requirement for this  
> firmware?
> I see no explicit license, so I assume GPL.

The firmware gets built as part of the s390-tools package which is  
GPLed FWIW. But all source required for it is in the patch I linked  
to. So you would be able to build it without s390-tools and all new  
virtio specific files are basically PD.

Alex

Re: [Qemu-devel] [PATCH 3/6] Make char muxer more robust wrt small FIFOs

[Amit Shah]

On (Thu) Apr 01 2010 [18:42:38], Alexander Graf wrote:
> Virtio-Console can only process one character at a time.

The host can process as many as you give it, depending on the buffer
size exposed by the guest.

On older guests (guest kernels w/o multiport support), the guest reads
input from host, processes it and only then opens up another buffer for
the host to write into.

On newer guests (guest kernels that support multiport), the guest
fills the entire vq so that host can send as many buffers as possible
without getting throttled. I guess you're getting hit by this.

> Using it on S390
> gave me strage "lags" where I got the character I pressed before when
> pressing one. So I typed in "abc" and only received "a", then pressed "d"
> but the guest received "b" and so on.

This might be because qemu-char would not be able to send out 'b' while
the guest still processes 'a' and has no free buffers to write out to.
On seeing 'd', it flushes its queue.

Can you try using a 2.6.34-rc3 kernel without this patch to see if
things work fine?

		Amit

Re: [Qemu-devel] [PATCH 4/6] Always notify consumers of char devices if they're open

[Amit Shah]

On (Thu) Apr 01 2010 [18:42:39], Alexander Graf wrote:
> When using virtio-console on s390, the input doesn't work.
> 
> The root of the problem is rather simple. What happens is the following:
> 
>  1) create character device for stdio
>  2) char device is done creating, sends OPENED event
>  3) virtio-console adds handlers
>  4) no event comes because the char device is open already
>  5) virtio-console doesn't accept input because it didn't
>     receive an OPENED event
> 
> To make that sure virtio-console gets notified that the character device
> is open even when it's been open from the beginning, this patch introduces
> a variable that keeps track of the opened state. If the device is open when
> the event handlers get installed, we just notify the handler.
> 
> This fixes input with virtio-console on s390.
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>

Acked-by: Amit Shah <amit.shah@redhat.com>

		Amit

Re: [Qemu-devel] [PATCH 3/6] Make char muxer more robust wrt small FIFOs

[Alexander Graf]

Amit Shah wrote:
> On (Thu) Apr 01 2010 [18:42:38], Alexander Graf wrote:
>   
>> Virtio-Console can only process one character at a time.
>>     
>
> The host can process as many as you give it, depending on the buffer
> size exposed by the guest.
>
> On older guests (guest kernels w/o multiport support), the guest reads
> input from host, processes it and only then opens up another buffer for
> the host to write into.
>
> On newer guests (guest kernels that support multiport), the guest
> fills the entire vq so that host can send as many buffers as possible
> without getting throttled. I guess you're getting hit by this.
>   

Probably, yes.

>> Using it on S390
>> gave me strage "lags" where I got the character I pressed before when
>> pressing one. So I typed in "abc" and only received "a", then pressed "d"
>> but the guest received "b" and so on.
>>     
>
> This might be because qemu-char would not be able to send out 'b' while
> the guest still processes 'a' and has no free buffers to write out to.
> On seeing 'd', it flushes its queue.
>
> Can you try using a 2.6.34-rc3 kernel without this patch to see if
> things work fine?
>   

Hrm - would it actually matter? We need to have older guest kernels
working anyways. And my S390 LPAR doesn't exactly have a lot of disk
space, so compiling a kernel is anything but fun :-).


Alex

Re: [Qemu-devel] [PATCH 3/6] Make char muxer more robust wrt small FIFOs

[Amit Shah]

On (Wed) Apr 07 2010 [16:32:04], Alexander Graf wrote:
> Amit Shah wrote:
> > On (Thu) Apr 01 2010 [18:42:38], Alexander Graf wrote:
> >   
> >> Virtio-Console can only process one character at a time.
> >>     
> >
> > The host can process as many as you give it, depending on the buffer
> > size exposed by the guest.
> >
> > On older guests (guest kernels w/o multiport support), the guest reads
> > input from host, processes it and only then opens up another buffer for
> > the host to write into.
> >
> > On newer guests (guest kernels that support multiport), the guest
> > fills the entire vq so that host can send as many buffers as possible
> > without getting throttled. I guess you're getting hit by this.
> >   
> 
> Probably, yes.
> 
> >> Using it on S390
> >> gave me strage "lags" where I got the character I pressed before when
> >> pressing one. So I typed in "abc" and only received "a", then pressed "d"
> >> but the guest received "b" and so on.
> >>     
> >
> > This might be because qemu-char would not be able to send out 'b' while
> > the guest still processes 'a' and has no free buffers to write out to.
> > On seeing 'd', it flushes its queue.
> >
> > Can you try using a 2.6.34-rc3 kernel without this patch to see if
> > things work fine?
> >   
> 
> Hrm - would it actually matter? We need to have older guest kernels
> working anyways. And my S390 LPAR doesn't exactly have a lot of disk
> space, so compiling a kernel is anything but fun :-).

Oh I just want to find out if it works for you -- you shouldn't get hit
by the 1-char limit anymore.

		Amit
-- 
http://log.amitshah.net/

Re: [Qemu-devel] [PATCH 5/6] [S390] Implement virtio reset

[Aurelien Jarno]

On Thu, Apr 01, 2010 at 06:42:40PM +0200, Alexander Graf wrote:
> The guest may issue a RESET command for virtio. So far we didn't bother
> to implement it, but with my new bootloader we actually need it for Linux
> to get back to a safe state.

Thanks, applied.

> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
>  hw/s390-virtio-bus.c |    3 +--
>  hw/s390-virtio-bus.h |    1 +
>  hw/s390-virtio.c     |    9 +++++----
>  3 files changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/s390-virtio-bus.c b/hw/s390-virtio-bus.c
> index 9fc01e9..b52d08d 100644
> --- a/hw/s390-virtio-bus.c
> +++ b/hw/s390-virtio-bus.c
> @@ -56,7 +56,6 @@ typedef struct {
>  static const VirtIOBindings virtio_s390_bindings;
>  
>  static ram_addr_t s390_virtio_device_num_vq(VirtIOS390Device *dev);
> -static void s390_virtio_device_sync(VirtIOS390Device *dev);
>  
>  VirtIOS390Bus *s390_virtio_bus_init(ram_addr_t *ram_size)
>  {
> @@ -185,7 +184,7 @@ static ram_addr_t s390_virtio_next_ring(VirtIOS390Bus *bus)
>      return r;
>  }
>  
> -static void s390_virtio_device_sync(VirtIOS390Device *dev)
> +void s390_virtio_device_sync(VirtIOS390Device *dev)
>  {
>      VirtIOS390Bus *bus = DO_UPCAST(VirtIOS390Bus, bus, dev->qdev.parent_bus);
>      ram_addr_t cur_offs;
> diff --git a/hw/s390-virtio-bus.h b/hw/s390-virtio-bus.h
> index 0ea8f54..333fea8 100644
> --- a/hw/s390-virtio-bus.h
> +++ b/hw/s390-virtio-bus.h
> @@ -65,3 +65,4 @@ extern VirtIOS390Device *s390_virtio_bus_find_vring(VirtIOS390Bus *bus,
>                                                      int *vq_num);
>  extern VirtIOS390Device *s390_virtio_bus_find_mem(VirtIOS390Bus *bus,
>                                                    ram_addr_t mem);
> +extern void s390_virtio_device_sync(VirtIOS390Device *dev);
> diff --git a/hw/s390-virtio.c b/hw/s390-virtio.c
> index ad3386f..c36a8b2 100644
> --- a/hw/s390-virtio.c
> +++ b/hw/s390-virtio.c
> @@ -99,10 +99,11 @@ int s390_virtio_hypercall(CPUState *env)
>          break;
>      case KVM_S390_VIRTIO_RESET:
>      {
> -        /* Virtio_reset resets the internal addresses, so we'd have to sync
> -           them up again. We don't want to reallocate a vring though, so let's
> -           just not reset. */
> -        /* virtio_reset(dev->vdev); */
> +        VirtIOS390Device *dev;
> +
> +        dev = s390_virtio_bus_find_mem(s390_bus, mem);
> +        virtio_reset(dev->vdev);
> +        s390_virtio_device_sync(dev);
>          break;
>      }
>      case KVM_S390_VIRTIO_SET_STATUS:
> -- 
> 1.6.0.2
> 
> 
> 
> 

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [PATCH 4/6] Always notify consumers of char devices if they're open

[Aurelien Jarno]

On Thu, Apr 01, 2010 at 06:42:39PM +0200, Alexander Graf wrote:
> When using virtio-console on s390, the input doesn't work.
> 
> The root of the problem is rather simple. What happens is the following:
> 
>  1) create character device for stdio
>  2) char device is done creating, sends OPENED event
>  3) virtio-console adds handlers
>  4) no event comes because the char device is open already
>  5) virtio-console doesn't accept input because it didn't
>     receive an OPENED event
> 
> To make that sure virtio-console gets notified that the character device
> is open even when it's been open from the beginning, this patch introduces
> a variable that keeps track of the opened state. If the device is open when
> the event handlers get installed, we just notify the handler.
>
> This fixes input with virtio-console on s390.

Thanks, applied.

> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
>  qemu-char.c |   20 ++++++++++++++++++++
>  qemu-char.h |    1 +
>  2 files changed, 21 insertions(+), 0 deletions(-)
> 
> diff --git a/qemu-char.c b/qemu-char.c
> index 6ad6609..a9d9442 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -109,6 +109,16 @@ static QTAILQ_HEAD(CharDriverStateHead, CharDriverState) chardevs =
>  
>  static void qemu_chr_event(CharDriverState *s, int event)
>  {
> +    /* Keep track if the char device is open */
> +    switch (event) {
> +        case CHR_EVENT_OPENED:
> +            s->opened = 1;
> +            break;
> +        case CHR_EVENT_CLOSED:
> +            s->opened = 0;
> +            break;
> +    }
> +
>      if (!s->chr_event)
>          return;
>      s->chr_event(s->handler_opaque, event);
> @@ -193,6 +203,12 @@ void qemu_chr_add_handlers(CharDriverState *s,
>      s->handler_opaque = opaque;
>      if (s->chr_update_read_handler)
>          s->chr_update_read_handler(s);
> +
> +    /* We're connecting to an already opened device, so let's make sure we
> +       also get the open event */
> +    if (s->opened) {
> +        qemu_chr_generic_open(s);
> +    }
>  }
>  
>  static int null_chr_write(CharDriverState *chr, const uint8_t *buf, int len)
> @@ -475,6 +491,10 @@ static CharDriverState *qemu_chr_open_mux(CharDriverState *drv)
>      chr->chr_write = mux_chr_write;
>      chr->chr_update_read_handler = mux_chr_update_read_handler;
>      chr->chr_accept_input = mux_chr_accept_input;
> +
> +    /* Muxes are always open on creation */
> +    qemu_chr_generic_open(chr);
> +
>      return chr;
>  }
>  
> diff --git a/qemu-char.h b/qemu-char.h
> index 3a9427b..e3a0783 100644
> --- a/qemu-char.h
> +++ b/qemu-char.h
> @@ -67,6 +67,7 @@ struct CharDriverState {
>      QEMUBH *bh;
>      char *label;
>      char *filename;
> +    int opened;
>      QTAILQ_ENTRY(CharDriverState) next;
>  };
>  
> -- 
> 1.6.0.2
> 
> 
> 
> 

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [PATCH 6/6] [S390] Add firmware code

[Aurelien Jarno]

On Thu, Apr 01, 2010 at 06:42:41PM +0200, Alexander Graf wrote:
> This patch adds a firmware blob to the S390 target. The blob is a simple
> implementation of a virtio client that tries to read the second stage
> bootloader from sectors described as of offset 0x20 in the MBR.
> 
> In combination with an updated zipl this allows for booting from virtio
> block devices. This firmware is built from the same sources as the second
> stage bootloader. You can find the zipl patch to build both here:
> 
> http://alex.csgraf.de/qemu/0001-Zipl-VirtIO-bootloader-code.patch

I am not fully comfortable introducing a binary firmware based on a
patch posted on a website. I see two options:
- Get your patch merged into ZIPL, so that we can build the firmware
  directly from the ZIPL sources
- Add the patch to the pc-bios/ directory

Also it would be nice to update pc-bios/README to provide details about
the ZIPL version used to build pc-bios/s390-zipl.rom.

> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
>  hw/s390-virtio.c      |   20 ++++++++++++++++++++
>  pc-bios/s390-zipl.rom |  Bin 0 -> 2448 bytes
>  2 files changed, 20 insertions(+), 0 deletions(-)
>  create mode 100755 pc-bios/s390-zipl.rom
> 
> diff --git a/hw/s390-virtio.c b/hw/s390-virtio.c
> index c36a8b2..91a3065 100644
> --- a/hw/s390-virtio.c
> +++ b/hw/s390-virtio.c
> @@ -52,6 +52,10 @@
>  #define INITRD_PARM_SIZE                0x010410UL
>  #define PARMFILE_START                  0x001000UL
>  
> +#define ZIPL_START			0x001000UL
> +#define ZIPL_LOAD_ADDR			0x001000UL
> +#define ZIPL_FILENAME			"s390-zipl.rom"
> +
>  #define MAX_BLK_DEVS                    10
>  
>  static VirtIOS390Bus *s390_bus;
> @@ -140,6 +144,8 @@ static void s390_init(ram_addr_t ram_size,
>      ram_addr_t kernel_size = 0;
>      ram_addr_t initrd_offset;
>      ram_addr_t initrd_size = 0;
> +    ram_addr_t bios_size = 0;
> +    char *bios_filename;
>      int i;
>  
>      /* XXX we only work on KVM for now */
> @@ -178,6 +184,20 @@ static void s390_init(ram_addr_t ram_size,
>      env->halted = 0;
>      env->exception_index = 0;
>  
> +    /* Load zipl bootloader */
> +    if (bios_name == NULL)
> +        bios_name = ZIPL_FILENAME;

You are missing curly braces here.

> +    bios_filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
> +    bios_size = load_image(bios_filename, qemu_get_ram_ptr(ZIPL_LOAD_ADDR));
> +
> +    if ((long)bios_size < 0) {
> +        hw_error("could not load bootloader '%s'\n", bios_name);
> +    }
> +
> +    env->psw.addr = ZIPL_START;
> +    env->psw.mask = 0x0000000180000000ULL;
> +

This probably has to be put in a reset handler so that this address is
reloaded upon reboot.

Also do you really want to make the firmware mandatory? What about a
warning and falling back to the direct kernel boot instead (if provided), 
as it is already now. Some other machines are doing that.

>      if (kernel_filename) {
>          kernel_size = load_image(kernel_filename, qemu_get_ram_ptr(0));
>  
> diff --git a/pc-bios/s390-zipl.rom b/pc-bios/s390-zipl.rom
> new file mode 100755
> index 0000000000000000000000000000000000000000..0f1b26afd5ad530035cdc6fbe285bae278ad5983
> GIT binary patch
> literal 2448
> zcmZuyX>1%t7Jl9Cqz1=MDvoU?BtY32h)oXKjv<!`-A(`p7Pb=%IGpBOj)j<AvxIv9
> zbMC>|5ZGN{GmwmH6&7LzAs7Twb4f`2NQ49u%AbMeSTYi6vqKaSaPz&g7uXFW^>o#%
> zcYN=w_g?D*w~($>eHz8_Cdz~Xk<*|L?(6eqoG&`mrdwrxm?Y^rWckf86&2O#fn@)1
> zHhxy8C3(<`B%47}*r{mN&Sp{%Dw?YoQ<Wa1nRwfJfEsj#rbROTCk;I$slHfGO^`Uc
> zHA(bAnK$trG5&!Sj|3@1_(oTfy+V{<i+d@f*I5KKZ{m3oj{|?{NGeHA<SC*?-AWX>
> z(x#!}1C%K?QggQ(PAxZ+MYUeLbb4jza70KPs?|r36{#>qsq_hjrQ=E3*Xzg%NTocf
> zrbnajnDJay6Tk|Qra>2xx}~z&5;}Gy!!$Vto8x)X7UShzaUb~GO;mM<KA7|ZWfz{a
> zcTQn`;yh<cRBodhs);1j93X9i#uPkn6jR`;LPtl+OObPLW(pN;ItT1Evc1X9_?WPV
> z<-%eUEHX4zw~z%(2}>Z-v%!N>;9-OE96gwPgmcnqJj;jz=`jn<k>n_mH^w>a8pGOi
> zWsT8rWlB`w4>bX_`{I7&Bth^#<kYvB>tK+M0pdEGJHC`w+Mm;9<O9jKkhjDCnvkz#
> zTyG!>KTDFN$?W&*<?L|`OokOmubo(naon-t4iUN;DZIR;*OG)iZVC00@!dEdAb;cM
> z6X;xy-8S|;{vT*}8u^EMtxbA^Rb}K~z~*ImvCP@KklRMS$LvGKVzA8z3oYP6#V=D1
> zT!0Z#;V!WMQJzPNYRXH<r5e3}u|S+H*3dl0Y;bRhyC?44{$qI-Ycoq+BNJAIMw3mK
> zUIk<WRGWSVXx{-tkLm5WwO0(wt;{rJ3vjkR@=zJISY_&(VUc3A_Ll}}C#0=-I?e7H
> z{UkgzQG)w$%zeFw7_UM$lI%fFU>Uj#%Uj}qb4K8JW9&|1UCt;`Ux;933@Z-bal<$)
> zJ_$#$`Y!Cp4K51%u>LyCVl8+E7_Gfw4*MCrkEleHtK*s|MXF>`&JRjl&s~u$*_6{~
> zQrce=ke*Jq-yzB0YU<}E?5mL7>ybN}rp#vQqsSYTl!;0Ro{92kz)Td3rqK5TwEgv_
> z-GsWf_1m-!cOYpf3IfzDy1YFKvd}88<~|4K+}mf&Nc^yLWT3KKNsIj@0Yw3N-HF^R
> z#|Uz;@H<?mur$WZ$O3`xMpt_-P$0vhz{?SQcO2rCJ-Ipf_{lo=GwWfeWIA`)>f`P?
> z%dpimk>jEHanJmi<1&-u0CixJa(-$uzqUa3)Bod$R6jEVM*Jnp<QTdA|AVUk7f=~A
> zwZj&o%2E~Oz|G!bHGQmmlA{VIXNhNwsfEtM(b*QRyM~i`UFl#a16P3?iJ666I`VAz
> zj}-4CW%FH`dNcEFW4wE^-gd+RP)<z3cC9ew2WSOmHY$d_TlaJ;MV^XBct_v(O^P-P
> z#kDja*xm~BknZ(I;}LK<ZFO6o{4lh-T<D|kzkdl67@nZH0F^uw-;3CI?J=h#j0JW)
> zbFIlaw<A8l$jSy6&uJ<$I_Aal;pa_be<D^}-?!res-5H94Fm-~slOj5D|w<|E}2>K
> z-Am|NGkfYc8C?rKJBW$gidwB;LIazTUlO{AHi#<2jmHJ_MT<qNrAvAxSv~j`%x>Ub
> z@AC5I7GOJFpN=@kCi@IGUl}i*hvtGBuneOa&iCivAlhRl(SPsMDeTHB-3oRqREw_}
> z-<-=mVLWdEv+@J6n?kOp^~e#^D*4L9ex5awxq7xrVLHN2j0u5kCVn>h4*dQ2o6+S>
> ztdnF?c7-*$z$+okt`didyD&Yc)znU(A!pXj64fb@o(+ll%@v~h@Hu8VWh!2cyma`l
> zzCj$Jl6wd5%ZvLT*ml9<&XxsD$KLbcLk~amC-LWxFUlO(y7J;h$BVx}^^t5rEIj6t
> zRVRxaTME(Nw_=_*|D<Md>1CH+aphINx%!$Hc0fFK;M(hcd;JX?`)~Z+h#nHd;`2Ly
> zKXB6@Zr*gut<OKlc27R_^tX8Z>p8LgcHZCScdcG?%BiPyuN99zw)t6c$LVMM>ddo#
> z9jsd~9>4pEt$)36|Jgm~oO@pHhV#D{e}lf;*1n|UgcFx8TQ0=kv1ck4Q*AZTqE(7c
> QS)s0MzqFbZ_CNi800lxdZvX%Q
> 
> literal 0
> HcmV?d00001
> 
> -- 
> 1.6.0.2
> 
> 
> 
> 

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [PATCH 6/6] [S390] Add firmware code

[Alexander Graf]

On 09.04.2010, at 22:17, Aurelien Jarno wrote:

> On Thu, Apr 01, 2010 at 06:42:41PM +0200, Alexander Graf wrote:
>> This patch adds a firmware blob to the S390 target. The blob is a simple
>> implementation of a virtio client that tries to read the second stage
>> bootloader from sectors described as of offset 0x20 in the MBR.
>> 
>> In combination with an updated zipl this allows for booting from virtio
>> block devices. This firmware is built from the same sources as the second
>> stage bootloader. You can find the zipl patch to build both here:
>> 
>> http://alex.csgraf.de/qemu/0001-Zipl-VirtIO-bootloader-code.patch
> 
> I am not fully comfortable introducing a binary firmware based on a
> patch posted on a website. I see two options:
> - Get your patch merged into ZIPL, so that we can build the firmware
>  directly from the ZIPL sources

IBM wants to keep the copyright on the zipl sources, so this one's out.

> - Add the patch to the pc-bios/ directory

Sounds good.

> Also it would be nice to update pc-bios/README to provide details about
> the ZIPL version used to build pc-bios/s390-zipl.rom.

Hrm. I wonder where the s390-tools package comes from. I'll see what I can do there.

> 
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>> ---
>> hw/s390-virtio.c      |   20 ++++++++++++++++++++
>> pc-bios/s390-zipl.rom |  Bin 0 -> 2448 bytes
>> 2 files changed, 20 insertions(+), 0 deletions(-)
>> create mode 100755 pc-bios/s390-zipl.rom
>> 
>> diff --git a/hw/s390-virtio.c b/hw/s390-virtio.c
>> index c36a8b2..91a3065 100644
>> --- a/hw/s390-virtio.c
>> +++ b/hw/s390-virtio.c
>> @@ -52,6 +52,10 @@
>> #define INITRD_PARM_SIZE                0x010410UL
>> #define PARMFILE_START                  0x001000UL
>> 
>> +#define ZIPL_START			0x001000UL
>> +#define ZIPL_LOAD_ADDR			0x001000UL
>> +#define ZIPL_FILENAME			"s390-zipl.rom"
>> +
>> #define MAX_BLK_DEVS                    10
>> 
>> static VirtIOS390Bus *s390_bus;
>> @@ -140,6 +144,8 @@ static void s390_init(ram_addr_t ram_size,
>>     ram_addr_t kernel_size = 0;
>>     ram_addr_t initrd_offset;
>>     ram_addr_t initrd_size = 0;
>> +    ram_addr_t bios_size = 0;
>> +    char *bios_filename;
>>     int i;
>> 
>>     /* XXX we only work on KVM for now */
>> @@ -178,6 +184,20 @@ static void s390_init(ram_addr_t ram_size,
>>     env->halted = 0;
>>     env->exception_index = 0;
>> 
>> +    /* Load zipl bootloader */
>> +    if (bios_name == NULL)
>> +        bios_name = ZIPL_FILENAME;
> 
> You are missing curly braces here.
> 
>> +    bios_filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
>> +    bios_size = load_image(bios_filename, qemu_get_ram_ptr(ZIPL_LOAD_ADDR));
>> +
>> +    if ((long)bios_size < 0) {
>> +        hw_error("could not load bootloader '%s'\n", bios_name);
>> +    }
>> +
>> +    env->psw.addr = ZIPL_START;
>> +    env->psw.mask = 0x0000000180000000ULL;
>> +
> 
> This probably has to be put in a reset handler so that this address is
> reloaded upon reboot.

A lot needs to go into a reset handler. In fact, we don't implement reset at all so far. I'd rather move it over to a reset handler when we actually introduce reset functionality.

> 
> Also do you really want to make the firmware mandatory? What about a
> warning and falling back to the direct kernel boot instead (if provided), 
> as it is already now. Some other machines are doing that.

Yes, I do. It doesn't hurt to have it loaded and on -kernel we can just set the PSW differently, thus making the guest jump directly into the kernel. So the firmware is loaded and completely ignored. That's btw what happens with this patch already. -kernel overrides the firmware.


Alex

Re: [Qemu-devel] [PATCH 6/6] [S390] Add firmware code

[Aurelien Jarno]

On Sat, Apr 10, 2010 at 01:29:55AM +0200, Alexander Graf wrote:
> 
> On 09.04.2010, at 22:17, Aurelien Jarno wrote:
> 
> > On Thu, Apr 01, 2010 at 06:42:41PM +0200, Alexander Graf wrote:
> >> This patch adds a firmware blob to the S390 target. The blob is a simple
> >> implementation of a virtio client that tries to read the second stage
> >> bootloader from sectors described as of offset 0x20 in the MBR.
> >> 
> >> In combination with an updated zipl this allows for booting from virtio
> >> block devices. This firmware is built from the same sources as the second
> >> stage bootloader. You can find the zipl patch to build both here:
> >> 
> >> http://alex.csgraf.de/qemu/0001-Zipl-VirtIO-bootloader-code.patch
> > 
> > I am not fully comfortable introducing a binary firmware based on a
> > patch posted on a website. I see two options:
> > - Get your patch merged into ZIPL, so that we can build the firmware
> >  directly from the ZIPL sources
> 
> IBM wants to keep the copyright on the zipl sources, so this one's out.

You can't transfer the copyright, as it is done for example for GNU
projects?

> > - Add the patch to the pc-bios/ directory
> 
> Sounds good.
> 
> > Also it would be nice to update pc-bios/README to provide details about
> > the ZIPL version used to build pc-bios/s390-zipl.rom.
> 
> Hrm. I wonder where the s390-tools package comes from. I'll see what I can do there.
> 
> > 
> >> Signed-off-by: Alexander Graf <agraf@suse.de>
> >> ---
> >> hw/s390-virtio.c      |   20 ++++++++++++++++++++
> >> pc-bios/s390-zipl.rom |  Bin 0 -> 2448 bytes
> >> 2 files changed, 20 insertions(+), 0 deletions(-)
> >> create mode 100755 pc-bios/s390-zipl.rom
> >> 
> >> diff --git a/hw/s390-virtio.c b/hw/s390-virtio.c
> >> index c36a8b2..91a3065 100644
> >> --- a/hw/s390-virtio.c
> >> +++ b/hw/s390-virtio.c
> >> @@ -52,6 +52,10 @@
> >> #define INITRD_PARM_SIZE                0x010410UL
> >> #define PARMFILE_START                  0x001000UL
> >> 
> >> +#define ZIPL_START			0x001000UL
> >> +#define ZIPL_LOAD_ADDR			0x001000UL
> >> +#define ZIPL_FILENAME			"s390-zipl.rom"
> >> +
> >> #define MAX_BLK_DEVS                    10
> >> 
> >> static VirtIOS390Bus *s390_bus;
> >> @@ -140,6 +144,8 @@ static void s390_init(ram_addr_t ram_size,
> >>     ram_addr_t kernel_size = 0;
> >>     ram_addr_t initrd_offset;
> >>     ram_addr_t initrd_size = 0;
> >> +    ram_addr_t bios_size = 0;
> >> +    char *bios_filename;
> >>     int i;
> >> 
> >>     /* XXX we only work on KVM for now */
> >> @@ -178,6 +184,20 @@ static void s390_init(ram_addr_t ram_size,
> >>     env->halted = 0;
> >>     env->exception_index = 0;
> >> 
> >> +    /* Load zipl bootloader */
> >> +    if (bios_name == NULL)
> >> +        bios_name = ZIPL_FILENAME;
> > 
> > You are missing curly braces here.
> > 
> >> +    bios_filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
> >> +    bios_size = load_image(bios_filename, qemu_get_ram_ptr(ZIPL_LOAD_ADDR));
> >> +
> >> +    if ((long)bios_size < 0) {
> >> +        hw_error("could not load bootloader '%s'\n", bios_name);
> >> +    }
> >> +
> >> +    env->psw.addr = ZIPL_START;
> >> +    env->psw.mask = 0x0000000180000000ULL;
> >> +
> > 
> > This probably has to be put in a reset handler so that this address is
> > reloaded upon reboot.
> 
> A lot needs to go into a reset handler. In fact, we don't implement reset at all so far. I'd rather move it over to a reset handler when we actually introduce reset functionality.

ok.

> > 
> > Also do you really want to make the firmware mandatory? What about a
> > warning and falling back to the direct kernel boot instead (if provided), 
> > as it is already now. Some other machines are doing that.
> 
> Yes, I do. It doesn't hurt to have it loaded and on -kernel we can just set the PSW differently, thus making the guest jump directly into the kernel. So the firmware is loaded and completely ignored. That's btw what happens with this patch already. -kernel overrides the firmware.
> 

That means people needs to have the firmware installed even if they
don't need it.

-- 
Aurelien Jarno	                        GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [PATCH 6/6] [S390] Add firmware code

[Alexander Graf]

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


On 10.04.2010, at 02:00, Aurelien Jarno wrote:

> On Sat, Apr 10, 2010 at 01:29:55AM +0200, Alexander Graf wrote:
>> 
>> On 09.04.2010, at 22:17, Aurelien Jarno wrote:
>> 
>>> On Thu, Apr 01, 2010 at 06:42:41PM +0200, Alexander Graf wrote:
>>>> This patch adds a firmware blob to the S390 target. The blob is a simple
>>>> implementation of a virtio client that tries to read the second stage
>>>> bootloader from sectors described as of offset 0x20 in the MBR.
>>>> 
>>>> In combination with an updated zipl this allows for booting from virtio
>>>> block devices. This firmware is built from the same sources as the second
>>>> stage bootloader. You can find the zipl patch to build both here:
>>>> 
>>>> http://alex.csgraf.de/qemu/0001-Zipl-VirtIO-bootloader-code.patch
>>> 
>>> I am not fully comfortable introducing a binary firmware based on a
>>> patch posted on a website. I see two options:
>>> - Get your patch merged into ZIPL, so that we can build the firmware
>>> directly from the ZIPL sources
>> 
>> IBM wants to keep the copyright on the zipl sources, so this one's out.
> 
> You can't transfer the copyright, as it is done for example for GNU
> projects?

I don't think so. Apart from it being illegal in Germany (you can't transfer full copyrights) I'm not sure that'd really help.

Another idea:

How about I set up a git tree on repo.or.cz and put it there? That git tree would contain all my changes, be a single public source and I'd try to pull all 'upstream' changes back in?

>>> 
>>> Also do you really want to make the firmware mandatory? What about a
>>> warning and falling back to the direct kernel boot instead (if provided), 
>>> as it is already now. Some other machines are doing that.
>> 
>> Yes, I do. It doesn't hurt to have it loaded and on -kernel we can just set the PSW differently, thus making the guest jump directly into the kernel. So the firmware is loaded and completely ignored. That's btw what happens with this patch already. -kernel overrides the firmware.
>> 
> 
> That means people needs to have the firmware installed even if they
> don't need it.

I don't see a problem there. It's less than 4k. Plus it's mandatory for x86 and ppc too, so why make it different?


Alex


[-- Attachment #2: Type: text/html, Size: 4819 bytes --]

Re: [Qemu-devel] [PATCH 6/6] [S390] Add firmware code

[Aurelien Jarno]

On Sat, Apr 10, 2010 at 11:22:37AM +0200, Alexander Graf wrote:
> 
> On 10.04.2010, at 02:00, Aurelien Jarno wrote:
> 
> > On Sat, Apr 10, 2010 at 01:29:55AM +0200, Alexander Graf wrote:
> >> 
> >> On 09.04.2010, at 22:17, Aurelien Jarno wrote:
> >> 
> >>> On Thu, Apr 01, 2010 at 06:42:41PM +0200, Alexander Graf wrote:
> >>>> This patch adds a firmware blob to the S390 target. The blob is a simple
> >>>> implementation of a virtio client that tries to read the second stage
> >>>> bootloader from sectors described as of offset 0x20 in the MBR.
> >>>> 
> >>>> In combination with an updated zipl this allows for booting from virtio
> >>>> block devices. This firmware is built from the same sources as the second
> >>>> stage bootloader. You can find the zipl patch to build both here:
> >>>> 
> >>>> http://alex.csgraf.de/qemu/0001-Zipl-VirtIO-bootloader-code.patch
> >>> 
> >>> I am not fully comfortable introducing a binary firmware based on a
> >>> patch posted on a website. I see two options:
> >>> - Get your patch merged into ZIPL, so that we can build the firmware
> >>> directly from the ZIPL sources
> >> 
> >> IBM wants to keep the copyright on the zipl sources, so this one's out.
> > 
> > You can't transfer the copyright, as it is done for example for GNU
> > projects?
> 
> I don't think so. Apart from it being illegal in Germany (you can't transfer full copyrights) I'm not sure that'd really help.
> 
> Another idea:
> 
> How about I set up a git tree on repo.or.cz and put it there? That git tree would contain all my changes, be a single public source and I'd try to pull all 'upstream' changes back in?

Also looks a good idea.

> >>> 
> >>> Also do you really want to make the firmware mandatory? What about a
> >>> warning and falling back to the direct kernel boot instead (if provided), 
> >>> as it is already now. Some other machines are doing that.
> >> 
> >> Yes, I do. It doesn't hurt to have it loaded and on -kernel we can just set the PSW differently, thus making the guest jump directly into the kernel. So the firmware is loaded and completely ignored. That's btw what happens with this patch already. -kernel overrides the firmware.
> >> 
> > 
> > That means people needs to have the firmware installed even if they
> > don't need it.
> 
> I don't see a problem there. It's less than 4k. Plus it's mandatory for x86 and ppc too, so why make it different?
> 

It's mandatory for x86 and ppc as the bootloader is actually doing the
jump to the kernel (for ppc it even provide some services to the kernel).

The main problem I see is for distributions that want to rebuild the
firmwares from sources. Also making it optional is just a few lines
more.

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [PATCH 6/6] [S390] Add firmware code

[Carsten Otte]

Aurelien Jarno wrote:
> I am not fully comfortable introducing a binary firmware based on a
> patch posted on a website. I see two options:
> - Get your patch merged into ZIPL, so that we can build the firmware
>   directly from the ZIPL sources
> - Add the patch to the pc-bios/ directory
Looks like we'll have to sort out how to work with patches against 
s390-tools from the legal side. So far s390-tools is IBM-Code only and 
released as GPL, maybe it needs to go to sourceforge or alike in order 
be able to receive contributions. While we're at it, I think it would be 
good to have it as a blob in QEMU so that users can actually boot :-).