Re: [Qemu-devel] [PATCH 05/10] ppc405_boards: Don't size flash memory to match backing image

[David Gibson <david@gibson.dropbear.id.au>]

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

On Thu, Feb 21, 2019 at 05:31:30PM +0100, Markus Armbruster wrote:
> Alex Bennée <alex.bennee@linaro.org> writes:
> 
> > Markus Armbruster <armbru@redhat.com> writes:
> >
> >> 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 a 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>
> >> ---
> >>  hw/ppc/ppc405_boards.c | 53 +++++++++++++-----------------------------
> >>  1 file changed, 16 insertions(+), 37 deletions(-)
> >>
> >> diff --git a/hw/ppc/ppc405_boards.c b/hw/ppc/ppc405_boards.c
> >> index f47b15f10e..728154aebb 100644
> >> --- a/hw/ppc/ppc405_boards.c
> >> +++ b/hw/ppc/ppc405_boards.c
> >> @@ -158,7 +158,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();
> >>
> >> @@ -185,26 +185,19 @@ static void ref405ep_init(MachineState *machine)
> >>  #ifdef DEBUG_BOARD_INIT
> >>      printf("%s: register BIOS\n", __func__);
> >>  #endif
> >> -    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;
> >>  #ifdef DEBUG_BOARD_INIT
> >> -        printf("Register parallel flash %d size %lx"
> >> -               " at addr %lx '%s' %d\n",
> >> -               fl_idx, bios_size, -bios_size,
> >> -               blk_name(blk), fl_sectors);
> >> +        printf("Register parallel flash\n");
> >>  #endif
> >> -        pflash_cfi02_register((uint32_t)(-bios_size),
> >> +        bios_size = 0x80000;
> >
> >  bios_size = 8 * MiB?
> 
> The next line has base address 0xFFF80000.  I picked 0x80000 to make
> 0xFFF80000 + 0x80000 == 0 mod 2^32 more obvious.
> 
> If I change 0x80000 to 8 * MiB, the size is more obvious, but "at end of
> 32 bit address space" less so.
> 
> If I additionally change the base address back to ((uint32_t)-bios_size,
> "at end of 32 bit address space" is obvious again, but the actual base
> address less so.

I have a weak preference for ((uint32_t)-bios_size), with bios_size =
8 * MiB.

> 
> I don't really care myself.  David, you're the maintainer, do you have a
> preference?
> 
> >> +        pflash_cfi02_register(0xFFF80000,
> >>                                NULL, "ef405ep.bios", bios_size,
> >> -                              blk, 65536, fl_sectors, 1,
> >> +                              dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
> >> +                              65536, bios_size / 65536, 1,
> >
> > 64 * KiB?
> 
> David, same question (two additional instances below).

Here I think 64 * KiB would be nice in each of those places.  Again,
only a weak preference.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]