Re: [RFC PATCH v2 7/7] hw/core/loader: Assert loading ROM regions succeeds at reset

[Peter Maydell <peter.maydell@linaro.org>]

On Mon, 18 May 2020 at 16:53, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> If we are unable to load a blob in a ROM region, we should not
> ignore it and let the machine boot.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> RFC: Maybe more polite with user to use hw_error()?
> ---
>  hw/core/loader.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index 8bbb1797a4..4e046388b4 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -1146,8 +1146,12 @@ static void rom_reset(void *unused)
>              void *host = memory_region_get_ram_ptr(rom->mr);
>              memcpy(host, rom->data, rom->datasize);
>          } else {
> -            address_space_write_rom(rom->as, rom->addr, MEMTXATTRS_UNSPECIFIED,
> -                                    rom->data, rom->datasize);
> +            MemTxResult res;
> +
> +            res = address_space_write_rom(rom->as, rom->addr,
> +                                          MEMTXATTRS_UNSPECIFIED,
> +                                          rom->data, rom->datasize);
> +            assert(res == MEMTX_OK);

We shouln't assert(), because this is easy for a user to trigger
by loading an ELF file that's been linked to the wrong address.
Something helpful that ideally includes the name of the ELF file
and perhaps the address might be nice.

(But overall I'm a bit wary of this and other patches in the series
just because they add checks that were previously not there, and
I'm not sure whether users might be accidentally relying on
the continues-anyway behaviour.)

thanks
-- PMM