| Message ID | 20250318091904.52903-3-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | x86/ubsan: fix ubsan on clang + code fixes | expand |
On 18/03/2025 9:19 am, 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 using ioremap() for the VGA > text buffer. > > Fixes: 81d195c6c0e2 ('x86: introduce ioremap_wc()') > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> This is somewhat ugly. ioremap() isn't really any better than ioremap_wc(); this only works because plain ioremap() has a special case for the bottom 1M where it does nothing and exits. What's the consequence of ioremap_wc() failing? Presumably nothing out on the screen when using legacy text mode? From that point of view, this is an improvement I suppose. ~Andrew
On 18.03.2025 14:11, Andrew Cooper wrote: > On 18/03/2025 9:19 am, 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 using ioremap() for the VGA >> text buffer. >> >> Fixes: 81d195c6c0e2 ('x86: introduce ioremap_wc()') >> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > > This is somewhat ugly. > > ioremap() isn't really any better than ioremap_wc(); this only works > because plain ioremap() has a special case for the bottom 1M where it > does nothing and exits. And this is exactly why I screwed up back then. Imo we would be better off moving to using __va() directly here. Otherwise the same mistake could be made again by someone (perhaps even me) noticing the less efficient ioremap(), when ioremap_wc() really would be wanted. Jan
On Tue, Mar 18, 2025 at 03:28:32PM +0100, Jan Beulich wrote: > On 18.03.2025 14:11, Andrew Cooper wrote: > > On 18/03/2025 9:19 am, 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 using ioremap() for the VGA > >> text buffer. > >> > >> Fixes: 81d195c6c0e2 ('x86: introduce ioremap_wc()') > >> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > > > > This is somewhat ugly. > > > > ioremap() isn't really any better than ioremap_wc(); this only works > > because plain ioremap() has a special case for the bottom 1M where it > > does nothing and exits. > > And this is exactly why I screwed up back then. Imo we would be better > off moving to using __va() directly here. Otherwise the same mistake > could be made again by someone (perhaps even me) noticing the less > efficient ioremap(), when ioremap_wc() really would be wanted. I can switch to using __va(), that's fine. I guess we should then remove the special casing for the low 1MB in ioremap()? Thanks, Roger.
On 18.03.2025 16:31, Roger Pau Monné wrote: > On Tue, Mar 18, 2025 at 03:28:32PM +0100, Jan Beulich wrote: >> On 18.03.2025 14:11, Andrew Cooper wrote: >>> On 18/03/2025 9:19 am, 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 using ioremap() for the VGA >>>> text buffer. >>>> >>>> Fixes: 81d195c6c0e2 ('x86: introduce ioremap_wc()') >>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> >>> >>> This is somewhat ugly. >>> >>> ioremap() isn't really any better than ioremap_wc(); this only works >>> because plain ioremap() has a special case for the bottom 1M where it >>> does nothing and exits. >> >> And this is exactly why I screwed up back then. Imo we would be better >> off moving to using __va() directly here. Otherwise the same mistake >> could be made again by someone (perhaps even me) noticing the less >> efficient ioremap(), when ioremap_wc() really would be wanted. > > I can switch to using __va(), that's fine. I guess we should then > remove the special casing for the low 1MB in ioremap()? I'm not sure we can - DMI and ACPI might use that, and who knows what else. But of course if you're sure nothing depends on that anymore ... Jan
diff --git a/xen/drivers/video/vga.c b/xen/drivers/video/vga.c index b4d018326128..ee6cf0a7079a 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 = ioremap(0xB8000, 0x8000)) == NULL) ) return; outw(0x200a, 0x3d4); /* disable cursor */ columns = vga_console_info.u.text_mode_3.columns; @@ -158,7 +158,7 @@ void __init video_endboot(void) if ( !vgacon_keep ) { memset(video, 0, columns * lines * 2); - iounmap(video); + /* No iounmap(), as it's a directmap mapping. */ 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 using ioremap() for the VGA text buffer. Fixes: 81d195c6c0e2 ('x86: introduce ioremap_wc()') Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- 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 | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)