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

[Alexander Graf]

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 <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.

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