[PATCH v2 0/4] smbus: SPD fixes

[PATCH v2 0/4] smbus: SPD fixes

[Markus Armbruster]

v2:
* PATCH 1: Do not revert commit 08fd99179a's silent change of memory
  type [Zoltan]
* PATCH 3: Improve error message some more [Philippe]

Markus Armbruster (4):
  sam460ex: Suppress useless warning on -m 32 and -m 64
  smbus: Fix spd_data_generate() error API violation
  bamboo, sam460ex: Tidy up error message for unsupported RAM size
  smbus: Fix spd_data_generate() for number of banks > 2

 include/hw/i2c/smbus_eeprom.h |  2 +-
 hw/i2c/smbus_eeprom.c         | 32 +++++---------------------------
 hw/mips/mips_fulong2e.c       | 10 ++--------
 hw/ppc/ppc4xx_devs.c          |  8 ++++----
 hw/ppc/sam460ex.c             | 13 ++++---------
 5 files changed, 16 insertions(+), 49 deletions(-)

-- 
2.21.1

[PATCH v2 1/4] sam460ex: Suppress useless warning on -m 32 and -m 64

[Markus Armbruster]

Requesting 32 or 64 MiB of RAM with the sam460ex machine type produces
a useless warning:

    qemu-system-ppc: warning: Memory size is too small for SDRAM type, adjusting type

This is because sam460ex_init() asks spd_data_generate() for DDR2,
which is impossible, so spd_data_generate() corrects it to DDR.

The warning goes back to commit 08fd99179a "sam460ex: Clean up SPD
EEPROM creation".

Make sam460ex_init() pass the correct SDRAM type to get rid of the
warning.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/ppc/sam460ex.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
index 898453cf30..1e3eaac0db 100644
--- a/hw/ppc/sam460ex.c
+++ b/hw/ppc/sam460ex.c
@@ -335,7 +335,8 @@ static void sam460ex_init(MachineState *machine)
     dev = sysbus_create_simple(TYPE_PPC4xx_I2C, 0x4ef600700, uic[0][2]);
     i2c = PPC4xx_I2C(dev)->bus;
     /* SPD EEPROM on RAM module */
-    spd_data = spd_data_generate(DDR2, ram_sizes[0], &err);
+    spd_data = spd_data_generate(ram_sizes[0] < 128 * MiB ? DDR : DDR2,
+                                 ram_sizes[0], &err);
     if (err) {
         warn_report_err(err);
     }
-- 
2.21.1

[PATCH v2 2/4] smbus: Fix spd_data_generate() error API violation

[Markus Armbruster]

The Error ** argument must be NULL, &error_abort, &error_fatal, or a
pointer to a variable containing NULL.  Passing an argument of the
latter kind twice without clearing it in between is wrong: if the
first call sets an error, it no longer points to NULL for the second
call.

spd_data_generate() can pass @errp to error_setg() more than once when
it adjusts both memory size and type.  Harmless, because no caller
passes anything that needs adjusting.  Until the previous commit,
sam460ex passed types that needed adjusting, but not sizes.

spd_data_generate()'s contract is rather awkward:

    If everything's fine, return non-null and don't set an error.

    Else, if memory size or type need adjusting, return non-null and
    set an error describing the adjustment.

    Else, return null and set an error reporting why no data can be
    generated.

Its callers treat the error as a warning even when null is returned.
They don't create the "smbus-eeprom" device then.  Suspicious.

Since the previous commit, only "everything's fine" can actually
happen.  Drop the unused code and simplify the callers.  This gets rid
of the error API violation.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 include/hw/i2c/smbus_eeprom.h |  2 +-
 hw/i2c/smbus_eeprom.c         | 30 ++++--------------------------
 hw/mips/mips_fulong2e.c       | 10 ++--------
 hw/ppc/sam460ex.c             | 12 +++---------
 4 files changed, 10 insertions(+), 44 deletions(-)

diff --git a/include/hw/i2c/smbus_eeprom.h b/include/hw/i2c/smbus_eeprom.h
index 15e2151b50..68b0063ab6 100644
--- a/include/hw/i2c/smbus_eeprom.h
+++ b/include/hw/i2c/smbus_eeprom.h
@@ -31,6 +31,6 @@ void smbus_eeprom_init(I2CBus *bus, int nb_eeprom,
                        const uint8_t *eeprom_spd, int size);
 
 enum sdram_type { SDR = 0x4, DDR = 0x7, DDR2 = 0x8 };
-uint8_t *spd_data_generate(enum sdram_type type, ram_addr_t size, Error **errp);
+uint8_t *spd_data_generate(enum sdram_type type, ram_addr_t size);
 
 #endif
diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
index 5adf3b15b5..07fbbf87f1 100644
--- a/hw/i2c/smbus_eeprom.c
+++ b/hw/i2c/smbus_eeprom.c
@@ -195,8 +195,7 @@ void smbus_eeprom_init(I2CBus *smbus, int nb_eeprom,
 }
 
 /* Generate SDRAM SPD EEPROM data describing a module of type and size */
