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

["Philippe Mathieu-Daudé" <f4bug@amsat.org>]

On 5/18/20 6:12 PM, Peter Maydell wrote:
> 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.)

I understand. Thanks for reviewing, I'll rework this one and the 
previous set_kernel_args().

> 
> thanks
> -- PMM
>