Re: [edk2-devel] [PATCH 05/13] OvmfPkg: Add VBE2 mode info structure to LegacyVgaBios.h

["Laszlo Ersek" <lersek@redhat.com>]

On 04/16/20 01:09, Rebecca Cran wrote:
> bhyve uses the older VESA BIOS Extensions 2.0, so add the mode info
> structure to IndustryStandard/LegacyVgaBios.h
> 
> Signed-off-by: Rebecca Cran <rebecca@bsdio.com>
> ---
>  .../Include/IndustryStandard/LegacyVgaBios.h  | 39 +++++++++++++++++++
>  1 file changed, 39 insertions(+)

This header file currently only exists in support of
"OvmfPkg/QemuVideoDxe/VbeShim.c".

But, I still agree that VBE2_MODE_INFO belongs here. So it's OK.

> diff --git a/OvmfPkg/Include/IndustryStandard/LegacyVgaBios.h b/OvmfPkg/Include/IndustryStandard/LegacyVgaBios.h
> index 317dac3a56..016ff95a5d 100644
> --- a/OvmfPkg/Include/IndustryStandard/LegacyVgaBios.h
> +++ b/OvmfPkg/Include/IndustryStandard/LegacyVgaBios.h
> @@ -88,4 +88,43 @@ typedef struct {
>  } VBE_MODE_INFO;
>  #pragma pack ()
>  
> +typedef struct {
> +  UINT16 ModeAttr;
> +  UINT8  WindowAAttr;
> +  UINT8  WindowBAttr;
> +  UINT16 WindowGranularityKB;
> +  UINT16 WindowSizeKB;
> +  UINT16 WindowAStartSegment;
> +  UINT16 WindowBStartSegment;
> +  UINT32 WindowPositioningAddress;
> +  UINT16 BytesPerScanLine;
> +
> +  UINT16 Width;
> +  UINT16 Height;
> +  UINT8  CharCellWidth;
> +  UINT8  CharCellHeight;
> +  UINT8  NumPlanes;
> +  UINT8  BitsPerPixel;
> +  UINT8  NumBanks;
> +  UINT8  MemoryModel;
> +  UINT8  BankSizeKB;
> +  UINT8  NumImagePagesLessOne;
> +  UINT8  Vbe3;
> +
> +  UINT8  RedMaskSize;
> +  UINT8  RedMaskPos;
> +  UINT8  GreenMaskSize;
> +  UINT8  GreenMaskPos;
> +  UINT8  BlueMaskSize;
> +  UINT8  BlueMaskPos;
> +  UINT8  ReservedMaskSize;
> +  UINT8  ReservedMaskPos;
> +  UINT8  DirectColorModeInfo;
> +
> +  UINT32 LfbAddress;
> +  UINT32 OffScreenAddress;
> +  UINT16 OffScreenSizeKB;
> +} VBE2_MODE_INFO;
> +#pragma pack ()
> +
>  #endif
> 

I'm not going to review the contents of the new structure, so I'm going
to give an A-b, not an R-b.

However, the "#pragma pack ()" that you add at the end is unbalanced.
The pre-patch file already brackets the whole set of structs with

#pragma pack (1)
...
#pragma pack ()

Thus, you need not add any #pragma, just insert the VBE2_MODE_INFO
typedef between the existent pragmas.

Put differently: please remove the "#pragma pack ()" right after
"VBE_MODE_INFO" (*not* after VBE2_MODE_INFO), as a part of this patch.

With that update:

Acked-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo