[Qemu-devel] [PATCH] cirrus_vga: adding sanity check for vram size

[Qemu-devel] [PATCH] cirrus_vga: adding sanity check for vram size

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

when configure a invalid vram size for cirrus card, such as less
2 MB, which will crash qemu. Follow the real hardware, the cirrus
card has 4 MB video memory. Also for backward compatibility, accept
8 MB and 16 MB vram size.

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
For isa cirrus vga device, its' init function has been droped at
commit db895a1e6a97e919f9b86d60c969377357b05066. I have no idea how adding
check on isa_cirrus_vga device. Any ideas? Thanks.
 
 hw/display/cirrus_vga.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c
index d1afc76..5fec068 100644
--- a/hw/display/cirrus_vga.c
+++ b/hw/display/cirrus_vga.c
@@ -2959,6 +2959,13 @@ static int pci_cirrus_vga_initfn(PCIDevice *dev)
      PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(dev);
      int16_t device_id = pc->device_id;
 
+     /* follow real hardware, cirrus card emulated has 4 MB video memory.
+       Also accept 8 MB/16 MB for backward compatibility. */
+     if (s->vga.vram_size_mb != 4 || s->vga.vram_size_mb != 8 ||
+         s->vga.vram_size_mb != 16) {
+         error_report("Invalid cirrus_vga ram size '%u'\n", s->vga.vram_size_mb);
+         return -1;
+     }
      /* setup VGA */
      vga_common_init(&s->vga, OBJECT(dev), true);
      cirrus_init_common(s, OBJECT(dev), device_id, 1, pci_address_space(dev),
-- 
1.7.12.4

Re: [Qemu-devel] [PATCH] cirrus_vga: adding sanity check for vram size [checkpatch false positive?]

[Gerd Hoffmann]

On Fr, 2014-05-09 at 18:21 +0800, arei.gonglei@huawei.com wrote:
> From: Gonglei <arei.gonglei@huawei.com>
> 
> when configure a invalid vram size for cirrus card, such as less
> 2 MB, which will crash qemu. Follow the real hardware, the cirrus
> card has 4 MB video memory. Also for backward compatibility, accept
> 8 MB and 16 MB vram size.

Fails checkpatch:

=== checkpatch complains ===
WARNING: suspect code indent for conditional statements (5, 9)
#12: FILE: hw/display/cirrus_vga.c:2964:
+     if (s->vga.vram_size_mb != 4 || s->vga.vram_size_mb != 8 ||
[...]
+         error_report("Invalid cirrus_vga ram size '%u'\n",
s->vga.vram_size_mb);

WARNING: line over 80 characters
#14: FILE: hw/display/cirrus_vga.c:2966:
+         error_report("Invalid cirrus_vga ram size '%u'\n",
s->vga.vram_size_mb);

total: 0 errors, 2 warnings, 13 lines checked

First warning looks like a false positive though.

MAINTAINERS lists blue swirl as checkpatch maintainer, Cc'ing.
Havn't seen him on the list for quite a while though, is that still
up-to-date?

cheers,
  Gerd

> 
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> ---
> For isa cirrus vga device, its' init function has been droped at
> commit db895a1e6a97e919f9b86d60c969377357b05066. I have no idea how adding
> check on isa_cirrus_vga device. Any ideas? Thanks.
>  
>  hw/display/cirrus_vga.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c
> index d1afc76..5fec068 100644
> --- a/hw/display/cirrus_vga.c
> +++ b/hw/display/cirrus_vga.c
> @@ -2959,6 +2959,13 @@ static int pci_cirrus_vga_initfn(PCIDevice *dev)
>       PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(dev);
>       int16_t device_id = pc->device_id;
>  
> +     /* follow real hardware, cirrus card emulated has 4 MB video memory.
> +       Also accept 8 MB/16 MB for backward compatibility. */
> +     if (s->vga.vram_size_mb != 4 || s->vga.vram_size_mb != 8 ||
> +         s->vga.vram_size_mb != 16) {
> +         error_report("Invalid cirrus_vga ram size '%u'\n", s->vga.vram_size_mb);
> +         return -1;
> +     }
>       /* setup VGA */
>       vga_common_init(&s->vga, OBJECT(dev), true);
>       cirrus_init_common(s, OBJECT(dev), device_id, 1, pci_address_space(dev),

Re: [Qemu-devel] [PATCH] cirrus_vga: adding sanity check for vram size [checkpatch false positive?]

[Gonglei (Arei)]

Hi,

> -----Original Message-----
> From: Gerd Hoffmann [mailto:kraxel@redhat.com]
> Sent: Friday, May 09, 2014 6:31 PM
> To: Gonglei (Arei)
> Cc: qemu-devel@nongnu.org; afaerber@suse.de; mst@redhat.com;
> pbonzini@redhat.com; Huangweidong (C); Blue Swirl
> Subject: Re: [PATCH] cirrus_vga: adding sanity check for vram size [checkpatch
> false positive?]
> 
> On Fr, 2014-05-09 at 18:21 +0800, arei.gonglei@huawei.com wrote:
> > From: Gonglei <arei.gonglei@huawei.com>
> >
> > when configure a invalid vram size for cirrus card, such as less
> > 2 MB, which will crash qemu. Follow the real hardware, the cirrus
> > card has 4 MB video memory. Also for backward compatibility, accept
> > 8 MB and 16 MB vram size.
> 
> Fails checkpatch:
> 
> === checkpatch complains ===
> WARNING: suspect code indent for conditional statements (5, 9)
> #12: FILE: hw/display/cirrus_vga.c:2964:
> +     if (s->vga.vram_size_mb != 4 || s->vga.vram_size_mb != 8 ||
> [...]
> +         error_report("Invalid cirrus_vga ram size '%u'\n",
> s->vga.vram_size_mb);
> 
> WARNING: line over 80 characters
> #14: FILE: hw/display/cirrus_vga.c:2966:
> +         error_report("Invalid cirrus_vga ram size '%u'\n",
> s->vga.vram_size_mb);
> 
> total: 0 errors, 2 warnings, 13 lines checked
> 
Sorry for my negligence. I will post another patch.

BTW, what's your opinion about isa cirrus vga device, Gerd? 


Best regards,
-Gonglei

Re: [Qemu-devel] [PATCH] cirrus_vga: adding sanity check for vram size [checkpatch false positive?]

[Gerd Hoffmann]

  Hi,

> BTW, what's your opinion about isa cirrus vga device, Gerd? 

I'd do the same check there.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH] cirrus_vga: adding sanity check for vram size [checkpatch false positive?]

[Gonglei (Arei)]

> -----Original Message-----
> From: Gerd Hoffmann [mailto:kraxel@redhat.com]
> Sent: Friday, May 09, 2014 6:55 PM
> To: Gonglei (Arei)
> Cc: qemu-devel@nongnu.org; afaerber@suse.de; mst@redhat.com;
> pbonzini@redhat.com; Huangweidong (C); Blue Swirl
> Subject: Re: [PATCH] cirrus_vga: adding sanity check for vram size [checkpatch
> false positive?]
> 
>   Hi,
> 
> > BTW, what's your opinion about isa cirrus vga device, Gerd?
> 
> I'd do the same check there.
> 
But isa_cirrus_vga_realizefn() has no return value for judgment.
Adding the isa init function back?

Best regards,
-Gonglei

Re: [Qemu-devel] [PATCH] cirrus_vga: adding sanity check for vram size

[Andreas Färber]

Am 09.05.2014 12:59, schrieb Gonglei (Arei):
>> -----Original Message-----
>> From: Gerd Hoffmann [mailto:kraxel@redhat.com]
>> Sent: Friday, May 09, 2014 6:55 PM
>> To: Gonglei (Arei)
>> Cc: qemu-devel@nongnu.org; afaerber@suse.de; mst@redhat.com;
>> pbonzini@redhat.com; Huangweidong (C); Blue Swirl
>> Subject: Re: [PATCH] cirrus_vga: adding sanity check for vram size [checkpatch
>> false positive?]
>>
>>   Hi,
>>
>>> BTW, what's your opinion about isa cirrus vga device, Gerd?
>>
>> I'd do the same check there.
>>
> But isa_cirrus_vga_realizefn() has no return value for judgment.
> Adding the isa init function back?

No, realizefn is the replacement for the old initfns. Adding qtests for
verifying that things still work is what's holding up the conversion of
PCI devices.

You need to set *errp via error_setg(errp, "..."); instead of
error_report() and then just return.

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH] cirrus_vga: adding sanity check for vram size

[Dr. David Alan Gilbert]

* arei.gonglei@huawei.com (arei.gonglei@huawei.com) wrote:
> From: Gonglei <arei.gonglei@huawei.com>
> 
> when configure a invalid vram size for cirrus card, such as less
> 2 MB, which will crash qemu. Follow the real hardware, the cirrus
> card has 4 MB video memory. Also for backward compatibility, accept
> 8 MB and 16 MB vram size.

virt-manager/libvirt seems to default to 9 MByte of Vram for cirrus,
so this would break a lot of setups.

Looking at datasheets on the web seems to say the chips actually went
down to 1 MB or less.

I think before doing this change, it would be good to understand where
the weird 9MB in libvirt/virt-manager came from, and what the limits of
the emulator/drivers are.

Also, is there something broken at the moment - why make the change?

Dave

> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> ---
> For isa cirrus vga device, its' init function has been droped at
> commit db895a1e6a97e919f9b86d60c969377357b05066. I have no idea how adding
> check on isa_cirrus_vga device. Any ideas? Thanks.
>  
>  hw/display/cirrus_vga.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c
> index d1afc76..5fec068 100644
> --- a/hw/display/cirrus_vga.c
> +++ b/hw/display/cirrus_vga.c
> @@ -2959,6 +2959,13 @@ static int pci_cirrus_vga_initfn(PCIDevice *dev)
>       PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(dev);
>       int16_t device_id = pc->device_id;
>  
> +     /* follow real hardware, cirrus card emulated has 4 MB video memory.
> +       Also accept 8 MB/16 MB for backward compatibility. */
> +     if (s->vga.vram_size_mb != 4 || s->vga.vram_size_mb != 8 ||
> +         s->vga.vram_size_mb != 16) {
> +         error_report("Invalid cirrus_vga ram size '%u'\n", s->vga.vram_size_mb);
> +         return -1;
> +     }
>       /* setup VGA */
>       vga_common_init(&s->vga, OBJECT(dev), true);
>       cirrus_init_common(s, OBJECT(dev), device_id, 1, pci_address_space(dev),
> -- 
> 1.7.12.4
> 
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH] cirrus_vga: adding sanity check for vram size

[Paolo Bonzini]

Il 09/05/2014 13:18, Dr. David Alan Gilbert ha scritto:
> * arei.gonglei@huawei.com (arei.gonglei@huawei.com) wrote:
>> From: Gonglei <arei.gonglei@huawei.com>
>>
>> when configure a invalid vram size for cirrus card, such as less
>> 2 MB, which will crash qemu. Follow the real hardware, the cirrus
>> card has 4 MB video memory. Also for backward compatibility, accept
>> 8 MB and 16 MB vram size.
>
> virt-manager/libvirt seems to default to 9 MByte of Vram for cirrus,
> so this would break a lot of setups.

libvirt is broken and doesn't use the vram property for anything but QXL.

Paolo

Re: [Qemu-devel] [PATCH] cirrus_vga: adding sanity check for vram size

[Gerd Hoffmann]

  Hi,

> virt-manager/libvirt seems to default to 9 MByte of Vram for cirrus,
> so this would break a lot of setups.

It wouldn't.  libvirt sticks that into the xml, but it doesn't set any
qemu parameters.  The libvirt parameter actually predates the qemu
property for setting the size.

> Looking at datasheets on the web seems to say the chips actually went
> down to 1 MB or less.

I have my doubts we emulate that correctly (register telling the guest
how much memory is actually there etc.).  Also it is pretty much useless
these days, even the 4MB imply serious constrains when FullHD displays
are commonplace.  Newer cirrus drivers such as the kernel's drm driver
are specifically written to qemu's cirrus cards, I have my doubs that
they are prepared to handle 1MB cirrus cards correctly.

Bottom line: Allowing less than 4MB is asking for trouble for no good
reason ;)

cheers,
  Gerd

Re: [Qemu-devel] [PATCH] cirrus_vga: adding sanity check for vram size

[Dr. David Alan Gilbert]

* Gerd Hoffmann (kraxel@redhat.com) wrote:
>   Hi,
> 
> > virt-manager/libvirt seems to default to 9 MByte of Vram for cirrus,
> > so this would break a lot of setups.
> 
> It wouldn't.  libvirt sticks that into the xml, but it doesn't set any
> qemu parameters.  The libvirt parameter actually predates the qemu
> property for setting the size.

Yeuch messy.

> > Looking at datasheets on the web seems to say the chips actually went
> > down to 1 MB or less.
> 
> I have my doubts we emulate that correctly (register telling the guest
> how much memory is actually there etc.).  Also it is pretty much useless
> these days, even the 4MB imply serious constrains when FullHD displays
> are commonplace.  Newer cirrus drivers such as the kernel's drm driver
> are specifically written to qemu's cirrus cards, I have my doubs that
> they are prepared to handle 1MB cirrus cards correctly.
> 
> Bottom line: Allowing less than 4MB is asking for trouble for no good
> reason ;)

OK, so checking for 4MB/8MB/16MB is probably safe, and it also would
have the benefit of shouting if someone fixed libvirt and it started
trying to configure a 9MB version.

Dave
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH] cirrus_vga: adding sanity check for vram size

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1341 bytes --]

[adding libvirt]

On 05/09/2014 05:54 AM, Gerd Hoffmann wrote:
>   Hi,
> 
>> virt-manager/libvirt seems to default to 9 MByte of Vram for cirrus,
>> so this would break a lot of setups.
> 
> It wouldn't.  libvirt sticks that into the xml, but it doesn't set any
> qemu parameters.  The libvirt parameter actually predates the qemu
> property for setting the size.
> 

Then we should probably re-evaluate what libvirt does with the
parameters, which avoids breaking any guest that happens to be
pre-existing with the odd 9MB sizing in the XML.

>> Looking at datasheets on the web seems to say the chips actually went
>> down to 1 MB or less.
> 
> I have my doubts we emulate that correctly (register telling the guest
> how much memory is actually there etc.).  Also it is pretty much useless
> these days, even the 4MB imply serious constrains when FullHD displays
> are commonplace.  Newer cirrus drivers such as the kernel's drm driver
> are specifically written to qemu's cirrus cards, I have my doubs that
> they are prepared to handle 1MB cirrus cards correctly.
> 
> Bottom line: Allowing less than 4MB is asking for trouble for no good
> reason ;)
> 
> cheers,
>   Gerd
> 
> 
> 
> 
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH] cirrus_vga: adding sanity check for vram size

[Andreas Färber]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Am 12.05.2014 19:05, schrieb Eric Blake:
> [adding libvirt]
> 
> On 05/09/2014 05:54 AM, Gerd Hoffmann wrote:
>> Hi,
>> 
>>> virt-manager/libvirt seems to default to 9 MByte of Vram for
>>> cirrus, so this would break a lot of setups.
>> 
>> It wouldn't.  libvirt sticks that into the xml, but it doesn't
>> set any qemu parameters.  The libvirt parameter actually predates
>> the qemu property for setting the size.

Déjà vu... we had the same problem with pvpanic.

> Then we should probably re-evaluate what libvirt does with the 
> parameters, which avoids breaking any guest that happens to be 
> pre-existing with the odd 9MB sizing in the XML.

If libvirt cannot or doesn't want to pass a user-supplied tag to QEMU,
the only sane and future-proof solution is for libvirt to signal an
error to the user for *any* flags it cannot handle rather than
throwing up on a case-by-case basis the question of how libvirt should
behave when some previously ignored feature gets implemented. That
would prevent us from running into this situation again and again.

As further examples, apic, acpi and other x86-centric features got
auto-added for s390x guests last time I checked. Apart from anything
the user might have specified herself, there's really no reason to add
stuff the user did not ask for and that are not applicable.

