Re: [PATCH RFC 3/3] xen: Introduce a platform sub-op to retrieve the VGA information

[Julien Grall <julien@xxxxxxx>]

Hi Roger,

On 07/02/2022 11:58, Roger Pau Monné wrote:
> On Mon, Feb 07, 2022 at 09:57:55AM +0100, Jan Beulich wrote:
> > On 06.02.2022 20:28, Julien Grall wrote:
> > > From: Julien Grall <jgrall@xxxxxxxxxx>
> > >
> > > When using EFI, the VGA information is fetched using the EFI
> > > boot services. However, Xen will have exited the boot services.
> > > Therefore, we need to find a different way to pass the information
> > > to dom0.
> > >
> > > For PV dom0, they are part of the start_info. But this is not
> > > something that exists on Arm. So the best way would to be to
> > > use a hypercall.
> > >
> > > For now the structure layout is based on dom0_vga_console_info
> > > for convenience. I am open on another proposal.
> > >
> > > Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>
> > Cc-ing Roger as this may want using for PVH Dom0 also on x86; my
> > first attempt to propagate this information was rejected.
> I think it's easier to use a Xen specific layout in XENPF, as that's
> already a Xen specific interface.
>
> I wonder however if passing the information here (instead of doing it
> in the start info or equivalent) could cause a delay in the
> initialization of the video console.
My current plan for Arm is to issue the hypercall as part of an 
earlyinit call. But we can do much earlier (i.e. xen_early_init() which 
is called from setup_arch()) if necessary.
This should be early enough for Arm. How about x86?

> I guess the same happens when
> using the Xen consoles (either the hypercall one or the shared ring),
> so it's fine.
>
> > > --- a/xen/include/public/platform.h
> > > +++ b/xen/include/public/platform.h
> > > @@ -244,6 +244,7 @@ DEFINE_XEN_GUEST_HANDLE(xenpf_efi_runtime_call_t);
> > >   #define  XEN_FW_EFI_PCI_ROM        5
> > >   #define  XEN_FW_EFI_APPLE_PROPERTIES 6
> > >   #define XEN_FW_KBD_SHIFT_FLAGS    5
> > > +#define XEN_FW_VGA_INFO           6
> > Perhaps s/VGA/VIDEO/, despite ...
> >
> > >   struct xenpf_firmware_info {
> > >       /* IN variables. */
> > >       uint32_t type;
> > > @@ -311,6 +312,7 @@ struct xenpf_firmware_info {
> > >   
> > >           /* Int16, Fn02: Get keyboard shift flags. */
> > >           uint8_t kbd_shift_flags; /* XEN_FW_KBD_SHIFT_FLAGS */
> > > +        struct dom0_vga_console_info vga;
> > ... the structure name including "vga" (but if the #define is adjusted,
> > the field name would want to become "video" as well).
[...]

(Re-ordered the quote as it makes more sense for my reply)

> There's no need to propagate XEN_VGATYPE_TEXT_MODE_3 into this
> interface.
So for Arm, we are only caring about XEN_VGATYPE_EFI_LFB. I wasn't sure 
what would be your need on x86. Hence, why I keep it.
If you don't need then, then I am happy to trim the structure for the 
new hypercall.
> It's my understanding that this will forcefully be
> XEN_VGATYPE_EFI_LFB, at which point we could consider giving a type
> name to the vesa_lfb field of dom0_vga_console_info (video_lfb) and
> use the same struct here?>

Just to clarify, are you suggesting to only pass video_lfb? IOW, we will 
always assume it is an EFI framebuffer and not pass the video_type.
Cheers,

--
Julien Grall