[PATCH 0/4] Subject: [PATCH 0/4] smbus: SPD fixes

[PATCH 0/4] Subject: [PATCH 0/4] smbus: SPD fixes

[Markus Armbruster]

PATCH 1 fixes a regression, but it's a rather old one: regressed in
v4.0.0.  I doubt it needs to go into 5.0 at this stage.  But it's up
to the maintainer(s).

Markus Armbruster (4):
  sam460ex: Revert change to SPD memory type for <= 128 MiB
  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          |  4 ++--
 hw/ppc/sam460ex.c             | 13 ++++---------
 5 files changed, 14 insertions(+), 47 deletions(-)

-- 
2.21.1

[PATCH 1/4] sam460ex: Revert change to SPD memory type for <= 128 MiB

[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".  Turns out that commit changed memory type and
number of banks to

    RAM size    #banks  type    bank size
     128 MiB         1   DDR2     128 MiB
      64 MiB         2   DDR       32 MiB
      32 MiB         1   DDR       32 MiB

from

    RAM size    #banks  type    bank size
     128 MiB         2   SDR       64 MiB
      64 MiB         2   SDR       32 MiB
      32 MiB         2   SDR       16 MiB

Reverting that change also gets rid of the warning.

I doubt physical Sam460ex boards can take SDR or DDR modules, though.

The commit changed SPD contents in other places, too.  So does commit
fb1b0fcc03 "target/mips: fulong2e: Dynamically generate SPD EEPROM
data" for machine type fulong2e.  I'm not reverting these changes.

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..856bc0b5a3 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] < 256 * MiB ? SDR : DDR2,
+                                 ram_sizes[0], &err);
     if (err) {
         warn_report_err(err);
     }
-- 
2.21.1

Re: [PATCH 1/4] sam460ex: Revert change to SPD memory type for <= 128 MiB

[BALATON Zoltan]

On Mon, 20 Apr 2020, 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

Why is it useless? It lets user know there was a change so it could help 
debugging for example.

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

This is correct and intended. The idea is that the board code should not 
need to know about SPD data, all knowledge about that should be in 
spd_data_genereate().

> The warning goes back to commit 08fd99179a "sam460ex: Clean up SPD
> EEPROM creation".  Turns out that commit changed memory type and
> number of banks to
>
>    RAM size    #banks  type    bank size
>     128 MiB         1   DDR2     128 MiB
>      64 MiB         2   DDR       32 MiB
>      32 MiB         1   DDR       32 MiB
>
> from
>
>    RAM size    #banks  type    bank size
>     128 MiB         2   SDR       64 MiB
>      64 MiB         2   SDR       32 MiB
>      32 MiB         2   SDR       16 MiB
>
> Reverting that change also gets rid of the warning.
>
> I doubt physical Sam460ex boards can take SDR or DDR modules, though.

It can't but it can use both DDR and DDR2 (the board can't but the SoC can 
and the firmware is OK with that too). This is what the commit fixed, 
please don't break it. The firmware may be confused if presented with 
different type of SDRAM than DDR or DDR2. Does it still boot and finds 
correct mem size after your change? (I think bdinfo U-Boot command tells 
what it detects.)

Regards,
BALATON Zoltan

> The commit changed SPD contents in other places, too.  So does commit
> fb1b0fcc03 "target/mips: fulong2e: Dynamically generate SPD EEPROM
> data" for machine type fulong2e.  I'm not reverting these changes.
>
> 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..856bc0b5a3 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] < 256 * MiB ? SDR : DDR2,
> +                                 ram_sizes[0], &err);
>     if (err) {
>         warn_report_err(err);
>     }
>

Re: [PATCH 1/4] sam460ex: Revert change to SPD memory type for <= 128 MiB

[Markus Armbruster]

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

> On Mon, 20 Apr 2020, 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
>
> Why is it useless? It lets user know there was a change so it could
> help debugging for example.

The memory type is chosen by QEMU, not the user.  Why should QEMU warn
the user when it chooses DDR, but not when it chooses DDR2?

>> This is because sam460ex_init() asks spd_data_generate() for DDR2,
>> which is impossible, so spd_data_generate() corrects it to DDR.
>
> This is correct and intended. The idea is that the board code should
> not need to know about SPD data, all knowledge about that should be in
> spd_data_genereate().

I challenge this idea.

The kind of RAM module a board accepts is a property of the board.
Modelling that in board code is sensible and easy.  Attempting to model
it in a one size fits all helper function is unlikely to work for all
boards.

Apparently some boards (including malta) need two banks, so your helper
increases the number of banks from one to two, but only when that's
possible without changing the type.

What if another board needs one bank?  Four?  Two even if that requires
changing the type?  You'll end up with a bunch of flags to drive the
helper's magic.  Not yet because the helper has a grand total of *two*
users, and much of its magic is used by neither, as demonstrated by
PATCH 2.

If you want magic, have a non-magic function that does exactly what it's
told, and a magic one to tell it what to do.  The non-magic one will be
truly reusable.  You can have any number of magic ones.  Boards with
sufficiently similar requirements can share a magic one.

