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

["Roger Pau Monné" <roger.pau@citrix.com>]

(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@citrix.com>
> > ---
> >  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?

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.

Thanks, Roger.