| Message ID | 20190305162829.20079-1-philmd@redhat.com |
|---|---|
| Headers | show |
| Series | mips_malta: Clean up definition of flash memory size | expand |
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~
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 > > >
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.
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.
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. "
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.