>> The warning goes back to commit 08fd99179a "sam460ex: Clean up SPD
>> EEPROM creation".  Turns out that commit changed memory type and
>> number of banks to
>>
>>    RAM size    #banks  type    bank size
>>     128 MiB         1   DDR2     128 MiB
>>      64 MiB         2   DDR       32 MiB
>>      32 MiB         1   DDR       32 MiB
>>
>> from
>>
>>    RAM size    #banks  type    bank size
>>     128 MiB         2   SDR       64 MiB
>>      64 MiB         2   SDR       32 MiB
>>      32 MiB         2   SDR       16 MiB
>>
>> Reverting that change also gets rid of the warning.
>>
>> I doubt physical Sam460ex boards can take SDR or DDR modules, though.
>
> It can't but it can use both DDR and DDR2 (the board can't but the SoC
> can and the firmware is OK with that too). This is what the commit
> fixed, please don't break it.

When a commit fixes something, it should say so.  This one does not:

    commit 08fd99179a8c5d34c7065e2ad76c7f8b6afe239e
    Author: BALATON Zoltan <balaton@eik.bme.hu>
    Date:   Thu Jan 3 17:27:24 2019 +0100

        sam460ex: Clean up SPD EEPROM creation

        Get rid of code from MIPS Malta board used to create SPD EEPROM data
        (parts of which was not even needed for sam460ex) and use the generic
        spd_data_generate() function to simplify this.

        Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
        Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

This commit message certainly suggests it was a refactoring that did not
change SPD data at all.  Not the case, but you have to look at the patch
closely to see.  Water under the bridge, of course.

It misled me to assume the change was unintentional.

>                               The firmware may be confused if
> presented with different type of SDRAM than DDR or DDR2. Does it still
> boot and finds correct mem size after your change? (I think bdinfo
> U-Boot command tells what it detects.)

You're right: 08fd99179a8^ appears not to boot with -m 128 and lower.
08fd99179a8 was indeed a silent bug fix.

"make check" passes, though :)

I'll replace this patch by one that merely suppresses the warning.

Thanks!

Re: [PATCH 1/4] sam460ex: Revert change to SPD memory type for <= 128 MiB

[BALATON Zoltan]

On Tue, 21 Apr 2020, Markus Armbruster wrote:
> BALATON Zoltan <balaton@eik.bme.hu> writes:
>> On Mon, 20 Apr 2020, 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
>>
>> Why is it useless? It lets user know there was a change so it could
>> help debugging for example.
>
> The memory type is chosen by QEMU, not the user.  Why should QEMU warn
> the user when it chooses DDR, but not when it chooses DDR2?
>
>>> This is because sam460ex_init() asks spd_data_generate() for DDR2,
>>> which is impossible, so spd_data_generate() corrects it to DDR.
>>
>> This is correct and intended. The idea is that the board code should
>> not need to know about SPD data, all knowledge about that should be in
>> spd_data_genereate().
>
> I challenge this idea.
>
> The kind of RAM module a board accepts is a property of the board.
> Modelling that in board code is sensible and easy.  Attempting to model
> it in a one size fits all helper function is unlikely to work for all
> boards.
>
> Apparently some boards (including malta) need two banks, so your helper
> increases the number of banks from one to two, but only when that's
> possible without changing the type.
>
> What if another board needs one bank?  Four?  Two even if that requires
> changing the type?  You'll end up with a bunch of flags to drive the
> helper's magic.  Not yet because the helper has a grand total of *two*
> users, and much of its magic is used by neither, as demonstrated by
> PATCH 2.
>
> If you want magic, have a non-magic function that does exactly what it's
> told, and a magic one to tell it what to do.  The non-magic one will be
> truly reusable.  You can have any number of magic ones.  Boards with
> sufficiently similar requirements can share a magic one.

So far we have only sufficiently similar boards that can share the only 
magic function. Not many boards use SPD data (these are mostly needed for 
real board firmware so anything purely virtual don't model it usually). 
The refactoring you propose could be needed if we had more dissimilar 
boards but I think we could do that at that time. Until then I've tried to 
make it simple for board code and put all magic in one place instead of 
having separate implementation of this in several boards. Maybe someone 
should try to convert the remaining boards (MIPS Malta and ARM 
integratorcp) to see if any refactoring is needed before doing those 
refactoring without checking first what's needed. I did not try to convert 
those boards because I cannot test them.

>>> The warning goes back to commit 08fd99179a "sam460ex: Clean up SPD
>>> EEPROM creation".  Turns out that commit changed memory type and
>>> number of banks to
>>>
>>>    RAM size    #banks  type    bank size
>>>     128 MiB         1   DDR2     128 MiB
>>>      64 MiB         2   DDR       32 MiB
>>>      32 MiB         1   DDR       32 MiB
>>>
>>> from
>>>
>>>    RAM size    #banks  type    bank size
>>>     128 MiB         2   SDR       64 MiB
>>>      64 MiB         2   SDR       32 MiB
>>>      32 MiB         2   SDR       16 MiB
>>>
>>> Reverting that change also gets rid of the warning.
>>>
>>> I doubt physical Sam460ex boards can take SDR or DDR modules, though.
>>
>> It can't but it can use both DDR and DDR2 (the board can't but the SoC
>> can and the firmware is OK with that too). This is what the commit
>> fixed, please don't break it.
>
> When a commit fixes something, it should say so.  This one does not:
>
>    commit 08fd99179a8c5d34c7065e2ad76c7f8b6afe239e
>    Author: BALATON Zoltan <balaton@eik.bme.hu>
>    Date:   Thu Jan 3 17:27:24 2019 +0100
>
>        sam460ex: Clean up SPD EEPROM creation
>
>        Get rid of code from MIPS Malta board used to create SPD EEPROM data
>        (parts of which was not even needed for sam460ex) and use the generic
>        spd_data_generate() function to simplify this.
>
>        Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>        Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
>
> This commit message certainly suggests it was a refactoring that did not
> change SPD data at all.  Not the case, but you have to look at the patch
> closely to see.  Water under the bridge, of course.
>
> It misled me to assume the change was unintentional.

