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

[Aurelien Jarno <aurelien@aurel32.net>]

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