| Message ID | 1424121788-24560-2-git-send-email-rkrcmar@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Mo, 2015-02-16 at 22:23 +0100, Radim Krčmář wrote: > Automatic shrinking of vram_size leads to a segfault, because other > variables depend on being smaller and don't get shrinked. --verbose please. Which other variables? > Implications of shrinking would make the code needlessly complicated; > assert instead. assert isn't an option. vram_size_mb may come from the user (via vga device properties), so we need a friendly error message here. Also the loop you are removing makes sure vram_size is a power of two, removing that is not correct too. cheers, Gerd
2015-02-17 09:00+0100, Gerd Hoffmann: > On Mo, 2015-02-16 at 22:23 +0100, Radim Krčmář wrote: > > Automatic shrinking of vram_size leads to a segfault, because other > > variables depend on being smaller and don't get shrinked. > > --verbose please. Which other variables? I'm sorry, at least rom->surface0_area_size. (It is sourced from qxl->vgamem_size.) rom->surface0_area_size shouldn't be bigger than qxl->vga.vram_size, because it accesses memory allocated by it. > > Implications of shrinking would make the code needlessly complicated; > > assert instead. > > assert isn't an option. vram_size_mb may come from the user (via vga > device properties), so we need a friendly error message here. Agreed, it would require a lot of code to do it though (in cover letter) ... what about just dropping [1/2]? :) ([2/2] does the same job) > Also the loop you are removing makes sure vram_size is a power of two, > removing that is not correct too. (True, the patch should have asserted that as well.)
On Di, 2015-02-17 at 11:29 +0100, Radim Krčmář wrote: > 2015-02-17 09:00+0100, Gerd Hoffmann: > > On Mo, 2015-02-16 at 22:23 +0100, Radim Krčmář wrote: > > > Automatic shrinking of vram_size leads to a segfault, because other > > > variables depend on being smaller and don't get shrinked. > > > > --verbose please. Which other variables? > > I'm sorry, at least rom->surface0_area_size. > (It is sourced from qxl->vgamem_size.) Which command line triggers it? In theory qxl_init_ramsize() *should* make sure this can't happen ... I'd like to find & fix the bug instead of plugging an assert into some random place. cheers, Gerd
2015-02-17 11:37+0100, Gerd Hoffmann: > On Di, 2015-02-17 at 11:29 +0100, Radim Krčmář wrote: > > 2015-02-17 09:00+0100, Gerd Hoffmann: > > > On Mo, 2015-02-16 at 22:23 +0100, Radim Krčmář wrote: > > > > Automatic shrinking of vram_size leads to a segfault, because other > > > > variables depend on being smaller and don't get shrinked. > > > > > > --verbose please. Which other variables? > > > > I'm sorry, at least rom->surface0_area_size. > > (It is sourced from qxl->vgamem_size.) > > Which command line triggers it? The important subset is: -vga qxl -global qxl-vga.vgamem_mb=512 The segfault can then be triggered by any operation that dirties the memory (pause for example). > In theory qxl_init_ramsize() *should* make sure this can't happen ... > > I'd like to find & fix the bug instead of plugging an assert into some > random place. The bug happened because the init code is ovewriting variables, which made the code unmanageable. I added an assert, so we would fix the callers. Upper layers should also have no idea that our limit is 256, so we would ideally return an error from vga_common_init() instead of silently mangling sizes.
diff --git a/hw/display/vga.c b/hw/display/vga.c index 9c62fbf48823..a09dd19a6042 100644 --- a/hw/display/vga.c +++ b/hw/display/vga.c @@ -2122,13 +2122,9 @@ void vga_common_init(VGACommonState *s, Object *obj, bool global_vmstate) expand4to8[i] = v; } - /* valid range: 1 MB -> 256 MB */ - s->vram_size = 1024 * 1024; - while (s->vram_size < (s->vram_size_mb << 20) && - s->vram_size < (256 << 20)) { - s->vram_size <<= 1; - } - s->vram_size_mb = s->vram_size >> 20; + assert(1 <= s->vram_size_mb && s->vram_size_mb <= 256); + + s->vram_size = s->vram_size_mb << 20; if (!s->vbe_size) { s->vbe_size = s->vram_size; }
Automatic shrinking of vram_size leads to a segfault, because other variables depend on being smaller and don't get shrinked. Implications of shrinking would make the code needlessly complicated; assert instead. Signed-off-by: Radim Krčmář <rkrcmar@redhat.com> --- hw/display/vga.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-)