| Message ID | 20250320151109.88228-1-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] x86/vga: fix mapping of the VGA text buffer | expand |
On 20.03.2025 16:11, Roger Pau Monne wrote: > The call to ioremap_wc() in video_init() will always fail, because > video_init() is called ahead of vm_init_type(), and so the underlying > __vmap() call will fail to allocate the linear address space. > > Fix by reverting to the previous behavior and use __va() for the VGA text > buffer, as it's below the 1MB boundary, and thus always mapped in the > directmap. > > Fixes: 81d195c6c0e2 ('x86: introduce ioremap_wc()') > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> Reviewed-by: Jan Beulich <jbeulich@suse.com> Nevertheless a remark: > --- a/xen/drivers/video/vga.c > +++ b/xen/drivers/video/vga.c > @@ -71,7 +71,7 @@ void __init video_init(void) > { > case XEN_VGATYPE_TEXT_MODE_3: > if ( page_is_ram_type(paddr_to_pfn(0xB8000), RAM_TYPE_CONVENTIONAL) || > - ((video = ioremap_wc(0xB8000, 0x8000)) == NULL) ) > + ((video = __va(0xB8000)) == NULL) ) Without having a good suggestion, find such dependencies upon the low 1Mb always being mapped (in case we wanted to revisit this, for example) is going to be moderately hard. It might be good to somehow mark them. Jan
On Thu, Mar 20, 2025 at 04:25:25PM +0100, Jan Beulich wrote: > On 20.03.2025 16:11, Roger Pau Monne wrote: > > The call to ioremap_wc() in video_init() will always fail, because > > video_init() is called ahead of vm_init_type(), and so the underlying > > __vmap() call will fail to allocate the linear address space. > > > > Fix by reverting to the previous behavior and use __va() for the VGA text > > buffer, as it's below the 1MB boundary, and thus always mapped in the > > directmap. > > > > Fixes: 81d195c6c0e2 ('x86: introduce ioremap_wc()') > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > > Reviewed-by: Jan Beulich <jbeulich@suse.com> Thanks. > Nevertheless a remark: > > > --- a/xen/drivers/video/vga.c > > +++ b/xen/drivers/video/vga.c > > @@ -71,7 +71,7 @@ void __init video_init(void) > > { > > case XEN_VGATYPE_TEXT_MODE_3: > > if ( page_is_ram_type(paddr_to_pfn(0xB8000), RAM_TYPE_CONVENTIONAL) || > > - ((video = ioremap_wc(0xB8000, 0x8000)) == NULL) ) > > + ((video = __va(0xB8000)) == NULL) ) > > Without having a good suggestion, find such dependencies upon the low > 1Mb always being mapped (in case we wanted to revisit this, for example) > is going to be moderately hard. It might be good to somehow mark them. Hm, if we where to change this, we should likely go over all the usages of ioremap() and __va() to find them? I assume you are fine with this going in as-is. Thanks, Roger.
On 20.03.2025 17:15, Roger Pau Monné wrote: > On Thu, Mar 20, 2025 at 04:25:25PM +0100, Jan Beulich wrote: >> On 20.03.2025 16:11, Roger Pau Monne wrote: >>> --- a/xen/drivers/video/vga.c >>> +++ b/xen/drivers/video/vga.c >>> @@ -71,7 +71,7 @@ void __init video_init(void) >>> { >>> case XEN_VGATYPE_TEXT_MODE_3: >>> if ( page_is_ram_type(paddr_to_pfn(0xB8000), RAM_TYPE_CONVENTIONAL) || >>> - ((video = ioremap_wc(0xB8000, 0x8000)) == NULL) ) >>> + ((video = __va(0xB8000)) == NULL) ) >> >> Without having a good suggestion, find such dependencies upon the low >> 1Mb always being mapped (in case we wanted to revisit this, for example) >> is going to be moderately hard. It might be good to somehow mark them. > > Hm, if we where to change this, we should likely go over all the > usages of ioremap() and __va() to find them? Going over the ioremap()s ought to be fine. There may be many __va() though, plus that one has further aliases iirc (e.g. maddr_to_virt()). > I assume you are fine with this going in as-is. Yes, as said - short of having any good suggestion. Jan
diff --git a/xen/drivers/video/vga.c b/xen/drivers/video/vga.c index b4d018326128..b577b2461942 100644 --- a/xen/drivers/video/vga.c +++ b/xen/drivers/video/vga.c @@ -71,7 +71,7 @@ void __init video_init(void) { case XEN_VGATYPE_TEXT_MODE_3: if ( page_is_ram_type(paddr_to_pfn(0xB8000), RAM_TYPE_CONVENTIONAL) || - ((video = ioremap_wc(0xB8000, 0x8000)) == NULL) ) + ((video = __va(0xB8000)) == NULL) ) return; outw(0x200a, 0x3d4); /* disable cursor */ columns = vga_console_info.u.text_mode_3.columns; @@ -158,7 +158,6 @@ void __init video_endboot(void) if ( !vgacon_keep ) { memset(video, 0, columns * lines * 2); - iounmap(video); video = ZERO_BLOCK_PTR; } break;
The call to ioremap_wc() in video_init() will always fail, because video_init() is called ahead of vm_init_type(), and so the underlying __vmap() call will fail to allocate the linear address space. Fix by reverting to the previous behavior and use __va() for the VGA text buffer, as it's below the 1MB boundary, and thus always mapped in the directmap. Fixes: 81d195c6c0e2 ('x86: introduce ioremap_wc()') Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- Changes since v2: - Use __va() instead of ioremap(). Changes since v1: - No not attempt to adjust the directmap VGA text buffer mappings to be WC, just revert to the previous usage of UC- mappings for the whole VGA hole. --- xen/drivers/video/vga.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)