| Message ID | 20220206192839.75711-4-julien@xen.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/arm: Allow dom0 to use the EFI framebuffer | expand |
On 06.02.2022 20:28, Julien Grall wrote: > From: Julien Grall <jgrall@amazon.com> > > 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@amazon.com> Cc-ing Roger as this may want using for PVH Dom0 also on x86; my first attempt to propagate this information was rejected. > --- 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). Jan
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@amazon.com> > > > > 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@amazon.com> > > 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. 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). 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? There's no need to propagate XEN_VGATYPE_TEXT_MODE_3 into this interface. Thanks, Roger.
On 07.02.2022 12: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: >>> @@ -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). > > 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? > > There's no need to propagate XEN_VGATYPE_TEXT_MODE_3 into this > interface. Hmm, yes, this is probably better / more clean. Julien, thoughts? Jan
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@amazon.com> >>> >>> 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@amazon.com> >> >> 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,
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@amazon.com> > > > > > > > > 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@amazon.com> > > > > > > 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.
diff --git a/xen/arch/arm/platform_hypercall.c b/xen/arch/arm/platform_hypercall.c index 8efac7ee602a..78ad328e2ab8 100644 --- a/xen/arch/arm/platform_hypercall.c +++ b/xen/arch/arm/platform_hypercall.c @@ -10,6 +10,7 @@ #include <xen/sched.h> #include <xen/guest_access.h> #include <xen/spinlock.h> +#include <xen/vga.h> #include <public/platform.h> #include <xsm/xsm.h> #include <asm/current.h> @@ -58,6 +59,20 @@ long do_platform_op(XEN_GUEST_HANDLE_PARAM(xen_platform_op_t) u_xenpf_op) ret = -EINVAL; break; + case XENPF_firmware_info: + switch ( op->u.firmware_info.type ) + { + case XEN_FW_VGA_INFO: + BUILD_BUG_ON(sizeof(op->u.firmware_info.u.vga) != + sizeof(vga_console_info)); + memcpy(&op->u.firmware_info.u.vga, &vga_console_info, + sizeof(vga_console_info)); + if ( __copy_to_guest(u_xenpf_op, op, 1) ) + ret = -EFAULT; + break; + } + break; + default: ret = -ENOSYS; break; diff --git a/xen/include/public/platform.h b/xen/include/public/platform.h index a4c0eb62249a..4de42ce6cbc5 100644 --- 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 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; } u; }; typedef struct xenpf_firmware_info xenpf_firmware_info_t;