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 <address@hidden>
> >> ---
> >> 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
address@hidden                 http://www.aurel32.net