[Bug 228755] libvgl under syscons causes system reboot (via SDL 1.2)

[Mahmoud Al-Qudsi]

On Tue, Jun 5, 2018 at 7:32 AM, Bruce Evans <brde at optusnet.com.au> wrote:
> This is the driver getting the check wrong for all frame buffer sizes that
> are not a multiple of PAGE_SIZE.  The frame buffer size is not a multiple
> of PAGE SIZE mainly for the interesting geometries 800x600x8 and
> 1920x1080x8.  I use this fix:
>
> Index: vesa.c
> ===================================================================
> --- vesa.c      (revision 334595)
> +++ vesa.c      (working copy)
> @@ -1642,7 +1642,7 @@
>             (adp->va_info.vi_flags & V_INFO_LINEAR) != 0) {
>                 /* va_window_size == va_buffer_size/vi_planes */
>                 /* XXX: is this correct? */
> -               if (offset > adp->va_window_size - PAGE_SIZE)
> +               if (offset > trunc_page(adp->va_window_size))
>                         return (-1);
>                 *paddr = adp->va_info.vi_buffer + offset;
>  #ifdef VM_MEMATTR_WRITE_COMBINING
>
> After fixing this, even 1920x1080x8 works.

I submitted a (different) patch [0] that we've been using in production since
2014 for that particular code multiple times, most recently in January of 2018
[1]. It's not a libvgl-specific issue and honestly deserves to be fixed
already.  With it, we've been able to use sc as a pretty much bulletproof X
target, with the caveat that it's insanely slow at higher (native) resolutions.

This particular issue is actually already tracked in kern/162373, the same
patch was submitted there by Mikhail Kupchik way back in November of 2011.

Since you seem to know what you're doing - is the comparison with
`trunc_page(adp->va_window_size)` here the way to go or the alternative
approach with `offset >= adp->va_info.vi_buffer_size` range checking instead?

Mahmoud Al-Qudsi
NeoSmart Technologies


[0]: https://github.com/neosmart/freebsd/commit/b3af2d1ed4a63fe1a5d771ed3a052242250098d7
[1]: http://freebsd.1045724.x6.nabble.com/PATCH-validated-against-50k-i686-machines-for-vmparam-h-and-vesa-c-td6235962.html