[v5] hw/i386/pc: add max combined fw size as machine configuration option

Commit Message

Erich Mcmillan Sept. 25, 2020, 3:36 a.m. UTC

From: Erich McMillan <[e-mail]>

At HPi we have a need for increased fw size to enable testing of our custom fw.

Signed-off-by: Erich McMillan <[e-mail]>

Change since v4:
     Add explicit return to pc_machine_set_max_fw_size.
     Remove /* default */ from max_fw_size initialization.
---

 hw/i386/pc.c         | 52 ++++++++++++++++++++++++++++++++++++++++++++
 hw/i386/pc_sysfw.c   | 13 ++---------
 include/hw/i386/pc.h |  2 ++
 3 files changed, 56 insertions(+), 11 deletions(-)

Comments

Laszlo Ersek Sept. 25, 2020, 7 a.m. UTC | #1

Hi Erich,

when processing review feedback, please pay attention to *where* the
review comments are inserted, in response to your patch email.

I'm pointing this out not because I want to annoy you with my
obsessions, but because I consider this discussion a kind of "git +
mailing lists" training for you. (In accordance with your first message
on the topic.)

Please see specifics below:

On 09/25/20 05:36, Erich Mcmillan wrote:
> From: Erich McMillan <[e-mail]>
> 
> At HPi we have a need for increased fw size to enable testing of our custom fw.
> 
> Signed-off-by: Erich McMillan <[e-mail]>
> 
> Change since v4:
>      Add explicit return to pc_machine_set_max_fw_size.
>      Remove /* default */ from max_fw_size initialization.
> ---
> 
>  hw/i386/pc.c         | 52 ++++++++++++++++++++++++++++++++++++++++++++
>  hw/i386/pc_sysfw.c   | 13 ++---------
>  include/hw/i386/pc.h |  2 ++
>  3 files changed, 56 insertions(+), 11 deletions(-)

Please refer to my earlier feedback, archived at the following location:

http://mid.mail-archive.com/[e-mail]

As I say in that message, the v(n)->v(n+1) changelog belongs 'between
the "---" separator and the diffstat'. In that message, I marked the
specific location with [*].

Basically the "---" separator terminates the commit message, and the
first "diff --git" line starts the code changes. What's between them is
thrown away, when the patch is applied. So in that throwaway area,
git-format-patch places the diffstat automatically (because it gives
reviewers a helpful overview of the patch, but is not useful for patch
application). And that's also the area where the v(n)->v(n+1) changelog
should be included. Traditionally, we place that log above the diffstat.

> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index d11daacc23..6e66cbbc41 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1869,6 +1869,51 @@ 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;
> +
> +    return;
> +}

This return statement is useless. Please see my review at:

http://mid.mail-archive.com/[e-mail]

There I wrote, 'Please put a "return" statement here'. And, my request
was placed *exactly between* the error_setg() call and the closing brace.

The idea being that, if we take the (value > 16 * MiB) branch, then yes
we need to set the error, but we also need to abandon the rest of the
function. If "value" is invalid (out of bounds), then "pcms->max_fw_size
= value" is exactly the assignment that we do *not* want to reach.

