| Message ID | 20200923131829.3849-1-erich.mcmillan@hp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4] hw/i386/pc: add max combined fw size as machine configuration option | expand |
On 09/23/20 15:18, Erich Mcmillan wrote: > From: Erich McMillan <erich.mcmillan@hp.com> > > At HPi we have a need for increased fw size to enable testing of our custom fw. > > Signed-off-by: Erich McMillan <erich.mcmillan@hp.com> > --- > hw/i386/pc.c | 50 ++++++++++++++++++++++++++++++++++++++++++++ > hw/i386/pc_sysfw.c | 13 ++---------- > include/hw/i386/pc.h | 2 ++ > 3 files changed, 54 insertions(+), 11 deletions(-) > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > index d11daacc23..89775e7d5b 100644 > --- a/hw/i386/pc.c > +++ b/hw/i386/pc.c > @@ -1869,6 +1869,49 @@ 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) { > + error_setg(errp, > + "User specified max allowed firmware size %" PRIu64 " is " > + "greater than 16MiB. If combined firwmare size exceeds " > + "16MiB the system may not boot, or experience intermittent" > + "stability issues.", > + value); (1) Please put a "return" statement here. > + } > + > + pcms->max_fw_size = value; > +} > + > static void pc_machine_initfn(Object *obj) > { > PCMachineState *pcms = PC_MACHINE(obj); > @@ -1884,6 +1927,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; /* use default */ (2) While I'm not 100% certain, I suspect that the /* use default */ comment is not justified here. The model is of course pcms->max_ram_below_4g = 0; /* use default */ but there, the value we set in pc_machine_initfn() has a different meaning -- zero is stored deliberately so that the machine type specific logic in "hw/i386/pc_piix.c" and "hw/i386/pc_q35.c" *overwrite* it. "hw/i386/pc_piix.c": if (!pcms->max_ram_below_4g) { pcms->max_ram_below_4g = 0xe0000000; /* default: 3.5G */ } "hw/i386/pc_q35.c": if (!pcms->max_ram_below_4g) { pcms->max_ram_below_4g = 4 * GiB; } In other words, the comment "use default" in pc_machine_initfn() actually means "defer to the i440fx/q35-specific default". And that's not what we're doing with "max_fw_size". All I'm saying is that the "/* use default */" comment should not be copied from "max_ram_below_4g" to "max_fw_size", because the (assumed) meaning of the original comment does not apply to the new field. CC'ing Gerd wrt. (2). With (1) fixed, and with (2) optionally fixed: Reviewed-by: Laszlo Ersek <lersek@redhat.com> Thanks Laszlo > > pc_system_flash_create(pcms); > pcms->pcspk = isa_new(TYPE_PC_SPEAKER); > @@ -2004,6 +2048,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: >
Hi, > > + pcms->max_fw_size = 8 * MiB; /* use default */ [ ... ] > All I'm saying is that the "/* use default */" comment should not be > copied from "max_ram_below_4g" to "max_fw_size", because the (assumed) > meaning of the original comment does not apply to the new field. Fully agree. take care, Gerd
diff --git a/hw/i386/pc.c b/hw/i386/pc.c index d11daacc23..89775e7d5b 100644 --- a/hw/i386/pc.c +++ b/hw/i386/pc.c @@ -1869,6 +1869,49 @@ 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) { + error_setg(errp, + "User specified max allowed firmware size %" PRIu64 " is " + "greater than 16MiB. If combined firwmare size exceeds " + "16MiB the 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 +1927,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; /* use default */ pc_system_flash_create(pcms); pcms->pcspk = isa_new(TYPE_PC_SPEAKER); @@ -2004,6 +2048,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: