| Message ID | 20200518155308.15851-8-f4bug@amsat.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | exec/memory: Enforce checking MemTxResult values | expand |
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
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 >
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); } if (rom->isrom) { /* rom needs to be written only once */
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(-)