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