> +
>  static void pc_machine_initfn(Object *obj)
>  {
>      PCMachineState *pcms = PC_MACHINE(obj);
> @@ -1884,6 +1929,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;

Thank you for dropping the comment.

Summary:

- the changelog is helpful (thanks!), it's placement is not perfect yet
- the return statement should be moved so that it terminate the (value >
16 * MiB) branch.

Also, I think "HPi" (rather than "HP") in the commit message *could* be
a typo (I'm not sure).

Thanks,
Laszlo


>  
>      pc_system_flash_create(pcms);
>      pcms->pcspk = isa_new(TYPE_PC_SPEAKER);
> @@ -2004,6 +2050,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:
>

Erich Mcmillan Sept. 25, 2020, 5:14 p.m. UTC | #2

Hi Laszlo,

Thank you for the feedback, apologies that I missed the exact line references I was moving too fast.
I appreciate you taking the time to explain the nuances.

On an unrelated note, it seems that my patches are no longer appearing in https://lists.nongnu.org/archive/html/qemu-devel/2020-09/index.html is this because I need to cc [e-mail]<mailto:[e-mail]> rather than –to?

-Erich

From: Laszlo Ersek [mailto:[e-mail]]
Sent: Friday, September 25, 2020 2:00 AM
To: McMillan, Erich <[e-mail]>; [e-mail]
Cc: [e-mail]; [e-mail]; [e-mail]; [e-mail]; [e-mail]
Subject: Re: [PATCH v5] hw/i386/pc: add max combined fw size as machine configuration option

Hi Erich,

when processing review feedback, please pay attention to *where* the
review comments are inserted, in response to your patch email.

I'm pointing this out not because I want to annoy you with my
obsessions, but because I consider this discussion a kind of "git +
mailing lists" training for you. (In accordance with your first message
on the topic.)

Please see specifics below:

On 09/25/20 05:36, Erich Mcmillan wrote:
> From: Erich McMillan <[e-mail]<mailto:[e-mail]>>
>
> At HPi we have a need for increased fw size to enable testing of our custom fw.
>
> Signed-off-by: Erich McMillan <[e-mail]<mailto:[e-mail]>>
>
> Change since v4:
> Add explicit return to pc_machine_set_max_fw_size.
> Remove /* default */ from max_fw_size initialization.
> ---
>
> hw/i386/pc.c | 52 ++++++++++++++++++++++++++++++++++++++++++++
> hw/i386/pc_sysfw.c | 13 ++---------
> include/hw/i386/pc.h | 2 ++
> 3 files changed, 56 insertions(+), 11 deletions(-)

Please refer to my earlier feedback, archived at the following location:

http://mid.mail-archive.com/[e-mail]<http://mid.mail-archive.com/[e-mail]>

As I say in that message, the v(n)->v(n+1) changelog belongs 'between
the "---" separator and the diffstat'. In that message, I marked the
specific location with [*].

Basically the "---" separator terminates the commit message, and the
first "diff --git" line starts the code changes. What's between them is
thrown away, when the patch is applied. So in that throwaway area,
git-format-patch places the diffstat automatically (because it gives
reviewers a helpful overview of the patch, but is not useful for patch
application). And that's also the area where the v(n)->v(n+1) changelog
should be included. Traditionally, we place that log above the diffstat.

>
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index d11daacc23..6e66cbbc41 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1869,6 +1869,51 @@ 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;
> +
> + return;
> +}

This return statement is useless. Please see my review at:

http://mid.mail-archive.com/[e-mail]<http://mid.mail-archive.com/[e-mail]>

There I wrote, 'Please put a "return" statement here'. And, my request
was placed *exactly between* the error_setg() call and the closing brace.

The idea being that, if we take the (value > 16 * MiB) branch, then yes
we need to set the error, but we also need to abandon the rest of the
function. If "value" is invalid (out of bounds), then "pcms->max_fw_size
= value" is exactly the assignment that we do *not* want to reach.