Sorry, I may have forgotten it by the time I was refactorig commits for 
submission.

Regards,
BALATON Zoltan

Re: [PATCH 1/4] sam460ex: Revert change to SPD memory type for <= 128 MiB

[Markus Armbruster]

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

> On Tue, 21 Apr 2020, Markus Armbruster wrote:
>> BALATON Zoltan <balaton@eik.bme.hu> writes:
>>> On Mon, 20 Apr 2020, 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
>>>
>>> Why is it useless? It lets user know there was a change so it could
>>> help debugging for example.
>>
>> The memory type is chosen by QEMU, not the user.  Why should QEMU warn
>> the user when it chooses DDR, but not when it chooses DDR2?
>>
>>>> This is because sam460ex_init() asks spd_data_generate() for DDR2,
>>>> which is impossible, so spd_data_generate() corrects it to DDR.
>>>
>>> This is correct and intended. The idea is that the board code should
>>> not need to know about SPD data, all knowledge about that should be in
>>> spd_data_genereate().
>>
>> I challenge this idea.
>>
>> The kind of RAM module a board accepts is a property of the board.
>> Modelling that in board code is sensible and easy.  Attempting to model
>> it in a one size fits all helper function is unlikely to work for all
>> boards.
>>
>> Apparently some boards (including malta) need two banks, so your helper
>> increases the number of banks from one to two, but only when that's
>> possible without changing the type.
>>
>> What if another board needs one bank?  Four?  Two even if that requires
>> changing the type?  You'll end up with a bunch of flags to drive the
>> helper's magic.  Not yet because the helper has a grand total of *two*
>> users, and much of its magic is used by neither, as demonstrated by
>> PATCH 2.
>>
>> If you want magic, have a non-magic function that does exactly what it's
>> told, and a magic one to tell it what to do.  The non-magic one will be
>> truly reusable.  You can have any number of magic ones.  Boards with
>> sufficiently similar requirements can share a magic one.
>
> So far we have only sufficiently similar boards that can share the
> only magic function. Not many boards use SPD data (these are mostly
> needed for real board firmware so anything purely virtual don't model
> it usually). The refactoring you propose could be needed if we had
> more dissimilar boards but I think we could do that at that
> time. Until then I've tried to make it simple for board code and put

Keeping things simple and just serve the needs you actually have is
good.  We're in a much better position to figure out how to best serve
more complicated needs once we actually have them :)

> all magic in one place instead of having separate implementation of
> this in several boards. Maybe someone should try to convert the
> remaining boards (MIPS Malta and ARM integratorcp) to see if any
> refactoring is needed before doing those refactoring without checking
> first what's needed. I did not try to convert those boards because I
> cannot test them.

That's fair.

[...]

[PATCH 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 856bc0b5a3..029fb6191a 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] < 256 * MiB ? SDR : 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

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

[BALATON Zoltan]

On Mon, 20 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.

The idea here again is to make it work if there's a way it could work 
rather than throw back an error to user and bail. This is for user 
convenience in the likely case the user knows nothing about the board or 
SPD data restrictions. You seem to disagree with this and want to remove 
all such logic from everywhere. Despite the title of this patch it's not 
just fixing error API usage but removing those fix ups.

If Error cannot be used for this could you change error_setg to 
warn_report and keep the fix ups untouched? That fixes the problem with 
error (although leaves no chance to board code to handle errors). Maybe 
fix Error to be usable for what it's intended for rather than removing 
cases it can't handle.

Regards,
BALATON Zoltan

> 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 856bc0b5a3..029fb6191a 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] < 256 * MiB ? SDR : 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 2/4] smbus: Fix spd_data_generate() error API violation

[Markus Armbruster]

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

> On Mon, 20 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.
>
> The idea here again is to make it work if there's a way it could work
> rather than throw back an error to user and bail. This is for user
> convenience in the likely case the user knows nothing about the board
> or SPD data restrictions.

We're in perfect agreement that the user of QEMU should not need to know
anything about memory type and number of banks.  QEMU should pick a
sensible configuration for any memory size it supports.

>                           You seem to disagree with this

Here's what I actually disagree with:

1. QEMU warning the user about its choice of memory type, but only
sometimes.  Why warn?  There is nothing wrong, and there is nothing the
user can do to avoid the condition that triggers the warning.

