Re: [Qemu-devel] [qemu PATCH] hw/i386/pc_sysfw: support more than one flash drive

[Markus Armbruster <armbru@redhat.com>]

Laszlo Ersek <lersek@redhat.com> writes:

> On 11/26/13 14:11, Markus Armbruster wrote:
>> Laszlo Ersek <lersek@redhat.com> writes:
>> 
>>> On 11/25/13 16:32, Markus Armbruster wrote:
>>>> Laszlo Ersek <lersek@redhat.com> writes:
>>>>
>>>>> This patch allows the user to usefully specify
>>>>>
>>>>>   -drive file=img_1,if=pflash,format=raw,readonly \
>>>>>   -drive file=img_2,if=pflash,format=raw
>>>>>
>>>>> on the command line. The flash images will be mapped under 4G in their
>>>>> reverse unit order -- that is, with their base addresses progressing
>>>>> downwards, in increasing unit order.
>>>>>
>>>>> (The unit number increases with command line order if not explicitly
>>>>> specified.)
>>>>>
>>>>> This accommodates the following use case: suppose that OVMF is split in
>>>>> two parts, a writeable host file for non-volatile variable storage, and a
>>>>> read-only part for bootstrap and decompressible executable code.
>>>>>
>>>>> The binary code part would be read-only, centrally managed on the host
>>>>> system, and passed in as unit 0. The variable store would be writeable,
>>>>> VM-specific, and passed in as unit 1.
>>>>>
>>>>>   00000000ffe00000-00000000ffe1ffff (prio 0, R-): system.flash1
>>>>>   00000000ffe20000-00000000ffffffff (prio 0, R-): system.flash0
>>>>>
>>>>> (If the guest tries to write to the flash range that is backed by the
>>>>> read-only drive, bdrv_write() in pflash_update() will correctly deny the
>>>>> write with -EACCES, and pflash_update() won't care, which suits us well.)
>>>>>
>>>>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>>>>> ---
>>>>>  hw/i386/pc_sysfw.c | 60 ++++++++++++++++++++++++++++++++++++++++--------------
>>>>>  1 file changed, 45 insertions(+), 15 deletions(-)
>>>>>
>>>>> diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
>>>>> index e917c83..1c3e3d6 100644
>>>>> --- a/hw/i386/pc_sysfw.c
>>>>> +++ b/hw/i386/pc_sysfw.c
>>>>> @@ -72,35 +72,65 @@ static void pc_isa_bios_init(MemoryRegion *rom_memory,
>>>>>      memory_region_set_readonly(isa_bios, true);
>>>>>  }
>>>>>  
>>>>> +/* This function maps flash drives from 4G downward, in order of their unit
>>>>> + * numbers. Addressing within one flash drive is of course not reversed.
>>>>> + *
>>>>> + * The drive with unit number 0 is mapped at the highest address, and it is
>>>>> + * passed to pc_isa_bios_init(). Merging severral drives for isa-bios is not
>>>>> + * supported.
>>>>> + *
>>>>> + * The caller is responsible to pass in the non-NULL @pflash_drv that
>>>>> + * corresponds to the flash drive with unit number 0.
>>>>> + */
>>>>>  static void pc_system_flash_init(MemoryRegion *rom_memory,
>>>>>                                   DriveInfo *pflash_drv)
>>>>>  {
>>>>> +    int unit = 0;
>>>>>      BlockDriverState *bdrv;
>>>>>      int64_t size;
>>>>> -    hwaddr phys_addr;
>>>>> +    hwaddr phys_addr = 0x100000000ULL;
>>>>>      int sector_bits, sector_size;
>>>>>      pflash_t *system_flash;
>>>>>      MemoryRegion *flash_mem;
>>>>> +    char name[64];
>>>>>  
>>>>> -    bdrv = pflash_drv->bdrv;
>>>>> -    size = bdrv_getlength(pflash_drv->bdrv);
>>>>>      sector_bits = 12;
>>>>>      sector_size = 1 << sector_bits;
>>>>>  
>>>>> -    if ((size % sector_size) != 0) {
>>>>> -        fprintf(stderr,
>>>>> -                "qemu: PC system firmware (pflash) must be a multiple of 0x%x\n",
>>>>> -                sector_size);
>>>>> -        exit(1);
>>>>> -    }
>>>>> +    do {
>>>>> +        bdrv = pflash_drv->bdrv;
>>>>> +        size = bdrv_getlength(bdrv);
>>>>> +        if ((size % sector_size) != 0) {
>>>>> +            fprintf(stderr,
>>>>> +                    "qemu: PC system firmware (pflash segment %d) must be a "
>>>>> +                    "multiple of 0x%x\n", unit, sector_size);
>>>>> +            exit(1);
>>>>> +        }
>>>>> +        if (size > phys_addr) {
>>>>> +            fprintf(stderr, "qemu: pflash segments must fit under 4G "
>>>>> +                    "cumulatively\n");
>> 
>> You're just following existing bad practice here, but correct use of
>> error_report() would give you nicer messages.  Happy to explain if
>> you're interested.
>
> You have the Location thing in mind, eg. automatically binding the error
> message to the offending command line option, right?

Yes, including a location in the message is one benefit.  Getting the
right one outside the initial parse can take a bit of extra work.  I'm
happy to assist with it.

Other benefits: consistent message format, timestamping support, and
making the intent of the message more obvious to readers of the code.

> I've seen you use it before in another patch. Hm... It was commit
> ec2df8c10a4585ba4641ae482cf2f5f13daa810e.
>
> I will try to address the rest of your comments too when I get back to
> this patch.

Okay :)