[Qemu-devel] [PATCH 0/5] mips_malta: Clean up definition of flash memory size

[Qemu-devel] [PATCH 0/5] mips_malta: Clean up definition of flash memory size

[Philippe Mathieu-Daudé]

Hi Markus,

this is a rework of your 'mips_malta: Clean up definition of flash memory size somewhat' patch:
https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg07177.html

Regards,

Phil.

Based-on: <20190226193408.23862-9-armbru@redhat.com>

Markus Armbruster (1):
  mips_malta: Clean up definition of flash memory size somewhat

Philippe Mathieu-Daudé (4):
  hw/mips/malta: Fix the DEBUG_BOARD_INIT code
  hw/mips/malta: Remove fl_sectors variable (used one single time)
  hw/mips/malta: Restrict 'bios_size' variable scope
  hw/mips/malta: Only accept 'monitor' pflash of 4MiB

 hw/mips/mips_malta.c | 27 +++++++++++++++++----------
 1 file changed, 17 insertions(+), 10 deletions(-)

-- 
2.20.1

[Qemu-devel] [PATCH 1/5] hw/mips/malta: Fix the DEBUG_BOARD_INIT code

[Philippe Mathieu-Daudé]

Commit fa1d36df746 missed to convert this ifdef'ed out code.
Introduce the pflash_blk variable.

This fixes:

  hw/mips/mips_malta.c:1273:16: error: implicit declaration of function ‘blk_name’; did you mean ‘basename’? [-Werror=implicit-function-declaration]
                blk_name(dinfo->bdrv), fl_sectors);
                ^~~~~~~~
  hw/mips/mips_malta.c:1273:16: error: nested extern declaration of ‘blk_name’ [-Werror=nested-externs]
  hw/mips/mips_malta.c:1273:30: error: ‘DriveInfo’ {aka ‘struct DriveInfo’} has no member named ‘bdrv’
                blk_name(dinfo->bdrv), fl_sectors);
                                ^~

Fixes: fa1d36df746
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/mips/mips_malta.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c
index 2827074e9b..b4cd8f02ad 100644
--- a/hw/mips/mips_malta.c
+++ b/hw/mips/mips_malta.c
@@ -40,6 +40,7 @@
 #include "hw/pci/pci.h"
 #include "sysemu/sysemu.h"
 #include "sysemu/arch_init.h"
+#include "sysemu/block-backend.h"
 #include "qemu/log.h"
 #include "hw/mips/bios.h"
 #include "hw/ide.h"
@@ -1205,6 +1206,7 @@ void mips_malta_init(MachineState *machine)
     qemu_irq cbus_irq, i8259_irq;
     int piix4_devfn;
     I2CBus *smbus;
+    BlockBackend *pflash_blk = NULL;
     DriveInfo *dinfo;
     DriveInfo *hd[MAX_IDE_BUS * MAX_IDE_DEVS];
     int fl_idx = 0;
@@ -1265,17 +1267,18 @@ void mips_malta_init(MachineState *machine)
 
     /* Load firmware in flash / BIOS. */
     dinfo = drive_get(IF_PFLASH, 0, fl_idx);
-#ifdef DEBUG_BOARD_INIT
     if (dinfo) {
+        pflash_blk = blk_by_legacy_dinfo(dinfo);
+#ifdef DEBUG_BOARD_INIT
         printf("Register parallel flash %d size " TARGET_FMT_lx " at "
-               "addr %08llx '%s' %x\n",
-               fl_idx, bios_size, FLASH_ADDRESS,
-               blk_name(dinfo->bdrv), fl_sectors);
-    }
+               "addr %08llx '%s'\n",
+               fl_idx, blk_getlength(pflash_blk), FLASH_ADDRESS,
+               blk_name(pflash_blk));
 #endif
+    }
     fl = pflash_cfi01_register(FLASH_ADDRESS, NULL, "mips_malta.bios",
                                BIOS_SIZE,
-                               dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
+                               pflash_blk,
                                65536, fl_sectors,
                                4, 0x0000, 0x0000, 0x0000, 0x0000, be);
     bios = pflash_cfi01_get_memory(fl);
-- 
2.20.1

Re: [Qemu-devel] [PATCH 1/5] hw/mips/malta: Fix the DEBUG_BOARD_INIT code

[Markus Armbruster]

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

> Commit fa1d36df746 missed to convert this ifdef'ed out code.

My fault...

