Re: [PATCH 6/7] x86/vga: fix mapping of the VGA text buffer

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Fri, Mar 14, 2025 at 12:23:32PM +0100, Jan Beulich wrote:
> On 14.03.2025 11:39, Roger Pau Monné wrote:
> > (resending because I seem to have inadvertently corrupted the Cc field)
> > 
> > On Thu, Mar 13, 2025 at 07:39:58PM +0000, Andrew Cooper wrote:
> >> On 13/03/2025 3:30 pm, 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 the directmap entries
> >>> in the low 1MB.  Note the VGA text buffer directmap entries are also
> >>> adjusted to map the VGA text buffer as WC instead of UC-.
> >>>
> >>> Fixes: 81d195c6c0e2 ('x86: introduce ioremap_wc()')
> >>> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> >>> ---
> >>>  xen/arch/x86/boot/x86_64.S        | 10 +++++++---
> >>>  xen/arch/x86/include/asm/config.h |  5 +++++
> >>>  xen/drivers/video/vga.c           | 11 ++++++++---
> >>>  3 files changed, 20 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/xen/arch/x86/boot/x86_64.S b/xen/arch/x86/boot/x86_64.S
> >>> index 26b9d1c2df9a..07f4bdf46e31 100644
> >>> --- a/xen/arch/x86/boot/x86_64.S
> >>> +++ b/xen/arch/x86/boot/x86_64.S
> >>> @@ -84,15 +84,19 @@ ENTRY(__high_start)
> >>>  /*
> >>>   * Mapping of first 2 megabytes of memory. This is mapped with 4kB 
> >>> mappings
> >>>   * to avoid type conflicts with fixed-range MTRRs covering the lowest 
> >>> megabyte
> >>> - * of physical memory. In any case the VGA hole should be mapped with 
> >>> type UC.
> >>> + * of physical memory. VGA hole should be mapped with type UC, with the
> >>> + * exception of the text buffer that uses WC.
> >>>   * Uses 1x 4k page.
> >>>   */
> >>>  l1_directmap:
> >>>          pfn = 0
> >>>          .rept L1_PAGETABLE_ENTRIES
> >>> -        /* VGA hole (0xa0000-0xc0000) should be mapped UC-. */
> >>> -        .if pfn >= 0xa0 && pfn < 0xc0
> >>> +        /* VGA hole (0xa0000-0xb8000) should be mapped UC-. */
> >>> +        .if pfn >= 0xa0 && pfn < 0xb8
> >>>          .quad (pfn << PAGE_SHIFT) | __PAGE_HYPERVISOR_UCMINUS | 
> >>> _PAGE_GLOBAL | MAP_SMALL_PAGES
> >>> +        /* VGA text buffer (0xb80000-0xc0000) should be mapped WC. */
> >>> +        .elseif pfn >= 0xb8 && pfn < 0xc0
> >>> +        .quad (pfn << PAGE_SHIFT) | __PAGE_HYPERVISOR_WC | _PAGE_GLOBAL 
> >>> | MAP_SMALL_PAGES
> >>>          .else
> >>>          .quad (pfn << PAGE_SHIFT) | PAGE_HYPERVISOR_RWX | MAP_SMALL_PAGES
> >>>          .endif
> >>
> >> We have to be careful doing this.
> >>
> >> It probably is safe to use WC in the pagetables.  We don't start using
> >> the pagetables until after we're sure we're on a 64bit CPU, which means
> >> WC is available.
> >>
> >> However, doing so now means that we need explicit SFENCE's when using
> >> this, even in places like early_error.  The IN/OUT instructions do flush
> >> WC buffers, but the UART is written to before the screen, so there's a
> >> chance that you'll lose the final character of the message on the screen.
> > 
> > I don't think early_error will ever use this mapping.
> > 
> > `vga_text_buffer` contains the address 0xb8000, and AFAICT it's
> > exclusively used with paging disabled (as the multiboot2 efi path
> > explicitly sets vga_text_buffer = 0).  The WC mapping created above is
> > on the directmap, so va > DIRECTMAP_VIRT_START.
> > 
> > vga_text_puts() might need such SFENCE, but arguably that should be a
> > different patch IMO.  Might be best to ask Jan whether this is on
> > purpose?
> 
> I think that was wrongly omitted before already.

OK, so we should likely have an SFENCE in vga_text_puts() then.

> > My hypothesis is that the SFENCE might only be needed in
> > video_endboot() and before reboot if Xen crashed ahead of
> > relinquishing the VGA console.
> 
> This might suffice for being able to see the final picture, but it may
> result in display artifacts earlier on.
> 
> Question is whether simply undoing the ioremap_wc() (for not functioning
> correctly) isn't going to be good enough.

Yeah, that was indeed my first approach, as I was under the impression
that not use WC wouldn't make that much of a difference in the text
buffer (as opposed to not using WC for the frame buffer).  But then I
saw that making the directmap mappings of the text buffer WC wasn't
that complicated.

> Prior to the change to use that,
> we had been using UC- quite okay (even if a bit slow). An option may be
> to make a WC mapping a little later, when __vmap() is usable.

Hm, yes, that's another possibility, albeit it seems to add even more
complexity, for a display mode that I assume is not that used anymore.
Otherwise someone would have complained of not getting any output
since 81d195c6c0e2?

If there are no further objections I will just revert to use ioremap()
and leave it like that.

Thanks, Roger.