| Message ID | 706a1e5f87ae789197fba3a268b18183fd4b8e5b.1557431250.git-series.marmarek@invisiblethingslab.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fixes for large framebuffer, placed above 4GB | expand |
On 09/05/2019 21:48, Marek Marczykowski-Górecki wrote: > On some machines (for example Thinkpad P52), UEFI GOP reports > framebuffer located above 4GB (0x4000000000 on that machine). This > address does not fit in {xen,dom0}_vga_console_info.u.vesa_lfb.lfb_base > field, which is 32bit. The overflow here cause all kind of memory > corruption when anything tries to write something on the screen, > starting with zeroing the whole framebuffer in vesa_init(). > > Fix this similar to how it's done in Linux: add ext_lfb_base field at > the end of the structure, to hold upper 32bits of the address. Since the > field is added at the end of the structure, it will work with older > Linux versions too (other than using possibly truncated address - no > worse than without this change). Thanks to ABI containing size of the > structure (start_info.console.dom0.info_size), Linux can detect when > this field is present and use it appropriately then. > > Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> Public interface part: Reviewed-by: Juergen Gross <jgross@suse.com> Juergen
>>> On 09.05.19 at 21:48, <marmarek@invisiblethingslab.com> wrote: > --- a/xen/drivers/video/vesa.c > +++ b/xen/drivers/video/vesa.c > @@ -40,6 +40,11 @@ static int __init parse_font_height(const char *s) > } > custom_param("font", parse_font_height); > > +static inline paddr_t lfb_base(void) > +{ > + return (paddr_t)(vlfb_info.ext_lfb_base) << 32 | vlfb_info.lfb_base; This wants another set of parentheses around the operands of << for disambiguation purposes. We really only leave the school math operators (+, -, *, /, %, and often the relational ones when used in isolation) un-parenthesized, for it being (hopefully) known to everyone what their precedence is. > @@ -97,15 +102,15 @@ void __init vesa_init(void) > lfbp.text_columns = vlfb_info.width / font->width; > lfbp.text_rows = vlfb_info.height / font->height; > > - lfbp.lfb = lfb = ioremap(vlfb_info.lfb_base, vram_remap); > + lfbp.lfb = lfb = ioremap(lfb_base(), vram_remap); > if ( !lfb ) > return; > > memset(lfb, 0, vram_remap); > > - printk(XENLOG_INFO "vesafb: framebuffer at %#x, mapped to 0x%p, " > + printk(XENLOG_INFO "vesafb: framebuffer at %" PRIpaddr ", mapped to 0x%p, " This drops the 0x prefix from the logged line; it needs to be made explicit not in the format string. I think this would also be a good opportunity to un-wrap the format string. > --- a/xen/include/public/xen.h > +++ b/xen/include/public/xen.h > @@ -923,6 +923,10 @@ typedef struct dom0_vga_console_info { > /* Mode attributes (offset 0x0, VESA command 0x4f01). */ > uint16_t mode_attrs; > #endif > +#if __XEN_INTERFACE_VERSION__ >= 0x00040d00 || defined(__XEN__) The "defined(__XEN__)" is unnecessary for master, and should hence be dropped. I've suggested this for backporting only. There's also an unnamed padding field that you introduce. This should be made explicit, so it can be assigned a meaning later on (and in particular can be checked to be zero, if need be). Afaict the only instance of this structure type is a static variable, in which case there's no need to add explicit zero initialization anywhere. With all of them taken care of Reviewed-by: Jan Beulich <jbeulich@suse.com> Jan
diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h index 5789d2c..7a13a30 100644 --- a/xen/arch/x86/efi/efi-boot.h +++ b/xen/arch/x86/efi/efi-boot.h @@ -550,6 +550,7 @@ static void __init efi_arch_video_init(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop, vga_console_info.u.vesa_lfb.bytes_per_line = (mode_info->PixelsPerScanLine * bpp + 7) >> 3; vga_console_info.u.vesa_lfb.lfb_base = gop->Mode->FrameBufferBase; + vga_console_info.u.vesa_lfb.ext_lfb_base = gop->Mode->FrameBufferBase >> 32; vga_console_info.u.vesa_lfb.lfb_size = (gop->Mode->FrameBufferSize + 0xffff) >> 16; } diff --git a/xen/drivers/video/vesa.c b/xen/drivers/video/vesa.c index c92497e..f07d293 100644 --- a/xen/drivers/video/vesa.c +++ b/xen/drivers/video/vesa.c @@ -40,6 +40,11 @@ static int __init parse_font_height(const char *s) } custom_param("font", parse_font_height); +static inline paddr_t lfb_base(void) +{ + return (paddr_t)(vlfb_info.ext_lfb_base) << 32 | vlfb_info.lfb_base; +} + void __init vesa_early_init(void) { unsigned int vram_vmode; @@ -97,15 +102,15 @@ void __init vesa_init(void) lfbp.text_columns = vlfb_info.width / font->width; lfbp.text_rows = vlfb_info.height / font->height; - lfbp.lfb = lfb = ioremap(vlfb_info.lfb_base, vram_remap); + lfbp.lfb = lfb = ioremap(lfb_base(), vram_remap); if ( !lfb ) return; memset(lfb, 0, vram_remap); - printk(XENLOG_INFO "vesafb: framebuffer at %#x, mapped to 0x%p, " + printk(XENLOG_INFO "vesafb: framebuffer at %" PRIpaddr ", mapped to 0x%p, " "using %uk, total %uk\n", - vlfb_info.lfb_base, lfb, + lfb_base(), lfb, vram_remap >> 10, vram_total >> 10); printk(XENLOG_INFO "vesafb: mode is %dx%dx%u, linelength=%d, font %ux%u\n", vlfb_info.width, vlfb_info.height, @@ -167,7 +172,7 @@ void __init vesa_mtrr_init(void) /* Try and find a power of two to add */ do { - rc = mtrr_add(vlfb_info.lfb_base, size_total, type, 1); + rc = mtrr_add(lfb_base(), size_total, type, 1); size_total >>= 1; } while ( (size_total >= PAGE_SIZE) && (rc == -EINVAL) ); } diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h index ccdffc0..1752252 100644 --- a/xen/include/public/xen.h +++ b/xen/include/public/xen.h @@ -923,6 +923,10 @@ typedef struct dom0_vga_console_info { /* Mode attributes (offset 0x0, VESA command 0x4f01). */ uint16_t mode_attrs; #endif +#if __XEN_INTERFACE_VERSION__ >= 0x00040d00 || defined(__XEN__) + /* high 32 bits of lfb_base */ + uint32_t ext_lfb_base; +#endif } vesa_lfb; } u; } dom0_vga_console_info_t;
On some machines (for example Thinkpad P52), UEFI GOP reports framebuffer located above 4GB (0x4000000000 on that machine). This address does not fit in {xen,dom0}_vga_console_info.u.vesa_lfb.lfb_base field, which is 32bit. The overflow here cause all kind of memory corruption when anything tries to write something on the screen, starting with zeroing the whole framebuffer in vesa_init(). Fix this similar to how it's done in Linux: add ext_lfb_base field at the end of the structure, to hold upper 32bits of the address. Since the field is added at the end of the structure, it will work with older Linux versions too (other than using possibly truncated address - no worse than without this change). Thanks to ABI containing size of the structure (start_info.console.dom0.info_size), Linux can detect when this field is present and use it appropriately then. Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> --- Changes in v2: - guard ext_lfb_base with #if __XEN_INTERFACE_VERSION__, but always include whe building Xen itself - add a helper function for lfb_base --- xen/arch/x86/efi/efi-boot.h | 1 + xen/drivers/video/vesa.c | 13 +++++++++---- xen/include/public/xen.h | 4 ++++ 3 files changed, 14 insertions(+), 4 deletions(-)