On 02/05/2019 13:44, Jan Beulich wrote: >>>> On 02.05.19 at 13:52, <[email protected]> wrote: >> c/s ebb26b509f "xen/x86: make VGA support selectable" added an #ifdef >> CONFIG_VIDEO into the middle the backing space for early_boot_opts_t, >> but didn't adjust the structure definition in cmdline.c >> >> This only functions correctly because the affected fields are at the end >> of the structure, and cmdline.c doesn't write to them in this case. >> >> To retain the slimming effect of compiling out CONFIG_VIDEO, adjust >> cmdline.c with enough #ifdef-ary to make C's idea of the structure match >> the declaration in asm. This requires adding __maybe_unused annotations >> to two helper functions. >> >> Signed-off-by: Andrew Cooper <[email protected]> > Reviewed-by: Jan Beulich <[email protected]> > with a remark and a question: > >> --- a/xen/arch/x86/boot/cmdline.c >> +++ b/xen/arch/x86/boot/cmdline.c >> @@ -40,10 +40,12 @@ typedef struct __packed { >> u8 opt_edd; >> u8 opt_edid; >> u8 padding; >> +#ifdef CONFIG_VIDEO >> u16 boot_vid_mode; >> u16 vesa_width; >> u16 vesa_height; >> u16 vesa_depth; >> +#endif > Since apparently the "Keep in sync" comment in trampoline.S > wasn't sufficient, and since - with what said commit did - the > comment now looks unrelated to these data items (for there > being a blank line in between now) perhaps this should be > accompanied by both a START and END marker?
I've got a followup patch which cleans up the ASM, but I don't want to interfere with the work that David is currently preparing. > And perhaps the comment next to vesa_size should also > get corrected to say "width x height x depth". I had already addressed this, and the fact that boot_vid_mode has never needed to be global (ever since its introduction). > >> --- a/xen/arch/x86/boot/defs.h >> +++ b/xen/arch/x86/boot/defs.h >> @@ -23,6 +23,7 @@ >> #include "../../../include/xen/stdbool.h" >> >> #define __packed __attribute__((__packed__)) >> +#define __maybe_unused __attribute__((__unused__)) >> #define __stdcall __attribute__((__stdcall__)) > Purely out of curiosity (I don't really care about the ordering > here as long as the set doesn't meaningfully grow): Based on > what did you decide this best goes between the two existing > ones? I forgot to sort the lines. Fixed. ~Andrew _______________________________________________ Xen-devel mailing list [email protected] https://lists.xenproject.org/mailman/listinfo/xen-devel