-uint8_t *spd_data_generate(enum sdram_type type, ram_addr_t ram_size,
-                           Error **errp)
+uint8_t *spd_data_generate(enum sdram_type type, ram_addr_t ram_size)
 {
     uint8_t *spd;
     uint8_t nbanks;
@@ -222,29 +221,10 @@ uint8_t *spd_data_generate(enum sdram_type type, ram_addr_t ram_size,
         g_assert_not_reached();
     }
     size = ram_size >> 20; /* work in terms of megabytes */
-    if (size < 4) {
-        error_setg(errp, "SDRAM size is too small");
-        return NULL;
-    }
     sz_log2 = 31 - clz32(size);
     size = 1U << sz_log2;
-    if (ram_size > size * MiB) {
-        error_setg(errp, "SDRAM size 0x"RAM_ADDR_FMT" is not a power of 2, "
-                   "truncating to %u MB", ram_size, size);
-    }
-    if (sz_log2 < min_log2) {
-        error_setg(errp,
-                   "Memory size is too small for SDRAM type, adjusting type");
-        if (size >= 32) {
-            type = DDR;
-            min_log2 = 5;
-            max_log2 = 12;
-        } else {
-            type = SDR;
-            min_log2 = 2;
-            max_log2 = 9;
-        }
-    }
+    assert(ram_size == size * MiB);
+    assert(sz_log2 >= min_log2);
 
     nbanks = 1;
     while (sz_log2 > max_log2 && nbanks < 8) {
@@ -252,9 +232,7 @@ uint8_t *spd_data_generate(enum sdram_type type, ram_addr_t ram_size,
         nbanks++;
     }
 
-    if (size > (1ULL << sz_log2) * nbanks) {
-        error_setg(errp, "Memory size is too big for SDRAM, truncating");
-    }
+    assert(size == (1ULL << sz_log2) * nbanks);
 
     /* split to 2 banks if possible to avoid a bug in MIPS Malta firmware */
     if (nbanks == 1 && sz_log2 > min_log2) {
diff --git a/hw/mips/mips_fulong2e.c b/hw/mips/mips_fulong2e.c
index 5040afd581..ef02d54b33 100644
--- a/hw/mips/mips_fulong2e.c
+++ b/hw/mips/mips_fulong2e.c
@@ -297,7 +297,6 @@ static void mips_fulong2e_init(MachineState *machine)
     MemoryRegion *bios = g_new(MemoryRegion, 1);
     long bios_size;
     uint8_t *spd_data;
-    Error *err = NULL;
     int64_t kernel_entry;
     PCIBus *pci_bus;
     ISABus *isa_bus;
@@ -377,13 +376,8 @@ static void mips_fulong2e_init(MachineState *machine)
     }
 
     /* Populate SPD eeprom data */
-    spd_data = spd_data_generate(DDR, machine->ram_size, &err);
-    if (err) {
-        warn_report_err(err);
-    }
-    if (spd_data) {
-        smbus_eeprom_init_one(smbus, 0x50, spd_data);
-    }
+    spd_data = spd_data_generate(DDR, machine->ram_size);
+    smbus_eeprom_init_one(smbus, 0x50, spd_data);
 
     mc146818_rtc_init(isa_bus, 2000, NULL);
 
diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
index 1e3eaac0db..42a8c9fb7f 100644
--- a/hw/ppc/sam460ex.c
+++ b/hw/ppc/sam460ex.c
@@ -292,7 +292,6 @@ static void sam460ex_init(MachineState *machine)
     SysBusDevice *sbdev;
     struct boot_info *boot_info;
     uint8_t *spd_data;
-    Error *err = NULL;
     int success;
 
     cpu = POWERPC_CPU(cpu_create(machine->cpu_type));
@@ -336,14 +335,9 @@ static void sam460ex_init(MachineState *machine)
     i2c = PPC4xx_I2C(dev)->bus;
     /* SPD EEPROM on RAM module */
     spd_data = spd_data_generate(ram_sizes[0] < 128 * MiB ? DDR : DDR2,
-                                 ram_sizes[0], &err);
-    if (err) {
-        warn_report_err(err);
-    }
-    if (spd_data) {
-        spd_data[20] = 4; /* SO-DIMM module */
-        smbus_eeprom_init_one(i2c, 0x50, spd_data);
-    }
+                                 ram_sizes[0]);
+    spd_data[20] = 4; /* SO-DIMM module */
+    smbus_eeprom_init_one(i2c, 0x50, spd_data);
     /* RTC */
     i2c_create_slave(i2c, "m41t80", 0x68);
 
-- 
2.21.1

[PATCH v2 3/4] bamboo, sam460ex: Tidy up error message for unsupported RAM size

[Markus Armbruster]

Improve

    $ ppc-softmmu/qemu-system-ppc -M sam460ex -m 4096
    qemu-system-ppc: Max 1 banks of 2048 ,1024 ,512 ,256 ,128 ,64 ,32 MB DIMM/bank supported
    qemu-system-ppc: Possible valid RAM size: 2048

to

    qemu-system-ppc: at most 1 bank of 2048, 1024, 512, 256, 128, 64, 32 MiB each supported
    Possible valid RAM size: 1024 MiB

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/ppc/ppc4xx_devs.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/ppc/ppc4xx_devs.c b/hw/ppc/ppc4xx_devs.c
index 3376c43ff5..f1651e04d9 100644
--- a/hw/ppc/ppc4xx_devs.c
+++ b/hw/ppc/ppc4xx_devs.c
@@ -716,11 +716,11 @@ void ppc4xx_sdram_banks(MemoryRegion *ram, int nr_banks,
         for (i = 0; sdram_bank_sizes[i]; i++) {
             g_string_append_printf(s, "%" PRIi64 "%s",
                                    sdram_bank_sizes[i] / MiB,
-                                   sdram_bank_sizes[i + 1] ? " ," : "");
+                                   sdram_bank_sizes[i + 1] ? ", " : "");
         }
-        error_report("Max %d banks of %s MB DIMM/bank supported",
-            nr_banks, s->str);
-        error_report("Possible valid RAM size: %" PRIi64,
+        error_report("at most %d bank%s of %s MiB each supported",
+                     nr_banks, nr_banks == 1 ? "" : "s", s->str);
+        error_printf("Possible valid RAM size: %" PRIi64 " MiB \n",
             used_size ? used_size / MiB : sdram_bank_sizes[i - 1] / MiB);
 
         g_string_free(s, true);
-- 
2.21.1

[PATCH v2 4/4] smbus: Fix spd_data_generate() for number of banks > 2

[Markus Armbruster]

spd_data_generate() splits @ram_size bytes into @nbanks RAM banks of
1 << sz_log2 MiB each, like this:

    size = ram_size >> 20; /* work in terms of megabytes */
    [...]
    nbanks = 1;
    while (sz_log2 > max_log2 && nbanks < 8) {
        sz_log2--;
        nbanks++;
    }

Each iteration halves the size of a bank, and increments the number of
banks.  Wrong: it should double the number of banks.

The bug goes back all the way to commit b296b664ab "smbus: Add a
helper to generate SPD EEPROM data".

It can't bite because spd_data_generate()'s current users pass only
@ram_size that result in *zero* iterations:

    machine     RAM size    #banks  type    bank size
    fulong2e     256 MiB         1   DDR      256 MiB
    sam460ex    2048 MiB         1   DDR2    2048 MiB
                1024 MiB         1   DDR2    1024 MiB
                 512 MiB         1   DDR2     512 MiB
                 256 MiB         1   DDR2     256 MiB
                 128 MiB         1   SDR      128 MiB
                  64 MiB         1   SDR       64 MiB
                  32 MiB         1   SDR       32 MiB

Apply the obvious, minimal fix.  I admit I'm tempted to rip out the
unused (and obviously untested) feature instead, because YAGNI.

Note that this is not the final result, as spd_data_generate() next
increases #banks from 1 to 2 if possible.  This is done "to avoid a
bug in MIPS Malta firmware".  We don't even use this function with
machine type malta.  *Shrug*

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/i2c/smbus_eeprom.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
index 07fbbf87f1..e199fc8678 100644
--- a/hw/i2c/smbus_eeprom.c
+++ b/hw/i2c/smbus_eeprom.c
@@ -229,7 +229,7 @@ uint8_t *spd_data_generate(enum sdram_type type, ram_addr_t ram_size)
     nbanks = 1;
     while (sz_log2 > max_log2 && nbanks < 8) {
         sz_log2--;
-        nbanks++;
+        nbanks *= 2;
     }
 
     assert(size == (1ULL << sz_log2) * nbanks);
-- 
2.21.1

Re: [PATCH v2 3/4] bamboo, sam460ex: Tidy up error message for unsupported RAM size

[BALATON Zoltan]

On Wed, 22 Apr 2020, Markus Armbruster wrote:
> Improve
>
>    $ ppc-softmmu/qemu-system-ppc -M sam460ex -m 4096
>    qemu-system-ppc: Max 1 banks of 2048 ,1024 ,512 ,256 ,128 ,64 ,32 MB DIMM/bank supported
>    qemu-system-ppc: Possible valid RAM size: 2048
>
> to
>
>    qemu-system-ppc: at most 1 bank of 2048, 1024, 512, 256, 128, 64, 32 MiB each supported
>    Possible valid RAM size: 1024 MiB
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu>

> ---
> hw/ppc/ppc4xx_devs.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/hw/ppc/ppc4xx_devs.c b/hw/ppc/ppc4xx_devs.c
> index 3376c43ff5..f1651e04d9 100644
> --- a/hw/ppc/ppc4xx_devs.c
> +++ b/hw/ppc/ppc4xx_devs.c
> @@ -716,11 +716,11 @@ void ppc4xx_sdram_banks(MemoryRegion *ram, int nr_banks,
>         for (i = 0; sdram_bank_sizes[i]; i++) {
>             g_string_append_printf(s, "%" PRIi64 "%s",
>                                    sdram_bank_sizes[i] / MiB,
> -                                   sdram_bank_sizes[i + 1] ? " ," : "");
> +                                   sdram_bank_sizes[i + 1] ? ", " : "");
>         }
> -        error_report("Max %d banks of %s MB DIMM/bank supported",
> -            nr_banks, s->str);
> -        error_report("Possible valid RAM size: %" PRIi64,
> +        error_report("at most %d bank%s of %s MiB each supported",
> +                     nr_banks, nr_banks == 1 ? "" : "s", s->str);
> +        error_printf("Possible valid RAM size: %" PRIi64 " MiB \n",
>             used_size ? used_size / MiB : sdram_bank_sizes[i - 1] / MiB);
>
>         g_string_free(s, true);
>

Re: [PATCH v2 3/4] bamboo, sam460ex: Tidy up error message for unsupported RAM size

[Philippe Mathieu-Daudé]

On 4/22/20 3:48 PM, Markus Armbruster wrote:
> Improve
> 
>      $ ppc-softmmu/qemu-system-ppc -M sam460ex -m 4096
>      qemu-system-ppc: Max 1 banks of 2048 ,1024 ,512 ,256 ,128 ,64 ,32 MB DIMM/bank supported
>      qemu-system-ppc: Possible valid RAM size: 2048
> 
> to
> 
>      qemu-system-ppc: at most 1 bank of 2048, 1024, 512, 256, 128, 64, 32 MiB each supported
>      Possible valid RAM size: 1024 MiB

Thanks,

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   hw/ppc/ppc4xx_devs.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/ppc/ppc4xx_devs.c b/hw/ppc/ppc4xx_devs.c
> index 3376c43ff5..f1651e04d9 100644
> --- a/hw/ppc/ppc4xx_devs.c
> +++ b/hw/ppc/ppc4xx_devs.c
> @@ -716,11 +716,11 @@ void ppc4xx_sdram_banks(MemoryRegion *ram, int nr_banks,
>           for (i = 0; sdram_bank_sizes[i]; i++) {
>               g_string_append_printf(s, "%" PRIi64 "%s",
>                                      sdram_bank_sizes[i] / MiB,
> -                                   sdram_bank_sizes[i + 1] ? " ," : "");
> +                                   sdram_bank_sizes[i + 1] ? ", " : "");
>           }
> -        error_report("Max %d banks of %s MB DIMM/bank supported",
> -            nr_banks, s->str);
> -        error_report("Possible valid RAM size: %" PRIi64,
> +        error_report("at most %d bank%s of %s MiB each supported",
> +                     nr_banks, nr_banks == 1 ? "" : "s", s->str);
> +        error_printf("Possible valid RAM size: %" PRIi64 " MiB \n",
>               used_size ? used_size / MiB : sdram_bank_sizes[i - 1] / MiB);
>   
>           g_string_free(s, true);
> 

Re: [PATCH v2 2/4] smbus: Fix spd_data_generate() error API violation

[BALATON Zoltan]

On Wed, 22 Apr 2020, Markus Armbruster wrote:
> The Error ** argument must be NULL, &error_abort, &error_fatal, or a
> pointer to a variable containing NULL.  Passing an argument of the
> latter kind twice without clearing it in between is wrong: if the
> first call sets an error, it no longer points to NULL for the second
> call.
>
> spd_data_generate() can pass @errp to error_setg() more than once when
> it adjusts both memory size and type.  Harmless, because no caller
> passes anything that needs adjusting.  Until the previous commit,
> sam460ex passed types that needed adjusting, but not sizes.
>
> spd_data_generate()'s contract is rather awkward:
>
>    If everything's fine, return non-null and don't set an error.
>
>    Else, if memory size or type need adjusting, return non-null and
>    set an error describing the adjustment.
>
>    Else, return null and set an error reporting why no data can be
>    generated.
>
> Its callers treat the error as a warning even when null is returned.
> They don't create the "smbus-eeprom" device then.  Suspicious.
>
> Since the previous commit, only "everything's fine" can actually
> happen.  Drop the unused code and simplify the callers.  This gets rid
> of the error API violation.

This leaves board code no chance to recover from values given by user that 
won't fit without duplicating checks that this function does. Also this 
will abort without giving meaningful errors if an invalid value does get 
through and result in a crash which is not used friendly. So I don't like 
this but if others think this is acceptable maybe at least unit test 
should be adjusted to make sure aborts cannot be triggered by user for 
values that are not usually tested during development.

Regards,
BALATON Zoltan

> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> include/hw/i2c/smbus_eeprom.h |  2 +-
> hw/i2c/smbus_eeprom.c         | 30 ++++--------------------------
> hw/mips/mips_fulong2e.c       | 10 ++--------
> hw/ppc/sam460ex.c             | 12 +++---------
> 4 files changed, 10 insertions(+), 44 deletions(-)
>
> diff --git a/include/hw/i2c/smbus_eeprom.h b/include/hw/i2c/smbus_eeprom.h
> index 15e2151b50..68b0063ab6 100644
> --- a/include/hw/i2c/smbus_eeprom.h
> +++ b/include/hw/i2c/smbus_eeprom.h
> @@ -31,6 +31,6 @@ void smbus_eeprom_init(I2CBus *bus, int nb_eeprom,
>                        const uint8_t *eeprom_spd, int size);
>
> enum sdram_type { SDR = 0x4, DDR = 0x7, DDR2 = 0x8 };
> -uint8_t *spd_data_generate(enum sdram_type type, ram_addr_t size, Error **errp);
> +uint8_t *spd_data_generate(enum sdram_type type, ram_addr_t size);
>
> #endif
> diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
> index 5adf3b15b5..07fbbf87f1 100644
> --- a/hw/i2c/smbus_eeprom.c
> +++ b/hw/i2c/smbus_eeprom.c
> @@ -195,8 +195,7 @@ void smbus_eeprom_init(I2CBus *smbus, int nb_eeprom,
> }
>
> /* Generate SDRAM SPD EEPROM data describing a module of type and size */
> -uint8_t *spd_data_generate(enum sdram_type type, ram_addr_t ram_size,
> -                           Error **errp)
> +uint8_t *spd_data_generate(enum sdram_type type, ram_addr_t ram_size)
> {
>     uint8_t *spd;
>     uint8_t nbanks;
> @@ -222,29 +221,10 @@ uint8_t *spd_data_generate(enum sdram_type type, ram_addr_t ram_size,
>         g_assert_not_reached();
>     }
>     size = ram_size >> 20; /* work in terms of megabytes */
> -    if (size < 4) {
> -        error_setg(errp, "SDRAM size is too small");
> -        return NULL;
> -    }
>     sz_log2 = 31 - clz32(size);
>     size = 1U << sz_log2;
> -    if (ram_size > size * MiB) {
> -        error_setg(errp, "SDRAM size 0x"RAM_ADDR_FMT" is not a power of 2, "
> -                   "truncating to %u MB", ram_size, size);
> -    }
> -    if (sz_log2 < min_log2) {
> -        error_setg(errp,
> -                   "Memory size is too small for SDRAM type, adjusting type");
> -        if (size >= 32) {
> -            type = DDR;
> -            min_log2 = 5;
> -            max_log2 = 12;
> -        } else {
> -            type = SDR;
> -            min_log2 = 2;
> -            max_log2 = 9;
> -        }
> -    }
> +    assert(ram_size == size * MiB);
> +    assert(sz_log2 >= min_log2);
>
>     nbanks = 1;
>     while (sz_log2 > max_log2 && nbanks < 8) {
> @@ -252,9 +232,7 @@ uint8_t *spd_data_generate(enum sdram_type type, ram_addr_t ram_size,
>         nbanks++;
>     }
>
> -    if (size > (1ULL << sz_log2) * nbanks) {
> -        error_setg(errp, "Memory size is too big for SDRAM, truncating");
> -    }
> +    assert(size == (1ULL << sz_log2) * nbanks);
>
>     /* split to 2 banks if possible to avoid a bug in MIPS Malta firmware */
>     if (nbanks == 1 && sz_log2 > min_log2) {
> diff --git a/hw/mips/mips_fulong2e.c b/hw/mips/mips_fulong2e.c
> index 5040afd581..ef02d54b33 100644
> --- a/hw/mips/mips_fulong2e.c
> +++ b/hw/mips/mips_fulong2e.c
> @@ -297,7 +297,6 @@ static void mips_fulong2e_init(MachineState *machine)
>     MemoryRegion *bios = g_new(MemoryRegion, 1);
>     long bios_size;
>     uint8_t *spd_data;
> -    Error *err = NULL;
>     int64_t kernel_entry;
>     PCIBus *pci_bus;
>     ISABus *isa_bus;
> @@ -377,13 +376,8 @@ static void mips_fulong2e_init(MachineState *machine)
>     }
>
>     /* Populate SPD eeprom data */
> -    spd_data = spd_data_generate(DDR, machine->ram_size, &err);
> -    if (err) {
> -        warn_report_err(err);
> -    }
> -    if (spd_data) {
> -        smbus_eeprom_init_one(smbus, 0x50, spd_data);
> -    }
> +    spd_data = spd_data_generate(DDR, machine->ram_size);
> +    smbus_eeprom_init_one(smbus, 0x50, spd_data);
>
>     mc146818_rtc_init(isa_bus, 2000, NULL);
>
> diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
> index 1e3eaac0db..42a8c9fb7f 100644
> --- a/hw/ppc/sam460ex.c
> +++ b/hw/ppc/sam460ex.c
> @@ -292,7 +292,6 @@ static void sam460ex_init(MachineState *machine)
>     SysBusDevice *sbdev;
>     struct boot_info *boot_info;
>     uint8_t *spd_data;
> -    Error *err = NULL;
>     int success;
>
>     cpu = POWERPC_CPU(cpu_create(machine->cpu_type));
> @@ -336,14 +335,9 @@ static void sam460ex_init(MachineState *machine)
>     i2c = PPC4xx_I2C(dev)->bus;
>     /* SPD EEPROM on RAM module */
>     spd_data = spd_data_generate(ram_sizes[0] < 128 * MiB ? DDR : DDR2,
> -                                 ram_sizes[0], &err);
> -    if (err) {
> -        warn_report_err(err);
> -    }
> -    if (spd_data) {
> -        spd_data[20] = 4; /* SO-DIMM module */
> -        smbus_eeprom_init_one(i2c, 0x50, spd_data);
> -    }
> +                                 ram_sizes[0]);
> +    spd_data[20] = 4; /* SO-DIMM module */
> +    smbus_eeprom_init_one(i2c, 0x50, spd_data);
>     /* RTC */
>     i2c_create_slave(i2c, "m41t80", 0x68);
>
>

Re: [PATCH v2 1/4] sam460ex: Suppress useless warning on -m 32 and -m 64

[Philippe Mathieu-Daudé]

On 4/22/20 3:48 PM, Markus Armbruster wrote:
> Requesting 32 or 64 MiB of RAM with the sam460ex machine type produces
> a useless warning:
> 
>      qemu-system-ppc: warning: Memory size is too small for SDRAM type, adjusting type
> 
> This is because sam460ex_init() asks spd_data_generate() for DDR2,
> which is impossible, so spd_data_generate() corrects it to DDR.
> 
> The warning goes back to commit 08fd99179a "sam460ex: Clean up SPD
> EEPROM creation".
> 
> Make sam460ex_init() pass the correct SDRAM type to get rid of the
> warning.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   hw/ppc/sam460ex.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
> index 898453cf30..1e3eaac0db 100644
> --- a/hw/ppc/sam460ex.c
> +++ b/hw/ppc/sam460ex.c
> @@ -335,7 +335,8 @@ static void sam460ex_init(MachineState *machine)
>       dev = sysbus_create_simple(TYPE_PPC4xx_I2C, 0x4ef600700, uic[0][2]);
>       i2c = PPC4xx_I2C(dev)->bus;
>       /* SPD EEPROM on RAM module */
> -    spd_data = spd_data_generate(DDR2, ram_sizes[0], &err);
> +    spd_data = spd_data_generate(ram_sizes[0] < 128 * MiB ? DDR : DDR2,
> +                                 ram_sizes[0], &err);

Previous to this patch question, don't we need one for each module? We 
have 4 banks... Ah, there is a comment earlier "/* put all RAM on first 
bank because board has one slot". OK...

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

>       if (err) {
>           warn_report_err(err);
>       }
> 

Re: [PATCH v2 2/4] smbus: Fix spd_data_generate() error API violation

[Philippe Mathieu-Daudé]

On 4/22/20 4:27 PM, BALATON Zoltan wrote:
> On Wed, 22 Apr 2020, Markus Armbruster wrote:
>> The Error ** argument must be NULL, &error_abort, &error_fatal, or a
>> pointer to a variable containing NULL.  Passing an argument of the
>> latter kind twice without clearing it in between is wrong: if the
>> first call sets an error, it no longer points to NULL for the second
>> call.
>>
>> spd_data_generate() can pass @errp to error_setg() more than once when
>> it adjusts both memory size and type.  Harmless, because no caller
>> passes anything that needs adjusting.  Until the previous commit,
>> sam460ex passed types that needed adjusting, but not sizes.
>>
>> spd_data_generate()'s contract is rather awkward:
>>
>>    If everything's fine, return non-null and don't set an error.
>>
>>    Else, if memory size or type need adjusting, return non-null and
>>    set an error describing the adjustment.
>>
>>    Else, return null and set an error reporting why no data can be
>>    generated.
>>
>> Its callers treat the error as a warning even when null is returned.
>> They don't create the "smbus-eeprom" device then.  Suspicious.
>>
>> Since the previous commit, only "everything's fine" can actually
>> happen.  Drop the unused code and simplify the callers.  This gets rid
>> of the error API violation.
> 
> This leaves board code no chance to recover from values given by user 
> that won't fit without duplicating checks that this function does. Also 
> this will abort without giving meaningful errors if an invalid value 
> does get through and result in a crash which is not used friendly. So I 
> don't like this but if others think this is acceptable maybe at least 
> unit test should be adjusted to make sure aborts cannot be triggered by 
> user for values that are not usually tested during development.

Agreed. Do you have an example (or more) to better show Markus this code 
use? So we can add tests.

Personally I'd use a script to generate a dumb static array of all 
possible sizes...

> 
> Regards,
> BALATON Zoltan
> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>> include/hw/i2c/smbus_eeprom.h |  2 +-
>> hw/i2c/smbus_eeprom.c         | 30 ++++--------------------------
>> hw/mips/mips_fulong2e.c       | 10 ++--------
>> hw/ppc/sam460ex.c             | 12 +++---------
>> 4 files changed, 10 insertions(+), 44 deletions(-)
>>
>> diff --git a/include/hw/i2c/smbus_eeprom.h 
>> b/include/hw/i2c/smbus_eeprom.h
>> index 15e2151b50..68b0063ab6 100644
>> --- a/include/hw/i2c/smbus_eeprom.h
>> +++ b/include/hw/i2c/smbus_eeprom.h
>> @@ -31,6 +31,6 @@ void smbus_eeprom_init(I2CBus *bus, int nb_eeprom,
>>                        const uint8_t *eeprom_spd, int size);
>>
>> enum sdram_type { SDR = 0x4, DDR = 0x7, DDR2 = 0x8 };
>> -uint8_t *spd_data_generate(enum sdram_type type, ram_addr_t size, 
>> Error **errp);
>> +uint8_t *spd_data_generate(enum sdram_type type, ram_addr_t size);
>>
>> #endif
>> diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
>> index 5adf3b15b5..07fbbf87f1 100644
>> --- a/hw/i2c/smbus_eeprom.c
>> +++ b/hw/i2c/smbus_eeprom.c
>> @@ -195,8 +195,7 @@ void smbus_eeprom_init(I2CBus *smbus, int nb_eeprom,
>> }
>>
>> /* Generate SDRAM SPD EEPROM data describing a module of type and size */
>> -uint8_t *spd_data_generate(enum sdram_type type, ram_addr_t ram_size,
>> -                           Error **errp)
>> +uint8_t *spd_data_generate(enum sdram_type type, ram_addr_t ram_size)
>> {
>>     uint8_t *spd;
>>     uint8_t nbanks;
>> @@ -222,29 +221,10 @@ uint8_t *spd_data_generate(enum sdram_type type, 
>> ram_addr_t ram_size,
>>         g_assert_not_reached();
>>     }
>>     size = ram_size >> 20; /* work in terms of megabytes */
>> -    if (size < 4) {
>> -        error_setg(errp, "SDRAM size is too small");
>> -        return NULL;
>> -    }
>>     sz_log2 = 31 - clz32(size);
>>     size = 1U << sz_log2;
>> -    if (ram_size > size * MiB) {
>> -        error_setg(errp, "SDRAM size 0x"RAM_ADDR_FMT" is not a power 
>> of 2, "
>> -                   "truncating to %u MB", ram_size, size);
>> -    }
>> -    if (sz_log2 < min_log2) {
>> -        error_setg(errp,
>> -                   "Memory size is too small for SDRAM type, 
>> adjusting type");
>> -        if (size >= 32) {
>> -            type = DDR;
>> -            min_log2 = 5;
>> -            max_log2 = 12;
>> -        } else {
>> -            type = SDR;
>> -            min_log2 = 2;
>> -            max_log2 = 9;
>> -        }
>> -    }
>> +    assert(ram_size == size * MiB);
>> +    assert(sz_log2 >= min_log2);
>>
>>     nbanks = 1;
>>     while (sz_log2 > max_log2 && nbanks < 8) {
>> @@ -252,9 +232,7 @@ uint8_t *spd_data_generate(enum sdram_type type, 
>> ram_addr_t ram_size,
>>         nbanks++;
>>     }
>>
>> -    if (size > (1ULL << sz_log2) * nbanks) {
>> -        error_setg(errp, "Memory size is too big for SDRAM, 
>> truncating");
>> -    }
>> +    assert(size == (1ULL << sz_log2) * nbanks);
>>
>>     /* split to 2 banks if possible to avoid a bug in MIPS Malta 
>> firmware */
>>     if (nbanks == 1 && sz_log2 > min_log2) {
>> diff --git a/hw/mips/mips_fulong2e.c b/hw/mips/mips_fulong2e.c
>> index 5040afd581..ef02d54b33 100644
>> --- a/hw/mips/mips_fulong2e.c
>> +++ b/hw/mips/mips_fulong2e.c
>> @@ -297,7 +297,6 @@ static void mips_fulong2e_init(MachineState *machine)
>>     MemoryRegion *bios = g_new(MemoryRegion, 1);
>>     long bios_size;
>>     uint8_t *spd_data;
>> -    Error *err = NULL;
>>     int64_t kernel_entry;
>>     PCIBus *pci_bus;
>>     ISABus *isa_bus;
>> @@ -377,13 +376,8 @@ static void mips_fulong2e_init(MachineState 
>> *machine)
>>     }
>>
>>     /* Populate SPD eeprom data */
>> -    spd_data = spd_data_generate(DDR, machine->ram_size, &err);
>> -    if (err) {
>> -        warn_report_err(err);
>> -    }
>> -    if (spd_data) {
>> -        smbus_eeprom_init_one(smbus, 0x50, spd_data);
>> -    }
>> +    spd_data = spd_data_generate(DDR, machine->ram_size);
>> +    smbus_eeprom_init_one(smbus, 0x50, spd_data);
>>
>>     mc146818_rtc_init(isa_bus, 2000, NULL);
>>
>> diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
>> index 1e3eaac0db..42a8c9fb7f 100644
>> --- a/hw/ppc/sam460ex.c
>> +++ b/hw/ppc/sam460ex.c
>> @@ -292,7 +292,6 @@ static void sam460ex_init(MachineState *machine)
>>     SysBusDevice *sbdev;
>>     struct boot_info *boot_info;
>>     uint8_t *spd_data;
>> -    Error *err = NULL;
>>     int success;
>>
>>     cpu = POWERPC_CPU(cpu_create(machine->cpu_type));
>> @@ -336,14 +335,9 @@ static void sam460ex_init(MachineState *machine)
>>     i2c = PPC4xx_I2C(dev)->bus;
>>     /* SPD EEPROM on RAM module */
>>     spd_data = spd_data_generate(ram_sizes[0] < 128 * MiB ? DDR : DDR2,
>> -                                 ram_sizes[0], &err);
>> -    if (err) {
>> -        warn_report_err(err);
>> -    }
>> -    if (spd_data) {
>> -        spd_data[20] = 4; /* SO-DIMM module */
>> -        smbus_eeprom_init_one(i2c, 0x50, spd_data);
>> -    }
>> +                                 ram_sizes[0]);
>> +    spd_data[20] = 4; /* SO-DIMM module */
>> +    smbus_eeprom_init_one(i2c, 0x50, spd_data);
>>     /* RTC */
>>     i2c_create_slave(i2c, "m41t80", 0x68);
>>
>>
> 

Re: [PATCH v2 0/4] smbus: SPD fixes

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200422134815.1584-1-armbru@redhat.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Subject: [PATCH v2 0/4] smbus: SPD fixes
Message-id: 20200422134815.1584-1-armbru@redhat.com
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
   7769c23..ee573f5  master     -> master
 - [tag update]      patchew/20200421122236.24867-1-f4bug@amsat.org -> patchew/20200421122236.24867-1-f4bug@amsat.org
 - [tag update]      patchew/20200422011722.13287-1-richard.henderson@linaro.org -> patchew/20200422011722.13287-1-richard.henderson@linaro.org
 - [tag update]      patchew/20200422100211.30614-1-kraxel@redhat.com -> patchew/20200422100211.30614-1-kraxel@redhat.com
 - [tag update]      patchew/20200422124501.28015-1-peter.maydell@linaro.org -> patchew/20200422124501.28015-1-peter.maydell@linaro.org
 * [new tag]         patchew/20200422171305.10923-1-jonathan.derrick@intel.com -> patchew/20200422171305.10923-1-jonathan.derrick@intel.com
 * [new tag]         patchew/20200422172351.26583-1-pbonzini@redhat.com -> patchew/20200422172351.26583-1-pbonzini@redhat.com
Switched to a new branch 'test'
d2614d2 smbus: Fix spd_data_generate() for number of banks > 2
1156589 bamboo, sam460ex: Tidy up error message for unsupported RAM size
fcdd256 smbus: Fix spd_data_generate() error API violation
4de6998 sam460ex: Suppress useless warning on -m 32 and -m 64

=== OUTPUT BEGIN ===
1/4 Checking commit 4de6998f69d4 (sam460ex: Suppress useless warning on -m 32 and -m 64)
2/4 Checking commit fcdd25692f49 (smbus: Fix spd_data_generate() error API violation)
3/4 Checking commit 1156589650a7 (bamboo, sam460ex: Tidy up error message for unsupported RAM size)
ERROR: unnecessary whitespace before a quoted newline
#37: FILE: hw/ppc/ppc4xx_devs.c:723:
+        error_printf("Possible valid RAM size: %" PRIi64 " MiB \n",

total: 1 errors, 0 warnings, 15 lines checked

Patch 3/4 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

4/4 Checking commit d2614d2e379d (smbus: Fix spd_data_generate() for number of banks > 2)
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20200422134815.1584-1-armbru@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH v2 2/4] smbus: Fix spd_data_generate() error API violation

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 3859 bytes --]

On Wed, 22 Apr 2020, Philippe Mathieu-Daudé wrote:
> On 4/22/20 4:27 PM, BALATON Zoltan wrote:
>> On Wed, 22 Apr 2020, Markus Armbruster wrote:
>>> The Error ** argument must be NULL, &error_abort, &error_fatal, or a
>>> pointer to a variable containing NULL.  Passing an argument of the
>>> latter kind twice without clearing it in between is wrong: if the
>>> first call sets an error, it no longer points to NULL for the second
>>> call.
>>> 
>>> spd_data_generate() can pass @errp to error_setg() more than once when
>>> it adjusts both memory size and type.  Harmless, because no caller
>>> passes anything that needs adjusting.  Until the previous commit,
>>> sam460ex passed types that needed adjusting, but not sizes.
>>> 
>>> spd_data_generate()'s contract is rather awkward:
>>> 
>>>    If everything's fine, return non-null and don't set an error.
>>> 
>>>    Else, if memory size or type need adjusting, return non-null and
>>>    set an error describing the adjustment.
>>> 
>>>    Else, return null and set an error reporting why no data can be
>>>    generated.
>>> 
>>> Its callers treat the error as a warning even when null is returned.
>>> They don't create the "smbus-eeprom" device then.  Suspicious.
>>> 
>>> Since the previous commit, only "everything's fine" can actually
>>> happen.  Drop the unused code and simplify the callers.  This gets rid
>>> of the error API violation.
>> 
>> This leaves board code no chance to recover from values given by user that 
>> won't fit without duplicating checks that this function does. Also this 
>> will abort without giving meaningful errors if an invalid value does get 
>> through and result in a crash which is not used friendly. So I don't like 
>> this but if others think this is acceptable maybe at least unit test should 
>> be adjusted to make sure aborts cannot be triggered by user for values that 
>> are not usually tested during development.
>
> Agreed. Do you have an example (or more) to better show Markus this code use? 
> So we can add tests.

After Markus's patches probably nothing uses it any more but this comes 
with the result that previously giving some random value such as -m 100 
did produce a working sam460ex machine after some warnings but now it just 
thows back some errors to the user which may or may not be helpful to 
them.

> Personally I'd use a script to generate a dumb static array of all possible 
> sizes...

Maybe testing with the biggest valid value such as -m 2048 (that's 
commonly used probably) and an invalid value such as -m 100 might be 
enough. Testing all possible values might take too long and would not test 
what happens with invalid values. Ideally those invalud values should also 
work like before a0258e4afa but should at least give a meaningful warning 
so the user can fix the command line without too much head scratching. 
Actually that commit was from Igor not from Marcus so sorry for 
attributing that to Marcus too, I remembered wrong.

By the way you could argue that on real machine you cannot plug certain 
combinations of memory modules so it's enough to model that but I think 
QEMU does not have to be that strict and also support configs that cannot 
happen on real hadware but would work. This might be useful for example if 
you have some ammount of memory to set aside for a VM on a host but that's 
not a size that exists in memory modules on real hardware. This also works 
on pc machine in qemu-system-i386 for example: it accepts -m 100 and does 
its best to create a machine with such unrealistic size. The sam460ex did 
the same (within SoC's limits) and before a0258e4afa -m 100 was fixed up 
to 96 MB which is now not possible due to change in QEMU internal APIs. 
This probably isn't important enough to worth the extra effort to support 
but would have been nice to preserve.

Regards,
BALATON Zoltan

Re: [PATCH v2 0/4] smbus: SPD fixes

[Markus Armbruster]

Queued.

Re: [PATCH v2 2/4] smbus: Fix spd_data_generate() error API violation

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 5426 bytes --]

On Wed, 22 Apr 2020, BALATON Zoltan wrote:
> On Wed, 22 Apr 2020, Philippe Mathieu-Daudé wrote:
>> On 4/22/20 4:27 PM, BALATON Zoltan wrote:
>>> On Wed, 22 Apr 2020, Markus Armbruster wrote:
>>>> The Error ** argument must be NULL, &error_abort, &error_fatal, or a
>>>> pointer to a variable containing NULL.  Passing an argument of the
>>>> latter kind twice without clearing it in between is wrong: if the
>>>> first call sets an error, it no longer points to NULL for the second
>>>> call.
>>>> 
>>>> spd_data_generate() can pass @errp to error_setg() more than once when
>>>> it adjusts both memory size and type.  Harmless, because no caller
>>>> passes anything that needs adjusting.  Until the previous commit,
>>>> sam460ex passed types that needed adjusting, but not sizes.
>>>> 
>>>> spd_data_generate()'s contract is rather awkward:
>>>> 
>>>>    If everything's fine, return non-null and don't set an error.
>>>> 
>>>>    Else, if memory size or type need adjusting, return non-null and
>>>>    set an error describing the adjustment.
>>>> 
>>>>    Else, return null and set an error reporting why no data can be
>>>>    generated.
>>>> 
>>>> Its callers treat the error as a warning even when null is returned.
>>>> They don't create the "smbus-eeprom" device then.  Suspicious.
>>>> 
>>>> Since the previous commit, only "everything's fine" can actually
>>>> happen.  Drop the unused code and simplify the callers.  This gets rid
>>>> of the error API violation.
>>> 
>>> This leaves board code no chance to recover from values given by user that 
>>> won't fit without duplicating checks that this function does. Also this 
>>> will abort without giving meaningful errors if an invalid value does get 
>>> through and result in a crash which is not used friendly. So I don't like 
>>> this but if others think this is acceptable maybe at least unit test 
>>> should be adjusted to make sure aborts cannot be triggered by user for 
>>> values that are not usually tested during development.
>> 
>> Agreed. Do you have an example (or more) to better show Markus this code 
>> use? So we can add tests.
>
> After Markus's patches probably nothing uses it any more but this comes with 
> the result that previously giving some random value such as -m 100 did 
> produce a working sam460ex machine after some warnings but now it just thows 
> back some errors to the user which may or may not be helpful to them.
>
>> Personally I'd use a script to generate a dumb static array of all possible 
>> sizes...
>
> Maybe testing with the biggest valid value such as -m 2048 (that's commonly 
> used probably) and an invalid value such as -m 100 might be enough. Testing 
> all possible values might take too long and would not test what happens with 
> invalid values. Ideally those invalud values should also work like before 
> a0258e4afa but should at least give a meaningful warning so the user can fix 
> the command line without too much head scratching. Actually that commit was 
> from Igor not from Marcus so sorry for attributing that to Marcus too, I 
> remembered wrong.
>
> By the way you could argue that on real machine you cannot plug certain 
> combinations of memory modules so it's enough to model that but I think QEMU 
> does not have to be that strict and also support configs that cannot happen 
> on real hadware but would work. This might be useful for example if you have 
> some ammount of memory to set aside for a VM on a host but that's not a size 
> that exists in memory modules on real hardware. This also works on pc machine 
> in qemu-system-i386 for example: it accepts -m 100 and does its best to 
> create a machine with such unrealistic size. The sam460ex did the same 
> (within SoC's limits) and before a0258e4afa -m 100 was fixed up to 96 MB 
> which is now not possible due to change in QEMU internal APIs. This probably 
> isn't important enough to worth the extra effort to support but would have 
> been nice to preserve.

Besides the above here's another use case of the fix ups that I wanted to 
keep:

https://patchew.org/QEMU/cover.1592315226.git.balaton@eik.bme.hu/b5f4598529a77f15f554c593e9be2d0ff9e5fab3.1592315226.git.balaton@eik.bme.hu/

This board normally uses OpenBIOS which gets RAM size from fw_cfg and so 
works with whatever amount of RAM (also Linux booted with -kernel probably 
does not care) so any -memory value is valid. However some may want to 
also use original firmware ROM for compatibility which detects RAM reading 
SPD eeproms (the i2c emulation needed for that is not working yet but once 
that's fixed this will be the case). I want to add smbus_eeproms for this 
but do not want to just abort for cases where -memory given by user cannot 
be covered with SPD data. Instead a warning and covering as much RAM as 
possible should be enough (the ROM will detect less RAM than given with -m 
but that's OK and better than just bailing out without a message tripping 
an assert). But I don't want to replicate in board code the calculation 
and checks the spd_data_generate() function does anyway (that would just 
puzzle reviewers for every use of this functions).

Previously this was possible with my original spd_data_generate() 
implementation. What's your suggestion to bring that functionality back 
without breaking Error API? Maybe adding new parameters to tell the 
spd_data_generate() which fixups are allowed?

Regards,
BALATON Zoltan

Re: [PATCH v2 2/4] smbus: Fix spd_data_generate() error API violation

[Markus Armbruster]

BALATON Zoltan <balaton@eik.bme.hu> writes:

> On Wed, 22 Apr 2020, BALATON Zoltan wrote:
>> On Wed, 22 Apr 2020, Philippe Mathieu-Daudé wrote:
>>> On 4/22/20 4:27 PM, BALATON Zoltan wrote:
>>>> On Wed, 22 Apr 2020, Markus Armbruster wrote:
>>>>> The Error ** argument must be NULL, &error_abort, &error_fatal, or a
>>>>> pointer to a variable containing NULL.  Passing an argument of the
>>>>> latter kind twice without clearing it in between is wrong: if the
>>>>> first call sets an error, it no longer points to NULL for the second
>>>>> call.
>>>>>
>>>>> spd_data_generate() can pass @errp to error_setg() more than once when
>>>>> it adjusts both memory size and type.  Harmless, because no caller
>>>>> passes anything that needs adjusting.  Until the previous commit,
>>>>> sam460ex passed types that needed adjusting, but not sizes.
>>>>>
>>>>> spd_data_generate()'s contract is rather awkward:
>>>>>
>>>>>    If everything's fine, return non-null and don't set an error.
>>>>>
>>>>>    Else, if memory size or type need adjusting, return non-null and
>>>>>    set an error describing the adjustment.
>>>>>
>>>>>    Else, return null and set an error reporting why no data can be
>>>>>    generated.
>>>>>
>>>>> Its callers treat the error as a warning even when null is returned.
>>>>> They don't create the "smbus-eeprom" device then.  Suspicious.
>>>>>
>>>>> Since the previous commit, only "everything's fine" can actually
>>>>> happen.  Drop the unused code and simplify the callers.  This gets rid
>>>>> of the error API violation.
>>>>
>>>> This leaves board code no chance to recover from values given by
>>>> user that won't fit without duplicating checks that this function
>>>> does. Also this will abort without giving meaningful errors if an
>>>> invalid value does get through and result in a crash which is not
>>>> used friendly. So I don't like this but if others think this is
>>>> acceptable maybe at least unit test should be adjusted to make
>>>> sure aborts cannot be triggered by user for values that are not
>>>> usually tested during development.
>>>
>>> Agreed. Do you have an example (or more) to better show Markus this
>>> code use? So we can add tests.
>>
>> After Markus's patches probably nothing uses it any more but this
>> comes with the result that previously giving some random value such
>> as -m 100 did produce a working sam460ex machine after some warnings
>> but now it just thows back some errors to the user which may or may
>> not be helpful to them.
>>
>>> Personally I'd use a script to generate a dumb static array of all
>>> possible sizes...
>>
>> Maybe testing with the biggest valid value such as -m 2048 (that's
>> commonly used probably) and an invalid value such as -m 100 might be
>> enough. Testing all possible values might take too long and would
>> not test what happens with invalid values. Ideally those invalud
>> values should also work like before a0258e4afa but should at least
>> give a meaningful warning so the user can fix the command line
>> without too much head scratching. Actually that commit was from Igor
>> not from Marcus so sorry for attributing that to Marcus too, I
>> remembered wrong.
>>
>> By the way you could argue that on real machine you cannot plug
>> certain combinations of memory modules so it's enough to model that
>> but I think QEMU does not have to be that strict and also support
>> configs that cannot happen on real hadware but would work. This
>> might be useful for example if you have some ammount of memory to
>> set aside for a VM on a host but that's not a size that exists in
>> memory modules on real hardware. This also works on pc machine in
>> qemu-system-i386 for example: it accepts -m 100 and does its best to
>> create a machine with such unrealistic size. The sam460ex did the
>> same (within SoC's limits) and before a0258e4afa -m 100 was fixed up
>> to 96 MB which is now not possible due to change in QEMU internal
>> APIs. This probably isn't important enough to worth the extra effort
>> to support but would have been nice to preserve.
>
> Besides the above here's another use case of the fix ups that I wanted
> to keep:
>
> https://patchew.org/QEMU/cover.1592315226.git.balaton@eik.bme.hu/b5f4598529a77f15f554c593e9be2d0ff9e5fab3.1592315226.git.balaton@eik.bme.hu/
>
> This board normally uses OpenBIOS which gets RAM size from fw_cfg and
> so works with whatever amount of RAM (also Linux booted with -kernel
> probably does not care) so any -memory value is valid. However some
> may want to also use original firmware ROM for compatibility which
> detects RAM reading SPD eeproms (the i2c emulation needed for that is
> not working yet but once that's fixed this will be the case). I want
> to add smbus_eeproms for this but do not want to just abort for cases
> where -memory given by user cannot be covered with SPD data. Instead a
> warning and covering as much RAM as possible should be enough (the ROM
> will detect less RAM than given with -m 
> but that's OK and better than just bailing out without a message
> tripping an assert). But I don't want to replicate in board code the
> calculation and checks the spd_data_generate() function does anyway
> (that would just puzzle reviewers for every use of this functions).
>
> Previously this was possible with my original spd_data_generate()
> implementation. What's your suggestion to bring that functionality
> back without breaking Error API? Maybe adding new parameters to tell
> the spd_data_generate() which fixups are allowed?

Quick reply without having thought through the issues at all: I'm not
opposed to you doing work to enable additional or even arbitrary memory
sizes where these actually work.  I'm first and foremost opposed to me
wasting time on "improving" code that is not used for anything.  That's
why I dumbed down spd_data_generate().

Secondly, I'm opposed to QEMU "correcting" user configuration.  I want
QEMU do exactly what it's told, and fail with a clear error message when
that is not possible.  The error message may include hints for the user
on how to correct the configuration.

Re: [PATCH v2 2/4] smbus: Fix spd_data_generate() error API violation

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 6991 bytes --]

On Sat, 27 Jun 2020, Markus Armbruster wrote:
> BALATON Zoltan <balaton@eik.bme.hu> writes:
>> On Wed, 22 Apr 2020, BALATON Zoltan wrote:
>>> On Wed, 22 Apr 2020, Philippe Mathieu-Daudé wrote:
>>>> On 4/22/20 4:27 PM, BALATON Zoltan wrote:
>>>>> On Wed, 22 Apr 2020, Markus Armbruster wrote:
>>>>>> The Error ** argument must be NULL, &error_abort, &error_fatal, or a
>>>>>> pointer to a variable containing NULL.  Passing an argument of the
>>>>>> latter kind twice without clearing it in between is wrong: if the
>>>>>> first call sets an error, it no longer points to NULL for the second
>>>>>> call.
>>>>>>
>>>>>> spd_data_generate() can pass @errp to error_setg() more than once when
>>>>>> it adjusts both memory size and type.  Harmless, because no caller
>>>>>> passes anything that needs adjusting.  Until the previous commit,
>>>>>> sam460ex passed types that needed adjusting, but not sizes.
>>>>>>
>>>>>> spd_data_generate()'s contract is rather awkward:
>>>>>>
>>>>>>    If everything's fine, return non-null and don't set an error.
>>>>>>
>>>>>>    Else, if memory size or type need adjusting, return non-null and
>>>>>>    set an error describing the adjustment.
>>>>>>
>>>>>>    Else, return null and set an error reporting why no data can be
>>>>>>    generated.
>>>>>>
>>>>>> Its callers treat the error as a warning even when null is returned.
>>>>>> They don't create the "smbus-eeprom" device then.  Suspicious.
>>>>>>
>>>>>> Since the previous commit, only "everything's fine" can actually
>>>>>> happen.  Drop the unused code and simplify the callers.  This gets rid
>>>>>> of the error API violation.
>>>>>
>>>>> This leaves board code no chance to recover from values given by
>>>>> user that won't fit without duplicating checks that this function
>>>>> does. Also this will abort without giving meaningful errors if an
>>>>> invalid value does get through and result in a crash which is not
>>>>> used friendly. So I don't like this but if others think this is
>>>>> acceptable maybe at least unit test should be adjusted to make
>>>>> sure aborts cannot be triggered by user for values that are not
>>>>> usually tested during development.
>>>>
>>>> Agreed. Do you have an example (or more) to better show Markus this
>>>> code use? So we can add tests.
>>>
>>> After Markus's patches probably nothing uses it any more but this
>>> comes with the result that previously giving some random value such
>>> as -m 100 did produce a working sam460ex machine after some warnings
>>> but now it just thows back some errors to the user which may or may
>>> not be helpful to them.
>>>
>>>> Personally I'd use a script to generate a dumb static array of all
>>>> possible sizes...
>>>
>>> Maybe testing with the biggest valid value such as -m 2048 (that's
>>> commonly used probably) and an invalid value such as -m 100 might be
>>> enough. Testing all possible values might take too long and would
>>> not test what happens with invalid values. Ideally those invalud
>>> values should also work like before a0258e4afa but should at least
>>> give a meaningful warning so the user can fix the command line
>>> without too much head scratching. Actually that commit was from Igor
>>> not from Marcus so sorry for attributing that to Marcus too, I
>>> remembered wrong.
>>>
>>> By the way you could argue that on real machine you cannot plug
>>> certain combinations of memory modules so it's enough to model that
>>> but I think QEMU does not have to be that strict and also support
>>> configs that cannot happen on real hadware but would work. This
>>> might be useful for example if you have some ammount of memory to
>>> set aside for a VM on a host but that's not a size that exists in
>>> memory modules on real hardware. This also works on pc machine in
>>> qemu-system-i386 for example: it accepts -m 100 and does its best to
>>> create a machine with such unrealistic size. The sam460ex did the
>>> same (within SoC's limits) and before a0258e4afa -m 100 was fixed up
>>> to 96 MB which is now not possible due to change in QEMU internal
>>> APIs. This probably isn't important enough to worth the extra effort
>>> to support but would have been nice to preserve.
>>
>> Besides the above here's another use case of the fix ups that I wanted
>> to keep:
>>
>> https://patchew.org/QEMU/cover.1592315226.git.balaton@eik.bme.hu/b5f4598529a77f15f554c593e9be2d0ff9e5fab3.1592315226.git.balaton@eik.bme.hu/
>>
>> This board normally uses OpenBIOS which gets RAM size from fw_cfg and
>> so works with whatever amount of RAM (also Linux booted with -kernel
>> probably does not care) so any -memory value is valid. However some
>> may want to also use original firmware ROM for compatibility which
>> detects RAM reading SPD eeproms (the i2c emulation needed for that is
>> not working yet but once that's fixed this will be the case). I want
>> to add smbus_eeproms for this but do not want to just abort for cases
>> where -memory given by user cannot be covered with SPD data. Instead a
>> warning and covering as much RAM as possible should be enough (the ROM
>> will detect less RAM than given with -m
>> but that's OK and better than just bailing out without a message
>> tripping an assert). But I don't want to replicate in board code the
>> calculation and checks the spd_data_generate() function does anyway
>> (that would just puzzle reviewers for every use of this functions).
>>
>> Previously this was possible with my original spd_data_generate()
>> implementation. What's your suggestion to bring that functionality
>> back without breaking Error API? Maybe adding new parameters to tell
>> the spd_data_generate() which fixups are allowed?
>
> Quick reply without having thought through the issues at all: I'm not

Does that mean you'll reply later with more detail or this is all you had 
to say about this? (Just to know if I should wait for another reply.)

> opposed to you doing work to enable additional or even arbitrary memory
> sizes where these actually work.  I'm first and foremost opposed to me
> wasting time on "improving" code that is not used for anything.  That's
> why I dumbed down spd_data_generate().

It was used by sam460ex until moving ram allocation to memdev broke it.

> Secondly, I'm opposed to QEMU "correcting" user configuration.  I want
> QEMU do exactly what it's told, and fail with a clear error message when
> that is not possible.  The error message may include hints for the user
> on how to correct the configuration.

I don't agree with that. It's already hard enough for non-expert users to 
figure out the needed command line switches, making that even harder by 
throwing back an error for everything that could work just not exactly 
specified is needlessly annoying them further. To the point of chasing 
them away from using QEMU. A lot of people prefer VMWare or VirtualBox for 
this reason and only try QEMU if there's no other way.

Regards,
BALATON Zoltan

Re: [PATCH v2 2/4] smbus: Fix spd_data_generate() error API violation

[Markus Armbruster]

BALATON Zoltan <balaton@eik.bme.hu> writes:

> On Sat, 27 Jun 2020, Markus Armbruster wrote:
>> Quick reply without having thought through the issues at all: I'm not
>
> Does that mean you'll reply later with more detail or this is all you
> had to say about this? (Just to know if I should wait for another
> reply.)

Not sure I can find the time before the soft freeze.  Best not to wait
for me.

>> opposed to you doing work to enable additional or even arbitrary memory
>> sizes where these actually work.  I'm first and foremost opposed to me
>> wasting time on "improving" code that is not used for anything.  That's
>> why I dumbed down spd_data_generate().
>
> It was used by sam460ex until moving ram allocation to memdev broke it.
>
>> Secondly, I'm opposed to QEMU "correcting" user configuration.  I want
>> QEMU do exactly what it's told, and fail with a clear error message when
>> that is not possible.  The error message may include hints for the user
>> on how to correct the configuration.
>
> I don't agree with that. It's already hard enough for non-expert users
> to figure out the needed command line switches, making that even
> harder by throwing back an error for everything that could work just
> not exactly specified is needlessly annoying them further. To the
> point of chasing them away from using QEMU. A lot of people prefer
> VMWare or VirtualBox for this reason and only try QEMU if there's no
> other way.

We don't have to agree on everything.  I'm not the QEMU CLI dictator.
The status quo is pretty clear, though:

    $ qemu-system-ppc64 -help
    [...]
    -m [size=]megs[,slots=n,maxmem=size]
                    configure guest RAM
                    size: initial amount of guest memory
    [...]

It says "Initial amount of guest memory", not "Approximate amount of
guest memory" or something like that.

If we decide we want to change it from "Initial amount of guest memory"
to some "do what I mean" behavior, then that behavior needs to be
documented.

Moreover, if DWIM is appropriate for one machine, it's probably
appropriate for all of them.  The CLI should be as consistent as we can
make it across machines.

Re: [PATCH v2 2/4] smbus: Fix spd_data_generate() error API violation

[Philippe Mathieu-Daudé]

On 6/27/20 9:17 AM, Markus Armbruster wrote:
> BALATON Zoltan <balaton@eik.bme.hu> writes:
> 
>> On Wed, 22 Apr 2020, BALATON Zoltan wrote:
>>> On Wed, 22 Apr 2020, Philippe Mathieu-Daudé wrote:
>>>> On 4/22/20 4:27 PM, BALATON Zoltan wrote:
>>>>> On Wed, 22 Apr 2020, Markus Armbruster wrote:
>>>>>> The Error ** argument must be NULL, &error_abort, &error_fatal, or a
>>>>>> pointer to a variable containing NULL.  Passing an argument of the
>>>>>> latter kind twice without clearing it in between is wrong: if the
>>>>>> first call sets an error, it no longer points to NULL for the second
>>>>>> call.
>>>>>>
>>>>>> spd_data_generate() can pass @errp to error_setg() more than once when
>>>>>> it adjusts both memory size and type.  Harmless, because no caller
>>>>>> passes anything that needs adjusting.  Until the previous commit,
>>>>>> sam460ex passed types that needed adjusting, but not sizes.
>>>>>>
>>>>>> spd_data_generate()'s contract is rather awkward:
>>>>>>
>>>>>>    If everything's fine, return non-null and don't set an error.
>>>>>>
>>>>>>    Else, if memory size or type need adjusting, return non-null and
>>>>>>    set an error describing the adjustment.
>>>>>>
>>>>>>    Else, return null and set an error reporting why no data can be
>>>>>>    generated.
>>>>>>
>>>>>> Its callers treat the error as a warning even when null is returned.
>>>>>> They don't create the "smbus-eeprom" device then.  Suspicious.
>>>>>>
>>>>>> Since the previous commit, only "everything's fine" can actually
>>>>>> happen.  Drop the unused code and simplify the callers.  This gets rid
>>>>>> of the error API violation.
>>>>>
>>>>> This leaves board code no chance to recover from values given by
>>>>> user that won't fit without duplicating checks that this function
>>>>> does. Also this will abort without giving meaningful errors if an
>>>>> invalid value does get through and result in a crash which is not
>>>>> used friendly. So I don't like this but if others think this is
>>>>> acceptable maybe at least unit test should be adjusted to make
>>>>> sure aborts cannot be triggered by user for values that are not
>>>>> usually tested during development.
>>>>
>>>> Agreed. Do you have an example (or more) to better show Markus this
>>>> code use? So we can add tests.
>>>
>>> After Markus's patches probably nothing uses it any more but this
>>> comes with the result that previously giving some random value such
>>> as -m 100 did produce a working sam460ex machine after some warnings
>>> but now it just thows back some errors to the user which may or may
>>> not be helpful to them.
>>>
>>>> Personally I'd use a script to generate a dumb static array of all
>>>> possible sizes...
>>>
>>> Maybe testing with the biggest valid value such as -m 2048 (that's
>>> commonly used probably) and an invalid value such as -m 100 might be
>>> enough. Testing all possible values might take too long and would
>>> not test what happens with invalid values. Ideally those invalud
>>> values should also work like before a0258e4afa but should at least
>>> give a meaningful warning so the user can fix the command line
>>> without too much head scratching. Actually that commit was from Igor
>>> not from Marcus so sorry for attributing that to Marcus too, I
>>> remembered wrong.
>>>
>>> By the way you could argue that on real machine you cannot plug
>>> certain combinations of memory modules so it's enough to model that
>>> but I think QEMU does not have to be that strict and also support
>>> configs that cannot happen on real hadware but would work. This
>>> might be useful for example if you have some ammount of memory to
>>> set aside for a VM on a host but that's not a size that exists in
>>> memory modules on real hardware. This also works on pc machine in
>>> qemu-system-i386 for example: it accepts -m 100 and does its best to
>>> create a machine with such unrealistic size. The sam460ex did the
>>> same (within SoC's limits) and before a0258e4afa -m 100 was fixed up
>>> to 96 MB which is now not possible due to change in QEMU internal
>>> APIs. This probably isn't important enough to worth the extra effort
>>> to support but would have been nice to preserve.
>>
>> Besides the above here's another use case of the fix ups that I wanted
>> to keep:
>>
>> https://patchew.org/QEMU/cover.1592315226.git.balaton@eik.bme.hu/b5f4598529a77f15f554c593e9be2d0ff9e5fab3.1592315226.git.balaton@eik.bme.hu/
>>
>> This board normally uses OpenBIOS which gets RAM size from fw_cfg and
>> so works with whatever amount of RAM (also Linux booted with -kernel
>> probably does not care) so any -memory value is valid. However some
>> may want to also use original firmware ROM for compatibility which
>> detects RAM reading SPD eeproms (the i2c emulation needed for that is
>> not working yet but once that's fixed this will be the case). I want
>> to add smbus_eeproms for this but do not want to just abort for cases
>> where -memory given by user cannot be covered with SPD data. Instead a
>> warning and covering as much RAM as possible should be enough (the ROM
>> will detect less RAM than given with -m 
>> but that's OK and better than just bailing out without a message
>> tripping an assert). But I don't want to replicate in board code the
>> calculation and checks the spd_data_generate() function does anyway
>> (that would just puzzle reviewers for every use of this functions).
>>
>> Previously this was possible with my original spd_data_generate()
>> implementation. What's your suggestion to bring that functionality
>> back without breaking Error API? Maybe adding new parameters to tell
>> the spd_data_generate() which fixups are allowed?
> 
> Quick reply without having thought through the issues at all: I'm not
> opposed to you doing work to enable additional or even arbitrary memory
> sizes where these actually work.  I'm first and foremost opposed to me
> wasting time on "improving" code that is not used for anything.  That's
> why I dumbed down spd_data_generate().

I'm starting to understand Zoltan point. What I'm seeing is Zoltan using
a hobbyist code, that just happens to work for hobbyists, but get in the
way of enterprise quality standards.

Zoltan doesn't have the skills/time/motivation to rework its working
code to meet the enterprise quality level. Enterprise developers tried
to understand twice (first Igor, then Markus) the hobbyist use to get
it done safer, so it can stay maintained.

Zoltan, I guess I understood your use and have an idea to rework it in
a way that everybody is happy, but as Markus said, since the freeze is
next week, I won't have time to get it done in this short amount of
time.

From the PPC460EX-NUB800T-AMCC-datasheet-11553412.pdf datasheet I
understand the 460EX can support "Up to 8 GB in four external banks",
but the SAM 460ex board only wires a single bank (to the SODIMM
connector). You want to use a virtual board with up-to 4 banks in
use, right?

> Secondly, I'm opposed to QEMU "correcting" user configuration.  I want
> QEMU do exactly what it's told, and fail with a clear error message when
> that is not possible.  The error message may include hints for the user
> on how to correct the configuration.
> 

Re: [PATCH v2 2/4] smbus: Fix spd_data_generate() error API violation

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 15590 bytes --]

On Mon, 29 Jun 2020, Philippe Mathieu-Daudé wrote:
> On 6/27/20 9:17 AM, Markus Armbruster wrote:
>> BALATON Zoltan <balaton@eik.bme.hu> writes:
>>> On Wed, 22 Apr 2020, BALATON Zoltan wrote:
>>>> On Wed, 22 Apr 2020, Philippe Mathieu-Daudé wrote:
>>>>> On 4/22/20 4:27 PM, BALATON Zoltan wrote:
>>>>>> On Wed, 22 Apr 2020, Markus Armbruster wrote:
>>>>>>> The Error ** argument must be NULL, &error_abort, &error_fatal, or a
>>>>>>> pointer to a variable containing NULL.  Passing an argument of the
>>>>>>> latter kind twice without clearing it in between is wrong: if the
>>>>>>> first call sets an error, it no longer points to NULL for the second
>>>>>>> call.
>>>>>>>
>>>>>>> spd_data_generate() can pass @errp to error_setg() more than once when
>>>>>>> it adjusts both memory size and type.  Harmless, because no caller
>>>>>>> passes anything that needs adjusting.  Until the previous commit,
>>>>>>> sam460ex passed types that needed adjusting, but not sizes.
>>>>>>>
>>>>>>> spd_data_generate()'s contract is rather awkward:
>>>>>>>
>>>>>>>    If everything's fine, return non-null and don't set an error.
>>>>>>>
>>>>>>>    Else, if memory size or type need adjusting, return non-null and
>>>>>>>    set an error describing the adjustment.
>>>>>>>
>>>>>>>    Else, return null and set an error reporting why no data can be
>>>>>>>    generated.
>>>>>>>
>>>>>>> Its callers treat the error as a warning even when null is returned.
>>>>>>> They don't create the "smbus-eeprom" device then.  Suspicious.
>>>>>>>
>>>>>>> Since the previous commit, only "everything's fine" can actually
>>>>>>> happen.  Drop the unused code and simplify the callers.  This gets rid
>>>>>>> of the error API violation.
>>>>>>
>>>>>> This leaves board code no chance to recover from values given by
>>>>>> user that won't fit without duplicating checks that this function
>>>>>> does. Also this will abort without giving meaningful errors if an
>>>>>> invalid value does get through and result in a crash which is not
>>>>>> used friendly. So I don't like this but if others think this is
>>>>>> acceptable maybe at least unit test should be adjusted to make
>>>>>> sure aborts cannot be triggered by user for values that are not
>>>>>> usually tested during development.
>>>>>
>>>>> Agreed. Do you have an example (or more) to better show Markus this
>>>>> code use? So we can add tests.
>>>>
>>>> After Markus's patches probably nothing uses it any more but this
>>>> comes with the result that previously giving some random value such
>>>> as -m 100 did produce a working sam460ex machine after some warnings
>>>> but now it just thows back some errors to the user which may or may
>>>> not be helpful to them.
>>>>
>>>>> Personally I'd use a script to generate a dumb static array of all
>>>>> possible sizes...
>>>>
>>>> Maybe testing with the biggest valid value such as -m 2048 (that's
>>>> commonly used probably) and an invalid value such as -m 100 might be
>>>> enough. Testing all possible values might take too long and would
>>>> not test what happens with invalid values. Ideally those invalud
>>>> values should also work like before a0258e4afa but should at least
>>>> give a meaningful warning so the user can fix the command line
>>>> without too much head scratching. Actually that commit was from Igor
>>>> not from Marcus so sorry for attributing that to Marcus too, I
>>>> remembered wrong.
>>>>
>>>> By the way you could argue that on real machine you cannot plug
>>>> certain combinations of memory modules so it's enough to model that
>>>> but I think QEMU does not have to be that strict and also support
>>>> configs that cannot happen on real hadware but would work. This
>>>> might be useful for example if you have some ammount of memory to
>>>> set aside for a VM on a host but that's not a size that exists in
>>>> memory modules on real hardware. This also works on pc machine in
>>>> qemu-system-i386 for example: it accepts -m 100 and does its best to
>>>> create a machine with such unrealistic size. The sam460ex did the
>>>> same (within SoC's limits) and before a0258e4afa -m 100 was fixed up
>>>> to 96 MB which is now not possible due to change in QEMU internal
>>>> APIs. This probably isn't important enough to worth the extra effort
>>>> to support but would have been nice to preserve.
>>>
>>> Besides the above here's another use case of the fix ups that I wanted
>>> to keep:
>>>
>>> https://patchew.org/QEMU/cover.1592315226.git.balaton@eik.bme.hu/b5f4598529a77f15f554c593e9be2d0ff9e5fab3.1592315226.git.balaton@eik.bme.hu/
>>>
>>> This board normally uses OpenBIOS which gets RAM size from fw_cfg and
>>> so works with whatever amount of RAM (also Linux booted with -kernel
>>> probably does not care) so any -memory value is valid. However some
>>> may want to also use original firmware ROM for compatibility which
>>> detects RAM reading SPD eeproms (the i2c emulation needed for that is
>>> not working yet but once that's fixed this will be the case). I want
>>> to add smbus_eeproms for this but do not want to just abort for cases
>>> where -memory given by user cannot be covered with SPD data. Instead a
>>> warning and covering as much RAM as possible should be enough (the ROM
>>> will detect less RAM than given with -m
>>> but that's OK and better than just bailing out without a message
>>> tripping an assert). But I don't want to replicate in board code the
>>> calculation and checks the spd_data_generate() function does anyway
>>> (that would just puzzle reviewers for every use of this functions).
>>>
>>> Previously this was possible with my original spd_data_generate()
>>> implementation. What's your suggestion to bring that functionality
>>> back without breaking Error API? Maybe adding new parameters to tell
>>> the spd_data_generate() which fixups are allowed?
>>
>> Quick reply without having thought through the issues at all: I'm not
>> opposed to you doing work to enable additional or even arbitrary memory
>> sizes where these actually work.  I'm first and foremost opposed to me
>> wasting time on "improving" code that is not used for anything.  That's
>> why I dumbed down spd_data_generate().
>
> I'm starting to understand Zoltan point. What I'm seeing is Zoltan using
> a hobbyist code, that just happens to work for hobbyists, but get in the
> way of enterprise quality standards.

This is not necessarily a conflict between hobbyist vs enterprise but more 
like different view on what the qemu-system-* CLI should be. I think the 
CLI is the main human interface of QEMU as it does not really provide a 
GUI for configuring or running VMs (as for example VirtualBox does, QEMU 
only has minimal GUI to view and control running VMs) so users are forced 
to use either the command line or maybe an external management frontend, 
but for simple things (like hobbyist use) that's an overkill and also not 
a good match as those are designed for enterprise use. (Also these 
hobbyist are on Windows or macOS where these management apps are not 
available and getting a working QEMU binary is already a challenge.)

The problem is that these management frontends don't have a proper API to 
control QEMU but abuse the CLI and QEMU monitor for this which are 
supposed to be human interfaces at the first place but changing the 
commands for the needs of management apps result in arcane command lines. 
Note that humans and management apps likely have different requirements so 
if you mean hobbyist = human and enterprise = management frontend then 
that's about what my problem is. I think humans and management apps could 
coexist using the same interfaces if these cannot be cleanly separated (as 
that would need either changing management apps to use something else than 
the main human interface or providing proper GUI or CLI frontend for 
humans) but if they use the same CLI then allowing some convenience 
commands to make the life of humans easier should not be forbidden. 
Running a VM should be simple and not require typing multiple lines of 
options just to result in an error that something is not what QEMU thinks 
is acceptable even though it could work and could be fixed. That's really 
annoying for a human but may be desirable for a management app so it does 
not need to check it got what it think it specified.

> Zoltan doesn't have the skills/time/motivation to rework its working
> code to meet the enterprise quality level. Enterprise developers tried
> to understand twice (first Igor, then Markus) the hobbyist use to get
> it done safer, so it can stay maintained.

Of course I don't have time or motivation to make it enterprise quality 
when I work unpayed on this in my free time and for fun. I already spend 
too much time with this so while I try to make it good enough to be 
included upstream the direction is clearly different than what enterprise 
users need. But that's OK as the machines I work with are not really used 
in an enterprise setting and mostly used by hobbyists, but if some of the 
components or machines could be useful to enterprise people I expect them 
to put in the effort to get them to enterprise level.

But this probably does not apply to the very problem discussed here. When 
I've added new machines (apart from sam460ex also pegasos2 which is not 
upstream yet and now hopefully Mac machines soon too) these needed SPD 
eeproms because their firmwares detected RAM based on it. There were some 
already existing boards which emulated SPD but these were ad-hoc 
implementations without any commonality. To avoid increasing the mess by 
adding a few more independent SPD emulations that would get out of sync 
I've spent some time to come up with a common function that could be used 
by all these boards and the new ones I wanted to add. The goal of this 
function was to put SPD emulation in a single place and make it easy for 
board code to use it without needing to duplicate code.

Also Marcus mentioned uniformity between machines: Most machines, like pc 
ones accept any memory size such as -m 100 even though on real hardware 
it's not possible but can work with the firmware in QEMU that usually take 
this info from FW_CFG or something else and not resticted by SPD data. I 
wanted to do the same in sam460ex and allow it to use any memory size 
exactly for uniformity besides used convenience, even though that machine 
has some constraints so it required to fix up RAM size to meet those 
constraints. So -m 100 would result in 96 MB of RAM that the SoC and 
firmware can handle and is closest to what the user intended. This worked 
well until Igor changed memory allocation to memdev (which I don't even 
know what it is: some enterprise stuff not really needed for hobbyists but 
maybe could be useful e.g. to save guest memory image so why not) but this 
required getting rid of fix ups of memory size in boards (sam460ex wasn't 
the only one) beacuse memdev could not support this for some reason and 
Igor did not want to add that (even though I've proposed some designs, you 
can look up in patch review). So this broke fix ups, then Marcus noticed 
that errors reporting via err object cannot be used for warnings as I've 
tried to use so to fix it he just removed all the reamaining traces of it 
thereby making it more difficult to add SPD eeproms to mac_oldworld 
without duplicating the removed checks in board code which I wanted to 
avoid because:

1. This is knowledge about SPD eeproms that should be in that func
2. Would duplicate non-trivial code in boards that would puzzle reviewers 
and is error prone too.

> Zoltan, I guess I understood your use and have an idea to rework it in
> a way that everybody is happy, but as Markus said, since the freeze is
> next week, I won't have time to get it done in this short amount of
> time.

It's not urgent but if we can agree on something that's acceptable for 
everyone I may be able to submit a patch but don't want to put in effort 
if it will be turned down anyway due to nothing else than the current 
solution being acceptable based on principles over convenience. Arguing 
with Markus about it before got me that impression so I'd rather ask 
before wasting time with it.

> From the PPC460EX-NUB800T-AMCC-datasheet-11553412.pdf datasheet I
> understand the 460EX can support "Up to 8 GB in four external banks",
> but the SAM 460ex board only wires a single bank (to the SODIMM
> connector). You want to use a virtual board with up-to 4 banks in
> use, right?

No, the firmware won't check additional banks because it only checks the 
one wired. So what we need is to put as much RAM as possible on that 
SODIMM (and we can use that SoC can handle both DDR and DDR2) but since 
it's already broken and limited to valid SODIMM sizes due to memdev not 
supporting memory size fix ups fixing this again is not high priority.

What I'd like is reverting f26740c61a57f and fix that some other way so I 
don't have to duplicate size check in board code as can be seen in the 
patchew link above but could just call spd_data_generate() to do its job. 
This was discussed at the time that patch was in review you can read it 
here:

http://patchwork.ozlabs.org/project/qemu-devel/patch/20200420132826.8879-3-armbru@redhat.com/

My points were not really considered then, now that I have another use 
case maybe it could be revisited and fixed. What I want is to be able to 
call spd_data_generate() from board code with whatever sizé (the board 
does not need to know about SPD limits and so cannot pre-check the size) 
and the function should return the largest possible size SPD and some 
indication if the size was not used completely. If Error cannot be used 
for this, return the message or error some other way but let the board 
code decide if it wants to abort or it can use the smaller SPD. Do not 
assert in the helper function. Maybe the DIMM type fix up can be dropped 
and only keep the size fix up so then we don't need to use error twice, 
the board could call the function again if a different type is also 
acceptable, since only sam460ex would need this I can do that there for 
type fixup and call spd_data_generate() again with DDR2 if first one with 
DDR could not fit all ram. But at least the asserts should be dropped for 
this and the size check brought back. Then adding SPD to mac_oldworld 
could also be done by calling spd_data_generate() instead of duplicating 
the checks this function does anyway. This board has three slots so if 
user says -m 1400 it would call spd_data_generate() with 1400 first, get 
back 512 SPD that it adds to first slot then calls spd_data_generate() 
again with 888, gets 512 again that it adds to 2nd slot and calls 
spd_data_generate() for last slot with 376 which would give 256 and 120 
remaining that it may warn the user about but still continue because the 
SPD data is only used by a ROM from real hardware (that may be used for 
compatibility with some software) but the default OpenBIOS disregards SPD 
data and would still use 1400 so it's not an error to abort on. Simply if 
using a firmare ROM then only 1280 MB of the 1400 will be available due to 
its limitations but that's not a reason to force users to change their 
command line. Printing a warning is enough to hint they may use different 
value but aborting without an error message on an assert which is the 
current situation is not really a user friendly way.

Hopefully at least somebody will read it up to this point, sorry for 
writing that much but hopefully this explains my point of view.

Regards,
BALATON Zoltan

Re: [PATCH v2 2/4] smbus: Fix spd_data_generate() error API violation

[BALATON Zoltan]

On Mon, 29 Jun 2020, Markus Armbruster wrote:
> BALATON Zoltan <balaton@eik.bme.hu> writes:
>> On Sat, 27 Jun 2020, Markus Armbruster wrote:
>>> Quick reply without having thought through the issues at all: I'm not
>>
>> Does that mean you'll reply later with more detail or this is all you
>> had to say about this? (Just to know if I should wait for another
>> reply.)
>
> Not sure I can find the time before the soft freeze.  Best not to wait
> for me.

OK I'm not sure my mac_oldworld patches can be finished or would be merged 
before the freeze anyway and this was already broken in 5.0 so it's not 
that urgent now but I'll need this in the future so eventually should 
find some way to come to an agreement.

>>> opposed to you doing work to enable additional or even arbitrary memory
>>> sizes where these actually work.  I'm first and foremost opposed to me
>>> wasting time on "improving" code that is not used for anything.  That's
>>> why I dumbed down spd_data_generate().
>>
>> It was used by sam460ex until moving ram allocation to memdev broke it.
>>
>>> Secondly, I'm opposed to QEMU "correcting" user configuration.  I want
>>> QEMU do exactly what it's told, and fail with a clear error message when
>>> that is not possible.  The error message may include hints for the user
>>> on how to correct the configuration.
>>
>> I don't agree with that. It's already hard enough for non-expert users
>> to figure out the needed command line switches, making that even
>> harder by throwing back an error for everything that could work just
>> not exactly specified is needlessly annoying them further. To the
>> point of chasing them away from using QEMU. A lot of people prefer
>> VMWare or VirtualBox for this reason and only try QEMU if there's no
>> other way.
>
> We don't have to agree on everything.  I'm not the QEMU CLI dictator.
> The status quo is pretty clear, though:
>
>    $ qemu-system-ppc64 -help
>    [...]
>    -m [size=]megs[,slots=n,maxmem=size]
>                    configure guest RAM
>                    size: initial amount of guest memory
>    [...]
>
> It says "Initial amount of guest memory", not "Approximate amount of
> guest memory" or something like that.
>
> If we decide we want to change it from "Initial amount of guest memory"
> to some "do what I mean" behavior, then that behavior needs to be
> documented.

This is sufficiently vague that it says "initial" which to me means it's 
not absolute and can change while the VM is running so a change due to fix 
up fits in that in my opinion :-)

> Moreover, if DWIM is appropriate for one machine, it's probably
> appropriate for all of them.  The CLI should be as consistent as we can
> make it across machines.

That's the point. Rummimg e.g. qemu-system-x86_64 -m 1000 does not abort 
but runs the VM with an odd RAM size even though that's not possible on 
real hardware. Other machines should behave the same, within their limits: 
for sam460ex that means we need to truncate memory size to largest valid 
value becuause of SoC limits, for mac_oldworld that means with OpenBIOS it 
will see all RAM and with firmware ROM somewhat less. I've implemented 
that originally both for consistency and user convenience but this was 
"cleaned up" afterwrds and also made impossible to implement again without 
duplicating code in boards or reverting to some previous state and fixing 
the problems in a way that allows my use case as well.

Regards,
BALATON Zoltan

Re: [PATCH v2 2/4] smbus: Fix spd_data_generate() error API violation

[Philippe Mathieu-Daudé]

On 6/29/20 11:31 PM, BALATON Zoltan wrote:
> On Mon, 29 Jun 2020, Philippe Mathieu-Daudé wrote:
>> On 6/27/20 9:17 AM, Markus Armbruster wrote:
>>> BALATON Zoltan <balaton@eik.bme.hu> writes:
>>>> On Wed, 22 Apr 2020, BALATON Zoltan wrote:
>>>>> On Wed, 22 Apr 2020, Philippe Mathieu-Daudé wrote:
>>>>>> On 4/22/20 4:27 PM, BALATON Zoltan wrote:
>>>>>>> On Wed, 22 Apr 2020, Markus Armbruster wrote:
>>>>>>>> The Error ** argument must be NULL, &error_abort, &error_fatal,
>>>>>>>> or a
>>>>>>>> pointer to a variable containing NULL.  Passing an
>>>>>>>> argument of the
>>>>>>>> latter kind twice without clearing it in between is wrong: if the
>>>>>>>> first call sets an error, it no longer points to NULL for the
>>>>>>>> second
>>>>>>>> call.
>>>>>>>>
>>>>>>>> spd_data_generate() can pass @errp to error_setg() more than
>>>>>>>> once when
>>>>>>>> it adjusts both memory size and type.  Harmless, because
>>>>>>>> no caller
>>>>>>>> passes anything that needs adjusting.  Until the previous
>>>>>>>> commit,
>>>>>>>> sam460ex passed types that needed adjusting, but not sizes.
>>>>>>>>
>>>>>>>> spd_data_generate()'s contract is rather awkward:
>>>>>>>>
>>>>>>>>    If everything's fine, return non-null and don't
>>>>>>>> set an error.
>>>>>>>>
>>>>>>>>    Else, if memory size or type need adjusting,
>>>>>>>> return non-null and
>>>>>>>>    set an error describing the adjustment.
>>>>>>>>
>>>>>>>>    Else, return null and set an error reporting
>>>>>>>> why no data can be
>>>>>>>>    generated.
>>>>>>>>
>>>>>>>> Its callers treat the error as a warning even when null is
>>>>>>>> returned.
>>>>>>>> They don't create the "smbus-eeprom" device then. 
>>>>>>>> Suspicious.
>>>>>>>>
>>>>>>>> Since the previous commit, only "everything's fine" can actually
>>>>>>>> happen.  Drop the unused code and simplify the
>>>>>>>> callers.  This gets rid
>>>>>>>> of the error API violation.
>>>>>>>
>>>>>>> This leaves board code no chance to recover from values given by
>>>>>>> user that won't fit without duplicating checks that this function
>>>>>>> does. Also this will abort without giving meaningful errors if an
>>>>>>> invalid value does get through and result in a crash which is not
>>>>>>> used friendly. So I don't like this but if others think this is
>>>>>>> acceptable maybe at least unit test should be adjusted to make
>>>>>>> sure aborts cannot be triggered by user for values that are not
>>>>>>> usually tested during development.
>>>>>>
>>>>>> Agreed. Do you have an example (or more) to better show Markus this
>>>>>> code use? So we can add tests.
>>>>>
>>>>> After Markus's patches probably nothing uses it any more but this
>>>>> comes with the result that previously giving some random value such
>>>>> as -m 100 did produce a working sam460ex machine after some warnings
>>>>> but now it just thows back some errors to the user which may or may
>>>>> not be helpful to them.
>>>>>
>>>>>> Personally I'd use a script to generate a dumb static array of all
>>>>>> possible sizes...
>>>>>
>>>>> Maybe testing with the biggest valid value such as -m 2048 (that's
>>>>> commonly used probably) and an invalid value such as -m 100 might be
>>>>> enough. Testing all possible values might take too long and would
>>>>> not test what happens with invalid values. Ideally those invalud
>>>>> values should also work like before a0258e4afa but should at least
>>>>> give a meaningful warning so the user can fix the command line
>>>>> without too much head scratching. Actually that commit was from Igor
>>>>> not from Marcus so sorry for attributing that to Marcus too, I
>>>>> remembered wrong.
>>>>>
>>>>> By the way you could argue that on real machine you cannot plug
>>>>> certain combinations of memory modules so it's enough to model that
>>>>> but I think QEMU does not have to be that strict and also support
>>>>> configs that cannot happen on real hadware but would work. This
>>>>> might be useful for example if you have some ammount of memory to
>>>>> set aside for a VM on a host but that's not a size that exists in
>>>>> memory modules on real hardware. This also works on pc machine in
>>>>> qemu-system-i386 for example: it accepts -m 100 and does its best to
>>>>> create a machine with such unrealistic size. The sam460ex did the
>>>>> same (within SoC's limits) and before a0258e4afa -m 100 was fixed up
>>>>> to 96 MB which is now not possible due to change in QEMU internal
>>>>> APIs. This probably isn't important enough to worth the extra effort
>>>>> to support but would have been nice to preserve.
>>>>
>>>> Besides the above here's another use case of the fix ups that I wanted
>>>> to keep:
>>>>
>>>> https://patchew.org/QEMU/cover.1592315226.git.balaton@eik.bme.hu/b5f4598529a77f15f554c593e9be2d0ff9e5fab3.1592315226.git.balaton@eik.bme.hu/
>>>>
>>>>
>>>> This board normally uses OpenBIOS which gets RAM size from fw_cfg and
>>>> so works with whatever amount of RAM (also Linux booted with -kernel
>>>> probably does not care) so any -memory value is valid. However some
>>>> may want to also use original firmware ROM for compatibility which
>>>> detects RAM reading SPD eeproms (the i2c emulation needed for that is
>>>> not working yet but once that's fixed this will be the case). I want
>>>> to add smbus_eeproms for this but do not want to just abort for cases
>>>> where -memory given by user cannot be covered with SPD data. Instead a
>>>> warning and covering as much RAM as possible should be enough (the ROM
>>>> will detect less RAM than given with -m
>>>> but that's OK and better than just bailing out without a message
>>>> tripping an assert). But I don't want to replicate in board code the
>>>> calculation and checks the spd_data_generate() function does anyway
>>>> (that would just puzzle reviewers for every use of this functions).
>>>>
>>>> Previously this was possible with my original spd_data_generate()
>>>> implementation. What's your suggestion to bring that functionality
>>>> back without breaking Error API? Maybe adding new parameters to tell
>>>> the spd_data_generate() which fixups are allowed?
>>>
>>> Quick reply without having thought through the issues at all: I'm not
>>> opposed to you doing work to enable additional or even arbitrary memory
>>> sizes where these actually work.  I'm first and foremost opposed to me
>>> wasting time on "improving" code that is not used for anything.  That's
>>> why I dumbed down spd_data_generate().
>>
>> I'm starting to understand Zoltan point. What I'm seeing is Zoltan using
>> a hobbyist code, that just happens to work for hobbyists, but get in the
>> way of enterprise quality standards.
> 
> This is not necessarily a conflict between hobbyist vs enterprise but
> more like different view on what the qemu-system-* CLI should be. I
> think the CLI is the main human interface of QEMU as it does not really
> provide a GUI for configuring or running VMs (as for example VirtualBox
> does, QEMU only has minimal GUI to view and control running VMs) so
> users are forced to use either the command line or maybe an external
> management frontend, but for simple things (like hobbyist use) that's an
> overkill and also not a good match as those are designed for enterprise
> use. (Also these hobbyist are on Windows or macOS where these management
> apps are not available and getting a working QEMU binary is already a
> challenge.)
> 
> The problem is that these management frontends don't have a proper API
> to control QEMU but abuse the CLI and QEMU monitor for this which are
> supposed to be human interfaces at the first place but changing the
> commands for the needs of management apps result in arcane command
> lines. Note that humans and management apps likely have different
> requirements so if you mean hobbyist = human and enterprise = management
> frontend then that's about what my problem is. I think humans and
> management apps could coexist using the same interfaces if these cannot
> be cleanly separated (as that would need either changing management apps
> to use something else than the main human interface or providing proper
> GUI or CLI frontend for humans) but if they use the same CLI then
> allowing some convenience commands to make the life of humans easier
> should not be forbidden. Running a VM should be simple and not require
> typing multiple lines of options just to result in an error that
> something is not what QEMU thinks is acceptable even though it could
> work and could be fixed. That's really annoying for a human but may be
> desirable for a management app so it does not need to check it got what
> it think it specified.
> 
>> Zoltan doesn't have the skills/time/motivation to rework its working
>> code to meet the enterprise quality level. Enterprise developers tried
>> to understand twice (first Igor, then Markus) the hobbyist use to get
>> it done safer, so it can stay maintained.
> 
> Of course I don't have time or motivation to make it enterprise quality
> when I work unpayed on this in my free time and for fun. I already spend
> too much time with this so while I try to make it good enough to be
> included upstream the direction is clearly different than what
> enterprise users need. But that's OK as the machines I work with are not
> really used in an enterprise setting and mostly used by hobbyists, but
> if some of the components or machines could be useful to enterprise
> people I expect them to put in the effort to get them to enterprise level.
> 
> But this probably does not apply to the very problem discussed here.
> When I've added new machines (apart from sam460ex also pegasos2 which is
> not upstream yet and now hopefully Mac machines soon too) these needed
> SPD eeproms because their firmwares detected RAM based on it. There were
> some already existing boards which emulated SPD but these were ad-hoc
> implementations without any commonality. To avoid increasing the mess by
> adding a few more independent SPD emulations that would get out of sync
> I've spent some time to come up with a common function that could be
> used by all these boards and the new ones I wanted to add. The goal of
> this function was to put SPD emulation in a single place and make it
> easy for board code to use it without needing to duplicate code.
> 
> Also Marcus mentioned uniformity between machines: Most machines, like
> pc ones accept any memory size such as -m 100 even though on real
> hardware it's not possible but can work with the firmware in QEMU that
> usually take this info from FW_CFG or something else and not resticted
> by SPD data. I wanted to do the same in sam460ex and allow it to use any
> memory size exactly for uniformity besides used convenience, even though
> that machine has some constraints so it required to fix up RAM size to
> meet those constraints. So -m 100 would result in 96 MB of RAM that the
> SoC and firmware can handle and is closest to what the user intended.
> This worked well until Igor changed memory allocation to memdev (which I
> don't even know what it is: some enterprise stuff not really needed for
> hobbyists but maybe could be useful e.g. to save guest memory image so
> why not) but this required getting rid of fix ups of memory size in
> boards (sam460ex wasn't the only one) beacuse memdev could not support
> this for some reason and Igor did not want to add that (even though I've
> proposed some designs, you can look up in patch review). So this broke
> fix ups, then Marcus noticed that errors reporting via err object cannot
> be used for warnings as I've tried to use so to fix it he just removed
> all the reamaining traces of it thereby making it more difficult to add
> SPD eeproms to mac_oldworld without duplicating the removed checks in
> board code which I wanted to avoid because:
> 
> 1. This is knowledge about SPD eeproms that should be in that func
> 2. Would duplicate non-trivial code in boards that would puzzle
> reviewers and is error prone too.
> 
>> Zoltan, I guess I understood your use and have an idea to rework it in
>> a way that everybody is happy, but as Markus said, since the freeze is
>> next week, I won't have time to get it done in this short amount of
>> time.
> 
> It's not urgent but if we can agree on something that's acceptable for
> everyone I may be able to submit a patch but don't want to put in effort
> if it will be turned down anyway due to nothing else than the current
> solution being acceptable based on principles over convenience. Arguing
> with Markus about it before got me that impression so I'd rather ask
> before wasting time with it.
> 
>> From the PPC460EX-NUB800T-AMCC-datasheet-11553412.pdf datasheet I
>> understand the 460EX can support "Up to 8 GB in four external banks",
>> but the SAM 460ex board only wires a single bank (to the SODIMM
>> connector). You want to use a virtual board with up-to 4 banks in
>> use, right?
> 
> No, the firmware won't check additional banks because it only checks the
> one wired. So what we need is to put as much RAM as possible on that
> SODIMM (and we can use that SoC can handle both DDR and DDR2) but since
> it's already broken and limited to valid SODIMM sizes due to memdev not
> supporting memory size fix ups fixing this again is not high priority.
> 
> What I'd like is reverting f26740c61a57f and fix that some other way so
> I don't have to duplicate size check in board code as can be seen in the
> patchew link above but could just call spd_data_generate() to do its
> job. This was discussed at the time that patch was in review you can
> read it here:
> 
> http://patchwork.ozlabs.org/project/qemu-devel/patch/20200420132826.8879-3-armbru@redhat.com/
> 
> 
> My points were not really considered then, now that I have another use
> case maybe it could be revisited and fixed. What I want is to be able to
> call spd_data_generate() from board code with whatever sizé (the
> board does not need to know about SPD limits and so cannot pre-check the
> size) and the function should return the largest possible size SPD and
> some indication if the size was not used completely. If Error cannot be
> used for this, return the message or error some other way but let the
> board code decide if it wants to abort or it can use the smaller SPD. Do
> not assert in the helper function. Maybe the DIMM type fix up can be
> dropped and only keep the size fix up so then we don't need to use error
> twice, the board could call the function again if a different type is
> also acceptable, since only sam460ex would need this I can do that there
> for type fixup and call spd_data_generate() again with DDR2 if first one
> with DDR could not fit all ram. But at least the asserts should be
> dropped for this and the size check brought back. Then adding SPD to
> mac_oldworld could also be done by calling spd_data_generate() instead
> of duplicating the checks this function does anyway. This board has
> three slots so if user says -m 1400 it would call spd_data_generate()
> with 1400 first, get back 512 SPD that it adds to first slot then calls
> spd_data_generate() again with 888, gets 512 again that it adds to 2nd
> slot and calls spd_data_generate() for last slot with 376 which would
> give 256 and 120 remaining that it may warn the user about but still
> continue because the SPD data is only used by a ROM from real hardware
> (that may be used for compatibility with some software) but the default
> OpenBIOS disregards SPD data and would still use 1400 so it's not an
> error to abort on. Simply if using a firmare ROM then only 1280 MB of
> the 1400 will be available due to its limitations but that's not a
> reason to force users to change their command line. Printing a warning
> is enough to hint they may use different value but aborting without an
> error message on an assert which is the current situation is not really
> a user friendly way.

I just noticed we have a MachineClass::fixup_ram_size() handler. There
is only one implementation (on s390x) which does a bit the opposite:
If the user asks for -m 1400, it will pad to a valid physical size,
so in your example to 1472 MiB. Then the guest can use only 1400 if it
is happy with it. 72 MiB are wasted, but this is still better than the
576 MiB wasted if we were using 2 GiB instead ;)

See commit 5c30ef937f5:

static ram_addr_t s390_fixup_ram_size(ram_addr_t sz)
{
    /* same logic as in sclp.c */
    int increment_size = 20;
    ram_addr_t newsz;

    while ((sz >> increment_size) > MAX_STORAGE_INCREMENTS) {
        increment_size++;
    }
    newsz = sz >> increment_size << increment_size;

    if (sz != newsz) {
        qemu_printf("Ram size %" PRIu64 "MB was fixed up to %" PRIu64
                    "MB to match machine restrictions. "
                    "Consider updating "
                    "the guest definition.\n", (uint64_t) (sz / MiB),
                    (uint64_t) (newsz / MiB));
    }
    return newsz;
}

> 
> Hopefully at least somebody will read it up to this point, sorry for
> writing that much but hopefully this explains my point of view.

Someone did ;) I know it is not always easy, English is not the default
language for various developers.

> 
> Regards,
> BALATON Zoltan

Re: [PATCH v2 2/4] smbus: Fix spd_data_generate() error API violation

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 6284 bytes --]

On Tue, 30 Jun 2020, Philippe Mathieu-Daudé wrote:
> On 6/29/20 11:31 PM, BALATON Zoltan wrote:
>>>>> Besides the above here's another use case of the fix ups that I wanted
>>>>> to keep:
>>>>>
>>>>> https://patchew.org/QEMU/cover.1592315226.git.balaton@eik.bme.hu/b5f4598529a77f15f554c593e9be2d0ff9e5fab3.1592315226.git.balaton@eik.bme.hu/
>>>>>
>>>>>
>>>>> This board normally uses OpenBIOS which gets RAM size from fw_cfg and
>>>>> so works with whatever amount of RAM (also Linux booted with -kernel
>>>>> probably does not care) so any -memory value is valid. However some
>>>>> may want to also use original firmware ROM for compatibility which
>>>>> detects RAM reading SPD eeproms (the i2c emulation needed for that is
>>>>> not working yet but once that's fixed this will be the case). I want
>>>>> to add smbus_eeproms for this but do not want to just abort for cases
>>>>> where -memory given by user cannot be covered with SPD data. Instead a
>>>>> warning and covering as much RAM as possible should be enough (the ROM
>>>>> will detect less RAM than given with -m
>>>>> but that's OK and better than just bailing out without a message
>>>>> tripping an assert). But I don't want to replicate in board code the
>>>>> calculation and checks the spd_data_generate() function does anyway
>>>>> (that would just puzzle reviewers for every use of this functions).
>>>>>
>>>>> Previously this was possible with my original spd_data_generate()
>>>>> implementation. What's your suggestion to bring that functionality
>>>>> back without breaking Error API? Maybe adding new parameters to tell
>>>>> the spd_data_generate() which fixups are allowed?
[...]
>> What I'd like is reverting f26740c61a57f and fix that some other way so
>> I don't have to duplicate size check in board code as can be seen in the
>> patchew link above but could just call spd_data_generate() to do its
>> job. This was discussed at the time that patch was in review you can
>> read it here:
>>
>> http://patchwork.ozlabs.org/project/qemu-devel/patch/20200420132826.8879-3-armbru@redhat.com/
>>
>>
>> My points were not really considered then, now that I have another use
>> case maybe it could be revisited and fixed. What I want is to be able to
>> call spd_data_generate() from board code with whatever siz�© (the
>> board does not need to know about SPD limits and so cannot pre-check the
>> size) and the function should return the largest possible size SPD and
>> some indication if the size was not used completely. If Error cannot be
>> used for this, return the message or error some other way but let the
>> board code decide if it wants to abort or it can use the smaller SPD. Do
>> not assert in the helper function. Maybe the DIMM type fix up can be
>> dropped and only keep the size fix up so then we don't need to use error
>> twice, the board could call the function again if a different type is
>> also acceptable, since only sam460ex would need this I can do that there
>> for type fixup and call spd_data_generate() again with DDR2 if first one
>> with DDR could not fit all ram. But at least the asserts should be
>> dropped for this and the size check brought back. Then adding SPD to
>> mac_oldworld could also be done by calling spd_data_generate() instead
>> of duplicating the checks this function does anyway. This board has
>> three slots so if user says -m 1400 it would call spd_data_generate()
>> with 1400 first, get back 512 SPD that it adds to first slot then calls
>> spd_data_generate() again with 888, gets 512 again that it adds to 2nd
>> slot and calls spd_data_generate() for last slot with 376 which would
>> give 256 and 120 remaining that it may warn the user about but still
>> continue because the SPD data is only used by a ROM from real hardware
>> (that may be used for compatibility with some software) but the default
>> OpenBIOS disregards SPD data and would still use 1400 so it's not an
>> error to abort on. Simply if using a firmare ROM then only 1280 MB of
>> the 1400 will be available due to its limitations but that's not a
>> reason to force users to change their command line. Printing a warning
>> is enough to hint they may use different value but aborting without an
>> error message on an assert which is the current situation is not really
>> a user friendly way.
>
> I just noticed we have a MachineClass::fixup_ram_size() handler. There
> is only one implementation (on s390x) which does a bit the opposite:
> If the user asks for -m 1400, it will pad to a valid physical size,
> so in your example to 1472 MiB. Then the guest can use only 1400 if it
> is happy with it. 72 MiB are wasted, but this is still better than the
> 576 MiB wasted if we were using 2 GiB instead ;)
>
> See commit 5c30ef937f5:
>
> static ram_addr_t s390_fixup_ram_size(ram_addr_t sz)
> {
>    /* same logic as in sclp.c */
>    int increment_size = 20;
>    ram_addr_t newsz;
>
>    while ((sz >> increment_size) > MAX_STORAGE_INCREMENTS) {
>        increment_size++;
>    }
>    newsz = sz >> increment_size << increment_size;
>
>    if (sz != newsz) {
>        qemu_printf("Ram size %" PRIu64 "MB was fixed up to %" PRIu64
>                    "MB to match machine restrictions. "
>                    "Consider updating "
>                    "the guest definition.\n", (uint64_t) (sz / MiB),
>                    (uint64_t) (newsz / MiB));
>    }
>    return newsz;
> }

May have a look but first sight it might work for sam460ex but not for the 
mac_oldworld problem as described above without duplicating size checks in 
board code that I'd like to avoid and have it only in spd_data_generate 
for clarity and avoiding mistakes in duplicated copies. What the above 
could be good for is to avoid unused RAM area but that's not related to 
spd_data_generate and if it returns error some way or asserts.

I think the problem with fix up was that memdev will allocate mem region 
and it does not have a callback for the board to apply fixup or any other 
way to specify constraints so by the time board code runs the memory 
region is already allocated with the user specified size so all the board 
code can do is use that or abort (or not use and waste some amount). Maybe 
s390x wasn't converted to this or using it differently?

Regards,
BALATON Zoltan