> Introduce the pflash_blk variable.
>
> This fixes:
>
>   hw/mips/mips_malta.c:1273:16: error: implicit declaration of function ‘blk_name’; did you mean ‘basename’? [-Werror=implicit-function-declaration]
>                 blk_name(dinfo->bdrv), fl_sectors);
>                 ^~~~~~~~
>   hw/mips/mips_malta.c:1273:16: error: nested extern declaration of ‘blk_name’ [-Werror=nested-externs]
>   hw/mips/mips_malta.c:1273:30: error: ‘DriveInfo’ {aka ‘struct DriveInfo’} has no member named ‘bdrv’
>                 blk_name(dinfo->bdrv), fl_sectors);
>                                 ^~
>
> Fixes: fa1d36df746
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/mips/mips_malta.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c
> index 2827074e9b..b4cd8f02ad 100644
> --- a/hw/mips/mips_malta.c
> +++ b/hw/mips/mips_malta.c
> @@ -40,6 +40,7 @@
>  #include "hw/pci/pci.h"
>  #include "sysemu/sysemu.h"
>  #include "sysemu/arch_init.h"
> +#include "sysemu/block-backend.h"
>  #include "qemu/log.h"
>  #include "hw/mips/bios.h"
>  #include "hw/ide.h"
> @@ -1205,6 +1206,7 @@ void mips_malta_init(MachineState *machine)
>      qemu_irq cbus_irq, i8259_irq;
>      int piix4_devfn;
>      I2CBus *smbus;
> +    BlockBackend *pflash_blk = NULL;
>      DriveInfo *dinfo;
>      DriveInfo *hd[MAX_IDE_BUS * MAX_IDE_DEVS];
>      int fl_idx = 0;
> @@ -1265,17 +1267,18 @@ void mips_malta_init(MachineState *machine)
>  
>      /* Load firmware in flash / BIOS. */
>      dinfo = drive_get(IF_PFLASH, 0, fl_idx);
> -#ifdef DEBUG_BOARD_INIT
>      if (dinfo) {
> +        pflash_blk = blk_by_legacy_dinfo(dinfo);
> +#ifdef DEBUG_BOARD_INIT
>          printf("Register parallel flash %d size " TARGET_FMT_lx " at "
> -               "addr %08llx '%s' %x\n",
> -               fl_idx, bios_size, FLASH_ADDRESS,
> -               blk_name(dinfo->bdrv), fl_sectors);
> -    }
> +               "addr %08llx '%s'\n",
> +               fl_idx, blk_getlength(pflash_blk), FLASH_ADDRESS,
> +               blk_name(pflash_blk));

You stop printing @fl_sectors.  Intentional?

Would the next patch be a better fit?

>  #endif
> +    }
>      fl = pflash_cfi01_register(FLASH_ADDRESS, NULL, "mips_malta.bios",
>                                 BIOS_SIZE,
> -                               dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
> +                               pflash_blk,
>                                 65536, fl_sectors,
>                                 4, 0x0000, 0x0000, 0x0000, 0x0000, be);
>      bios = pflash_cfi01_get_memory(fl);

We should probably join lines here.

Re: [Qemu-devel] [PATCH 1/5] hw/mips/malta: Fix the DEBUG_BOARD_INIT code

[Markus Armbruster]

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

> Commit fa1d36df746 missed to convert this ifdef'ed out code.
> Introduce the pflash_blk variable.
>
> This fixes:
>
>   hw/mips/mips_malta.c:1273:16: error: implicit declaration of function ‘blk_name’; did you mean ‘basename’? [-Werror=implicit-function-declaration]
>                 blk_name(dinfo->bdrv), fl_sectors);
>                 ^~~~~~~~
>   hw/mips/mips_malta.c:1273:16: error: nested extern declaration of ‘blk_name’ [-Werror=nested-externs]
>   hw/mips/mips_malta.c:1273:30: error: ‘DriveInfo’ {aka ‘struct DriveInfo’} has no member named ‘bdrv’
>                 blk_name(dinfo->bdrv), fl_sectors);
>                                 ^~
>
> Fixes: fa1d36df746
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Question for the maintainers: Aurelien, Aleksandar, fix the
DEBUG_BOARD_INIT code or drop it?  It looks rather stale...

Re: [Qemu-devel] [PATCH 1/5] hw/mips/malta: Fix the DEBUG_BOARD_INIT code

[Aleksandar Markovic]

