| Message ID | 1489656642-12925-5-git-send-email-kraxel@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hello Gerd, > -----Original Message----- > From: Qemu-devel > [mailto:qemu-devel-bounces+liqiang6-s=360.cn@nongnu.org] On Behalf Of > Gerd Hoffmann > Sent: Thursday, March 16, 2017 5:31 PM > To: qemu-devel@nongnu.org > Cc: Gerd Hoffmann > Subject: [Qemu-devel] [PULL for-2.9 4/7] cirrus: add option to disable blitter > > Ok, we have this beast in the cirrus code which is not used at all by modern > guests, except when you try to find security holes in qemu. So, add an option > to disable blitter altogether. Guests released within the last ten years should > not show any rendering issues if you turn off blitter support. > > There are no known bugs in the cirrus blitter code. But in the past we hoped a > few times already that we've finally nailed the last issue. So having some easy > way to mitigate in case yet another blitter issue shows up certainly makes me > sleep a bit better at night. > > For completeness: The by far better way to mitigate is to switch away from > cirrus and use stdvga instead. Or something more modern like virtio-vga in > case your guest has support for it. > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> > Message-id: 1489494540-15745-1-git-send-email-kraxel@redhat.com > --- > hw/display/cirrus_vga.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c index > 6ffe64f..326d511 100644 > --- a/hw/display/cirrus_vga.c > +++ b/hw/display/cirrus_vga.c > @@ -205,6 +205,7 @@ typedef struct CirrusVGAState { > uint32_t cirrus_bank_base[2]; > uint32_t cirrus_bank_limit[2]; > uint8_t cirrus_hidden_palette[48]; > + bool enable_blitter; > int cirrus_blt_pixelwidth; > int cirrus_blt_width; > int cirrus_blt_height; > @@ -960,6 +961,10 @@ static void cirrus_bitblt_start(CirrusVGAState * s) { > uint8_t blt_rop; > > + if (!s->enable_blitter) { > + goto bitblt_ignore; > + } > + > s->vga.gr[0x31] |= CIRRUS_BLT_BUSY; > > s->cirrus_blt_width = (s->vga.gr[0x20] | (s->vga.gr[0x21] << 8)) + 1; @@ > -3024,6 +3029,8 @@ static void isa_cirrus_vga_realizefn(DeviceState *dev, > Error **errp) static Property isa_cirrus_vga_properties[] = { > DEFINE_PROP_UINT32("vgamem_mb", struct ISACirrusVGAState, > cirrus_vga.vga.vram_size_mb, 4), > + DEFINE_PROP_BOOL("blitter", struct ISACirrusVGAState, > + cirrus_vga.enable_blitter, true), > DEFINE_PROP_END_OF_LIST(), > }; > > @@ -3093,6 +3100,8 @@ static void pci_cirrus_vga_realize(PCIDevice *dev, > Error **errp) static Property pci_vga_cirrus_properties[] = { > DEFINE_PROP_UINT32("vgamem_mb", struct PCICirrusVGAState, > cirrus_vga.vga.vram_size_mb, 4), > + DEFINE_PROP_BOOL("blitter", struct PCICirrusVGAState, > + cirrus_vga.enable_blitter, true), The default is 'ENABLE'? I think there should be 'false'. Thanks. Qiang > DEFINE_PROP_END_OF_LIST(), > }; > > -- > 1.8.3.1 >
On 16.03.2017 10:51, 李强 wrote: > Hello Gerd, > >> -----Original Message----- >> From: Qemu-devel >> [mailto:qemu-devel-bounces+liqiang6-s=360.cn@nongnu.org] On Behalf Of >> Gerd Hoffmann >> Sent: Thursday, March 16, 2017 5:31 PM >> To: qemu-devel@nongnu.org >> Cc: Gerd Hoffmann >> Subject: [Qemu-devel] [PULL for-2.9 4/7] cirrus: add option to disable blitter >> >> Ok, we have this beast in the cirrus code which is not used at all by modern >> guests, except when you try to find security holes in qemu. So, add an option >> to disable blitter altogether. Guests released within the last ten years should >> not show any rendering issues if you turn off blitter support. >> >> There are no known bugs in the cirrus blitter code. But in the past we hoped a >> few times already that we've finally nailed the last issue. So having some easy >> way to mitigate in case yet another blitter issue shows up certainly makes me >> sleep a bit better at night. >> >> For completeness: The by far better way to mitigate is to switch away from >> cirrus and use stdvga instead. Or something more modern like virtio-vga in >> case your guest has support for it. >> >> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> >> Message-id: 1489494540-15745-1-git-send-email-kraxel@redhat.com >> --- >> hw/display/cirrus_vga.c | 9 +++++++++ >> 1 file changed, 9 insertions(+) >> >> diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c index >> 6ffe64f..326d511 100644 >> --- a/hw/display/cirrus_vga.c >> +++ b/hw/display/cirrus_vga.c >> @@ -205,6 +205,7 @@ typedef struct CirrusVGAState { >> uint32_t cirrus_bank_base[2]; >> uint32_t cirrus_bank_limit[2]; >> uint8_t cirrus_hidden_palette[48]; >> + bool enable_blitter; >> int cirrus_blt_pixelwidth; >> int cirrus_blt_width; >> int cirrus_blt_height; >> @@ -960,6 +961,10 @@ static void cirrus_bitblt_start(CirrusVGAState * s) { >> uint8_t blt_rop; >> >> + if (!s->enable_blitter) { >> + goto bitblt_ignore; >> + } >> + >> s->vga.gr[0x31] |= CIRRUS_BLT_BUSY; >> >> s->cirrus_blt_width = (s->vga.gr[0x20] | (s->vga.gr[0x21] << 8)) + 1; @@ >> -3024,6 +3029,8 @@ static void isa_cirrus_vga_realizefn(DeviceState *dev, >> Error **errp) static Property isa_cirrus_vga_properties[] = { >> DEFINE_PROP_UINT32("vgamem_mb", struct ISACirrusVGAState, >> cirrus_vga.vga.vram_size_mb, 4), >> + DEFINE_PROP_BOOL("blitter", struct ISACirrusVGAState, >> + cirrus_vga.enable_blitter, true), >> DEFINE_PROP_END_OF_LIST(), >> }; >> >> @@ -3093,6 +3100,8 @@ static void pci_cirrus_vga_realize(PCIDevice *dev, >> Error **errp) static Property pci_vga_cirrus_properties[] = { >> DEFINE_PROP_UINT32("vgamem_mb", struct PCICirrusVGAState, >> cirrus_vga.vga.vram_size_mb, 4), >> + DEFINE_PROP_BOOL("blitter", struct PCICirrusVGAState, >> + cirrus_vga.enable_blitter, true), > > The default is 'ENABLE'? I think there should be 'false'. I think it has to be enabled at least for the older machine types - otherwise you change the hardware of guests during migration. Thomas
Hi, > > + DEFINE_PROP_BOOL("blitter", struct PCICirrusVGAState, > > > + cirrus_vga.enable_blitter, true), > > > The default is 'ENABLE'? Yes. > I think there should be 'false'. Just flipping it to false isn't an option. At minimum we would have to keep it enabled by default for older machine types, for backward compatibility with older qemu versions. Beside that I don't want rush it. When flipping the default I'd do it early in the 2.10 development cycle, so we have a few months to the next release to see what the fallout is and decide which default we want. But I think the most sensible approach would be to have upper management layer handle this. We have libosinfo, which records capabilities of guests, and we could add a field there telling whenever the guest os in question uses the blitter (and therefore breaks when we turn it off) or whenever it is safe to turn it off. cheers, Gerd
diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c index 6ffe64f..326d511 100644 --- a/hw/display/cirrus_vga.c +++ b/hw/display/cirrus_vga.c @@ -205,6 +205,7 @@ typedef struct CirrusVGAState { uint32_t cirrus_bank_base[2]; uint32_t cirrus_bank_limit[2]; uint8_t cirrus_hidden_palette[48]; + bool enable_blitter; int cirrus_blt_pixelwidth; int cirrus_blt_width; int cirrus_blt_height; @@ -960,6 +961,10 @@ static void cirrus_bitblt_start(CirrusVGAState * s) { uint8_t blt_rop; + if (!s->enable_blitter) { + goto bitblt_ignore; + } + s->vga.gr[0x31] |= CIRRUS_BLT_BUSY; s->cirrus_blt_width = (s->vga.gr[0x20] | (s->vga.gr[0x21] << 8)) + 1; @@ -3024,6 +3029,8 @@ static void isa_cirrus_vga_realizefn(DeviceState *dev, Error **errp) static Property isa_cirrus_vga_properties[] = { DEFINE_PROP_UINT32("vgamem_mb", struct ISACirrusVGAState, cirrus_vga.vga.vram_size_mb, 4), + DEFINE_PROP_BOOL("blitter", struct ISACirrusVGAState, + cirrus_vga.enable_blitter, true), DEFINE_PROP_END_OF_LIST(), }; @@ -3093,6 +3100,8 @@ static void pci_cirrus_vga_realize(PCIDevice *dev, Error **errp) static Property pci_vga_cirrus_properties[] = { DEFINE_PROP_UINT32("vgamem_mb", struct PCICirrusVGAState, cirrus_vga.vga.vram_size_mb, 4), + DEFINE_PROP_BOOL("blitter", struct PCICirrusVGAState, + cirrus_vga.enable_blitter, true), DEFINE_PROP_END_OF_LIST(), };
Ok, we have this beast in the cirrus code which is not used at all by modern guests, except when you try to find security holes in qemu. So, add an option to disable blitter altogether. Guests released within the last ten years should not show any rendering issues if you turn off blitter support. There are no known bugs in the cirrus blitter code. But in the past we hoped a few times already that we've finally nailed the last issue. So having some easy way to mitigate in case yet another blitter issue shows up certainly makes me sleep a bit better at night. For completeness: The by far better way to mitigate is to switch away from cirrus and use stdvga instead. Or something more modern like virtio-vga in case your guest has support for it. Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> Message-id: 1489494540-15745-1-git-send-email-kraxel@redhat.com --- hw/display/cirrus_vga.c | 9 +++++++++ 1 file changed, 9 insertions(+)