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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

Jan