2. QEMU changing the user's memory size.  Yes, I understand that's an
attempt to be helpful, but I prefer my tools not to second-guess my
intent.

>                                                          and want to
> remove all such logic from everywhere. Despite the title of this patch
> it's not just fixing error API usage but removing those fix ups.

I'm removing unused code that is also broken.  If you want to keep it,
please fix it, and provide a user and/or a unit test.  Without that, it
is a trap for future callers.

> If Error cannot be used for this could you change error_setg to
> warn_report and keep the fix ups untouched? That fixes the problem
> with error (although leaves no chance to board code to handle
> errors). Maybe fix Error to be usable for what it's intended for
> rather than removing cases it can't handle.

Error is designed for errors, not warnings.

A function either succeeds (no error) or fails (one error).

It can be pressed into service for warnings (you did, although in a
buggy way).  You still can return at most one Error per Error **
parameter.  If you need multiple warnings, use multiple parameters
(ugh!).  You could also try to squash multiple warnings into one the
hints mechanism.

I'd advise against combining warn_report() and Error ** in one function.
A function should either take care of talking to the user completely, or
leave it to its caller completely.

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

[BALATON Zoltan]

On Tue, 21 Apr 2020, Markus Armbruster wrote:
> BALATON Zoltan <balaton@eik.bme.hu> writes:
>> On Mon, 20 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.
>>
>> The idea here again is to make it work if there's a way it could work
>> rather than throw back an error to user and bail. This is for user
>> convenience in the likely case the user knows nothing about the board
>> or SPD data restrictions.
>
> We're in perfect agreement that the user of QEMU should not need to know
> anything about memory type and number of banks.  QEMU should pick a
> sensible configuration for any memory size it supports.

I though it could be useful to warn the users when QEMU had to fix up some 
values to notify them that what they get may not be what they expect and 
can then know why. If the message really annoys you you can remove it but 
I think it can be useful. I just think your real problem with it is that 
Error can't support it so it's easier to remove the warning than fixing 
Error or use warn_report instead.

>>                           You seem to disagree with this
>
> Here's what I actually disagree with:
>
> 1. QEMU warning the user about its choice of memory type, but only
> sometimes.  Why warn?  There is nothing wrong, and there is nothing the
> user can do to avoid the condition that triggers the warning.

The memory size that triggers the warning is specified by the user so the 
user can do someting about it.

> 2. QEMU changing the user's memory size.  Yes, I understand that's an
> attempt to be helpful, but I prefer my tools not to second-guess my
> intent.

I agree with that and also hate Windows's habit of trying to be more 
intelligent than the user and prefer the Unix way however the average 
users of QEMU (from my perpective, who wants to run Amiga like OSes 
typically on Windows and for the most part not knowing what they are 
doing) are already intimidated by the messy QEMU command line interface 
and will specify random values and complain or go away if it does not work 
so making their life a little easier is not useless. These users (or any 
CLI users) are apparently not relevant from your point of view but they do 
exist and I think should be supported better.

>>                                                          and want to
>> remove all such logic from everywhere. Despite the title of this patch
>> it's not just fixing error API usage but removing those fix ups.
>
> I'm removing unused code that is also broken.  If you want to keep it,
> please fix it, and provide a user and/or a unit test.  Without that, it
> is a trap for future callers.

What was broken in this patch? Isn't freeing the previous warning when 
adding a new one or combining them as you say below (although I don't 
really get what you mean) would fix it without removing fix ups? It's only 
unused now because in previous patches you've removed all usages: first 
broke memory fixup because of some internal QEMU API did not support 
fixing up memory size so instead of fixing that API removed what did not 
fit, then now removing type fix ups because it's not fitting Error API.

Originally it did work well and produced usable values for whatever number 
the user specified and it was convenient for users. (Especially the 
sam460ex is a problematic case because of SoC and firmware limits that the 
user should not need to know about.)

>> If Error cannot be used for this could you change error_setg to
>> warn_report and keep the fix ups untouched? That fixes the problem
>> with error (although leaves no chance to board code to handle
>> errors). Maybe fix Error to be usable for what it's intended for
>> rather than removing cases it can't handle.
>
> Error is designed for errors, not warnings.
>
> A function either succeeds (no error) or fails (one error).
>
> It can be pressed into service for warnings (you did, although in a
> buggy way).  You still can return at most one Error per Error **
> parameter.  If you need multiple warnings, use multiple parameters
> (ugh!).  You could also try to squash multiple warnings into one the
> hints mechanism.
>
> I'd advise against combining warn_report() and Error ** in one function.
> A function should either take care of talking to the user completely, or
> leave it to its caller completely.

I've tried to use error so the board code can decide what's an error and 
what's a warning but if this cannot be done with this QEMU facility I 
don't know a better way than using warn_report for warnings.

Regards,
BALATON Zoltan

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

[Markus Armbruster]

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