> +
> static void pc_machine_initfn(Object *obj)
> {
> PCMachineState *pcms = PC_MACHINE(obj);
> @@ -1884,6 +1929,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;

Thank you for dropping the comment.

Summary:

- the changelog is helpful (thanks!), it's placement is not perfect yet
- the return statement should be moved so that it terminate the (value >
16 * MiB) branch.

Also, I think "HPi" (rather than "HP") in the commit message *could* be
a typo (I'm not sure).

Thanks,
Laszlo


>
> pc_system_flash_create(pcms);
> pcms->pcspk = isa_new(TYPE_PC_SPEAKER);
> @@ -2004,6 +2050,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:
>

Erich Mcmillan Sept. 25, 2020, 5:17 p.m. UTC | #3

Additionally HPi is not a mistake, corporate requires that we refer to ourselves as Hewlett Packard Inc since the split in 2015. I will perhaps update this to be the full name for clarity.


From: McMillan, Erich
Sent: Friday, September 25, 2020 12:15 PM
To: Laszlo Ersek <[e-mail]>; [e-mail]
Cc: [e-mail]; [e-mail]; [e-mail]; [e-mail]; [e-mail]
Subject: RE: [PATCH v5] hw/i386/pc: add max combined fw size as machine configuration option

Hi Laszlo,

Thank you for the feedback, apologies that I missed the exact line references I was moving too fast.
I appreciate you taking the time to explain the nuances.

On an unrelated note, it seems that my patches are no longer appearing in https://lists.nongnu.org/archive/html/qemu-devel/2020-09/index.html is this because I need to cc [e-mail]<mailto:[e-mail]> rather than –to?

-Erich

From: Laszlo Ersek [mailto:[e-mail]]
Sent: Friday, September 25, 2020 2:00 AM
To: McMillan, Erich <[e-mail]<mailto:[e-mail]>>; [e-mail]<mailto:[e-mail]>
Cc: [e-mail]<mailto:[e-mail]>; [e-mail]<mailto:[e-mail]>; [e-mail]<mailto:[e-mail]>; [e-mail]<mailto:[e-mail]>; [e-mail]<mailto:[e-mail]>
Subject: Re: [PATCH v5] hw/i386/pc: add max combined fw size as machine configuration option

Hi Erich,

when processing review feedback, please pay attention to *where* the
review comments are inserted, in response to your patch email.

I'm pointing this out not because I want to annoy you with my
obsessions, but because I consider this discussion a kind of "git +
mailing lists" training for you. (In accordance with your first message
on the topic.)

Please see specifics below:

On 09/25/20 05:36, Erich Mcmillan wrote:
> From: Erich McMillan <[e-mail]<mailto:[e-mail]>>
>
> At HPi we have a need for increased fw size to enable testing of our custom fw.
>
> Signed-off-by: Erich McMillan <[e-mail]<mailto:[e-mail]>>
>
> Change since v4:
> Add explicit return to pc_machine_set_max_fw_size.
> Remove /* default */ from max_fw_size initialization.
> ---
>
> hw/i386/pc.c | 52 ++++++++++++++++++++++++++++++++++++++++++++
> hw/i386/pc_sysfw.c | 13 ++---------
> include/hw/i386/pc.h | 2 ++
> 3 files changed, 56 insertions(+), 11 deletions(-)

Please refer to my earlier feedback, archived at the following location:

http://mid.mail-archive.com/[e-mail]<http://mid.mail-archive.com/[e-mail]>

As I say in that message, the v(n)->v(n+1) changelog belongs 'between
the "---" separator and the diffstat'. In that message, I marked the
specific location with [*].

Basically the "---" separator terminates the commit message, and the
first "diff --git" line starts the code changes. What's between them is
thrown away, when the patch is applied. So in that throwaway area,
git-format-patch places the diffstat automatically (because it gives
reviewers a helpful overview of the patch, but is not useful for patch
application). And that's also the area where the v(n)->v(n+1) changelog
should be included. Traditionally, we place that log above the diffstat.

>
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index d11daacc23..6e66cbbc41 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1869,6 +1869,51 @@ 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;
> +
> + return;
> +}

This return statement is useless. Please see my review at:

http://mid.mail-archive.com/[e-mail]<http://mid.mail-archive.com/[e-mail]>

There I wrote, 'Please put a "return" statement here'. And, my request
was placed *exactly between* the error_setg() call and the closing brace.

The idea being that, if we take the (value > 16 * MiB) branch, then yes
we need to set the error, but we also need to abandon the rest of the
function. If "value" is invalid (out of bounds), then "pcms->max_fw_size
= value" is exactly the assignment that we do *not* want to reach.

