| Message ID | 20190311220843.4026-9-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [PULL,01/27] pflash: Rename pflash_t to PFlashCFI01, PFlashCFI02 | expand |
On Mon, 11 Mar 2019 at 22:10, Markus Armbruster <armbru@redhat.com> wrote: > > Machine "ref405ep" maps its flash memory at address 2^32 - image size. > Image size is rounded up to the next multiple of 64KiB. Useless, > because pflash_cfi02_realize() fails with "failed to read the initial > flash content" unless the rounding is a no-op. > > If the image size exceeds 0x80000 Bytes, we overlap first SRAM, then > other stuff. No idea how that would play out, but useful outcomes > seem unlikely. > > Map the flash memory at fixed address 0xFFF80000 with size 512KiB, > regardless of image size, to match the physical hardware. > > Machine "taihu" maps its boot flash memory similarly. The code even > has a comment /* XXX: should check that size is 2MB */, followed by > disabled code to adjust the size to 2MiB regardless of image size. > > Its code to map its application flash memory looks the same, except > there the XXX comment asks for 32MiB, and the code to adjust the size > isn't disabled. Note that pflash_cfi02_realize() fails with "failed > to read the initial flash content" for images smaller than 32MiB. > > Map the boot flash memory at fixed address 0xFFE00000 with size 2MiB, > to match the physical hardware. Delete dead code from application > flash mapping, and simplify some. > > Cc: David Gibson <david@gibson.dropbear.id.au> > Signed-off-by: Markus Armbruster <armbru@redhat.com> > Acked-by: David Gibson <david@gibson.dropbear.id.au> > Reviewed-by: Alex Bennée <alex.bennee@linaro.org> > Message-Id: <20190308094610.21210-9-armbru@redhat.com> > --- > hw/ppc/ppc405_boards.c | 36 ++++++++++++------------------------ > 1 file changed, 12 insertions(+), 24 deletions(-) Hi; Coverity has just noticed a minor bug in this patch (CID 1421917): > diff --git a/hw/ppc/ppc405_boards.c b/hw/ppc/ppc405_boards.c > index bb73d6d848..fe8e3cad12 100644 > --- a/hw/ppc/ppc405_boards.c > +++ b/hw/ppc/ppc405_boards.c > @@ -156,7 +156,7 @@ static void ref405ep_init(MachineState *machine) > target_ulong kernel_base, initrd_base; > long kernel_size, initrd_size; > int linux_boot; > - int fl_idx, fl_sectors, len; > + int len; > DriveInfo *dinfo; > MemoryRegion *sysmem = get_system_memory(); > > @@ -177,20 +177,16 @@ static void ref405ep_init(MachineState *machine) > &error_fatal); > memory_region_add_subregion(sysmem, 0xFFF00000, sram); > /* allocate and load BIOS */ > - fl_idx = 0; > #ifdef USE_FLASH_BIOS > - dinfo = drive_get(IF_PFLASH, 0, fl_idx); > + dinfo = drive_get(IF_PFLASH, 0, 0); > if (dinfo) { > - BlockBackend *blk = blk_by_legacy_dinfo(dinfo); > - > - bios_size = blk_getlength(blk); > - fl_sectors = (bios_size + 65535) >> 16; > + bios_size = 8 * MiB; > pflash_cfi02_register((uint32_t)(-bios_size), > NULL, "ef405ep.bios", bios_size, > - blk, 65536, fl_sectors, 1, > + dinfo ? blk_by_legacy_dinfo(dinfo) : NULL, This code is inside the "if (dinfo)" condition, so testing again here whether it is NULL is unnecessary. > + 64 * KiB, bios_size / (64 * KiB), 1, > 2, 0x0001, 0x22DA, 0x0000, 0x0000, 0x555, 0x2AA, > 1); > - fl_idx++; > } else > #endif > { > @@ -425,7 +421,7 @@ static void taihu_405ep_init(MachineState *machine) > target_ulong kernel_base, initrd_base; > long kernel_size, initrd_size; > int linux_boot; > - int fl_idx, fl_sectors; > + int fl_idx; > DriveInfo *dinfo; > > /* RAM is soldered to the board so the size cannot be changed */ > @@ -450,15 +446,11 @@ static void taihu_405ep_init(MachineState *machine) > #if defined(USE_FLASH_BIOS) > dinfo = drive_get(IF_PFLASH, 0, fl_idx); > if (dinfo) { > - BlockBackend *blk = blk_by_legacy_dinfo(dinfo); > - > - bios_size = blk_getlength(blk); > - /* XXX: should check that size is 2MB */ > - // bios_size = 2 * 1024 * 1024; > - fl_sectors = (bios_size + 65535) >> 16; > - pflash_cfi02_register((uint32_t)(-bios_size), > + bios_size = 2 * MiB; > + pflash_cfi02_register(0xFFE00000, > NULL, "taihu_405ep.bios", bios_size, > - blk, 65536, fl_sectors, 1, > + dinfo ? blk_by_legacy_dinfo(dinfo) : NULL, Same here... > + 64 * KiB, bios_size / (64 * KiB), 1, > 4, 0x0001, 0x22DA, 0x0000, 0x0000, 0x555, 0x2AA, > 1); > fl_idx++; > @@ -491,14 +483,10 @@ static void taihu_405ep_init(MachineState *machine) > /* Register Linux flash */ > dinfo = drive_get(IF_PFLASH, 0, fl_idx); > if (dinfo) { > - BlockBackend *blk = blk_by_legacy_dinfo(dinfo); > - > - bios_size = blk_getlength(blk); > - /* XXX: should check that size is 32MB */ > bios_size = 32 * MiB; > - fl_sectors = (bios_size + 65535) >> 16; > pflash_cfi02_register(0xfc000000, NULL, "taihu_405ep.flash", bios_size, > - blk, 65536, fl_sectors, 1, > + dinfo ? blk_by_legacy_dinfo(dinfo) : NULL, ...and here. > + 64 * KiB, bios_size / (64 * KiB), 1, > 4, 0x0001, 0x22DA, 0x0000, 0x0000, 0x555, 0x2AA, > 1); > fl_idx++; Anybody feel like sending a patch? thanks -- PMM
Peter Maydell <peter.maydell@linaro.org> writes: > On Mon, 11 Mar 2019 at 22:10, Markus Armbruster <armbru@redhat.com> wrote: >> >> Machine "ref405ep" maps its flash memory at address 2^32 - image size. >> Image size is rounded up to the next multiple of 64KiB. Useless, >> because pflash_cfi02_realize() fails with "failed to read the initial >> flash content" unless the rounding is a no-op. >> >> If the image size exceeds 0x80000 Bytes, we overlap first SRAM, then >> other stuff. No idea how that would play out, but useful outcomes >> seem unlikely. >> >> Map the flash memory at fixed address 0xFFF80000 with size 512KiB, >> regardless of image size, to match the physical hardware. >> >> Machine "taihu" maps its boot flash memory similarly. The code even >> has a comment /* XXX: should check that size is 2MB */, followed by >> disabled code to adjust the size to 2MiB regardless of image size. >> >> Its code to map its application flash memory looks the same, except >> there the XXX comment asks for 32MiB, and the code to adjust the size >> isn't disabled. Note that pflash_cfi02_realize() fails with "failed >> to read the initial flash content" for images smaller than 32MiB. >> >> Map the boot flash memory at fixed address 0xFFE00000 with size 2MiB, >> to match the physical hardware. Delete dead code from application >> flash mapping, and simplify some. >> >> Cc: David Gibson <david@gibson.dropbear.id.au> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> Acked-by: David Gibson <david@gibson.dropbear.id.au> >> Reviewed-by: Alex Bennée <alex.bennee@linaro.org> >> Message-Id: <20190308094610.21210-9-armbru@redhat.com> >> --- >> hw/ppc/ppc405_boards.c | 36 ++++++++++++------------------------ >> 1 file changed, 12 insertions(+), 24 deletions(-) > > Hi; Coverity has just noticed a minor bug in this patch > (CID 1421917): [...] > Anybody feel like sending a patch? > > thanks > -- PMM Philippe just posted the obvious fix. [...]
diff --git a/hw/ppc/ppc405_boards.c b/hw/ppc/ppc405_boards.c index bb73d6d848..fe8e3cad12 100644 --- a/hw/ppc/ppc405_boards.c +++ b/hw/ppc/ppc405_boards.c @@ -156,7 +156,7 @@ static void ref405ep_init(MachineState *machine) target_ulong kernel_base, initrd_base; long kernel_size, initrd_size; int linux_boot; - int fl_idx, fl_sectors, len; + int len; DriveInfo *dinfo; MemoryRegion *sysmem = get_system_memory(); @@ -177,20 +177,16 @@ static void ref405ep_init(MachineState *machine) &error_fatal); memory_region_add_subregion(sysmem, 0xFFF00000, sram); /* allocate and load BIOS */ - fl_idx = 0; #ifdef USE_FLASH_BIOS - dinfo = drive_get(IF_PFLASH, 0, fl_idx); + dinfo = drive_get(IF_PFLASH, 0, 0); if (dinfo) { - BlockBackend *blk = blk_by_legacy_dinfo(dinfo); - - bios_size = blk_getlength(blk); - fl_sectors = (bios_size + 65535) >> 16; + bios_size = 8 * MiB; pflash_cfi02_register((uint32_t)(-bios_size), NULL, "ef405ep.bios", bios_size, - blk, 65536, fl_sectors, 1, + dinfo ? blk_by_legacy_dinfo(dinfo) : NULL, + 64 * KiB, bios_size / (64 * KiB), 1, 2, 0x0001, 0x22DA, 0x0000, 0x0000, 0x555, 0x2AA, 1); - fl_idx++; } else #endif { @@ -425,7 +421,7 @@ static void taihu_405ep_init(MachineState *machine) target_ulong kernel_base, initrd_base; long kernel_size, initrd_size; int linux_boot; - int fl_idx, fl_sectors; + int fl_idx; DriveInfo *dinfo; /* RAM is soldered to the board so the size cannot be changed */ @@ -450,15 +446,11 @@ static void taihu_405ep_init(MachineState *machine) #if defined(USE_FLASH_BIOS) dinfo = drive_get(IF_PFLASH, 0, fl_idx); if (dinfo) { - BlockBackend *blk = blk_by_legacy_dinfo(dinfo); - - bios_size = blk_getlength(blk); - /* XXX: should check that size is 2MB */ - // bios_size = 2 * 1024 * 1024; - fl_sectors = (bios_size + 65535) >> 16; - pflash_cfi02_register((uint32_t)(-bios_size), + bios_size = 2 * MiB; + pflash_cfi02_register(0xFFE00000, NULL, "taihu_405ep.bios", bios_size, - blk, 65536, fl_sectors, 1, + dinfo ? blk_by_legacy_dinfo(dinfo) : NULL, + 64 * KiB, bios_size / (64 * KiB), 1, 4, 0x0001, 0x22DA, 0x0000, 0x0000, 0x555, 0x2AA, 1); fl_idx++; @@ -491,14 +483,10 @@ static void taihu_405ep_init(MachineState *machine) /* Register Linux flash */ dinfo = drive_get(IF_PFLASH, 0, fl_idx); if (dinfo) { - BlockBackend *blk = blk_by_legacy_dinfo(dinfo); - - bios_size = blk_getlength(blk); - /* XXX: should check that size is 32MB */ bios_size = 32 * MiB; - fl_sectors = (bios_size + 65535) >> 16; pflash_cfi02_register(0xfc000000, NULL, "taihu_405ep.flash", bios_size, - blk, 65536, fl_sectors, 1, + dinfo ? blk_by_legacy_dinfo(dinfo) : NULL, + 64 * KiB, bios_size / (64 * KiB), 1, 4, 0x0001, 0x22DA, 0x0000, 0x0000, 0x555, 0x2AA, 1); fl_idx++;