On Mon, Feb 07, 2022 at 07:24:12PM +0000, Julien Grall wrote:
> 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 <[email protected]>
> > > >
> > > > 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 <[email protected]>
> > >
> > > 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?
Yes, I think that's fine for x86 also.
> > 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.
Oh, so I was slightly confused. You are adding a top level
XEN_FW_VIDEO_INFO, not a EFI sub-op one. In which case, yes, we would
need to keep the MODE_3 as part of the interface.
> > 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.
That would be the case if we add a XEN_FW_EFI_VIDEO sub option, if
instead we add a top level one (XEN_FW_VIDEO_INFO) we need to keep the
mode 3 support.
It might be best for x86 to introduce a global XEN_FW_VIDEO_INFO, as
we can then convey all the video information regardless of the boot
mode.
FWIW, I'm not a huge fan of the struct name (I would rather prefer
video_info or some such), but that ship sailed long time ago.
Thanks, Roger.
