* [Qemu-devel] [PATCH v2] hw/i386/pc_sysfw: support two flash drives
@ 2013-11-27 23:52 Laszlo Ersek
2013-12-03 17:23 ` Markus Armbruster
0 siblings, 1 reply; 6+ messages in thread
From: Laszlo Ersek @ 2013-11-27 23:52 UTC (permalink / raw)
To: qemu-devel, armbru
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, pflash_update() is never called; various flash
programming/erase errors are returned to the guest instead. See the
callers of pflash_update(), and the initialization of "pfl->ro", in
"hw/block/pflash_cfi01.c".)
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
v2:
- don't map flash devices beyond unit#1 [Markus]
- explicit low bound on cumulative base address [Markus]
- updated comment block on pc_system_flash_init() [Laszlo]
- dropped (DriveInfo*) param of pc_system_flash_init(), use drive_get()
internally for unit#0 too [Markus]
- turn do-loop into for-loop [Markus]
- check bdrv_getlength() for errors [Laszlo]
- reject zero-sized flash [Laszlo]
- use Location of -pflash / -drive if=pflash,... option in error
reporting [Markus]
- describe the real spots where write attempts to r/o flash are caught
[Laszlo]
hw/i386/pc_sysfw.c | 105 +++++++++++++++++++++++++++++++++++++++++++----------
1 file changed, 86 insertions(+), 19 deletions(-)
diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
index e917c83..75a7ebb 100644
--- a/hw/i386/pc_sysfw.c
+++ b/hw/i386/pc_sysfw.c
@@ -72,35 +72,102 @@ static void pc_isa_bios_init(MemoryRegion *rom_memory,
memory_region_set_readonly(isa_bios, true);
}
-static void pc_system_flash_init(MemoryRegion *rom_memory,
- DriveInfo *pflash_drv)
+#define FLASH_MAP_UNIT_MAX 2
+
+/* We don't have a theoretically justifiable exact lower bound on the base
+ * address of any flash mapping. In practice, the IO-APIC MMIO range is
+ * [0xFEE00000..0xFEE01000[ -- see IO_APIC_DEFAULT_ADDRESS --, leaving free
+ * only 18MB-4KB below 4G. For now, restrict the cumulative mapping to 8MB in
+ * size.
+ */
+#define FLASH_MAP_BASE_MIN ((hwaddr)(0x100000000ULL - 8*1024*1024))
+
+/* This function maps flash drives from 4G downward, in order of their unit
+ * numbers. The mapping starts at unit#0, with unit number increments of 1, and
+ * stops before the first missing flash drive, or before
+ * unit#FLASH_MAP_UNIT_MAX, whichever is reached first.
+ *
+ * Addressing within one flash drive is of course not reversed.
+ *
+ * An error message is printed and the process exits if:
+ * - the size of the backing file for a flash drive is non-positive, or not a
+ * multiple of the required sector size, or
+ * - the current mapping's base address would fall below FLASH_MAP_BASE_MIN.
+ *
+ * The drive with unit#0 (if available) is mapped at the highest address, and
+ * it is passed to pc_isa_bios_init(). Merging several drives for isa-bios is
+ * not supported.
+ */
+static void pc_system_flash_init(MemoryRegion *rom_memory)
{
+ int unit;
+ DriveInfo *pflash_drv;
BlockDriverState *bdrv;
int64_t size;
- hwaddr phys_addr;
+ char *fatal_errmsg = NULL;
+ 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);
+ for (unit = 0;
+ (unit < FLASH_MAP_UNIT_MAX &&
+ (pflash_drv = drive_get(IF_PFLASH, 0, unit)) != NULL);
+ ++unit) {
+ bdrv = pflash_drv->bdrv;
+ size = bdrv_getlength(bdrv);
+ if (size < 0) {
+ fatal_errmsg = g_strdup_printf("failed to get backing file size");
+ } else if (size == 0) {
+ fatal_errmsg = g_strdup_printf("PC system firmware (pflash) "
+ "cannot have zero size");
+ } else if ((size % sector_size) != 0) {
+ fatal_errmsg = g_strdup_printf("PC system firmware (pflash) "
+ "must be a multiple of 0x%x", sector_size);
+ } else if (phys_addr < size || phys_addr - size < FLASH_MAP_BASE_MIN) {
+ fatal_errmsg = g_strdup_printf("oversized backing file, pflash "
+ "segments cannot be mapped under "
+ TARGET_FMT_plx, FLASH_MAP_BASE_MIN);
+ }
+ if (fatal_errmsg != NULL) {
+ Location loc;
+
+ /* push a new, "none" location on the location stack; overwrite its
+ * contents with the location saved in the option; print the error
+ * (includes location); pop the top
+ */
+ loc_push_none(&loc);
+ if (pflash_drv->opts != NULL) {
+ qemu_opts_loc_restore(pflash_drv->opts);
+ }
+ error_report("%s", fatal_errmsg);
+ loc_pop(&loc);
+ g_free(fatal_errmsg);
+ exit(1);
+ }
+
+ phys_addr -= size;
+
+ /* pflash_cfi01_register() creates a deep copy of the name */
+ snprintf(name, sizeof name, "system.flash%d", unit);
+ system_flash = pflash_cfi01_register(phys_addr, NULL /* qdev */, name,
+ size, bdrv, sector_size,
+ size >> sector_bits,
+ 1 /* width */,
+ 0x0000 /* id0 */,
+ 0x0000 /* id1 */,
+ 0x0000 /* id2 */,
+ 0x0000 /* id3 */,
+ 0 /* be */);
+ if (unit == 0) {
+ flash_mem = pflash_cfi01_get_memory(system_flash);
+ pc_isa_bios_init(rom_memory, flash_mem, size);
+ }
}
-
- phys_addr = 0x100000000ULL - size;
- system_flash = pflash_cfi01_register(phys_addr, NULL, "system.flash", size,
- bdrv, sector_size, size >> sector_bits,
- 1, 0x0000, 0x0000, 0x0000, 0x0000, 0);
- flash_mem = pflash_cfi01_get_memory(system_flash);
-
- pc_isa_bios_init(rom_memory, flash_mem, size);
}
static void old_pc_system_rom_init(MemoryRegion *rom_memory, bool isapc_ram_fw)
@@ -181,5 +248,5 @@ void pc_system_firmware_init(MemoryRegion *rom_memory, bool isapc_ram_fw)
exit(1);
}
- pc_system_flash_init(rom_memory, pflash_drv);
+ pc_system_flash_init(rom_memory);
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v2] hw/i386/pc_sysfw: support two flash drives
2013-11-27 23:52 [Qemu-devel] [PATCH v2] hw/i386/pc_sysfw: support two flash drives Laszlo Ersek
@ 2013-12-03 17:23 ` Markus Armbruster
2013-12-03 17:58 ` Paolo Bonzini
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Markus Armbruster @ 2013-12-03 17:23 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: qemu-devel
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, pflash_update() is never called; various flash
> programming/erase errors are returned to the guest instead. See the
> callers of pflash_update(), and the initialization of "pfl->ro", in
> "hw/block/pflash_cfi01.c".)
>
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
> v2:
> - don't map flash devices beyond unit#1 [Markus]
> - explicit low bound on cumulative base address [Markus]
> - updated comment block on pc_system_flash_init() [Laszlo]
> - dropped (DriveInfo*) param of pc_system_flash_init(), use drive_get()
> internally for unit#0 too [Markus]
> - turn do-loop into for-loop [Markus]
> - check bdrv_getlength() for errors [Laszlo]
> - reject zero-sized flash [Laszlo]
> - use Location of -pflash / -drive if=pflash,... option in error
> reporting [Markus]
> - describe the real spots where write attempts to r/o flash are caught
> [Laszlo]
>
> hw/i386/pc_sysfw.c | 105 +++++++++++++++++++++++++++++++++++++++++++----------
> 1 file changed, 86 insertions(+), 19 deletions(-)
>
> diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
> index e917c83..75a7ebb 100644
> --- a/hw/i386/pc_sysfw.c
> +++ b/hw/i386/pc_sysfw.c
> @@ -72,35 +72,102 @@ static void pc_isa_bios_init(MemoryRegion *rom_memory,
> memory_region_set_readonly(isa_bios, true);
> }
>
> -static void pc_system_flash_init(MemoryRegion *rom_memory,
> - DriveInfo *pflash_drv)
> +#define FLASH_MAP_UNIT_MAX 2
> +
> +/* We don't have a theoretically justifiable exact lower bound on the base
> + * address of any flash mapping. In practice, the IO-APIC MMIO range is
> + * [0xFEE00000..0xFEE01000[ -- see IO_APIC_DEFAULT_ADDRESS --, leaving free
> + * only 18MB-4KB below 4G. For now, restrict the cumulative mapping to 8MB in
> + * size.
> + */
> +#define FLASH_MAP_BASE_MIN ((hwaddr)(0x100000000ULL - 8*1024*1024))
> +
> +/* This function maps flash drives from 4G downward, in order of their unit
> + * numbers. The mapping starts at unit#0, with unit number increments of 1, and
> + * stops before the first missing flash drive, or before
> + * unit#FLASH_MAP_UNIT_MAX, whichever is reached first.
> + *
> + * Addressing within one flash drive is of course not reversed.
> + *
> + * An error message is printed and the process exits if:
> + * - the size of the backing file for a flash drive is non-positive, or not a
> + * multiple of the required sector size, or
> + * - the current mapping's base address would fall below FLASH_MAP_BASE_MIN.
> + *
> + * The drive with unit#0 (if available) is mapped at the highest address, and
> + * it is passed to pc_isa_bios_init(). Merging several drives for isa-bios is
> + * not supported.
> + */
> +static void pc_system_flash_init(MemoryRegion *rom_memory)
> {
> + int unit;
> + DriveInfo *pflash_drv;
> BlockDriverState *bdrv;
> int64_t size;
> - hwaddr phys_addr;
> + char *fatal_errmsg = NULL;
> + 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);
> + for (unit = 0;
> + (unit < FLASH_MAP_UNIT_MAX &&
> + (pflash_drv = drive_get(IF_PFLASH, 0, unit)) != NULL);
> + ++unit) {
> + bdrv = pflash_drv->bdrv;
Used just twice, sure it's worth a variable? Your choice, of course.
> + size = bdrv_getlength(bdrv);
> + if (size < 0) {
> + fatal_errmsg = g_strdup_printf("failed to get backing file size");
> + } else if (size == 0) {
> + fatal_errmsg = g_strdup_printf("PC system firmware (pflash) "
> + "cannot have zero size");
> + } else if ((size % sector_size) != 0) {
> + fatal_errmsg = g_strdup_printf("PC system firmware (pflash) "
> + "must be a multiple of 0x%x", sector_size);
> + } else if (phys_addr < size || phys_addr - size < FLASH_MAP_BASE_MIN) {
> + fatal_errmsg = g_strdup_printf("oversized backing file, pflash "
> + "segments cannot be mapped under "
"mapped below "?
> + TARGET_FMT_plx, FLASH_MAP_BASE_MIN);
> + }
> + if (fatal_errmsg != NULL) {
> + Location loc;
> +
> + /* push a new, "none" location on the location stack; overwrite its
> + * contents with the location saved in the option; print the error
> + * (includes location); pop the top
> + */
Sure this comment is worthwhile? Your decision.
I suspect we should have error_report_loc().
> + loc_push_none(&loc);
> + if (pflash_drv->opts != NULL) {
> + qemu_opts_loc_restore(pflash_drv->opts);
> + }
> + error_report("%s", fatal_errmsg);
> + loc_pop(&loc);
> + g_free(fatal_errmsg);
> + exit(1);
You hoist the reporting of a fatal error out of the conditionals. If we
had error_report_loc(), we could leave it there. But we don't.
> + }
> +
> + phys_addr -= size;
> +
> + /* pflash_cfi01_register() creates a deep copy of the name */
> + snprintf(name, sizeof name, "system.flash%d", unit);
> + system_flash = pflash_cfi01_register(phys_addr, NULL /* qdev */, name,
pflash_cfi01_register() doesn't use its second parameter, and all
callers pass NULL, ugh.
I looked because I don't like nor trust /* name of parameter */
comments. Matter of taste.
> + size, bdrv, sector_size,
> + size >> sector_bits,
> + 1 /* width */,
> + 0x0000 /* id0 */,
> + 0x0000 /* id1 */,
> + 0x0000 /* id2 */,
> + 0x0000 /* id3 */,
> + 0 /* be */);
> + if (unit == 0) {
> + flash_mem = pflash_cfi01_get_memory(system_flash);
> + pc_isa_bios_init(rom_memory, flash_mem, size);
> + }
> }
> -
> - phys_addr = 0x100000000ULL - size;
> - system_flash = pflash_cfi01_register(phys_addr, NULL, "system.flash", size,
> - bdrv, sector_size, size >> sector_bits,
> - 1, 0x0000, 0x0000, 0x0000, 0x0000, 0);
> - flash_mem = pflash_cfi01_get_memory(system_flash);
> -
> - pc_isa_bios_init(rom_memory, flash_mem, size);
> }
>
> static void old_pc_system_rom_init(MemoryRegion *rom_memory, bool isapc_ram_fw)
> @@ -181,5 +248,5 @@ void pc_system_firmware_init(MemoryRegion *rom_memory, bool isapc_ram_fw)
> exit(1);
> }
>
> - pc_system_flash_init(rom_memory, pflash_drv);
> + pc_system_flash_init(rom_memory);
> }
Nothing but nits, therefore
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Still pining for that star destroyer, though... ;)
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v2] hw/i386/pc_sysfw: support two flash drives
2013-12-03 17:23 ` Markus Armbruster
@ 2013-12-03 17:58 ` Paolo Bonzini
2013-12-03 22:08 ` Laszlo Ersek
2013-12-10 4:56 ` Laszlo Ersek
2 siblings, 0 replies; 6+ messages in thread
From: Paolo Bonzini @ 2013-12-03 17:58 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Laszlo Ersek, qemu-devel
Il 03/12/2013 18:23, Markus Armbruster ha scritto:
>> > + /* push a new, "none" location on the location stack; overwrite its
>> > + * contents with the location saved in the option; print the error
>> > + * (includes location); pop the top
>> > + */
> Sure this comment is worthwhile? Your decision.
>
> I suspect we should have error_report_loc().
Or qemu_opts_loc_push.
In any case, the comment is necessary but shouldn't be. :)
Paolo
>> > + loc_push_none(&loc);
>> > + if (pflash_drv->opts != NULL) {
>> > + qemu_opts_loc_restore(pflash_drv->opts);
>> > + }
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v2] hw/i386/pc_sysfw: support two flash drives
2013-12-03 17:23 ` Markus Armbruster
2013-12-03 17:58 ` Paolo Bonzini
@ 2013-12-03 22:08 ` Laszlo Ersek
2013-12-10 4:56 ` Laszlo Ersek
2 siblings, 0 replies; 6+ messages in thread
From: Laszlo Ersek @ 2013-12-03 22:08 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel
On 12/03/13 18:23, 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, pflash_update() is never called; various flash
>> programming/erase errors are returned to the guest instead. See the
>> callers of pflash_update(), and the initialization of "pfl->ro", in
>> "hw/block/pflash_cfi01.c".)
>>
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>> v2:
>> - don't map flash devices beyond unit#1 [Markus]
>> - explicit low bound on cumulative base address [Markus]
>> - updated comment block on pc_system_flash_init() [Laszlo]
>> - dropped (DriveInfo*) param of pc_system_flash_init(), use drive_get()
>> internally for unit#0 too [Markus]
>> - turn do-loop into for-loop [Markus]
>> - check bdrv_getlength() for errors [Laszlo]
>> - reject zero-sized flash [Laszlo]
>> - use Location of -pflash / -drive if=pflash,... option in error
>> reporting [Markus]
>> - describe the real spots where write attempts to r/o flash are caught
>> [Laszlo]
>>
>> hw/i386/pc_sysfw.c | 105 +++++++++++++++++++++++++++++++++++++++++++----------
>> 1 file changed, 86 insertions(+), 19 deletions(-)
>>
>> diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
>> index e917c83..75a7ebb 100644
>> --- a/hw/i386/pc_sysfw.c
>> +++ b/hw/i386/pc_sysfw.c
>> @@ -72,35 +72,102 @@ static void pc_isa_bios_init(MemoryRegion *rom_memory,
>> memory_region_set_readonly(isa_bios, true);
>> }
>>
>> -static void pc_system_flash_init(MemoryRegion *rom_memory,
>> - DriveInfo *pflash_drv)
>> +#define FLASH_MAP_UNIT_MAX 2
>> +
>> +/* We don't have a theoretically justifiable exact lower bound on the base
>> + * address of any flash mapping. In practice, the IO-APIC MMIO range is
>> + * [0xFEE00000..0xFEE01000[ -- see IO_APIC_DEFAULT_ADDRESS --, leaving free
>> + * only 18MB-4KB below 4G. For now, restrict the cumulative mapping to 8MB in
>> + * size.
>> + */
>> +#define FLASH_MAP_BASE_MIN ((hwaddr)(0x100000000ULL - 8*1024*1024))
>> +
>> +/* This function maps flash drives from 4G downward, in order of their unit
>> + * numbers. The mapping starts at unit#0, with unit number increments of 1, and
>> + * stops before the first missing flash drive, or before
>> + * unit#FLASH_MAP_UNIT_MAX, whichever is reached first.
>> + *
>> + * Addressing within one flash drive is of course not reversed.
>> + *
>> + * An error message is printed and the process exits if:
>> + * - the size of the backing file for a flash drive is non-positive, or not a
>> + * multiple of the required sector size, or
>> + * - the current mapping's base address would fall below FLASH_MAP_BASE_MIN.
>> + *
>> + * The drive with unit#0 (if available) is mapped at the highest address, and
>> + * it is passed to pc_isa_bios_init(). Merging several drives for isa-bios is
>> + * not supported.
>> + */
>> +static void pc_system_flash_init(MemoryRegion *rom_memory)
>> {
>> + int unit;
>> + DriveInfo *pflash_drv;
>> BlockDriverState *bdrv;
>> int64_t size;
>> - hwaddr phys_addr;
>> + char *fatal_errmsg = NULL;
>> + 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);
>> + for (unit = 0;
>> + (unit < FLASH_MAP_UNIT_MAX &&
>> + (pflash_drv = drive_get(IF_PFLASH, 0, unit)) != NULL);
>> + ++unit) {
>> + bdrv = pflash_drv->bdrv;
>
> Used just twice, sure it's worth a variable? Your choice, of course.
I sought to keep the initial block of definitions intact as much as I
could. Personally I would have wanted to kill off most of those, and
(re)introduce anything I'd need in the narrowest scope possible. But, I
remembered that you hate narrow scope declarations and like to keep
everything at the top of the function, so I didn't mess with that block
precisely to keep you happy.
:)
>
>> + size = bdrv_getlength(bdrv);
>> + if (size < 0) {
>> + fatal_errmsg = g_strdup_printf("failed to get backing file size");
>> + } else if (size == 0) {
>> + fatal_errmsg = g_strdup_printf("PC system firmware (pflash) "
>> + "cannot have zero size");
>> + } else if ((size % sector_size) != 0) {
>> + fatal_errmsg = g_strdup_printf("PC system firmware (pflash) "
>> + "must be a multiple of 0x%x", sector_size);
>> + } else if (phys_addr < size || phys_addr - size < FLASH_MAP_BASE_MIN) {
>> + fatal_errmsg = g_strdup_printf("oversized backing file, pflash "
>> + "segments cannot be mapped under "
>
> "mapped below "?
Too much finesse, can't care about everything! :)
>
>> + TARGET_FMT_plx, FLASH_MAP_BASE_MIN);
>> + }
>> + if (fatal_errmsg != NULL) {
>> + Location loc;
>> +
>> + /* push a new, "none" location on the location stack; overwrite its
>> + * contents with the location saved in the option; print the error
>> + * (includes location); pop the top
>> + */
>
> Sure this comment is worthwhile? Your decision.
It shows you that I did my homework on the Location thing.
More importantly, it shows the casual reader what's going on.
Considering that I found exactly one other such pattern in the entire
source, I think we can qualify all developers "casual readers" in this
regard. I myself would have been greatly helped by such a comment
(anywhere), so I added it.
>
> I suspect we should have error_report_loc().
I was happy that I managed to use the existing functions correctly.
Please feel free to introduce the new function and refactor this call
site :)
>
>> + loc_push_none(&loc);
>> + if (pflash_drv->opts != NULL) {
>> + qemu_opts_loc_restore(pflash_drv->opts);
>> + }
>> + error_report("%s", fatal_errmsg);
>> + loc_pop(&loc);
>> + g_free(fatal_errmsg);
>> + exit(1);
>
> You hoist the reporting of a fatal error out of the conditionals. If we
> had error_report_loc(), we could leave it there. But we don't.
Correct, I do that, with a smile on my face. :)
>
>> + }
>> +
>> + phys_addr -= size;
>> +
>> + /* pflash_cfi01_register() creates a deep copy of the name */
>> + snprintf(name, sizeof name, "system.flash%d", unit);
>> + system_flash = pflash_cfi01_register(phys_addr, NULL /* qdev */, name,
>
> pflash_cfi01_register() doesn't use its second parameter, and all
> callers pass NULL, ugh.
I looked too.
>
> I looked because I don't like nor trust /* name of parameter */
> comments. Matter of taste.
I don't care for such in-call comments, but I saw MST use them, so I
thought what the heck.
>
>> + size, bdrv, sector_size,
>> + size >> sector_bits,
>> + 1 /* width */,
>> + 0x0000 /* id0 */,
>> + 0x0000 /* id1 */,
>> + 0x0000 /* id2 */,
>> + 0x0000 /* id3 */,
>> + 0 /* be */);
>> + if (unit == 0) {
>> + flash_mem = pflash_cfi01_get_memory(system_flash);
>> + pc_isa_bios_init(rom_memory, flash_mem, size);
>> + }
>> }
>> -
>> - phys_addr = 0x100000000ULL - size;
>> - system_flash = pflash_cfi01_register(phys_addr, NULL, "system.flash", size,
>> - bdrv, sector_size, size >> sector_bits,
>> - 1, 0x0000, 0x0000, 0x0000, 0x0000, 0);
>> - flash_mem = pflash_cfi01_get_memory(system_flash);
>> -
>> - pc_isa_bios_init(rom_memory, flash_mem, size);
>> }
>>
>> static void old_pc_system_rom_init(MemoryRegion *rom_memory, bool isapc_ram_fw)
>> @@ -181,5 +248,5 @@ void pc_system_firmware_init(MemoryRegion *rom_memory, bool isapc_ram_fw)
>> exit(1);
>> }
>>
>> - pc_system_flash_init(rom_memory, pflash_drv);
>> + pc_system_flash_init(rom_memory);
>> }
>
> Nothing but nits, therefore
>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
Why thank you! :)
>
> Still pining for that star destroyer, though... ;)
>
I'm pining for 36-hour days! :)
Thanks!
Laszlo
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v2] hw/i386/pc_sysfw: support two flash drives
2013-12-03 17:23 ` Markus Armbruster
2013-12-03 17:58 ` Paolo Bonzini
2013-12-03 22:08 ` Laszlo Ersek
@ 2013-12-10 4:56 ` Laszlo Ersek
2013-12-10 7:48 ` Markus Armbruster
2 siblings, 1 reply; 6+ messages in thread
From: Laszlo Ersek @ 2013-12-10 4:56 UTC (permalink / raw)
To: qemu-devel; +Cc: Markus Armbruster, Anthony Liguori
On 12/03/13 18:23, Markus Armbruster wrote:
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
My horse for a commit, please?
Thanks
Laszlo
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v2] hw/i386/pc_sysfw: support two flash drives
2013-12-10 4:56 ` Laszlo Ersek
@ 2013-12-10 7:48 ` Markus Armbruster
0 siblings, 0 replies; 6+ messages in thread
From: Markus Armbruster @ 2013-12-10 7:48 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: qemu-devel, Anthony Liguori
Laszlo Ersek <lersek@redhat.com> writes:
> On 12/03/13 18:23, Markus Armbruster wrote:
>
>> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>
> My horse for a commit, please?
Your horse, two months, and four more rebases.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-12-10 7:48 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-11-27 23:52 [Qemu-devel] [PATCH v2] hw/i386/pc_sysfw: support two flash drives Laszlo Ersek
2013-12-03 17:23 ` Markus Armbruster
2013-12-03 17:58 ` Paolo Bonzini
2013-12-03 22:08 ` Laszlo Ersek
2013-12-10 4:56 ` Laszlo Ersek
2013-12-10 7:48 ` Markus Armbruster
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).