> On Tue, 21 Apr 2020, Markus Armbruster wrote:
>> BALATON Zoltan <balaton@eik.bme.hu> writes:
>>> On Mon, 20 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.
>>>
>>> The idea here again is to make it work if there's a way it could work
>>> rather than throw back an error to user and bail. This is for user
>>> convenience in the likely case the user knows nothing about the board
>>> or SPD data restrictions.
>>
>> We're in perfect agreement that the user of QEMU should not need to know
>> anything about memory type and number of banks.  QEMU should pick a
>> sensible configuration for any memory size it supports.
>
> I though it could be useful to warn the users when QEMU had to fix up
> some values to notify them that what they get may not be what they
> expect and can then know why.

*If* QEMU "fixed up" the user's configuration, then QEMU should indeed
warn the user.

But it doesn't fix up anything here.  This broken code is unused.

>                               If the message really annoys you you can
> remove it but I think it can be useful. I just think your real problem
> with it is that Error can't support it so it's easier to remove the
> warning than fixing Error or use warn_report instead.

It's indeed easier to remove broken unused code than to try fixing it.
YAGNI.

>>>                           You seem to disagree with this
>>
>> Here's what I actually disagree with:
>>
>> 1. QEMU warning the user about its choice of memory type, but only
>> sometimes.  Why warn?  There is nothing wrong, and there is nothing the
>> user can do to avoid the condition that triggers the warning.
>
> The memory size that triggers the warning is specified by the user so
> the user can do someting about it.

There is no way to trigger the warning.  If we dropped PATCH 1 instead
of fixing it as I did in v2, then the only way to trigger the warning is
-M sam460ex -m 64 or -m 32, and the only way to avoid it is to don't do
that.

Why would a user care whether he gets DDR or DDR2 memory?

>> 2. QEMU changing the user's memory size.  Yes, I understand that's an
>> attempt to be helpful, but I prefer my tools not to second-guess my
>> intent.
>
> I agree with that and also hate Windows's habit of trying to be more
> intelligent than the user and prefer the Unix way however the average
> users of QEMU (from my perpective, who wants to run Amiga like OSes
> typically on Windows and for the most part not knowing what they are
> doing) are already intimidated by the messy QEMU command line
> interface and will specify random values and complain or go away if it
> does not work so making their life a little easier is not
> useless. These users (or any CLI users) are apparently not relevant
> from your point of view but they do exist and I think should be
> supported better.

This theoretical.  Remember, we're talking about unused code.  Proof:

    $ 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

I figure commit a0258e4afa "ppc/{ppc440_bamboo, sam460ex}: drop RAM size
fixup" removed the only uses.  If you disagree with it, take it up with
Igor, please.

>>>                                                          and want to
>>> remove all such logic from everywhere. Despite the title of this patch
>>> it's not just fixing error API usage but removing those fix ups.
>>
>> I'm removing unused code that is also broken.  If you want to keep it,
>> please fix it, and provide a user and/or a unit test.  Without that, it
>> is a trap for future callers.
>
> What was broken in this patch? Isn't freeing the previous warning when

My commit message explains:

     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.

Now have a look at the code I delete:

    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;
        }
    }
    [...]
    if (size > (1ULL << sz_log2) * nbanks) {
--->    error_setg(errp, "Memory size is too big for SDRAM, truncating");
    }

If more than one of the conditions guarding these error_setg() is true,
and @errp is neither null, &error_abort, nor &error_fatal, then it'll
point to an Error * that is not null for the non-first error_setg()
call.  This will trip the assertion in error_setv().

> adding a new one or combining them as you say below (although I don't
> really get what you mean) would fix it without removing fix ups? It's
> only unused now because in previous patches you've removed all usages:
> first broke memory fixup because of some internal QEMU API did not
> support fixing up memory size so instead of fixing that API removed
> what did not fit, then now removing type fix ups because it's not
> fitting Error API.
>
> Originally it did work well and produced usable values for whatever
> number the user specified and it was convenient for users. (Especially
> the sam460ex is a problematic case because of SoC and firmware limits
> that the user should not need to know about.)

I don't doubt it worked in your testing.

It's still wrong.

>>> If Error cannot be used for this could you change error_setg to
>>> warn_report and keep the fix ups untouched? That fixes the problem
>>> with error (although leaves no chance to board code to handle
>>> errors). Maybe fix Error to be usable for what it's intended for
>>> rather than removing cases it can't handle.
>>
>> Error is designed for errors, not warnings.
>>
>> A function either succeeds (no error) or fails (one error).
>>
>> It can be pressed into service for warnings (you did, although in a
>> buggy way).  You still can return at most one Error per Error **
>> parameter.  If you need multiple warnings, use multiple parameters
>> (ugh!).  You could also try to squash multiple warnings into one the
>> hints mechanism.
>>
>> I'd advise against combining warn_report() and Error ** in one function.
>> A function should either take care of talking to the user completely, or
>> leave it to its caller completely.
>
> I've tried to use error so the board code can decide what's an error
> and what's a warning but if this cannot be done with this QEMU
> facility I don't know a better way than using warn_report for
> warnings.

Is there any board that genuinely wants to decide what's an error and
what's a warning?

Here's spd_data_generate() contract again:

        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.

It has a grand total of two users.  Both treat the second case as
warning, and the third as error.

Until you have a user that wants to handle things differently, you're
overcomplicating things.

YAGNI!

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

[Philippe Mathieu-Daudé]

