* [PATCH 2/2] add maximum combined fw size as machine configuration option
@ 2020-09-18 4:23 Erich Mcmillan
2020-09-18 8:17 ` Philippe Mathieu-Daudé
2020-09-18 8:31 ` Daniel P. Berrangé
0 siblings, 2 replies; 5+ messages in thread
From: Erich Mcmillan @ 2020-09-18 4:23 UTC (permalink / raw)
To: lersek, dgilbert, mst, marcel.apfelbaum, imammedo
Cc: qemu-devel, Erich McMillan
From: Erich McMillan <erich.mcmillan@hp.com>
Signed-off-by: Erich McMillan <erich.mcmillan@hp.com>
---
hw/i386/pc.c | 40 ++++++++++++++++++++++++++++++++++++++++
hw/i386/pc_sysfw.c | 13 ++-----------
include/hw/i386/pc.h | 22 ++++++++++++----------
3 files changed, 54 insertions(+), 21 deletions(-)
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index d11daac..b304988 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1869,6 +1869,39 @@ 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;
+ }
+
+ if (value > 16 * MiB) {
+ warn_report("User specifed 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 +1917,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 +2038,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 b6c0822..22450ba 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 fe52e16..cae213d 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -39,10 +39,11 @@ struct PCMachineState {
uint64_t max_ram_below_4g;
OnOffAuto vmport;
- bool acpi_build_enabled;
- bool smbus_enabled;
- bool sata_enabled;
- bool pit_enabled;
+ bool acpi_build_enabled;
+ bool smbus_enabled;
+ bool sata_enabled;
+ bool pit_enabled;
+ uint64_t max_fw_size;
/* NUMA information: */
uint64_t numa_nodes;
@@ -52,13 +53,14 @@ struct PCMachineState {
hwaddr memhp_io_base;
};
-#define PC_MACHINE_ACPI_DEVICE_PROP "acpi-device"
-#define PC_MACHINE_MAX_RAM_BELOW_4G "max-ram-below-4g"
+#define PC_MACHINE_ACPI_DEVICE_PROP "acpi-device"
+#define PC_MACHINE_MAX_RAM_BELOW_4G "max-ram-below-4g"
#define PC_MACHINE_DEVMEM_REGION_SIZE "device-memory-region-size"
-#define PC_MACHINE_VMPORT "vmport"
-#define PC_MACHINE_SMBUS "smbus"
-#define PC_MACHINE_SATA "sata"
-#define PC_MACHINE_PIT "pit"
+#define PC_MACHINE_VMPORT "vmport"
+#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:
--
2.25.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] add maximum combined fw size as machine configuration option
2020-09-18 4:23 [PATCH 2/2] add maximum combined fw size as machine configuration option Erich Mcmillan
@ 2020-09-18 8:17 ` Philippe Mathieu-Daudé
2020-09-22 7:34 ` Laszlo Ersek
2020-09-18 8:31 ` Daniel P. Berrangé
1 sibling, 1 reply; 5+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-09-18 8:17 UTC (permalink / raw)
To: Erich Mcmillan, lersek, dgilbert, mst, marcel.apfelbaum, imammedo
Cc: qemu-devel
Hi Erich,
On 9/18/20 6:23 AM, Erich Mcmillan wrote:
> From: Erich McMillan <erich.mcmillan@hp.com>
Description/rationale?
>
> Signed-off-by: Erich McMillan <erich.mcmillan@hp.com>
> ---
> hw/i386/pc.c | 40 ++++++++++++++++++++++++++++++++++++++++
> hw/i386/pc_sysfw.c | 13 ++-----------
> include/hw/i386/pc.h | 22 ++++++++++++----------
> 3 files changed, 54 insertions(+), 21 deletions(-)
>
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index d11daac..b304988 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1869,6 +1869,39 @@ 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;
> + }
> +
> + if (value > 16 * MiB) {
> + warn_report("User specifed max allowed firmware size %" PRIu64 " is greater than 16MiB,"
> + "if combined firwmare size exceeds 16MiB system may not boot,"
Typos "specified", "firmware".
> + "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 +1917,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;
I'm very confused by pc_system_flash_map()... Why not check
that no pflash exceeds 8MiB? Then 2 combined would be always
<16MiB.
>
> pc_system_flash_create(pcms);
> pcms->pcspk = isa_new(TYPE_PC_SPEAKER);
> @@ -2004,6 +2038,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 b6c0822..22450ba 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 fe52e16..cae213d 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -39,10 +39,11 @@ struct PCMachineState {
> uint64_t max_ram_below_4g;
> OnOffAuto vmport;
>
> - bool acpi_build_enabled;
> - bool smbus_enabled;
> - bool sata_enabled;
> - bool pit_enabled;
> + bool acpi_build_enabled;
> + bool smbus_enabled;
> + bool sata_enabled;
> + bool pit_enabled;
> + uint64_t max_fw_size;
>
> /* NUMA information: */
> uint64_t numa_nodes;
> @@ -52,13 +53,14 @@ struct PCMachineState {
> hwaddr memhp_io_base;
> };
>
> -#define PC_MACHINE_ACPI_DEVICE_PROP "acpi-device"
> -#define PC_MACHINE_MAX_RAM_BELOW_4G "max-ram-below-4g"
> +#define PC_MACHINE_ACPI_DEVICE_PROP "acpi-device"
> +#define PC_MACHINE_MAX_RAM_BELOW_4G "max-ram-below-4g"
> #define PC_MACHINE_DEVMEM_REGION_SIZE "device-memory-region-size"
> -#define PC_MACHINE_VMPORT "vmport"
> -#define PC_MACHINE_SMBUS "smbus"
> -#define PC_MACHINE_SATA "sata"
> -#define PC_MACHINE_PIT "pit"
> +#define PC_MACHINE_VMPORT "vmport"
> +#define PC_MACHINE_SMBUS "smbus"
> +#define PC_MACHINE_SATA "sata"
> +#define PC_MACHINE_PIT "pit"
> +#define PC_MACHINE_MAX_FW_SIZE "max-fw-size"
Having the space alignment changes in a previous "sanitize"
patch would ease the review of this one.
>
> /**
> * PCMachineClass:
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] add maximum combined fw size as machine configuration option
2020-09-18 4:23 [PATCH 2/2] add maximum combined fw size as machine configuration option Erich Mcmillan
2020-09-18 8:17 ` Philippe Mathieu-Daudé
@ 2020-09-18 8:31 ` Daniel P. Berrangé
2020-09-22 7:39 ` Laszlo Ersek
1 sibling, 1 reply; 5+ messages in thread
From: Daniel P. Berrangé @ 2020-09-18 8:31 UTC (permalink / raw)
To: Erich Mcmillan; +Cc: mst, qemu-devel, dgilbert, imammedo, lersek
On Fri, Sep 18, 2020 at 04:23:39AM +0000, Erich Mcmillan wrote:
> From: Erich McMillan <erich.mcmillan@hp.com>
>
> Signed-off-by: Erich McMillan <erich.mcmillan@hp.com>
> ---
> hw/i386/pc.c | 40 ++++++++++++++++++++++++++++++++++++++++
> hw/i386/pc_sysfw.c | 13 ++-----------
> include/hw/i386/pc.h | 22 ++++++++++++----------
> 3 files changed, 54 insertions(+), 21 deletions(-)
>
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index d11daac..b304988 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1869,6 +1869,39 @@ 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;
> + }
> +
Just here we should have a comment explaining why we pick this max limit.
The comment you removed later can be transplanted to here...
> + if (value > 16 * MiB) {
> + warn_report("User specifed 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 +1917,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 +2038,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 b6c0822..22450ba 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.
> - */
....this comment should be transplanted above^^
> -#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 fe52e16..cae213d 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -39,10 +39,11 @@ struct PCMachineState {
> uint64_t max_ram_below_4g;
> OnOffAuto vmport;
>
> - bool acpi_build_enabled;
> - bool smbus_enabled;
> - bool sata_enabled;
> - bool pit_enabled;
> + bool acpi_build_enabled;
> + bool smbus_enabled;
> + bool sata_enabled;
> + bool pit_enabled;
> + uint64_t max_fw_size;
Don't change whitespace in the existing fields - trying to
horizontally align fields has no benefit and needlessly
creates bigger diffs.
>
> /* NUMA information: */
> uint64_t numa_nodes;
> @@ -52,13 +53,14 @@ struct PCMachineState {
> hwaddr memhp_io_base;
> };
>
> -#define PC_MACHINE_ACPI_DEVICE_PROP "acpi-device"
> -#define PC_MACHINE_MAX_RAM_BELOW_4G "max-ram-below-4g"
> +#define PC_MACHINE_ACPI_DEVICE_PROP "acpi-device"
> +#define PC_MACHINE_MAX_RAM_BELOW_4G "max-ram-below-4g"
> #define PC_MACHINE_DEVMEM_REGION_SIZE "device-memory-region-size"
> -#define PC_MACHINE_VMPORT "vmport"
> -#define PC_MACHINE_SMBUS "smbus"
> -#define PC_MACHINE_SATA "sata"
> -#define PC_MACHINE_PIT "pit"
> +#define PC_MACHINE_VMPORT "vmport"
> +#define PC_MACHINE_SMBUS "smbus"
> +#define PC_MACHINE_SATA "sata"
> +#define PC_MACHINE_PIT "pit"
> +#define PC_MACHINE_MAX_FW_SIZE "max-fw-size"
Same here, just don't change whitespace alignment please.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] add maximum combined fw size as machine configuration option
2020-09-18 8:17 ` Philippe Mathieu-Daudé
@ 2020-09-22 7:34 ` Laszlo Ersek
0 siblings, 0 replies; 5+ messages in thread
From: Laszlo Ersek @ 2020-09-22 7:34 UTC (permalink / raw)
To: Philippe Mathieu-Daudé,
Erich Mcmillan, dgilbert, mst, marcel.apfelbaum, imammedo
Cc: qemu-devel
On 09/18/20 10:17, Philippe Mathieu-Daudé wrote:
> I'm very confused by pc_system_flash_map()... Why not check
> that no pflash exceeds 8MiB? Then 2 combined would be always
> <16MiB.
The requirement (limit) is on the total address range that is occupied
by the flash chips that are mapped in sequence. There is no size limit
that applies to an individual chip. It's also not required that chips
have the same size (or be limited by the same individual size).
Thanks
Laszlo
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] add maximum combined fw size as machine configuration option
2020-09-18 8:31 ` Daniel P. Berrangé
@ 2020-09-22 7:39 ` Laszlo Ersek
0 siblings, 0 replies; 5+ messages in thread
From: Laszlo Ersek @ 2020-09-22 7:39 UTC (permalink / raw)
To: Daniel P. Berrangé, Erich Mcmillan
Cc: qemu-devel, imammedo, dgilbert, mst
On 09/18/20 10:31, Daniel P. Berrangé wrote:
> On Fri, Sep 18, 2020 at 04:23:39AM +0000, Erich Mcmillan wrote:
>> From: Erich McMillan <erich.mcmillan@hp.com>
>>
>> Signed-off-by: Erich McMillan <erich.mcmillan@hp.com>
>> ---
>> hw/i386/pc.c | 40 ++++++++++++++++++++++++++++++++++++++++
>> hw/i386/pc_sysfw.c | 13 ++-----------
>> include/hw/i386/pc.h | 22 ++++++++++++----------
>> 3 files changed, 54 insertions(+), 21 deletions(-)
>>
>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> index d11daac..b304988 100644
>> --- a/hw/i386/pc.c
>> +++ b/hw/i386/pc.c
>> @@ -1869,6 +1869,39 @@ 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;
>> + }
>> +
>
> Just here we should have a comment explaining why we pick this max limit.
> The comment you removed later can be transplanted to here...
>
>> + if (value > 16 * MiB) {
>> + warn_report("User specifed 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 +1917,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 +2038,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 b6c0822..22450ba 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.
>> - */
>
> ....this comment should be transplanted above^^
>
>> -#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 fe52e16..cae213d 100644
>> --- a/include/hw/i386/pc.h
>> +++ b/include/hw/i386/pc.h
>> @@ -39,10 +39,11 @@ struct PCMachineState {
>> uint64_t max_ram_below_4g;
>> OnOffAuto vmport;
>>
>> - bool acpi_build_enabled;
>> - bool smbus_enabled;
>> - bool sata_enabled;
>> - bool pit_enabled;
>> + bool acpi_build_enabled;
>> + bool smbus_enabled;
>> + bool sata_enabled;
>> + bool pit_enabled;
>> + uint64_t max_fw_size;
>
> Don't change whitespace in the existing fields - trying to
> horizontally align fields has no benefit and needlessly
> creates bigger diffs.
>
>>
>> /* NUMA information: */
>> uint64_t numa_nodes;
>> @@ -52,13 +53,14 @@ struct PCMachineState {
>> hwaddr memhp_io_base;
>> };
>>
>> -#define PC_MACHINE_ACPI_DEVICE_PROP "acpi-device"
>> -#define PC_MACHINE_MAX_RAM_BELOW_4G "max-ram-below-4g"
>> +#define PC_MACHINE_ACPI_DEVICE_PROP "acpi-device"
>> +#define PC_MACHINE_MAX_RAM_BELOW_4G "max-ram-below-4g"
>> #define PC_MACHINE_DEVMEM_REGION_SIZE "device-memory-region-size"
>> -#define PC_MACHINE_VMPORT "vmport"
>> -#define PC_MACHINE_SMBUS "smbus"
>> -#define PC_MACHINE_SATA "sata"
>> -#define PC_MACHINE_PIT "pit"
>> +#define PC_MACHINE_VMPORT "vmport"
>> +#define PC_MACHINE_SMBUS "smbus"
>> +#define PC_MACHINE_SATA "sata"
>> +#define PC_MACHINE_PIT "pit"
>> +#define PC_MACHINE_MAX_FW_SIZE "max-fw-size"
>
> Same here, just don't change whitespace alignment please.
On a total tangent: I'm generally OK with changing whitespace for lining
up stuff visually, but whenever that's done, IMO it should be the *only*
thing done in a patch. First add the amount of whitespace that you know
you're going to need, to the existent fields / macros, and then
introduce the new fields / macros.
But, I understand that some maintainers dislike even that approach,
because it makes "git-blame" a bit more cumbersome to use. (The first
git-blame invocation gives you the whitespace-changing commit, and you
have to rerun git-blame at the *parent* of that commit, to get what you
actually want.)
Tangent ends, anyway...
Thanks
Laszlo
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-09-22 7:39 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-18 4:23 [PATCH 2/2] add maximum combined fw size as machine configuration option Erich Mcmillan
2020-09-18 8:17 ` Philippe Mathieu-Daudé
2020-09-22 7:34 ` Laszlo Ersek
2020-09-18 8:31 ` Daniel P. Berrangé
2020-09-22 7:39 ` Laszlo Ersek
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.