> +
> static void pc_machine_initfn(Object *obj)
> {
> PCMachineState *pcms = PC_MACHINE(obj);
> @@ -1884,6 +1929,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;

Thank you for dropping the comment.

Summary:

- the changelog is helpful (thanks!), it's placement is not perfect yet
- the return statement should be moved so that it terminate the (value >
16 * MiB) branch.

Also, I think "HPi" (rather than "HP") in the commit message *could* be
a typo (I'm not sure).

Thanks,
Laszlo


>
> pc_system_flash_create(pcms);
> pcms->pcspk = isa_new(TYPE_PC_SPEAKER);
> @@ -2004,6 +2050,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:
>

Dr. David Alan Gilbert Sept. 25, 2020, 5:59 p.m. UTC | #4

* McMillan, Erich ([e-mail]) wrote:
> Additionally HPi is not a mistake, corporate requires that we refer to ourselves as Hewlett Packard Inc since the split in 2015. I will perhaps update this to be the full name for clarity.

Please note that no one outside of HP knows what HPi is; so you might
want to spell it out a bit.

Dave

> 
> From: McMillan, Erich
> Sent: Friday, September 25, 2020 12:15 PM
> To: Laszlo Ersek <[e-mail]>; [e-mail]
> Cc: [e-mail]; [e-mail]; [e-mail]; [e-mail]; [e-mail]
> Subject: RE: [PATCH v5] hw/i386/pc: add max combined fw size as machine configuration option
> 
> Hi Laszlo,
> 
> Thank you for the feedback, apologies that I missed the exact line references I was moving too fast.
> I appreciate you taking the time to explain the nuances.
> 
> On an unrelated note, it seems that my patches are no longer appearing in https://lists.nongnu.org/archive/html/qemu-devel/2020-09/index.html is this because I need to cc [e-mail]<mailto:[e-mail]> rather than –to?
> 
> -Erich
> 
> From: Laszlo Ersek [mailto:[e-mail]]
> Sent: Friday, September 25, 2020 2:00 AM
> To: McMillan, Erich <[e-mail]<mailto:[e-mail]>>; [e-mail]<mailto:[e-mail]>
> Cc: [e-mail]<mailto:[e-mail]>; [e-mail]<mailto:[e-mail]>; [e-mail]<mailto:[e-mail]>; [e-mail]<mailto:[e-mail]>; [e-mail]<mailto:[e-mail]>
> Subject: Re: [PATCH v5] hw/i386/pc: add max combined fw size as machine configuration option
> 
> Hi Erich,
> 
> when processing review feedback, please pay attention to *where* the
> review comments are inserted, in response to your patch email.
> 
> I'm pointing this out not because I want to annoy you with my
> obsessions, but because I consider this discussion a kind of "git +
> mailing lists" training for you. (In accordance with your first message
> on the topic.)
> 
> Please see specifics below:
> 
> On 09/25/20 05:36, Erich Mcmillan wrote:
> > From: Erich McMillan <[e-mail]<mailto:[e-mail]>>
> >
> > At HPi we have a need for increased fw size to enable testing of our custom fw.
> >
> > Signed-off-by: Erich McMillan <[e-mail]<mailto:[e-mail]>>
> >
> > Change since v4:
> > Add explicit return to pc_machine_set_max_fw_size.
> > Remove /* default */ from max_fw_size initialization.
> > ---
> >
> > hw/i386/pc.c | 52 ++++++++++++++++++++++++++++++++++++++++++++
> > hw/i386/pc_sysfw.c | 13 ++---------
> > include/hw/i386/pc.h | 2 ++
> > 3 files changed, 56 insertions(+), 11 deletions(-)
> 
> Please refer to my earlier feedback, archived at the following location:
> 
> http://mid.mail-archive.com/[e-mail]<http://mid.mail-archive.com/[e-mail]>
> 
> As I say in that message, the v(n)->v(n+1) changelog belongs 'between
> the "---" separator and the diffstat'. In that message, I marked the
> specific location with [*].
> 
> Basically the "---" separator terminates the commit message, and the
> first "diff --git" line starts the code changes. What's between them is
> thrown away, when the patch is applied. So in that throwaway area,
> git-format-patch places the diffstat automatically (because it gives
> reviewers a helpful overview of the patch, but is not useful for patch
> application). And that's also the area where the v(n)->v(n+1) changelog
> should be included. Traditionally, we place that log above the diffstat.
> 
> >
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index d11daacc23..6e66cbbc41 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -1869,6 +1869,51 @@ 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;
> > +
> > + return;
> > +}
> 
> This return statement is useless. Please see my review at:
> 
> http://mid.mail-archive.com/[e-mail]<http://mid.mail-archive.com/[e-mail]>
> 
> There I wrote, 'Please put a "return" statement here'. And, my request
> was placed *exactly between* the error_setg() call and the closing brace.
> 
> The idea being that, if we take the (value > 16 * MiB) branch, then yes
> we need to set the error, but we also need to abandon the rest of the
> function. If "value" is invalid (out of bounds), then "pcms->max_fw_size
> = value" is exactly the assignment that we do *not* want to reach.
> 
> > +
> > static void pc_machine_initfn(Object *obj)
> > {
> > PCMachineState *pcms = PC_MACHINE(obj);
> > @@ -1884,6 +1929,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;
> 
> Thank you for dropping the comment.
> 
> Summary:
> 
> - the changelog is helpful (thanks!), it's placement is not perfect yet
> - the return statement should be moved so that it terminate the (value >
> 16 * MiB) branch.
> 
> Also, I think "HPi" (rather than "HP") in the commit message *could* be
> a typo (I'm not sure).
> 
> Thanks,
> Laszlo
> 
> 
> >
> > pc_system_flash_create(pcms);
> > pcms->pcspk = isa_new(TYPE_PC_SPEAKER);
> > @@ -2004,6 +2050,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:
> >

Philippe Mathieu-Daudé Sept. 25, 2020, 6:07 p.m. UTC | #5

On 9/25/20 7:17 PM, McMillan, Erich wrote:
> Additionally HPi is not a mistake, corporate requires that we refer to
> ourselves as Hewlett Packard Inc since the split in 2015. I will perhaps
> update this to be the full name for clarity.

Maybe worth be explicit, else it is confusing (since your
email is hp.com and not hpi.com).

> 
>  
> 
>  
> 
> *From:*McMillan, Erich
> *Sent:* Friday, September 25, 2020 12:15 PM
> *To:* Laszlo Ersek <[e-mail]>; [e-mail]
> *Cc:* [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]
> *Subject:* RE: [PATCH v5] hw/i386/pc: add max combined fw size as
> machine configuration option
> 
>  
> 
> Hi Laszlo,
> 
>  
> 
> Thank you for the feedback, apologies that I missed the exact line
> references I was moving too fast.
> 
> I appreciate you taking the time to explain the nuances.
> 
>  
> 
> On an unrelated note, it seems that my patches are no longer appearing
> in https://lists.nongnu.org/archive/html/qemu-devel/2020-09/index.html
> is this because I need to cc [e-mail]
> <mailto:[e-mail]> rather than –to?

It is:
https://lists.nongnu.org/archive/html/qemu-devel/2020-09/msg09212.html

The archive is updated twice an hour I guess.

> 
>  
> 
> -Erich

Laszlo Ersek Sept. 28, 2020, 6:10 p.m. UTC | #6

On 09/25/20 19:14, McMillan, Erich wrote:

> On an unrelated note, it seems that my patches are no longer appearing in https://lists.nongnu.org/archive/html/qemu-devel/2020-09/index.html is this because I need to cc [e-mail]<mailto:[e-mail]> rather than –to?

Cc: and To: are equally fine. I can see both your v5 and v6 postings there:

https://lists.nongnu.org/archive/html/qemu-devel/2020-09/msg09212.html
https://lists.nongnu.org/archive/html/qemu-devel/2020-09/msg09577.html

The qemu-devel list is very busy, plus <https://lists.nongnu.org/> hosts
a very large number of other lists -- so updates to the WebUI are done
in batches (I think once every 30 minutes, but I could be out of date on
that). A fresh posting almost never shows up immediately on the WebUI.
(I believe it may be delivered to subscribers via actual email more
quickly.)

Thanks
Laszlo

Zhijian Li (Fujitsu)" via Sept. 28, 2020, 6:21 p.m. UTC | #7

Laszlo,

Thanks double checking the archives. I did end up finding them the next day.

With regards to PATCH v6, I missed removing the squash commit message, so that will need to be fixed in v7 apologies for that.

-Erich

Patch

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index d11daacc23..6e66cbbc41 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1869,6 +1869,51 @@  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;
+
+    return;
+}
+
 static void pc_machine_initfn(Object *obj)
 {
     PCMachineState *pcms = PC_MACHINE(obj);
@@ -1884,6 +1929,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 +2050,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: