lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Wed, 30 Jul 2014 17:24:08 +0300
From:	Tomi Valkeinen <tomi.valkeinen@...com>
To:	Dexuan Cui <decui@...rosoft.com>
CC:	<gregkh@...uxfoundation.org>, <dan.carpenter@...cle.com>,
	<linux-kernel@...r.kernel.org>,
	<driverdev-devel@...uxdriverproject.org>, <plagnioj@...osoft.com>,
	<linux-fbdev@...r.kernel.org>, <olaf@...fle.de>,
	<apw@...onical.com>, <jasowang@...hat.com>, <kys@...rosoft.com>,
	<haiyangz@...rosoft.com>
Subject: Re: [PATCH v3] video: hyperv: hyperv_fb: refresh the VM screen by
 force on VM panic

On 09/07/14 06:04, Dexuan Cui wrote:
> Currently the VSC has no chance to notify the VSP of the dirty rectangle on VM
> panic because the notification work is done in a workqueue, and in panic() the
> kernel typically ends up in an infinite loop, and a typical kernel config has
> CONFIG_PREEMPT_VOLUNTARY=y and CONFIG_PREEMPT is not set, so a context switch
> can't happen in panic() and the workqueue won't have a chance to run. As a
> result, the VM Connection window can't refresh until it's closed and we
> re-connect to the VM.
> 
> We can register a handler on panic_notifier_list: the handler can notify
> the VSC and switch the framebuffer driver to a "synchronous mode", meaning
> the VSC flushes any future framebuffer change to the VSP immediately.
> 
> v2: removed the MS-TFS line in the commit message
> v3: remove some 'unlikely' markings
> 
> Signed-off-by: Dexuan Cui <decui@...rosoft.com>
> Reviewed-by: Haiyang Zhang <haiyangz@...rosoft.com>
> ---
>  drivers/video/fbdev/hyperv_fb.c | 58 ++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 55 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/video/fbdev/hyperv_fb.c b/drivers/video/fbdev/hyperv_fb.c
> index e23392e..a7b98e1 100644
> --- a/drivers/video/fbdev/hyperv_fb.c
> +++ b/drivers/video/fbdev/hyperv_fb.c
> @@ -226,11 +226,16 @@ struct hvfb_par {
>  	u8 recv_buf[MAX_VMBUS_PKT_SIZE];
>  };
>  
> +static struct fb_info *hvfb_info;

Static variables like these are usually a no-no. This prevents you from
having multiple device instances.

>  static uint screen_width = HVFB_WIDTH;
>  static uint screen_height = HVFB_HEIGHT;
>  static uint screen_depth;
>  static uint screen_fb_size;
>  
> +/* If true, the VSC notifies the VSP on every framebuffer change */
> +static bool synchronous_fb;
> +

Same comment here.

However, if (and only if) the driver is already designed to work only
with single device instance, then this patch is probably ok. But even
then, I'd prefer this to be handled without static variables so that the
driver could eventually be changed to support multiple device instances.

 Tomi



Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)

Powered by blists - more mailing lists