[Qemu-devel] [PULL 0/3] s390-ccw.img: fix sporadic boot errors

[Qemu-devel] [PULL 0/3] s390-ccw.img: fix sporadic boot errors

[Christian Borntraeger]

Peter, Anthony,

The following changes since commit 9d74f6fef0801ca2ce5c9d38d59b85bf03c27669:

  Merge remote-tracking branch 'remotes/alon/pull-libcacard.glusterfs' into staging (2014-02-12 17:53:31 +0000)

are available in the git repository at:


  git://github.com/borntraeger/qemu.git tags/ipl-20140213

for you to fetch changes up to 669d13daaf31e81463d081f59ee88e4f75dc7538:

  s390-ccw.img: new binary rom to match latest fixes (2014-02-13 09:59:21 +0100)


These changes make the reipl more robust.
----------------------------------------------------------------
This fixes two sporadic errors on reboot when the s390-ccw.img
is used. Both problems are related to qemus elf loader only caring
about PT_LOAD sections: This results in a non-zero bss section
after reboot. This makes static variables non-zero.

This quick fix initializes all variables. Long term we might
also want to clear out the bss section.

----------------------------------------------------------------
Christian Borntraeger (3):
      s390-ccw.img: Fix sporadic reboot hangs: Initialize next_idx
      s390-ccw.img: Fix sporadic errors with ccw boot image - initialize css
      s390-ccw.img: new binary rom to match latest fixes

 pc-bios/s390-ccw.img      | Bin 9336 -> 9336 bytes
 pc-bios/s390-ccw/main.c   |   3 +--
 pc-bios/s390-ccw/virtio.c |   1 +
 3 files changed, 2 insertions(+), 2 deletions(-)

[Qemu-devel] [PULL 1/3] s390-ccw.img: Fix sporadic reboot hangs: Initialize next_idx

[Christian Borntraeger]

The current code does not initialize next_idx as the qemu
elf loader does not zero the bss section.
Make the initialization explicit.

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 pc-bios/s390-ccw/virtio.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/pc-bios/s390-ccw/virtio.c b/pc-bios/s390-ccw/virtio.c
index 4d6e48f..a46914d 100644
--- a/pc-bios/s390-ccw/virtio.c
+++ b/pc-bios/s390-ccw/virtio.c
@@ -124,6 +124,7 @@ static void vring_init(struct vring *vr, unsigned int num, void *p,
     vr->used->flags = VRING_USED_F_NO_NOTIFY;
     vr->used->idx = 0;
     vr->used_idx = 0;
+    vr->next_idx = 0;
 
     debug_print_addr("init vr", vr);
 }
-- 
1.8.4.2

[Qemu-devel] [PULL 2/3] s390-ccw.img: Fix sporadic errors with ccw boot image - initialize css

[Christian Borntraeger]

We have to set the cssid to 0, otherwise the stsch code will
return an operand exception without the m bit. In the same way
we should set m=0.

This case was triggered in some cases during reboot, if for some
reason the location of blk_schid.cssid contains 1 and m was 0.
Turns out that the qemu elf loader does not zero out the bss section.
On first boot this is ok, but on reboots this might fail.

The symptom was an dump of the old kernel with several areas
overwritten. The bootloader does not register a program check
handler, so bios exception jumped back into the old kernel.

Lets just use a local struct with a designed initializer. That
will guarantee that all other subelements are initialized to 0.

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 pc-bios/s390-ccw/main.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
index c5d5332..d7fc727 100644
--- a/pc-bios/s390-ccw/main.c
+++ b/pc-bios/s390-ccw/main.c
@@ -10,7 +10,6 @@
 
 #include "s390-ccw.h"
 
-struct subchannel_id blk_schid;
 char stack[PAGE_SIZE * 8] __attribute__((__aligned__(PAGE_SIZE)));
 uint64_t boot_value;
 
