Laszlo Ersek <ler...@redhat.com> writes:
> On 11/26/13 14:11, Markus Armbruster wrote:
>> Laszlo Ersek <ler...@redhat.com> writes:
>>
>>> On 11/25/13 16:32, Markus Armbruster wrote:
>>>> Laszlo Ersek <ler...@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 <ler...@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 :)