> From: Markus Armbruster <armbru@redhat.com>
> Subject: Re: [Qemu-devel] [PATCH 1/5] hw/mips/malta: Fix the DEBUG_BOARD_INIT code
> 
> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
> 
> > Commit fa1d36df746 missed to convert this ifdef'ed out code.
> > Introduce the pflash_blk variable.
> > 
> > This fixes:
> > 
> >    hw/mips/mips_malta.c:1273:16: error: implicit declaration of function ‘blk_name’; did you mean ‘basename’? [-Werror=implicit-function-declaration]
> >                 blk_name(dinfo->bdrv), fl_sectors);
> >                 ^~~~~~~~
> >   hw/mips/mips_malta.c:1273:16: error: nested extern declaration of ‘blk_name’ [-Werror=nested-externs]
> >   hw/mips/mips_malta.c:1273:30: error: ‘DriveInfo’ {aka ‘struct DriveInfo’} has no member named ‘bdrv’
> >                 blk_name(dinfo->bdrv), fl_sectors);
> >                                 ^~
> > 
> > Fixes: fa1d36df746
> > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> 
> Question for the maintainers: Aurelien, Aleksandar, fix the
> DEBUG_BOARD_INIT code or drop it?  It looks rather stale...

Markus,

My vote is: drop it.

And, Markus, yes, if you agree with any patch from this series, please let them all go though your pull request.

Regards,
Aleksandar

Re: [Qemu-devel] [PATCH 1/5] hw/mips/malta: Fix the DEBUG_BOARD_INIT code

[Aleksandar Markovic]

> > From: Markus Armbruster <armbru@redhat.com>
> > Subject: Re: [Qemu-devel] [PATCH 1/5] hw/mips/malta: Fix the DEBUG_BOARD_INIT code
> > 
> > Philippe Mathieu-Daudé <philmd@redhat.com> writes:
> > 
> > > Commit fa1d36df746 missed to convert this ifdef'ed out code.
> > > Introduce the pflash_blk variable.
> > > 
> > > This fixes:
> > > 
> > >    hw/mips/mips_malta.c:1273:16: error: implicit declaration of function ‘blk_name’; > did you mean ‘basename’? [-Werror=implicit-function-declaration]
> > >                 blk_name(dinfo->bdrv), fl_sectors);
> > >                 ^~~~~~~~
> > >   hw/mips/mips_malta.c:1273:16: error: nested extern declaration of ‘blk_name’
> [-Werror=nested-externs]
> > >   hw/mips/mips_malta.c:1273:30: error: ‘DriveInfo’ {aka ‘struct DriveInfo’} has no
> member named ‘bdrv’
> > >                 blk_name(dinfo->bdrv), fl_sectors);
> > >                                 ^~
> > > 
> > > Fixes: fa1d36df746
> > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> > 
> > Question for the maintainers: Aurelien, Aleksandar, fix the
> > DEBUG_BOARD_INIT code or drop it?  It looks rather stale...
> 
> Markus,
> 
> My vote is: drop it.
> 
> And, Markus, yes, if you agree with any patch from this series,
> please let them all go > though your pull request.
> 
> Regards,
> Aleksandar

Markus, if you decide to drop the code segment, you have my

Reviewed-by: Aleksandar Markovic <amarkovic@wavecomp.com>

in advance.

I believe the whole patch series was also reviewed by Richard.

Sincerely,
Aleksandar

[Qemu-devel] [PATCH 2/5] hw/mips/malta: Remove fl_sectors variable (used one single time)

[Philippe Mathieu-Daudé]

fl_sectors is only used in one place, with bios_size = FLASH_SIZE.
Use it directly and remove the variable.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/mips/mips_malta.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c
index b4cd8f02ad..00e5aac98b 100644
--- a/hw/mips/mips_malta.c
+++ b/hw/mips/mips_malta.c
@@ -1210,7 +1210,6 @@ void mips_malta_init(MachineState *machine)
     DriveInfo *dinfo;
     DriveInfo *hd[MAX_IDE_BUS * MAX_IDE_DEVS];
     int fl_idx = 0;
-    int fl_sectors = bios_size >> 16;
     int be;
 
     DeviceState *dev = qdev_create(NULL, TYPE_MIPS_MALTA);
