Re: [Qemu-devel] [PULL for-2.9 4/7] cirrus: add option to disable blitter

[Thomas Huth]

On 16.03.2017 10:51, 李强 wrote:
> Hello Gerd,
> 
>> -----Original Message-----
>> From: Qemu-devel
>> [mailto:address@hidden On Behalf Of
>> Gerd Hoffmann
>> Sent: Thursday, March 16, 2017 5:31 PM
>> To: address@hidden
>> 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 <address@hidden>
>> Message-id: address@hidden
>> ---
>>  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