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

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

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 <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?

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.