@@ -1279,7 +1278,7 @@ void mips_malta_init(MachineState *machine)
     fl = pflash_cfi01_register(FLASH_ADDRESS, NULL, "mips_malta.bios",
                                BIOS_SIZE,
                                pflash_blk,
-                               65536, fl_sectors,
+                               65536, FLASH_SIZE >> 16,
                                4, 0x0000, 0x0000, 0x0000, 0x0000, be);
     bios = pflash_cfi01_get_memory(fl);
     fl_idx++;
-- 
2.20.1

[Qemu-devel] [PATCH 3/5] hw/mips/malta: Restrict 'bios_size' variable scope

[Philippe Mathieu-Daudé]

The 'bios_size' variable is only used in the 'if (!kernel_filename &&
!dinfo)' clause. This is the case when we don't provide -pflash command
line option, and also don't provide a -kernel option. In this case we
will check for the -bios option, or use the default BIOS_FILENAME file.

The 'bios' term is valid in this if statement, but is confuse in the
whole mips_malta_init() scope. Restrict his scope.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/mips/mips_malta.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c
index 00e5aac98b..396645b1a9 100644
--- a/hw/mips/mips_malta.c
+++ b/hw/mips/mips_malta.c
@@ -1196,7 +1196,6 @@ void mips_malta_init(MachineState *machine)
     MemoryRegion *ram_low_preio = g_new(MemoryRegion, 1);
     MemoryRegion *ram_low_postio;
     MemoryRegion *bios, *bios_copy = g_new(MemoryRegion, 1);
-    target_long bios_size = FLASH_SIZE;
     const size_t smbus_eeprom_size = 8 * 256;
     uint8_t *smbus_eeprom_buf = g_malloc0(smbus_eeprom_size);
     int64_t kernel_entry, bootloader_run_addr;