On 4/24/20 11:45 AM, Markus Armbruster wrote:
> BALATON Zoltan <balaton@eik.bme.hu> writes:
>> On Tue, 21 Apr 2020, Markus Armbruster wrote:
[...]
>>>
>>> Here's what I actually disagree with:
>>>
>>> 1. QEMU warning the user about its choice of memory type, but only
>>> sometimes.  Why warn?  There is nothing wrong, and there is nothing the
>>> user can do to avoid the condition that triggers the warning.
>>
>> The memory size that triggers the warning is specified by the user so
>> the user can do someting about it.
> 
> There is no way to trigger the warning.  If we dropped PATCH 1 instead
> of fixing it as I did in v2, then the only way to trigger the warning is
> -M sam460ex -m 64 or -m 32, and the only way to avoid it is to don't do
> that.
> 
> Why would a user care whether he gets DDR or DDR2 memory?

To use a different firmware code path!

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

[Markus Armbruster]

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

> On 4/24/20 11:45 AM, Markus Armbruster wrote:
>> BALATON Zoltan <balaton@eik.bme.hu> writes:
>>> On Tue, 21 Apr 2020, Markus Armbruster wrote:
> [...]
>>>>
>>>> Here's what I actually disagree with:
>>>>
>>>> 1. QEMU warning the user about its choice of memory type, but only
>>>> sometimes.  Why warn?  There is nothing wrong, and there is nothing the
>>>> user can do to avoid the condition that triggers the warning.
>>>
>>> The memory size that triggers the warning is specified by the user so
>>> the user can do someting about it.
>>
>> There is no way to trigger the warning.  If we dropped PATCH 1 instead
>> of fixing it as I did in v2, then the only way to trigger the warning is
>> -M sam460ex -m 64 or -m 32, and the only way to avoid it is to don't do
>> that.
>>
>> Why would a user care whether he gets DDR or DDR2 memory?
>
> To use a different firmware code path!

Let's see how that works out for users.

Assume machine foobar needs a non-default firmware for "small" memory
sizes, such as -m 64.

Alice doesn't know.  She starts QEMU like this:

    $ qemu-system-foo -M foobar -m 64

It hangs early in boot.  Not good.

Except the warning from spd_data_generate() rides to her rescue!

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

Since Alice is rather sharp, the warning makes her realize immediately
that she needs to use different firmware, like this:

    $ qemu-system-foo -M foobar -m 64 -bios roms/foobar/firmware-small.bin

Okay, I'm done telling my fairy tale.

If you want helpful, make it work out of the box: default to the
firmware that actually works, don't make users guess which one they
need.

But one more time: this is all theoretical.  We're talking about unused,
broken code.  If you want to keep it, please add users, and fix it up.

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

[BALATON Zoltan]

On Fri, 24 Apr 2020, Markus Armbruster wrote:
> BALATON Zoltan <balaton@eik.bme.hu> writes:
>> On Tue, 21 Apr 2020, Markus Armbruster wrote:
>>> BALATON Zoltan <balaton@eik.bme.hu> writes:
>>>> On Mon, 20 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.
>>>>
>>>> The idea here again is to make it work if there's a way it could work
>>>> rather than throw back an error to user and bail. This is for user
>>>> convenience in the likely case the user knows nothing about the board
>>>> or SPD data restrictions.
>>>
>>> We're in perfect agreement that the user of QEMU should not need to know
>>> anything about memory type and number of banks.  QEMU should pick a
>>> sensible configuration for any memory size it supports.
>>
>> I though it could be useful to warn the users when QEMU had to fix up
>> some values to notify them that what they get may not be what they
>> expect and can then know why.
>
> *If* QEMU "fixed up" the user's configuration, then QEMU should indeed
> warn the user.
>
> But it doesn't fix up anything here.  This broken code is unused.
>
>>                               If the message really annoys you you can
>> remove it but I think it can be useful. I just think your real problem
>> with it is that Error can't support it so it's easier to remove the
>> warning than fixing Error or use warn_report instead.
>
> It's indeed easier to remove broken unused code than to try fixing it.
> YAGNI.
>
>>>>                           You seem to disagree with this
>>>
>>> Here's what I actually disagree with:
>>>
>>> 1. QEMU warning the user about its choice of memory type, but only
>>> sometimes.  Why warn?  There is nothing wrong, and there is nothing the
>>> user can do to avoid the condition that triggers the warning.
>>
>> The memory size that triggers the warning is specified by the user so
>> the user can do someting about it.
>
> There is no way to trigger the warning.  If we dropped PATCH 1 instead
> of fixing it as I did in v2, then the only way to trigger the warning is
> -M sam460ex -m 64 or -m 32, and the only way to avoid it is to don't do
> that.
>
> Why would a user care whether he gets DDR or DDR2 memory?
>
>>> 2. QEMU changing the user's memory size.  Yes, I understand that's an
>>> attempt to be helpful, but I prefer my tools not to second-guess my
>>> intent.
>>
>> I agree with that and also hate Windows's habit of trying to be more
>> intelligent than the user and prefer the Unix way however the average
>> users of QEMU (from my perpective, who wants to run Amiga like OSes
>> typically on Windows and for the most part not knowing what they are
>> doing) are already intimidated by the messy QEMU command line
>> interface and will specify random values and complain or go away if it
>> does not work so making their life a little easier is not
>> useless. These users (or any CLI users) are apparently not relevant
>> from your point of view but they do exist and I think should be
>> supported better.
>
> This theoretical.  Remember, we're talking about unused code.  Proof:
>
>    $ 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
>
> I figure commit a0258e4afa "ppc/{ppc440_bamboo, sam460ex}: drop RAM size
> fixup" removed the only uses.  If you disagree with it, take it up with
> Igor, please.

