Message ID | 20200921165224.3691-1-erich.mcmillan@hp.com |
---|---|
State | New |
Headers | show |
Series | add maximum combined fw size as machine configuration option | expand |
On 09/21/20 18:52, Erich Mcmillan wrote: > From: Erich McMillan <erich.mcmillan@hp.com> > > Re-add rational for max fw size. Remove unrelated whitespace changes > > > Fix spelling error (1) The commit message should state the "why". The above v1->v2 changelog is useful, but it does not belong in the commit message. They are best placed between the "---" separator and the diffstat, indented by a few spaces [*]. > > Signed-off-by: Erich McMillan <erich.mcmillan@hp.com> > --- [*] here > hw/i386/pc.c | 47 ++++++++++++++++++++++++++++++++++++++++++++ > hw/i386/pc_sysfw.c | 13 ++---------- > include/hw/i386/pc.h | 2 ++ > 3 files changed, 51 insertions(+), 11 deletions(-) > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > index d11daacc23..681508174d 100644 > --- a/hw/i386/pc.c > +++ b/hw/i386/pc.c > @@ -1869,6 +1869,46 @@ static void pc_machine_set_max_ram_below_4g(Object *obj, Visitor *v, > pcms->max_ram_below_4g = value; > } > > +static void pc_machine_get_max_fw_size(Object *obj, Visitor *v, > + const char *name, void *opaque, > + Error **errp) (2) The indentation of the parameters after "v" seems off. (Probably after they were copied from pc_machine_get_max_ram_below_4g().) Please adjust. > +{ > + PCMachineState *pcms = PC_MACHINE(obj); > + uint64_t value = pcms->max_fw_size; > + > + visit_type_size(v, name, &value, errp); > +} > + > +static void pc_machine_set_max_fw_size(Object *obj, Visitor *v, > + const char *name, void *opaque, > + Error **errp) (3) same as (2) > +{ > + PCMachineState *pcms = PC_MACHINE(obj); > + Error *error = NULL; > + uint64_t value; > + > + visit_type_size(v, name, &value, &error); > + if (error) { > + error_propagate(errp, error); > + return; > + } > + > + /* > + * 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. > + */ > + if (value > 16 * MiB) { > + warn_report("User specified max allowed firmware size %" PRIu64 " is greater than 16MiB," (4) This source code line seems too long, please break it at an earlier spot. (5) A space character is missing after the comma. > + "if combined firwmare size exceeds 16MiB system may not boot," > + "or experience intermittent stability issues.", value); > + } > + > + pcms->max_fw_size = value; > +} (6) I think we need a hard 4GiB check here, similar to the one seen in pc_machine_set_max_ram_below_4g(). Otherwise, in pc_system_flash_map(), "total_size" may grow beyond 4GiB. While "total_size" will not overflow (due to having type "hwaddr"), the following subtraction will result in really bad things: sysbus_mmio_map(SYS_BUS_DEVICE(system_flash), 0, 0x100000000ULL - total_size); ... In fact I think I'd prefer turning the 16MiB warning into a hard error. It's one thing to mess up the guest config (we'd blame that on the QEMU user), but I think unintentionally mapping multiple MMIO regions on top of each other is a programming error, so we shouldn't permit the user to trigger such. I'll happily defer to other reviewers on this, of course. The rest looks OK to me. Thanks Laszlo > + > static void pc_machine_initfn(Object *obj) > { > PCMachineState *pcms = PC_MACHINE(obj); > @@ -1884,6 +1924,7 @@ static void pc_machine_initfn(Object *obj) > pcms->smbus_enabled = true; > pcms->sata_enabled = true; > pcms->pit_enabled = true; > + pcms->max_fw_size = 8 * MiB; > > pc_system_flash_create(pcms); > pcms->pcspk = isa_new(TYPE_PC_SPEAKER); > @@ -2004,6 +2045,12 @@ static void pc_machine_class_init(ObjectClass *oc, void *data) > > object_class_property_add_bool(oc, PC_MACHINE_PIT, > pc_machine_get_pit, pc_machine_set_pit); > + > + object_class_property_add(oc, PC_MACHINE_MAX_FW_SIZE, "size", > + pc_machine_get_max_fw_size, pc_machine_set_max_fw_size, > + NULL, NULL); > + object_class_property_set_description(oc, PC_MACHINE_MAX_FW_SIZE, > + "Maximum combined firmware size"); > } > > static const TypeInfo pc_machine_info = { > diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c > index b6c0822fe3..22450ba0ef 100644 > --- a/hw/i386/pc_sysfw.c > +++ b/hw/i386/pc_sysfw.c > @@ -39,15 +39,6 @@ > #include "hw/block/flash.h" > #include "sysemu/kvm.h" > > -/* > - * 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_SIZE_LIMIT (8 * MiB) > - > #define FLASH_SECTOR_SIZE 4096 > > static void pc_isa_bios_init(MemoryRegion *rom_memory, > @@ -182,10 +173,10 @@ static void pc_system_flash_map(PCMachineState *pcms, > } > if ((hwaddr)size != size > || total_size > HWADDR_MAX - size > - || total_size + size > FLASH_SIZE_LIMIT) { > + || total_size + size > pcms->max_fw_size) { > error_report("combined size of system firmware exceeds " > "%" PRIu64 " bytes", > - FLASH_SIZE_LIMIT); > + pcms->max_fw_size); > exit(1); > } > > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h > index fe52e165b2..f7c8e7cbfe 100644 > --- a/include/hw/i386/pc.h > +++ b/include/hw/i386/pc.h > @@ -43,6 +43,7 @@ struct PCMachineState { > bool smbus_enabled; > bool sata_enabled; > bool pit_enabled; > + uint64_t max_fw_size; > > /* NUMA information: */ > uint64_t numa_nodes; > @@ -59,6 +60,7 @@ struct PCMachineState { > #define PC_MACHINE_SMBUS "smbus" > #define PC_MACHINE_SATA "sata" > #define PC_MACHINE_PIT "pit" > +#define PC_MACHINE_MAX_FW_SIZE "max-fw-size" > > /** > * PCMachineClass: >
diff --git a/hw/i386/pc.c b/hw/i386/pc.c index d11daacc23..681508174d 100644 --- a/hw/i386/pc.c +++ b/hw/i386/pc.c @@ -1869,6 +1869,46 @@ static void pc_machine_set_max_ram_below_4g(Object *obj, Visitor *v, pcms->max_ram_below_4g = value; } +static void pc_machine_get_max_fw_size(Object *obj, Visitor *v, + const char *name, void *opaque, + Error **errp) +{ + PCMachineState *pcms = PC_MACHINE(obj); + uint64_t value = pcms->max_fw_size; + + visit_type_size(v, name, &value, errp); +} + +static void pc_machine_set_max_fw_size(Object *obj, Visitor *v, + const char *name, void *opaque, + Error **errp) +{ + PCMachineState *pcms = PC_MACHINE(obj); + Error *error = NULL; + uint64_t value; + + visit_type_size(v, name, &value, &error); + if (error) { + error_propagate(errp, error); + return; + } + + /* + * 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. + */ + if (value > 16 * MiB) { + warn_report("User specified max allowed firmware size %" PRIu64 " is greater than 16MiB," + "if combined firwmare size exceeds 16MiB system may not boot," + "or experience intermittent stability issues.", value); + } + + pcms->max_fw_size = value; +} + static void pc_machine_initfn(Object *obj) { PCMachineState *pcms = PC_MACHINE(obj); @@ -1884,6 +1924,7 @@ static void pc_machine_initfn(Object *obj) pcms->smbus_enabled = true; pcms->sata_enabled = true; pcms->pit_enabled = true; + pcms->max_fw_size = 8 * MiB; pc_system_flash_create(pcms); pcms->pcspk = isa_new(TYPE_PC_SPEAKER); @@ -2004,6 +2045,12 @@ static void pc_machine_class_init(ObjectClass *oc, void *data) object_class_property_add_bool(oc, PC_MACHINE_PIT, pc_machine_get_pit, pc_machine_set_pit); + + object_class_property_add(oc, PC_MACHINE_MAX_FW_SIZE, "size", + pc_machine_get_max_fw_size, pc_machine_set_max_fw_size, + NULL, NULL); + object_class_property_set_description(oc, PC_MACHINE_MAX_FW_SIZE, + "Maximum combined firmware size"); } static const TypeInfo pc_machine_info = { diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c index b6c0822fe3..22450ba0ef 100644 --- a/hw/i386/pc_sysfw.c +++ b/hw/i386/pc_sysfw.c @@ -39,15 +39,6 @@ #include "hw/block/flash.h" #include "sysemu/kvm.h" -/* - * 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_SIZE_LIMIT (8 * MiB) - #define FLASH_SECTOR_SIZE 4096 static void pc_isa_bios_init(MemoryRegion *rom_memory, @@ -182,10 +173,10 @@ static void pc_system_flash_map(PCMachineState *pcms, } if ((hwaddr)size != size || total_size > HWADDR_MAX - size - || total_size + size > FLASH_SIZE_LIMIT) { + || total_size + size > pcms->max_fw_size) { error_report("combined size of system firmware exceeds " "%" PRIu64 " bytes", - FLASH_SIZE_LIMIT); + pcms->max_fw_size); exit(1); } diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h index fe52e165b2..f7c8e7cbfe 100644 --- a/include/hw/i386/pc.h +++ b/include/hw/i386/pc.h @@ -43,6 +43,7 @@ struct PCMachineState { bool smbus_enabled; bool sata_enabled; bool pit_enabled; + uint64_t max_fw_size; /* NUMA information: */ uint64_t numa_nodes; @@ -59,6 +60,7 @@ struct PCMachineState { #define PC_MACHINE_SMBUS "smbus" #define PC_MACHINE_SATA "sata" #define PC_MACHINE_PIT "pit" +#define PC_MACHINE_MAX_FW_SIZE "max-fw-size" /** * PCMachineClass: