Patchwork [6/6,S390] Add firmware code

login
register
mail settings
Submitter Alexander Graf
Date April 1, 2010, 4:42 p.m.
Message ID <1270140161-17216-7-git-send-email-agraf@suse.de>
Download mbox | patch
Permalink /patch/49223/
State New
Headers show

Comments

Alexander Graf - April 1, 2010, 4:42 p.m.
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/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
Bastian Blank - April 1, 2010, 9:18 p.m.
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
Alexander Graf - April 1, 2010, 10:10 p.m.
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
Aurelien Jarno - April 9, 2010, 8:17 p.m.
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
> 
> 
> 
>
Alexander Graf - April 9, 2010, 11:29 p.m.
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
Aurelien Jarno - April 10, 2010, midnight
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.
Alexander Graf - April 10, 2010, 9:22 a.m.
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
Aurelien Jarno - April 10, 2010, 3:03 p.m.
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.
Carsten Otte - April 12, 2010, 8:43 a.m.
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 :-).

Patch

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));