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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

~Andrew