On Tue, Apr 05, 2022 at 12:57:51PM +0200, Jan Beulich wrote:
> On 05.04.2022 11:35, Roger Pau Monné wrote:
> > On Thu, Mar 31, 2022 at 11:45:02AM +0200, Jan Beulich wrote:
> >> --- a/xen/arch/x86/boot/head.S
> >> +++ b/xen/arch/x86/boot/head.S
> >> @@ -562,12 +562,18 @@ trampoline_setup:
> >> mov %esi, sym_esi(xen_phys_start)
> >> mov %esi, sym_esi(trampoline_xen_phys_start)
> >>
> >> - mov sym_esi(trampoline_phys), %ecx
> >> -
> >> /* Get bottom-most low-memory stack address. */
> >> + mov sym_esi(trampoline_phys), %ecx
> >> add $TRAMPOLINE_SPACE,%ecx
> >
> > Just for my understanding, since you are already touching the
> > instruction, why not switch it to a lea like you do below?
> >
> > Is that because you would also like to take the opportunity to fold
> > the add into the lea and that would be too much of a change?
>
> No. This MOV cannot be converted, as its source operand isn't an
> immediate (or register); such a conversion would also be undesirable,
> for increasing insn size. See the later patch doing conversions in
> the other direction, to reduce code size. Somewhat similarly ...
>
> >> +#ifdef CONFIG_VIDEO
> >> + lea sym_esi(boot_vid_info), %edx
>
> ... this LEA also cannot be expressed by a single MOV.
>
> >> @@ -32,6 +33,39 @@ asm (
> >> #include "../../../include/xen/kconfig.h"
> >> #include <public/arch-x86/hvm/start_info.h>
> >>
> >> +#ifdef CONFIG_VIDEO
> >> +# include "video.h"
> >> +
> >> +/* VESA control information */
> >> +struct __packed vesa_ctrl_info {
> >> + uint8_t signature[4];
> >> + uint16_t version;
> >> + uint32_t oem_name;
> >> + uint32_t capabilities;
> >> + uint32_t mode_list;
> >> + uint16_t mem_size;
> >> + /* We don't use any further fields. */
> >> +};
> >> +
> >> +/* VESA 2.0 mode information */
> >> +struct vesa_mode_info {
> >
> > Should we add __packed here just in case further added fields are no
> > longer naturally aligned? (AFAICT all field right now are aligned to
> > it's size so there's no need for it).
>
> I think we should avoid __packed whenever possible.
>
> >> + uint16_t attrib;
> >> + uint8_t window[14]; /* We don't use the individual fields. */
> >> + uint16_t bytes_per_line;
> >> + uint16_t width;
> >> + uint16_t height;
> >> + uint8_t cell_width;
> >> + uint8_t cell_height;
> >> + uint8_t nr_planes;
> >> + uint8_t depth;
> >> + uint8_t memory[5]; /* We don't use the individual fields. */
> >> + struct boot_video_colors colors;
> >> + uint8_t direct_color;
> >> + uint32_t base;
> >> + /* We don't use any further fields. */
> >> +};
> >
> > Would it make sense to put those struct definitions in boot/video.h
> > like you do for boot_video_info?
>
> Personally I prefer to expose things in headers only when multiple
> other files want to consume what is being declared/defined.
>
> >> @@ -254,17 +291,64 @@ static multiboot_info_t *mbi2_reloc(u32
> >> ++mod_idx;
> >> break;
> >>
> >> +#ifdef CONFIG_VIDEO
> >> + case MULTIBOOT2_TAG_TYPE_VBE:
> >> + if ( video_out )
> >> + {
> >> + const struct vesa_ctrl_info *ci;
> >> + const struct vesa_mode_info *mi;
> >> +
> >> + video = _p(video_out);
> >> + ci = (void *)get_mb2_data(tag, vbe, vbe_control_info);
> >> + mi = (void *)get_mb2_data(tag, vbe, vbe_mode_info);
> >> +
> >> + if ( ci->version >= 0x0200 && (mi->attrib & 0x9b) == 0x9b
> >> )
> >> + {
> >> + video->capabilities = ci->capabilities;
> >> + video->lfb_linelength = mi->bytes_per_line;
> >> + video->lfb_width = mi->width;
> >> + video->lfb_height = mi->height;
> >> + video->lfb_depth = mi->depth;
> >> + video->lfb_base = mi->base;
> >> + video->lfb_size = ci->mem_size;
> >> + video->colors = mi->colors;
> >> + video->vesa_attrib = mi->attrib;
> >> + }
> >> +
> >> + video->vesapm.seg = get_mb2_data(tag, vbe,
> >> vbe_interface_seg);
> >> + video->vesapm.off = get_mb2_data(tag, vbe,
> >> vbe_interface_off);
> >> + }
> >> + break;
> >> +
> >> + case MULTIBOOT2_TAG_TYPE_FRAMEBUFFER:
> >> + if ( (get_mb2_data(tag, framebuffer, framebuffer_type) !=
> >> + MULTIBOOT2_FRAMEBUFFER_TYPE_RGB) )
> >> + {
> >> + video_out = 0;
> >> + video = NULL;
> >> + }
> >
> > I'm confused, don't you need to store the information in the
> > framebuffer tag for use after relocation?
>
> If there was a consumer - yes. Right now this tag is used only to
> invalidate the information taken from the other tag (or to suppress
> taking values from there if that other tag came later) in case the
> framebuffer type doesn't match what we support.
>
> >> + break;
> >> +#endif /* CONFIG_VIDEO */
> >> +
> >> case MULTIBOOT2_TAG_TYPE_END:
> >> - return mbi_out;
> >> + goto end; /* Cannot "break;" here. */
> >>
> >> default:
> >> break;
> >> }
> >>
> >> + end:
> >> +
> >> +#ifdef CONFIG_VIDEO
> >> + if ( video )
> >> + video->orig_video_isVGA = 0x23;
> >
> > I see we use this elsewhere, what's the meaning of this (magic) 0x23?
>
> This is a value Linux uses (also as a plain number without any #define
> iirc; at least it was that way when we first inherited that value).
> Short of knowing where they took it from or what meaning they associate
> with the value it would be hard for us to give this a (meaningful) name
> and hence use a #define. And hence it's equally hard to properly answer
> your question.
OK, so it's a magic value. I'm happy with the rest, so:
Acked-by: Roger Pau Monné <roger....@citrix.com>
Thanks, Roger.