@@ -1314,6 +1313,7 @@ void mips_malta_init(MachineState *machine)
                              bootloader_run_addr, kernel_entry);
         }
     } else {
+        target_long bios_size = FLASH_SIZE;
         /* The flash region isn't executable from a KVM guest */
         if (kvm_enabled()) {
             error_report("KVM enabled but no -kernel argument was specified. "
-- 
2.20.1

[Qemu-devel] [PATCH 4/5] hw/mips/malta: Only accept 'monitor' pflash of 4MiB

[Philippe Mathieu-Daudé]

The Malta 'mother' board can use various 'daughter' core cards [1].
QEMU only models the CoreLV card.

The CoreLV card provides [2] a Galileo GT64120 as North bridge,
connecting the CPU via the 'CBUS'. The CBUS also connects a 'Monitor
flash' memory and maps it to the CPU RESET vector.
The Monitor flash size is exactly 4 MiB.
Refuse Monitor pflash of different size.

[1] https://www.linux-mips.org/wiki/MIPS_Malta#Core_cards
[2] "Malta User's Manual"  rev. 01.05 (MIPS Technologies doc number: MD00048)

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/mips/mips_malta.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c
index 396645b1a9..04788ff50a 100644
--- a/hw/mips/mips_malta.c
+++ b/hw/mips/mips_malta.c
@@ -1267,6 +1267,11 @@ void mips_malta_init(MachineState *machine)
     dinfo = drive_get(IF_PFLASH, 0, fl_idx);
     if (dinfo) {
         pflash_blk = blk_by_legacy_dinfo(dinfo);
+
+        if (blk_getlength(pflash_blk) != FLASH_SIZE) {
+                error_report("Malta CoreLV card expects a bios of 4MB");
+                exit(1);
+        }
 #ifdef DEBUG_BOARD_INIT
         printf("Register parallel flash %d size " TARGET_FMT_lx " at "
                "addr %08llx '%s'\n",
-- 
2.20.1

Re: [Qemu-devel] [PATCH 4/5] hw/mips/malta: Only accept 'monitor' pflash of 4MiB

[Richard Henderson]

On 3/5/19 8:28 AM, Philippe Mathieu-Daudé wrote:
> +
> +        if (blk_getlength(pflash_blk) != FLASH_SIZE) {
> +                error_report("Malta CoreLV card expects a bios of 4MB");
> +                exit(1);
> +        }

Indentation is off, somehow.  Tabs or extra spaces?


r~

Re: [Qemu-devel] [PATCH 4/5] hw/mips/malta: Only accept 'monitor' pflash of 4MiB

[Philippe Mathieu-Daudé]

On 3/6/19 12:53 AM, Richard Henderson wrote:
> On 3/5/19 8:28 AM, Philippe Mathieu-Daudé wrote:
>> +
>> +        if (blk_getlength(pflash_blk) != FLASH_SIZE) {
>> +                error_report("Malta CoreLV card expects a bios of 4MB");
>> +                exit(1);
>> +        }
> 
> Indentation is off, somehow.  Tabs or extra spaces?

Oops... Probably extra spaces, since checkpatch didn't complain.

I'll let Markus fix this if it takes this series ;)

Thanks!

Phil.

Re: [Qemu-devel] [PATCH 4/5] hw/mips/malta: Only accept 'monitor' pflash of 4MiB

[Markus Armbruster]

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

> The Malta 'mother' board can use various 'daughter' core cards [1].
> QEMU only models the CoreLV card.
>
> The CoreLV card provides [2] a Galileo GT64120 as North bridge,
> connecting the CPU via the 'CBUS'. The CBUS also connects a 'Monitor
> flash' memory and maps it to the CPU RESET vector.
> The Monitor flash size is exactly 4 MiB.
> Refuse Monitor pflash of different size.
>
> [1] https://www.linux-mips.org/wiki/MIPS_Malta#Core_cards
> [2] "Malta User's Manual"  rev. 01.05 (MIPS Technologies doc number: MD00048)
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/mips/mips_malta.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c
> index 396645b1a9..04788ff50a 100644
> --- a/hw/mips/mips_malta.c
> +++ b/hw/mips/mips_malta.c
> @@ -1267,6 +1267,11 @@ void mips_malta_init(MachineState *machine)
>      dinfo = drive_get(IF_PFLASH, 0, fl_idx);
>      if (dinfo) {
>          pflash_blk = blk_by_legacy_dinfo(dinfo);
> +
> +        if (blk_getlength(pflash_blk) != FLASH_SIZE) {
> +                error_report("Malta CoreLV card expects a bios of 4MB");
> +                exit(1);
> +        }
>  #ifdef DEBUG_BOARD_INIT
>          printf("Register parallel flash %d size " TARGET_FMT_lx " at "
>                 "addr %08llx '%s'\n",

I'm in favor of insisting on image size matching flash size, but I'd
rather do it in generic code.

We're already rejecting undersized images, with a sub-optimal error
message.  We're silently ignoring the tail of oversized images.  Alex
proposed a patch to generic code that does three things:

* We reject an undersized image with a sub-optimal error message.
  Improve that message.

* We silently ignore an oversized image's tail.  Warn instead.

* As a convenience feature, don't reject undersized read-only image, but
  pad it with 0xff instead, to simulate (1) above.

[v5] hw/block: better reporting on pflash backing file mismatch
Message-Id: <20190227111347.15063-1-alex.bennee@linaro.org>

I asked for the convenience feature split off into its own patch (and
offered to do it).

If we accept Alex's work (split or not), your patch merely kills the
convenience feature for this board.  I like killing it --- I consider it
a bad idea --- but if that's not in the cards, then I prefer all boards
to implement the same set of bad ideas.

[Qemu-devel] [PATCH 5/5] mips_malta: Clean up definition of flash memory size somewhat

[Philippe Mathieu-Daudé]

From: Markus Armbruster <armbru@redhat.com>

pflash_cfi01_register() takes a size in bytes, a block size in bytes
and a number of blocks.  mips_malta_init() passes BIOS_SIZE, 65536,
FLASH_SIZE >> 16.  Actually consistent only because BIOS_SIZE (defined
in include/hw/mips/bios.h as (4 * MiB)) matches FLASH_SIZE (defined
locally as 0x400000).  Confusing all the same.

Pass FLASH_SIZE instead of BIOS_SIZE.

Cc: Aurelien Jarno <aurelien@aurel32.net>
Cc: Aleksandar Rikalo <arikalo@wavecomp.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
[PMD: rebased]
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/mips/mips_malta.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c
index 04788ff50a..a5726edaa7 100644
--- a/hw/mips/mips_malta.c
+++ b/hw/mips/mips_malta.c
@@ -1280,7 +1280,7 @@ void mips_malta_init(MachineState *machine)
 #endif
     }
     fl = pflash_cfi01_register(FLASH_ADDRESS, NULL, "mips_malta.bios",
-                               BIOS_SIZE,
+                               FLASH_SIZE,
                                pflash_blk,
                                65536, FLASH_SIZE >> 16,
                                4, 0x0000, 0x0000, 0x0000, 0x0000, be);
-- 
2.20.1

Re: [Qemu-devel] [PATCH 5/5] mips_malta: Clean up definition of flash memory size somewhat

[Philippe Mathieu-Daudé]

On 3/5/19 5:28 PM, Philippe Mathieu-Daudé wrote:
> From: Markus Armbruster <armbru@redhat.com>
> 
> pflash_cfi01_register() takes a size in bytes, a block size in bytes
> and a number of blocks.  mips_malta_init() passes BIOS_SIZE, 65536,
> FLASH_SIZE >> 16.  Actually consistent only because BIOS_SIZE (defined
> in include/hw/mips/bios.h as (4 * MiB)) matches FLASH_SIZE (defined
> locally as 0x400000).  Confusing all the same.
> 
> Pass FLASH_SIZE instead of BIOS_SIZE.
> 
> Cc: Aurelien Jarno <aurelien@aurel32.net>
> Cc: Aleksandar Rikalo <arikalo@wavecomp.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

I forgot here:

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

> [PMD: rebased]
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/mips/mips_malta.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c
> index 04788ff50a..a5726edaa7 100644
> --- a/hw/mips/mips_malta.c
> +++ b/hw/mips/mips_malta.c
> @@ -1280,7 +1280,7 @@ void mips_malta_init(MachineState *machine)
>  #endif
>      }
>      fl = pflash_cfi01_register(FLASH_ADDRESS, NULL, "mips_malta.bios",
> -                               BIOS_SIZE,
> +                               FLASH_SIZE,
>                                 pflash_blk,
>                                 65536, FLASH_SIZE >> 16,
>                                 4, 0x0000, 0x0000, 0x0000, 0x0000, be);
> 

Re: [Qemu-devel] [PATCH 0/5] mips_malta: Clean up definition of flash memory size

[Richard Henderson]

On 3/5/19 8:28 AM, Philippe Mathieu-Daudé wrote:
> Markus Armbruster (1):
>   mips_malta: Clean up definition of flash memory size somewhat
> 
> Philippe Mathieu-Daudé (4):
>   hw/mips/malta: Fix the DEBUG_BOARD_INIT code
>   hw/mips/malta: Remove fl_sectors variable (used one single time)
>   hw/mips/malta: Restrict 'bios_size' variable scope
>   hw/mips/malta: Only accept 'monitor' pflash of 4MiB
> 
>  hw/mips/mips_malta.c | 27 +++++++++++++++++----------
>  1 file changed, 17 insertions(+), 10 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [PATCH 0/5] mips_malta: Clean up definition of flash memory size

[Aleksandar Markovic]

On Tuesday, March 5, 2019, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:

> Hi Markus,
>
> this is a rework of your 'mips_malta: Clean up definition of flash memory
> size somewhat' patch:
> https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg07177.html
>
> Regards,
>
> Phil.


Philippe,

Could you summarize end-user-visible changes resulting from this series?

Aleksandar


> Based-on: <20190226193408.23862-9-armbru@redhat.com>
>
> Markus Armbruster (1):
>   mips_malta: Clean up definition of flash memory size somewhat
>
> Philippe Mathieu-Daudé (4):
>   hw/mips/malta: Fix the DEBUG_BOARD_INIT code
>   hw/mips/malta: Remove fl_sectors variable (used one single time)
>   hw/mips/malta: Restrict 'bios_size' variable scope
>   hw/mips/malta: Only accept 'monitor' pflash of 4MiB
>
>  hw/mips/mips_malta.c | 27 +++++++++++++++++----------
>  1 file changed, 17 insertions(+), 10 deletions(-)
>
> --
> 2.20.1
>
>
>

Re: [Qemu-devel] [PATCH 0/5] mips_malta: Clean up definition of flash memory size

[Philippe Mathieu-Daudé]

Hi Aleksandar,

On 3/6/19 7:38 AM, Aleksandar Markovic wrote:
> On Tuesday, March 5, 2019, Philippe Mathieu-Daudé <philmd@redhat.com
> <mailto:philmd@redhat.com>> wrote:
> 
>     Hi Markus,
> 
>     this is a rework of your 'mips_malta: Clean up definition of flash
>     memory size somewhat' patch:
>     https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg07177.html
>     <https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg07177.html>
> 
>     Regards,
> 
>     Phil.
> 
> 
> Philippe,
> 
> Could you summarize end-user-visible changes resulting from this series?

Good maintainer reflex :)

There is an end-user visible change, if he provides a file that is not
exactly 4MiB he will now get a "Malta CoreLV card expects a bios of 4MB"
error message and QEMU will exit to his shell. This change is introduced
by patch 4/5.

Now I rather expect this series to get integrated in Markus current
work, because his subsequent patches change the PFlash API and it is
easier he takes this (to avoid merge conflicts).

Note: Markus series is expected to include the following patch from Alex
Bennée: "hw/block: better reporting on pflash backing file mismatch"
https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg07341.html
which is a more important user visible change. I think the the changelog
can be updated once, by Markus :)

Meanwhile, if this series is taken by Markus can I have your Ack-by?

Thanks,

Phil.

Re: [Qemu-devel] [PATCH 0/5] mips_malta: Clean up definition of flash memory size

[Markus Armbruster]

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

> Hi Aleksandar,
>
> On 3/6/19 7:38 AM, Aleksandar Markovic wrote:
>> On Tuesday, March 5, 2019, Philippe Mathieu-Daudé <philmd@redhat.com
>> <mailto:philmd@redhat.com>> wrote:
>> 
>>     Hi Markus,
>> 
>>     this is a rework of your 'mips_malta: Clean up definition of flash
>>     memory size somewhat' patch:
>>     https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg07177.html
>>     <https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg07177.html>
>> 
>>     Regards,
>> 
>>     Phil.
>> 
>> 
>> Philippe,
>> 
>> Could you summarize end-user-visible changes resulting from this series?
>
> Good maintainer reflex :)
>
> There is an end-user visible change, if he provides a file that is not
> exactly 4MiB he will now get a "Malta CoreLV card expects a bios of 4MB"
> error message and QEMU will exit to his shell. This change is introduced
> by patch 4/5.
>
> Now I rather expect this series to get integrated in Markus current
> work, because his subsequent patches change the PFlash API and it is
> easier he takes this (to avoid merge conflicts).

Definitely.

> Note: Markus series is expected to include the following patch from Alex
> Bennée: "hw/block: better reporting on pflash backing file mismatch"
> https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg07341.html
> which is a more important user visible change. I think the the changelog
> can be updated once, by Markus :)

Yes.

> Meanwhile, if this series is taken by Markus can I have your Ack-by?
>
> Thanks,
>
> Phil.

Re: [Qemu-devel] [PATCH 0/5] mips_malta: Clean up definition of flash memory size

[Markus Armbruster]

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

> Hi Markus,
>
> this is a rework of your 'mips_malta: Clean up definition of flash memory size somewhat' patch:
> https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg07177.html

Diff between my patch and your series:

diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c
index 9ade9b194c..2827074e9b 100644
--- a/hw/mips/mips_malta.c
+++ b/hw/mips/mips_malta.c
@@ -1269,12 +1269,12 @@ void mips_malta_init(MachineState *machine)
     if (dinfo) {
         printf("Register parallel flash %d size " TARGET_FMT_lx " at "
                "addr %08llx '%s' %x\n",
-               fl_idx, FLASH_SIZE, FLASH_ADDRESS,
+               fl_idx, bios_size, FLASH_ADDRESS,
                blk_name(dinfo->bdrv), fl_sectors);
     }
 #endif
     fl = pflash_cfi01_register(FLASH_ADDRESS, NULL, "mips_malta.bios",
-                               FLASH_SIZE,
+                               BIOS_SIZE,
                                dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
                                65536, fl_sectors,
                                4, 0x0000, 0x0000, 0x0000, 0x0000, be);

We have in include/hw/mips/bios.h

    #define BIOS_SIZE (4 * MiB)

and locally

    #define FLASH_SIZE    0x400000

    target_long bios_size = FLASH_SIZE;

Three names for the same value.  Therefore, there's no functional
difference, just more cleanup.  Good to know.

Re: [Qemu-devel] [PATCH 0/5] mips_malta: Clean up definition of flash memory size

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
>
>> Hi Markus,
>>
>> this is a rework of your 'mips_malta: Clean up definition of flash memory size somewhat' patch:
>> https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg07177.html
>
> Diff between my patch and your series:
>
> diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c
> index 9ade9b194c..2827074e9b 100644
> --- a/hw/mips/mips_malta.c
> +++ b/hw/mips/mips_malta.c
> @@ -1269,12 +1269,12 @@ void mips_malta_init(MachineState *machine)
>      if (dinfo) {
>          printf("Register parallel flash %d size " TARGET_FMT_lx " at "
>                 "addr %08llx '%s' %x\n",
> -               fl_idx, FLASH_SIZE, FLASH_ADDRESS,
> +               fl_idx, bios_size, FLASH_ADDRESS,
>                 blk_name(dinfo->bdrv), fl_sectors);
>      }
>  #endif
>      fl = pflash_cfi01_register(FLASH_ADDRESS, NULL, "mips_malta.bios",
> -                               FLASH_SIZE,
> +                               BIOS_SIZE,
>                                 dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
>                                 65536, fl_sectors,
>                                 4, 0x0000, 0x0000, 0x0000, 0x0000, be);
>
> We have in include/hw/mips/bios.h
>
>     #define BIOS_SIZE (4 * MiB)
>
> and locally
>
>     #define FLASH_SIZE    0x400000
>
>     target_long bios_size = FLASH_SIZE;
>
> Three names for the same value.  Therefore, there's no functional
> difference, just more cleanup.  Good to know.

Wrong diff, sorry.

diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c
index 9ade9b194c..a5726edaa7 100644
--- a/hw/mips/mips_malta.c
+++ b/hw/mips/mips_malta.c
@@ -40,6 +40,7 @@
 #include "hw/pci/pci.h"
 #include "sysemu/sysemu.h"
 #include "sysemu/arch_init.h"
+#include "sysemu/block-backend.h"
 #include "qemu/log.h"
 #include "hw/mips/bios.h"
 #include "hw/ide.h"
@@ -1195,7 +1196,6 @@ void mips_malta_init(MachineState *machine)
     MemoryRegion *ram_low_preio = g_new(MemoryRegion, 1);
     MemoryRegion *ram_low_postio;
     MemoryRegion *bios, *bios_copy = g_new(MemoryRegion, 1);
-    target_long bios_size = FLASH_SIZE;
     const size_t smbus_eeprom_size = 8 * 256;
     uint8_t *smbus_eeprom_buf = g_malloc0(smbus_eeprom_size);
     int64_t kernel_entry, bootloader_run_addr;
@@ -1205,10 +1205,10 @@ void mips_malta_init(MachineState *machine)
     qemu_irq cbus_irq, i8259_irq;
     int piix4_devfn;
     I2CBus *smbus;
+    BlockBackend *pflash_blk = NULL;
     DriveInfo *dinfo;
     DriveInfo *hd[MAX_IDE_BUS * MAX_IDE_DEVS];
     int fl_idx = 0;
-    int fl_sectors = bios_size >> 16;
     int be;
 
     DeviceState *dev = qdev_create(NULL, TYPE_MIPS_MALTA);
@@ -1265,18 +1265,24 @@ void mips_malta_init(MachineState *machine)
 
     /* Load firmware in flash / BIOS. */
     dinfo = drive_get(IF_PFLASH, 0, fl_idx);
-#ifdef DEBUG_BOARD_INIT
     if (dinfo) {
+        pflash_blk = blk_by_legacy_dinfo(dinfo);
+
+        if (blk_getlength(pflash_blk) != FLASH_SIZE) {
+                error_report("Malta CoreLV card expects a bios of 4MB");
+                exit(1);
+        }
+#ifdef DEBUG_BOARD_INIT
         printf("Register parallel flash %d size " TARGET_FMT_lx " at "
-               "addr %08llx '%s' %x\n",
-               fl_idx, FLASH_SIZE, FLASH_ADDRESS,
-               blk_name(dinfo->bdrv), fl_sectors);
-    }
+               "addr %08llx '%s'\n",
+               fl_idx, blk_getlength(pflash_blk), FLASH_ADDRESS,
+               blk_name(pflash_blk));
 #endif
+    }
     fl = pflash_cfi01_register(FLASH_ADDRESS, NULL, "mips_malta.bios",
                                FLASH_SIZE,
-                               dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
-                               65536, fl_sectors,
+                               pflash_blk,
+                               65536, FLASH_SIZE >> 16,
                                4, 0x0000, 0x0000, 0x0000, 0x0000, be);
     bios = pflash_cfi01_get_memory(fl);
     fl_idx++;
@@ -1312,6 +1318,7 @@ void mips_malta_init(MachineState *machine)
                              bootloader_run_addr, kernel_entry);
         }
     } else {
+        target_long bios_size = FLASH_SIZE;
         /* The flash region isn't executable from a KVM guest */
         if (kvm_enabled()) {
             error_report("KVM enabled but no -kernel argument was specified. "