Ignoring a value "9" specified by the user would be very odd; and I
guess you have no way of telling who added what to the XML for
choosing between dropping and error'ing out.

Regards,
Andreas

- -- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)

iQIcBAEBAgAGBQJTcQqiAAoJEPou0S0+fgE/CY8P/jRgyfqNtv2MLbilNXKKXe5A
CBftFQv1dlvL4VFVGD9W5ortlyOpPfxSmC6fgnLKNbTsTCtDy1k3zgWp8uSjYNrp
/W5oMpOstYxUBA7LtizCnCyPisffV6FlWQBLt8BGnRBPjEyVQXIIDHQuwvYv6Byr
EKjCGWMjw76Rfe84Hdl7xDQvfd+qTFjIDbKEfSa3fciJ/svCYKlnTqsPzvtNWbUg
907r5tI2LVT17xaclhex12GQ+uxV4hXcpcUnY3W/lCHvH98NdgFZRf1P8xTdjSmB
MbQ6WKOtGK9SqoyPiPzm8NwPduBbbmC69vumEmaG6QCqvTmcEKPNB5WMcXSEo4YB
SBJjZrkeNgS7XlUDwL4i45PYSbxi60CByrIrvYzWEielPCUQWMKzckfs+Zxmu0FT
nO51q334YmHU1X/oEPDnF8hakwzu9figWJ43uXSP/dm3ifjhFh35/Owmz+pd84Zi
cauzlygF0JAHGKurStveIqOZ0o6AJLxhWZP695PbKHAeHvOXo6dSsnLgG6POylP1
N/pTCcvQZV6oZ/NjscHcrmYxwrA4XWSzY8JdzS4iPSqzvCgxFNR/wZ1VTjCZ5Cbx
zs+bv+B7JKhjrYs/7dQt8kPv5ZXwh/kEkxkhXX7xpBfdPixx173Gdg3XvS86G6GI
q/dszTpb4kJNcsaxNp3y
=RZ98
-----END PGP SIGNATURE-----

Re: [Qemu-devel] [PATCH] cirrus_vga: adding sanity check for vram size

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1176 bytes --]

On 05/09/2014 05:18 AM, Dr. David Alan Gilbert wrote:
> * arei.gonglei@huawei.com (arei.gonglei@huawei.com) wrote:
>> From: Gonglei <arei.gonglei@huawei.com>
>>
>> when configure a invalid vram size for cirrus card, such as less
>> 2 MB, which will crash qemu. Follow the real hardware, the cirrus
>> card has 4 MB video memory. Also for backward compatibility, accept
>> 8 MB and 16 MB vram size.
> 
> virt-manager/libvirt seems to default to 9 MByte of Vram for cirrus,
> so this would break a lot of setups.

I think it was virt-manager that made the mistake, but it is indeed a
historical wart that we are now stuck with (unless you argue that taking
the user's request and silently rounding up to the next power of 2 will
not be a guest-visible change).

> 
> Looking at datasheets on the web seems to say the chips actually went
> down to 1 MB or less.
> 
> I think before doing this change, it would be good to understand where
> the weird 9MB in libvirt/virt-manager came from, and what the limits of
> the emulator/drivers are.
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]