Re: [PATCH v4 2/8] x86/boot: obtain video info from boot loader

["Roger Pau Monné" <roger.pau@citrix.com>]

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.pau@citrix.com>

Thanks, Roger.