@@ -23,13 +22,13 @@ void virtio_panic(const char *string)
 
 static void virtio_setup(uint64_t dev_info)
 {
+    struct subchannel_id blk_schid = { .one = 1};
     struct schib schib;
     int i;
     int r;
     bool found = false;
     bool check_devno = false;
     uint16_t dev_no = -1;
-    blk_schid.one = 1;
 
     if (dev_info != -1) {
         check_devno = true;
-- 
1.8.4.2

[Qemu-devel] [PULL 3/3] s390-ccw.img: new binary rom to match latest fixes

[Christian Borntraeger]

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 pc-bios/s390-ccw.img | Bin 9336 -> 9336 bytes
 1 file changed, 0 insertions(+), 0 deletions(-)

diff --git a/pc-bios/s390-ccw.img b/pc-bios/s390-ccw.img
index 6727f0ca39d6bf6d114974d1535cb7ad9e56355f..f6223e77c2aacfa86652d63b773dc05eca55570d 100644
GIT binary patch
delta 1460
zcmYLJeN0<b6hF7G@XCkqN=tdvmX?pn3@R*HV7iTuVX$VAG)}j&put29e-LGcWN1ui
z45AE}tCuWIX52H<J&ZGM;1XH<<0V7g2TmeeAWGP<@K+aom^uNW*K;om-lX?;-#Nc~
z&hMOiZ&w4Wfm1%x9`QG2<|-gYt`h02A$uaDAJ|Q9knd(~<9I!}k<;_Geia~YGCw<J
zECZ0=3<YdQ{)(T%2v+?pV|p)~Ond<fWFu30L7TQZulE~Zk6j0%8<4JdL7(0YgnQIN
z{ab*T3m~YNBMo8myxz`O>{B598rap=*kQJ&?u}Igs0Y<YTt{O^dw|4_0JsjR`7swd
zzjwJ~ZnPDk%`vt+3}YqW|IiN}{RxmziZepYi-_5I$8mPV6>|(CEtrhk^vfveG`nf|
zQG1G)0|AjT-{nR@TC54+G%-KBC7IeZ9IiLDU=L1enL5eI)&qGZdM~>Z@|zm}mik19
zMYN89O$5EdEu=%RH9ZNSkK!+iUFt?mvas+p^L~g}-C|c%#E}wznw?OFy>5HZ>wvMm
z8`Hka10~Ly0q)#HXG7Hk0rhwoVEyVg>Qw+M+!8TLd+ik@mtL?J<V$fsr%YG?rNh&P
z0Wu?W-rne&<w-3|LdH)*Mo7q*5$l&}ZQenWLr>)G>e+8pft>2o9~BoN#Ewl^K@@5k
z<oTtwkfMkKZBdME71D~a%oy7d_anzDBD^LtzJ(dPpct)ksuh)w{I6U>O}HoPwmc1n
zf_f`Bt2*^;;^!3`Sg{lyDv1;u76Q@&mntm^*>x!DF|m3|SkKT-hu3iidmfi%BZ$0T
zFqkd9fNG9La*-}M{FbyOn(nM5Gql?IIJq?bs<WJs9dt1Nb2L@C2FMWo&J`!KR4dp|
zCg?&zF`4;TZOnQ<0OmO!?9e1_R)-`|tn^paN0M}1t$1lcbo3q<LCSeO8&bS}WhYn@
zU-Kg5r0}G1&%gIk-f>w@+%ems%$UX<rTUYwq_+@!aw$CNS!Qac)_|9g^EWTh_X_vQ
z>)1_OmP98D8_6QIx&3Y#hhc*4`6;6f=WFrKHh?lcxqgM#y5F%e27E}=aI+pfF)$eW
zyBASI)DTU^?yq9@D#reqN5n_D>PHMCP9Y+1Z71SY#7V?e#@2`N_NZ6`UT{kgtE@sq
z{>DB;yzrYNh;xjkteDt_Irwk_&{~g|%+pqnFD-zPx@g$ras_k3(iQ_}3;-RBB}eIb
zPaQGnJx@(x2igv7GXOle-aye&y3gw)Wwg~>PC~TTTUm_q;3hN;WDMFYKTO@&-h&T`
z3}ZQ6^p>}V1Zj3r<t~&CncEDJ+2X_!KSA#6&~ze`M;Ve&rhk;ynlopytkaI7axzJW
ziz?IbNx~cB5i(#Y#tM05AU)IkB0TqH)3t$1e}rpZ`7!<X{*rT-9r{D`o8knCQmv$p
l?4fr{-pq(@C3Qz>sI(@--1>Uhq*46BKMFoP-E^*0`wtZv7?}V7

delta 1362
zcmYLIZA@EL7=BN0;TBlP?Z-;n_2U*;z`%sX1sJH{Ky0#U2+mlHKdOm_#APxi1kE!1
zKt;!N-I*DSm^9k%GBaZXnm9G&M(45wWHoG&7>y*C{V>IFmvz`p?D0Kk3!dcmdGEQ;
z`@GL{?rlDjj~ow?+PK<XTAl&1$}{nSX3`O#ddHsT8}f45CeCz_x$==dJqETX3x>&R
z*&+QJ0GW*{G0;`qT}%0Z^x&P*r~+8EcE+?(IFPLZ1-7Kz?3U-{{b~l@Rl)9daGs_>
zu{H?LXcIuRuK^xPm`QDnvD6SiY5@pW_Ozuc@U98KQ|oq)@V4n)37D<~bxZ}nb}vXc
z#2F{%S;R8OY5a8<iuoDDy%kt*(Y`_Pi|jYF2-YmO47;N$TE@z{$5E+`GE-O3{>iAS
zh+XL|!CJ;=NwA5&SrPRGeJ^kNNXr3e!<cRYF~?Fu>QSCT$bBqYJ`^HWS<bGR15k}8
z7Q1_55y2m(<MxpK>{ZE3D1b)q3`)}P>~%ZpjP!92SUFGfqUInpqeYp8473Q=^A<qc
z(536R7oaXjXXpm!T2{pTpTzwAV*aA2pQ1yKy`+?W@7OuATW<up@QHR00D6kDL+7pV
zgXkW|86e$^Dhl){X0lJwH+@Ne>DL%z+YJ>tR-y2^=y^%F3v+jcVa$^JKZ~>|ddjZK
z8PFB<TgO>Vh3B%DuiL<iQMgnNWo;M)q+hvJ>33nf9Zl7Xs#WZsqEpU*^AsMoA<KFX
z@)l6cR(u~_a(0r>so$kqih(}pY9MR$plb*Dd1lg8L&(GQ!q!j84chNMimaU5Ad7UV
z;sNp%b$i0(GHvm6mc1DP^9)yee1*R4IYukIL9$MpymjsT3m9dq+yU9n!)z289oK8|
zGx~xjkaOaR#Gm|=M{lp-%l^achiW~89buaA2FNs>^|r}*JThBZ_=K;MFxur)-MR_3
z-c|l*>iyFghyhe<x!VRk;(OHwm<}&Ev5k#jQqUQ@IgXe>oI*4i`x}8YBCa6zGnTJK
zd=2pmBJyrEB3?qAN6a&JdlEkm54ubv_F#?{MC5ODAx<#%FX~#B7%Sj#2`{l+0Q87I
zKvw9aKU6e^O087)yWPFzdCOf2!2MVqU@VuWtNwO!iPi_2y@R;7XOjWo$9jXY^#nZ}
z2$6a^8K@yKdLhsdMtiUT-A7BtZI+AkbDKT8aM($VbC|k<%_K_q1sisvbtv6rh{+Zg
zMoc32S?GSRl&c(*-!J~4=;`t$i)Edj3)Y~cE5U}MJoX)&iIX`?*oq}41L={+o&f1w
z=P!lRTid?<@%f?ozScKl!3d>ahO;;_QQ1y9=yK(YCDA)WeNh^%YA)e6aBWM|3B1E|
M<jdbem#Px~0b+sOZ~y=R

-- 
1.8.4.2

Re: [Qemu-devel] [PULL 0/3] s390-ccw.img: fix sporadic boot errors

[Peter Maydell]

On 13 February 2014 09:17, Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> Peter, Anthony,
>
> The following changes since commit 9d74f6fef0801ca2ce5c9d38d59b85bf03c27669:
>
>   Merge remote-tracking branch 'remotes/alon/pull-libcacard.glusterfs' into staging (2014-02-12 17:53:31 +0000)
>
> are available in the git repository at:
>
>
>   git://github.com/borntraeger/qemu.git tags/ipl-20140213
>
> for you to fetch changes up to 669d13daaf31e81463d081f59ee88e4f75dc7538:
>
>   s390-ccw.img: new binary rom to match latest fixes (2014-02-13 09:59:21 +0100)
>
>
> These changes make the reipl more robust.
> ----------------------------------------------------------------
> This fixes two sporadic errors on reboot when the s390-ccw.img
> is used. Both problems are related to qemus elf loader only caring
> about PT_LOAD sections: This results in a non-zero bss section
> after reboot. This makes static variables non-zero.
>
> This quick fix initializes all variables. Long term we might
> also want to clear out the bss section.
>
> ----------------------------------------------------------------
> Christian Borntraeger (3):
>       s390-ccw.img: Fix sporadic reboot hangs: Initialize next_idx
>       s390-ccw.img: Fix sporadic errors with ccw boot image - initialize css
>       s390-ccw.img: new binary rom to match latest fixes

I couldn't find these patches anywhere on the list when I searched
my archives -- have they been through the usual review process?

thanks
-- PMM

Re: [Qemu-devel] [PULL 0/3] s390-ccw.img: fix sporadic boot errors

[Christian Borntraeger]

On 13/02/14 10:21, Peter Maydell wrote:
> On 13 February 2014 09:17, Christian Borntraeger <borntraeger@de.ibm.com> wrote:
>> Peter, Anthony,
>>
>> The following changes since commit 9d74f6fef0801ca2ce5c9d38d59b85bf03c27669:
>>
>>   Merge remote-tracking branch 'remotes/alon/pull-libcacard.glusterfs' into staging (2014-02-12 17:53:31 +0000)
>>
>> are available in the git repository at:
>>
>>
>>   git://github.com/borntraeger/qemu.git tags/ipl-20140213
>>
>> for you to fetch changes up to 669d13daaf31e81463d081f59ee88e4f75dc7538:
>>
>>   s390-ccw.img: new binary rom to match latest fixes (2014-02-13 09:59:21 +0100)
>>
>>
>> These changes make the reipl more robust.
>> ----------------------------------------------------------------
>> This fixes two sporadic errors on reboot when the s390-ccw.img
>> is used. Both problems are related to qemus elf loader only caring
>> about PT_LOAD sections: This results in a non-zero bss section
>> after reboot. This makes static variables non-zero.
>>
>> This quick fix initializes all variables. Long term we might
>> also want to clear out the bss section.
>>
>> ----------------------------------------------------------------
>> Christian Borntraeger (3):
>>       s390-ccw.img: Fix sporadic reboot hangs: Initialize next_idx
>>       s390-ccw.img: Fix sporadic errors with ccw boot image - initialize css
>>       s390-ccw.img: new binary rom to match latest fixes
> 
> I couldn't find these patches anywhere on the list when I searched
> my archives -- have they been through the usual review process?

You are right, not yet on qemu-devel. Since these patches are also part
of the mail thread, lets just wait some days and let also wait for
Alex' NACK/ACK.

Christian

Re: [Qemu-devel] [PULL 1/3] s390-ccw.img: Fix sporadic reboot hangs: Initialize next_idx

[Cornelia Huck]

On Thu, 13 Feb 2014 10:17:09 +0100
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> The current code does not initialize next_idx as the qemu
> elf loader does not zero the bss section.
> Make the initialization explicit.
> 
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  pc-bios/s390-ccw/virtio.c | 1 +
>  1 file changed, 1 insertion(+)

Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>

Re: [Qemu-devel] [PULL 2/3] s390-ccw.img: Fix sporadic errors with ccw boot image - initialize css

[Cornelia Huck]

On Thu, 13 Feb 2014 10:17:10 +0100
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> We have to set the cssid to 0, otherwise the stsch code will
> return an operand exception without the m bit. In the same way
> we should set m=0.
> 
> This case was triggered in some cases during reboot, if for some
> reason the location of blk_schid.cssid contains 1 and m was 0.
> Turns out that the qemu elf loader does not zero out the bss section.
> On first boot this is ok, but on reboots this might fail.
> 
> The symptom was an dump of the old kernel with several areas
> overwritten. The bootloader does not register a program check
> handler, so bios exception jumped back into the old kernel.
> 
> Lets just use a local struct with a designed initializer. That
> will guarantee that all other subelements are initialized to 0.
> 
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  pc-bios/s390-ccw/main.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)

Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>

Re: [Qemu-devel] [PULL 2/3] s390-ccw.img: Fix sporadic errors with ccw boot image - initialize css

[Peter Maydell]

On 13 February 2014 09:17, Christian Borntraeger <borntraeger@de.ibm.com> wrote:
>  static void virtio_setup(uint64_t dev_info)
>  {
> +    struct subchannel_id blk_schid = { .one = 1};

Missing space before the "}" I think.

thanks
-- PMM

Re: [Qemu-devel] [PULL 2/3] s390-ccw.img: Fix sporadic errors with ccw boot image - initialize css

[Christian Borntraeger]

On 13/02/14 10:55, Peter Maydell wrote:
> On 13 February 2014 09:17, Christian Borntraeger <borntraeger@de.ibm.com> wrote:
>>  static void virtio_setup(uint64_t dev_info)
>>  {
>> +    struct subchannel_id blk_schid = { .one = 1};
> 
> Missing space before the "}" I think.

checkpatch accepts both ways:
a)    struct subchannel_id blk_schid = { .one = 1};
b)    struct subchannel_id blk_schid = { .one = 1 };

so, change it or keep it?

Christian

Re: [Qemu-devel] [PULL 2/3] s390-ccw.img: Fix sporadic errors with ccw boot image - initialize css

[Peter Maydell]

On 13 February 2014 10:05, Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> On 13/02/14 10:55, Peter Maydell wrote:
>> On 13 February 2014 09:17, Christian Borntraeger <borntraeger@de.ibm.com> wrote:
>>>  static void virtio_setup(uint64_t dev_info)
>>>  {
>>> +    struct subchannel_id blk_schid = { .one = 1};
>>
>> Missing space before the "}" I think.
>
> checkpatch accepts both ways:
> a)    struct subchannel_id blk_schid = { .one = 1};
> b)    struct subchannel_id blk_schid = { .one = 1 };
>
> so, change it or keep it?

checkpatch isn't infallible. I think having the space
looks better. In any case you should be consistent
about whether you use a space with both the opening
and the closing brace -- at the moment you've got a
space at one end and not the other.

thanks
-- PMM

Re: [Qemu-devel] [PULL 2/3] s390-ccw.img: Fix sporadic errors with ccw boot image - initialize css

[Christian Borntraeger]

On 13/02/14 12:04, Peter Maydell wrote:
> On 13 February 2014 10:05, Christian Borntraeger <borntraeger@de.ibm.com> wrote:
>> On 13/02/14 10:55, Peter Maydell wrote:
>>> On 13 February 2014 09:17, Christian Borntraeger <borntraeger@de.ibm.com> wrote:
>>>>  static void virtio_setup(uint64_t dev_info)
>>>>  {
>>>> +    struct subchannel_id blk_schid = { .one = 1};
>>>
>>> Missing space before the "}" I think.
>>
>> checkpatch accepts both ways:
>> a)    struct subchannel_id blk_schid = { .one = 1};
>> b)    struct subchannel_id blk_schid = { .one = 1 };
>>
>> so, change it or keep it?
> 
> checkpatch isn't infallible. I think having the space
> looks better. In any case you should be consistent
> about whether you use a space with both the opening
> and the closing brace -- at the moment you've got a
> space at one end and not the other.
> 
> thanks

Ok changed. Will wait for some more feedback and send
an updated pull requests.

Christian

Re: [Qemu-devel] [PULL 1/3] s390-ccw.img: Fix sporadic reboot hangs: Initialize next_idx

[Richard Henderson]

On 02/13/2014 01:17 AM, Christian Borntraeger wrote:
> The current code does not initialize next_idx as the qemu
> elf loader does not zero the bss section.
> Make the initialization explicit.
> 
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  pc-bios/s390-ccw/virtio.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/pc-bios/s390-ccw/virtio.c b/pc-bios/s390-ccw/virtio.c
> index 4d6e48f..a46914d 100644
> --- a/pc-bios/s390-ccw/virtio.c
> +++ b/pc-bios/s390-ccw/virtio.c
> @@ -124,6 +124,7 @@ static void vring_init(struct vring *vr, unsigned int num, void *p,
>      vr->used->flags = VRING_USED_F_NO_NOTIFY;
>      vr->used->idx = 0;
>      vr->used_idx = 0;
> +    vr->next_idx = 0;
>  
>      debug_print_addr("init vr", vr);
>  }
> 

FWIW, I believe that rom_reset needs to do this re-zeroing of the bss.
That seems to be the only place we don't take care for datasize != romsize.


r~

Re: [Qemu-devel] [PULL 1/3] s390-ccw.img: Fix sporadic reboot hangs: Initialize next_idx

[Christian Borntraeger]

On 13/02/14 16:15, Richard Henderson wrote:
> On 02/13/2014 01:17 AM, Christian Borntraeger wrote:
>> The current code does not initialize next_idx as the qemu
>> elf loader does not zero the bss section.
>> Make the initialization explicit.
>>
>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> ---
>>  pc-bios/s390-ccw/virtio.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/pc-bios/s390-ccw/virtio.c b/pc-bios/s390-ccw/virtio.c
>> index 4d6e48f..a46914d 100644
>> --- a/pc-bios/s390-ccw/virtio.c
>> +++ b/pc-bios/s390-ccw/virtio.c
>> @@ -124,6 +124,7 @@ static void vring_init(struct vring *vr, unsigned int num, void *p,
>>      vr->used->flags = VRING_USED_F_NO_NOTIFY;
>>      vr->used->idx = 0;
>>      vr->used_idx = 0;
>> +    vr->next_idx = 0;
>>  
>>      debug_print_addr("init vr", vr);
>>  }
>>
> 
> FWIW, I believe that rom_reset needs to do this re-zeroing of the bss.
> That seems to be the only place we don't take care for datasize != romsize.
> 

Indeed, initializing the data as in my patches isnt wrong (and allows to move
that structures around e.g. from a global variable to stack), so it still makes
sense to apply both patches,  but the main problem was that the bss section is 
not cleared on reset.

So we need to memset from rom->data+rom->datasize  to rom->data+rom->romsize
to avoid more of these kind of problems in an add-on patch.

Christian

[Qemu-devel] [PATCH/RFC] clear bss memory of ROMS

[Christian Borntraeger]

On 13/02/14 20:39, Christian Borntraeger wrote:
> On 13/02/14 16:15, Richard Henderson wrote:
>> On 02/13/2014 01:17 AM, Christian Borntraeger wrote:
>>> The current code does not initialize next_idx as the qemu
>>> elf loader does not zero the bss section.
>>> Make the initialization explicit.
>>>
>>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>>> ---
>>>  pc-bios/s390-ccw/virtio.c | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/pc-bios/s390-ccw/virtio.c b/pc-bios/s390-ccw/virtio.c
>>> index 4d6e48f..a46914d 100644
>>> --- a/pc-bios/s390-ccw/virtio.c
>>> +++ b/pc-bios/s390-ccw/virtio.c
>>> @@ -124,6 +124,7 @@ static void vring_init(struct vring *vr, unsigned int num, void *p,
>>>      vr->used->flags = VRING_USED_F_NO_NOTIFY;
>>>      vr->used->idx = 0;
>>>      vr->used_idx = 0;
>>> +    vr->next_idx = 0;
>>>  
>>>      debug_print_addr("init vr", vr);
>>>  }
>>>
>>
>> FWIW, I believe that rom_reset needs to do this re-zeroing of the bss.
>> That seems to be the only place we don't take care for datasize != romsize.
>>
> 
> Indeed, initializing the data as in my patches isnt wrong (and allows to move
> that structures around e.g. from a global variable to stack), so it still makes
> sense to apply both patches,  but the main problem was that the bss section is 
> not cleared on reset.
> 
> So we need to memset from rom->data+rom->datasize  to rom->data+rom->romsize
> to avoid more of these kind of problems in an add-on patch.

To correct myself. Actually only Patch 2/3 would be fixed by zeroing the bss.
Patch 1/3 is still necessary, since the bios creates the virtqueue not in bss but
in real memory. Still, bss clearing seems like a good idea, so what about something
like the following:


loader: reset bss sections of ROMS

The bss section of ELF roms must be zeroed on reset.

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
[cborntra@r17lp39 qemu]$ git diff
diff --git a/exec.c b/exec.c
index b69fd29..f0f6a94 100644
--- a/exec.c
+++ b/exec.c
@@ -2097,6 +2097,30 @@ void cpu_physical_memory_rw(hwaddr addr, uint8_t *buf,
     address_space_rw(&address_space_memory, addr, buf, len, is_write);
 }
 
+void cpu_physical_memory_clear_rom(AddressSpace *as, hwaddr addr, size_t len)
+{
+    hwaddr l;
+    uint8_t *ptr;
+    hwaddr addr1;
+    MemoryRegion *mr;
+
+    while (len > 0) {
+        l = len;
+        mr = address_space_translate(as, addr, &addr1, &l, true);
+
+        if (!(memory_region_is_ram(mr) ||
+              memory_region_is_romd(mr))) {
+            /* do nothing */
+        } else {
+            addr1 += memory_region_get_ram_addr(mr);
+            ptr = qemu_get_ram_ptr(addr1);
+            memset(ptr, 0, l);
+        }
+        len -= l;
+        addr += l;
+    }
+}
+
 enum write_rom_type {
     WRITE_DATA,
     FLUSH_CACHE,
diff --git a/hw/core/loader.c b/hw/core/loader.c
index e1a8318..7998a3e 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -786,13 +786,20 @@ static void rom_reset(void *unused)
         if (rom->fw_file) {
             continue;
         }
-        if (rom->data == NULL) {
-            continue;
-        }
         if (rom->mr) {
             void *host = memory_region_get_ram_ptr(rom->mr);
+            memset(host + rom->datasize, 0, rom->romsize - rom->datasize);
+            if (rom->data == NULL) {
+                continue;
+            }
             memcpy(host, rom->data, rom->datasize);
         } else {
+            cpu_physical_memory_clear_rom(&address_space_memory,
+                                          rom->addr + rom->datasize,
+                                          rom->romsize - rom->datasize);
+            if (rom->data == NULL) {
+                continue;
+            }
             cpu_physical_memory_write_rom(&address_space_memory,
                                           rom->addr, rom->data, rom->datasize);
         }
diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
index a21b65a..948de83 100644
--- a/include/exec/cpu-common.h
+++ b/include/exec/cpu-common.h
@@ -108,6 +108,7 @@ void stl_phys(AddressSpace *as, hwaddr addr, uint32_t val);
 void stq_phys(AddressSpace *as, hwaddr addr, uint64_t val);
 #endif
 
+void cpu_physical_memory_clear_rom(AddressSpace *as, hwaddr addr, size_t len);
 void cpu_physical_memory_write_rom(AddressSpace *as, hwaddr addr,
                                    const uint8_t *buf, int len);
 void cpu_flush_icache_range(hwaddr start, int len);