Re: [PATCH v3 4/9] x86/PVH: provide VGA console info to Dom0

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

On Tue, Sep 21, 2021 at 09:18:05AM +0200, Jan Beulich wrote:
> Like PV Dom0 in order to use the console if in a mode other than text
> 80x25 the kernel needs to be provided information about this mode. Bump
> HVM start info's "current" version to 2 and use a previously reserved
> 32-bit field to provide struct dom0_vga_console_info's position and
> size.
> 
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> v3: New.
> 
> --- a/xen/arch/x86/hvm/dom0_build.c
> +++ b/xen/arch/x86/hvm/dom0_build.c
> @@ -19,6 +19,7 @@
>   */
>  
>  #include <xen/acpi.h>
> +#include <xen/console.h>
>  #include <xen/init.h>
>  #include <xen/libelf.h>
>  #include <xen/multiboot.h>
> @@ -549,6 +550,11 @@ static int __init pvh_load_kernel(struct
>      paddr_t last_addr;
>      struct hvm_start_info start_info = { 0 };
>      struct hvm_modlist_entry mod = { 0 };
> +#ifdef CONFIG_VIDEO
> +    struct dom0_vga_console_info vga_info = { 0 };
> +#else
> +    struct {} __maybe_unused vga_info;
> +#endif
>      struct vcpu *v = d->vcpu[0];
>      int rc;
>  
> @@ -598,7 +604,7 @@ static int __init pvh_load_kernel(struct
>       * split into smaller allocations, done as a single region in order to
>       * simplify it.
>       */
> -    last_addr = find_memory(d, &elf, sizeof(start_info) +
> +    last_addr = find_memory(d, &elf, sizeof(start_info) + sizeof(vga_info) +
>                              (initrd ? ROUNDUP(initrd->mod_end, PAGE_SIZE) +
>                                        sizeof(mod)
>                                      : 0) +
> @@ -672,6 +678,22 @@ static int __init pvh_load_kernel(struct
>          last_addr += sizeof(mod);
>      }
>  
> +#ifdef CONFIG_VIDEO
> +    if ( fill_console_start_info(&vga_info) )
> +    {
> +        rc = hvm_copy_to_guest_phys(last_addr + sizeof(start_info),
> +                                    &vga_info, sizeof(vga_info), v);
> +        if ( !rc )
> +        {
> +            start_info.version = 2;
> +            start_info.vga_info.offset = sizeof(start_info);
> +            start_info.vga_info.size = sizeof(vga_info);
> +        }
> +        else
> +            printk("Unable to copy VGA info to guest\n");
> +    }
> +#endif
> +
>      start_info.magic = XEN_HVM_START_MAGIC_VALUE;
>      start_info.flags = SIF_PRIVILEGED | SIF_INITDOMAIN;
>      rc = hvm_copy_to_guest_phys(last_addr, &start_info, sizeof(start_info), 
> v);
> --- a/xen/include/public/arch-x86/hvm/start_info.h
> +++ b/xen/include/public/arch-x86/hvm/start_info.h
> @@ -33,7 +33,7 @@
>   *    | magic          | Contains the magic value XEN_HVM_START_MAGIC_VALUE
>   *    |                | ("xEn3" with the 0x80 bit of the "E" set).
>   *  4 +----------------+
> - *    | version        | Version of this structure. Current version is 1. New
> + *    | version        | Version of this structure. Current version is 2. New
>   *    |                | versions are guaranteed to be backwards-compatible.
>   *  8 +----------------+
>   *    | flags          | SIF_xxx flags.
> @@ -55,7 +55,15 @@
>   *    |                | if there is no memory map being provided. Only
>   *    |                | present in version 1 and newer of the structure.
>   * 52 +----------------+
> - *    | reserved       | Version 1 and newer only.
> + *    | vga_info.offset| Offset of struct dom0_vga_console_info from base of

I'm not sure we are supposed to reference external structures like
that. We took a lot of care to not depend on other headers, and to
make this as agnostic as possible (IIRC KVM is also capable of using
the PVH entry point natively, and hence depends on this header).

IF we want to add support for dom0_vga_console_info I think we need to
at least provide a binary layout for it, like all the other pieces
that are part of the HVM start info.

> + *    |                | struct hvm_start_info. Optional and only present in
> + *    |                | version 2 and newer of the structure when
> + *    |                | SIF_INITDOMAIN is set; zero if absent.

We have usually used an absolute physical address to reference other
data, and I think we should likely keep in that way for coherency.

> + * 54 +----------------+
> + *    | vga_info.size  | Size of present parts of struct 
> dom0_vga_console_info.
> + *    |                | Optional and only present in version 2 and newer of
> + *    |                | the structure when SIF_INITDOMAIN is set; zero if
> + *    |                | absent.
>   * 56 +----------------+
>   *
>   * The layout of each entry in the module structure is the following:
> @@ -139,7 +147,15 @@ struct hvm_start_info {
>      uint32_t memmap_entries;    /* Number of entries in the memmap table.    
> */
>                                  /* Value will be zero if there is no memory  
> */
>                                  /* map being provided.                       
> */
> -    uint32_t reserved;          /* Must be zero.                             
> */

This 'Must be zero' comment troubles me a bit, I'm not convinced we
can just place data here (ie: it would no longer be 0). It's even
worse because the must be zero comment is only present in the C
representation of the struct, the layout above just denotes the field
is reserved (which would imply it's fine to use for other means in
v2).

Thanks, Roger.