I did raise similar complaints at that patch series and proposed several 
alternatives to preserve the previous functionality (sam460ex wasn't the 
only board that fixed up memory size for users) but since current APIs 
don't support that and adding this extra feature for just this machine 
wasn't a priority, my comments were accepted and ignored and I did not 
feel it would be fair to hold back the whole series for this.

I think I've explained how it worked and how it should work in my opinion 
and hope this could be revived at some point when the QEMU CLI will be 
made more user friendly (if ever) but I'm not willing to fight for that 
now. Silence in this case does not mean agreement but I see no point 
arguing if you cannot be convinced.

Just one point I'd like to keep is that no matter how you fix it the board 
code should be the only one that decides if the error is recoverable or 
not so don't abort or assert from this helper but return error to board 
code and exit from there so it has a chnace to recover in the furure or 
add "magic function" to help users wihtout having to touch this helper 
again. When you're touching the helper it would be nice to also try to 
convert the remaining two machines with hard coded SPD data so the final 
function is truely covering all current users of the function.

Regards,
BALATON Zoltan

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

[Markus Armbruster]

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

> On Fri, 24 Apr 2020, Markus Armbruster wrote:
>> BALATON Zoltan <balaton@eik.bme.hu> writes:
>>> On Tue, 21 Apr 2020, Markus Armbruster wrote:
>>>> BALATON Zoltan <balaton@eik.bme.hu> writes:
>>>>> On Mon, 20 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.
>>>>>
>>>>> The idea here again is to make it work if there's a way it could work
>>>>> rather than throw back an error to user and bail. This is for user
>>>>> convenience in the likely case the user knows nothing about the board
>>>>> or SPD data restrictions.
>>>>
>>>> We're in perfect agreement that the user of QEMU should not need to know
>>>> anything about memory type and number of banks.  QEMU should pick a
>>>> sensible configuration for any memory size it supports.
>>>
>>> I though it could be useful to warn the users when QEMU had to fix up
>>> some values to notify them that what they get may not be what they
>>> expect and can then know why.
>>
>> *If* QEMU "fixed up" the user's configuration, then QEMU should indeed
>> warn the user.
>>
>> But it doesn't fix up anything here.  This broken code is unused.
>>
>>>                               If the message really annoys you you can
>>> remove it but I think it can be useful. I just think your real problem
>>> with it is that Error can't support it so it's easier to remove the
>>> warning than fixing Error or use warn_report instead.
>>
>> It's indeed easier to remove broken unused code than to try fixing it.
>> YAGNI.
>>
>>>>>                           You seem to disagree with this
>>>>
>>>> Here's what I actually disagree with:
>>>>
>>>> 1. QEMU warning the user about its choice of memory type, but only
>>>> sometimes.  Why warn?  There is nothing wrong, and there is nothing the
>>>> user can do to avoid the condition that triggers the warning.
>>>
>>> The memory size that triggers the warning is specified by the user so
>>> the user can do someting about it.
>>
>> There is no way to trigger the warning.  If we dropped PATCH 1 instead
>> of fixing it as I did in v2, then the only way to trigger the warning is
>> -M sam460ex -m 64 or -m 32, and the only way to avoid it is to don't do
>> that.
>>
>> Why would a user care whether he gets DDR or DDR2 memory?
>>
>>>> 2. QEMU changing the user's memory size.  Yes, I understand that's an
>>>> attempt to be helpful, but I prefer my tools not to second-guess my
>>>> intent.
>>>
>>> I agree with that and also hate Windows's habit of trying to be more
>>> intelligent than the user and prefer the Unix way however the average
>>> users of QEMU (from my perpective, who wants to run Amiga like OSes
>>> typically on Windows and for the most part not knowing what they are
>>> doing) are already intimidated by the messy QEMU command line
>>> interface and will specify random values and complain or go away if it
>>> does not work so making their life a little easier is not
>>> useless. These users (or any CLI users) are apparently not relevant
>>> from your point of view but they do exist and I think should be
>>> supported better.
>>
>> This theoretical.  Remember, we're talking about unused code.  Proof:
>>
>>    $ 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
>>
>> I figure commit a0258e4afa "ppc/{ppc440_bamboo, sam460ex}: drop RAM size
>> fixup" removed the only uses.  If you disagree with it, take it up with
>> Igor, please.
>
> I did raise similar complaints at that patch series and proposed
> several alternatives to preserve the previous functionality (sam460ex
> wasn't the only board that fixed up memory size for users) but since
> current APIs don't support that and adding this extra feature for just
> this machine wasn't a priority, my comments were accepted and ignored
> and I did not feel it would be fair to hold back the whole series for
> this.
>
> I think I've explained how it worked and how it should work in my
> opinion and hope this could be revived at some point when the QEMU CLI
> will be made more user friendly (if ever) but I'm not willing to fight
> for that now. Silence in this case does not mean agreement but I see
> no point arguing if you cannot be convinced.

I acknowledge your opinion.

CLI usability for humans matters.  We're not doing well there.

A big part of the usability problem is complexity.

"Correcting" the user's configuration makes the CLI *more* complex.
Even worse when done deep down in the bowels of board code, because that
can only lead to inconsistency between machines.

The error message for a memory size we can't support could perhaps be
more helpful.

Good defaults are important.  The default memory size for sam460ex looks
okay to me.

If this commit changed the CLI, I'd gladly document your opinion /
opposition in the commit message.  But it doesn't.  It merely deletes
unreachable code because it's buggy.

> Just one point I'd like to keep is that no matter how you fix it the
> board code should be the only one that decides if the error is
> recoverable or not so don't abort or assert from this helper but
> return error to board code and exit from there so it has a chnace to
> recover in the furure or add "magic function" to help users wihtout
> having to touch this helper again. When you're touching the helper it
> would be nice to also try to convert the remaining two machines with
> hard coded SPD data so the final function is truely covering all
> current users of the function.

I'm not going to add unreachable code in the hope it may someday be
needed, sorry.

Refactoring the helper when we actually *have* a need is much more
likely to result in a helper that satisfies them.

[PATCH 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: Max 1 banks of 2048, 1024, 512, 256, 128, 64, 32 MB DIMM/bank supported
    Possible valid RAM size: 2048

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

diff --git a/hw/ppc/ppc4xx_devs.c b/hw/ppc/ppc4xx_devs.c
index 3376c43ff5..dea39546ad 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_printf("Possible valid RAM size: %" PRIi64 "\n",
             used_size ? used_size / MiB : sdram_bank_sizes[i - 1] / MiB);
 
         g_string_free(s, true);
-- 
2.21.1

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

[Philippe Mathieu-Daudé]

On 4/20/20 3:28 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: Max 1 banks of 2048, 1024, 512, 256, 128, 64, 32 MB DIMM/bank supported
>      Possible valid RAM size: 2048
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   hw/ppc/ppc4xx_devs.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ppc/ppc4xx_devs.c b/hw/ppc/ppc4xx_devs.c
> index 3376c43ff5..dea39546ad 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_printf("Possible valid RAM size: %" PRIi64 "\n",

Maybe nicer:

            error_printf("Possible valid RAM size: %" PRIi64 " MB\n",

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

>               used_size ? used_size / MiB : sdram_bank_sizes[i - 1] / MiB);
>   
>           g_string_free(s, true);
> 

[PATCH 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 4/4] smbus: Fix spd_data_generate() for number of banks > 2

[Philippe Mathieu-Daudé]

On 4/20/20 3:28 PM, Markus Armbruster wrote:
> 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*

We would like to but lack testing so instead keep old (now duplicated) 
code :(

It should be easy to write SPD EEPROM tests although.

> 
> 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);
> 

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

[BALATON Zoltan]

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

On Mon, 20 Apr 2020, Markus Armbruster wrote:
> 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.

Hmm, why? SPD data has: spd[5]: Number of RAM banks on module (1–255) (and 
for DDR2: Ranks-1 (0–7)). So nothing says it has to be power of 2 even if 
it's commonly 2 or 4. Does this break anything that needs this to be power 
of 2? Otherwise I thik this change is wrong.

> 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*

The code that is generalised here is originally from MIPS Malta and the 
idea was to change that as well to use this but nobody did that so far.

Regards,
BALATON Zoltan

> 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);
>

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

[Markus Armbruster]

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

> On Mon, 20 Apr 2020, Markus Armbruster wrote:
>> 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.
>
> Hmm, why? SPD data has: spd[5]: Number of RAM banks on module (1–255)
> (and for DDR2: Ranks-1 (0–7)). So nothing says it has to be power of 2
> even if it's commonly 2 or 4. Does this break anything that needs this
> to be power of 2? Otherwise I thik this change is wrong.

Yes, SPD data does not require the number of banks to be a power of two.
But that's not why the loop above is wrong.  To see, let's execute it on
e-paper for type = SDR (thus max_log2 = 9) and ram_size = 2048 MiB:

    iteration   sz_log2  nbanks  bank size  total size
    0           11       1       2048 MiB   2048 MiB
    1           10       2       1024 MiB   2048 MiB
    2            9       3        512 MiB   1536 MiB    Oops!

The loop is wrong, because it fails to maintain its invariant

    nbanks * (1ull << sz_log2) == size

If you ever need magic to come up with nbanks that aren't powers of two,
you'll have to replace this loop.

But I'd rip it out instead, and ...

>> 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.

... have the board code pass the number of banks.

>> 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*
>
> The code that is generalised here is originally from MIPS Malta and
> the idea was to change that as well to use this but nobody did that so
> far.
>
> Regards,
> BALATON Zoltan
>
>> 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);
>>