On 05/10/14 00:49, Jordan Justen wrote:
> On Thu, May 8, 2014 at 10:45 AM, Laszlo Ersek <ler...@redhat.com> wrote:
>> The Windows 2008 R2 SP1 UEFI guest's default video driver is buggy -- it
>> dereferences the real mode Int10h vector, loads the pointed-to handler
>> code, and executes what it thinks to be VGA BIOS services in an internal
>> real-mode emulator. Consequently, video mode switching doesn't work in
>> Windows 2008 R2 SP1 when it runs on the pure UEFI build of OVMF, making
>> the guest uninstallable.
>
> Can you add a Windows 2008 section to the README?
Good point, will do.
> For example, I think -vga qxl needs to be used, right?
One of '-vga std' and '-vga qxl'. The latter is recommended because then
you can install the QXL XDDM guest driver, which gives you S3 and a
number of graphics resolutions.
[snip]
>> +#pragma pack (1)
>> +typedef struct {
>> + UINT8 Signature[4];
>> + UINT16 VesaVersion;
>> + UINT32 OemNameAddress;
>> + UINT32 Capabilities;
>> + UINT32 ModeListAddress;
>> + UINT16 VideoMem64K;
>> + UINT16 OemSoftwareVersion;
>> + UINT32 VendorNameAddress;
>> + UINT32 ProductNameAddress;
>> + UINT32 ProductRevAddress;
>> +} VBE_INFO_BASE;
>> +
>> +typedef struct {
>> + VBE_INFO_BASE Base;
>> + UINT8 Buffer[256 - sizeof (VBE_INFO_BASE)];
>> +} VBE_INFO;
>> +
>> +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;
>> +
>> + UINT16 BytesPerScanLineLinear;
>> + UINT8 NumImagesLessOneBanked;
>> + UINT8 NumImagesLessOneLinear;
>> + UINT8 RedMaskSizeLinear;
>> + UINT8 RedMaskPosLinear;
>> + UINT8 GreenMaskSizeLinear;
>> + UINT8 GreenMaskPosLinear;
>> + UINT8 BlueMaskSizeLinear;
>> + UINT8 BlueMaskPosLinear;
>> + UINT8 ReservedMaskSizeLinear;
>> + UINT8 ReservedMaskPosLinear;
>> + UINT32 MaxPixelClockHz;
>> + UINT8 Reserved[190];
>> +} VBE_MODE_INFO;
>
> OvmfPkg/Include/IndustryStandard/LegacyVgaBios.h?
In general I prefer to expose type definitions only if there are at
least two users, but I can certainly do this, yes.
>
>> +
>> +typedef struct {
>> + UINT16 Offset;
>> + UINT16 Segment;
>> +} IVT_ENTRY;
>> +#pragma pack ()
>> +
>> +//
>> +// This string is displayed by Windows 2008 R2 SP1 in the Screen Resolution,
>> +// Advanced Settings dialog. It should be short.
>> +//
>> +STATIC CONST CHAR8 mProductRevision[] = "OVMF Int10h (fake)";
>> +
>> +/**
>> + Install the VBE Info and VBE Mode Info structures, and the VBE service
>> + handler routine in the C segment. Point the real-mode Int10h interrupt
>> vector
>> + to the handler. The only advertised mode is 1024x768x32.
>> +
>> + @param[in] CardName Name of the video card to be exposed in the
>> + Product Name field of the VBE Info structure.
>> The
>> + parameter must originate from a
>> + QEMU_VIDEO_CARD.Name field.
>> + @param[in] FrameBufferBase Guest-physical base address of the video
>> card's
>> + frame buffer.
>> +
>> + @retval EFI_SUCCESS Shim successfully installed.
>> + @return gBS->AllocatePages() status codes.
>> +**/
>> +EFI_STATUS
>> +InstallVbeShim (
>> + IN CONST CHAR16 *CardName,
>> + IN EFI_PHYSICAL_ADDRESS FrameBufferBase
>> + )
>> +{
>> + UINTN Pam1Address;
>> + UINT8 Pam1;
>> + EFI_PHYSICAL_ADDRESS SegmentC;
>> + UINTN SegmentCPages;
>> + EFI_STATUS Status;
>> + VBE_INFO *VbeInfoFull;
>> + VBE_INFO_BASE *VbeInfo;
>> + UINT8 *Ptr;
>> + UINTN Printed;
>> + VBE_MODE_INFO *VbeModeInfo;
>> + EFI_PHYSICAL_ADDRESS Segment0;
>> + UINTN Segment0Pages;
>> + IVT_ENTRY *Int0x10;
>> +
>> + Pam1Address = PCI_LIB_ADDRESS (0, 0, 0, 0x5A);
>
> Should we use LegacyRegion here?
I had considered that, yes:
> I guess that would mean always enabling Csm/CsmSupportLib.
I had checked this source code, and I had decided against using it. It's
too big IMO, the LegacyRegion2*() interfaces are overkill, and I don't
like the "CsmSupportLib" name / dependency either (I want to avoid the
CSM with this patch).
(Side note: the header file with the lib's public interfaces should be
moved under OvmfPkg/Include/Library/ as well, shouldn't it?)
The sole purpose of this patch is bug compatibility with Windows 2008
R2. I think it's alright to cut corners and to get dirty here.
For example, in a perfect world, a UEFI video driver should have no
business messing with the PAM registers, be that via CsmSupportLib or
more directly with PciLib. Once we do that, we might as well opt for the
shortest / simplest solution.
>
>> + //
>> + // low nibble covers 0xC0000 to 0xC3FFF
>> + // high nibble covers 0xC4000 to 0xC7FFF
>> + // bit1 in each nibble is Write Enable
>> + // bit0 in each nibble is Read Enable
>> + //
>> + Pam1 = PciRead8 (Pam1Address);
>> + PciWrite8 (Pam1Address, Pam1 | (BIT1 | BIT0));
>> +
>> + //
>> + // We never added memory space durig PEI or DXE for the C segment, so we
>> + // don't need to (and can't) allocate from there. Also, guest operating
>> + // systems will see a hole in the UEFI memory map there.
>> + //
>> + SegmentC = 0xC0000;
>> + SegmentCPages = 4;
>> +
>> + ASSERT (sizeof mVbeShim <= EFI_PAGES_TO_SIZE (SegmentCPages));
>> + CopyMem ((VOID *)(UINTN)SegmentC, mVbeShim, sizeof mVbeShim);
>> +
>> + //
>> + // Fill in the VBE INFO structure.
>> + //
>> + VbeInfoFull = (VBE_INFO *)(UINTN)SegmentC;
>> + VbeInfo = &VbeInfoFull->Base;
>> + Ptr = VbeInfoFull->Buffer;
>> +
>> + CopyMem (VbeInfo->Signature, "VESA", 4);
>> + VbeInfo->VesaVersion = 0x0300;
>> +
>> + VbeInfo->OemNameAddress = (UINT32)(SegmentC << 12 | (UINT16)(UINTN)Ptr);
>> + CopyMem (Ptr, "QEMU", 5);
>> + Ptr += 5;
>> +
>> + VbeInfo->Capabilities = BIT0; // DAC can be switched into 8-bit mode
>> +
>> + VbeInfo->ModeListAddress = (UINT32)(SegmentC << 12 | (UINT16)(UINTN)Ptr);
>> + *(UINT16*)Ptr = 0x00f1; // mode number
>> + Ptr += 2;
>> + *(UINT16*)Ptr = 0xFFFF; // mode list terminator
>> + Ptr += 2;
>> +
>> + VbeInfo->VideoMem64K = (UINT16)((1024 * 768 * 4 + 65535) / 65536);
>> + VbeInfo->OemSoftwareVersion = 0x0000;
>> +
>> + VbeInfo->VendorNameAddress = (UINT32)(SegmentC << 12 |
>> (UINT16)(UINTN)Ptr);
>> + CopyMem (Ptr, "OVMF", 5);
>> + Ptr += 5;
>> +
>> + VbeInfo->ProductNameAddress = (UINT32)(SegmentC << 12 |
>> (UINT16)(UINTN)Ptr);
>> + Printed = AsciiSPrint ((CHAR8 *)Ptr,
>> + sizeof VbeInfoFull->Buffer - (Ptr - VbeInfoFull->Buffer),
>> "%s",
>> + CardName);
>> + Ptr += Printed + 1;
>> +
>> + VbeInfo->ProductRevAddress = (UINT32)(SegmentC << 12 |
>> (UINT16)(UINTN)Ptr);
>> + CopyMem (Ptr, mProductRevision, sizeof mProductRevision);
>> + Ptr += sizeof mProductRevision;
>> +
>> + ASSERT (sizeof VbeInfoFull->Buffer >= Ptr - VbeInfoFull->Buffer);
>> + ZeroMem (Ptr, sizeof VbeInfoFull->Buffer - (Ptr - VbeInfoFull->Buffer));
>> +
>> + //
>> + // Fil in the VBE MODE INFO structure.
>> + //
>> + VbeModeInfo = (VBE_MODE_INFO *)(VbeInfoFull + 1);
>> +
>> + //
>> + // bit0: mode supported by present hardware configuration
>> + // bit1: optional information available (must be =1 for VBE v1.2+)
>> + // bit3: set if color, clear if monochrome
>> + // bit4: set if graphics mode, clear if text mode
>> + // bit5: mode is not VGA-compatible
>> + // bit7: linear framebuffer mode supported
>> + //
>> + VbeModeInfo->ModeAttr = BIT7 | BIT5 | BIT4 | BIT3 | BIT1 | BIT0;
>> +
>> + //
>> + // bit0: exists
>> + // bit1: bit1: readable
>> + // bit2: writeable
>> + //
>> + VbeModeInfo->WindowAAttr = BIT2 | BIT1 | BIT0;
>> +
>> + VbeModeInfo->WindowBAttr = 0x00;
>> + VbeModeInfo->WindowGranularityKB = 0x0040;
>> + VbeModeInfo->WindowSizeKB = 0x0040;
>> + VbeModeInfo->WindowAStartSegment = 0xA000;
>> + VbeModeInfo->WindowBStartSegment = 0x0000;
>> + VbeModeInfo->WindowPositioningAddress = 0x0000;
>> + VbeModeInfo->BytesPerScanLine = 1024 * 4;
>> +
>> + VbeModeInfo->Width = 1024;
>> + VbeModeInfo->Height = 768;
>> + VbeModeInfo->CharCellWidth = 8;
>> + VbeModeInfo->CharCellHeight = 16;
>> + VbeModeInfo->NumPlanes = 1;
>> + VbeModeInfo->BitsPerPixel = 32;
>> + VbeModeInfo->NumBanks = 1;
>> + VbeModeInfo->MemoryModel = 6; // direct color
>> + VbeModeInfo->BankSizeKB = 0;
>> + VbeModeInfo->NumImagePagesLessOne = 0;
>> + VbeModeInfo->Vbe3 = 0x01;
>> +
>> + VbeModeInfo->RedMaskSize = 8;
>> + VbeModeInfo->RedMaskPos = 16;
>> + VbeModeInfo->GreenMaskSize = 8;
>> + VbeModeInfo->GreenMaskPos = 8;
>> + VbeModeInfo->BlueMaskSize = 8;
>> + VbeModeInfo->BlueMaskPos = 0;
>> + VbeModeInfo->ReservedMaskSize = 8;
>> + VbeModeInfo->ReservedMaskPos = 24;
>> +
>> + //
>> + // bit1: Bytes in reserved field may be used by application
>> + //
>> + VbeModeInfo->DirectColorModeInfo = BIT1;
>> +
>> + VbeModeInfo->LfbAddress = (UINT32)FrameBufferBase;
>> + VbeModeInfo->OffScreenAddress = 0;
>> + VbeModeInfo->OffScreenSizeKB = 0;
>> +
>> + VbeModeInfo->BytesPerScanLineLinear = 1024 * 4;
>> + VbeModeInfo->NumImagesLessOneBanked = 0;
>> + VbeModeInfo->NumImagesLessOneLinear = 0;
>> + VbeModeInfo->RedMaskSizeLinear = 8;
>> + VbeModeInfo->RedMaskPosLinear = 16;
>> + VbeModeInfo->GreenMaskSizeLinear = 8;
>> + VbeModeInfo->GreenMaskPosLinear = 8;
>> + VbeModeInfo->BlueMaskSizeLinear = 8;
>> + VbeModeInfo->BlueMaskPosLinear = 0;
>> + VbeModeInfo->ReservedMaskSizeLinear = 8;
>> + VbeModeInfo->ReservedMaskPosLinear = 24;
>> + VbeModeInfo->MaxPixelClockHz = 0;
>> +
>> + ZeroMem (VbeModeInfo->Reserved, sizeof VbeModeInfo->Reserved);
>> +
>> + //
>> + // Clear Write Enable (bit1), keep Read Enable (bit0) set
>> + //
>> + PciWrite8 (Pam1Address, (Pam1 & ~BIT1) | BIT0);
>> +
>> + Segment0 = 0;
>> + Segment0Pages = 1;
>> + Status = gBS->AllocatePages (AllocateAddress, EfiReservedMemoryType,
>> + Segment0Pages, &Segment0);
>> + if (EFI_ERROR (Status)) {
>> + goto RestorePam1;
>> + }
>
> If CSM is enabled, we will fail to allocate, right?
It's not clear what you mean by
If the CSM is enabled
In that premise you probably include -D CSM_ENABLE, and having placed
the SeaBIOS CSM binary at OvmfPkg/Csm/Csm16/Csm16.bin. As a consequence,
the following are added to the build (marking libraries and drivers with
their INF files):
- OvmfPkg/Csm/CsmSupportLib/CsmSupportLib.inf
(built into IntelFrameworkModulePkg/Universal/BdsDxe/BdsDxe.inf)
- IntelFrameworkModulePkg/Csm/BiosThunk/VideoDxe/VideoDxe.inf
- IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBiosDxe.inf
- OvmfPkg/Csm/Csm16/Csm16.inf (the SeaBIOS binary)
However, these are not sufficient to get a working CSM. At least the
following patch from David Woodhouse (or one of its variants) is needed
in addition (not committed in upstream):
LegacyBios: Add UmbStart,UmbEnd fields to EFI_COMPATIBILITY16_TABLE
Did you include the presence of this patch in your question?
Additionally, there's another (similarly downstream only) patch
OvmfPkg: Don't build in QemuVideoDxe when we have CSM
Did you include the presence of this patch in your question?
In order to test your question, I need to build a tree that matches your
question precisely.
If you did assume the presence of both patches in the build, then the
answer is
If the CSM is enabled, then this code never runs, because it is
excluded from the build.
> If we don't error here, and install the int10-stub, then wouldn't
> Windows 2008 still be able to use QemuVideoDxe?
Independently of the above: if allocating the page at guest-phys address
0 fails (for any reason), then
- we must not overwrite data in it (we don't own the page -- something
else stores data there)
- consequently we can't point the IVT entry to our handler,
- hence Windows 2008 won't find (and won't be able to emulate) our
handler code.
(At least I assume that by
Windows 2008 still be able to use QemuVideoDxe
you mean the emulation of the Int10h handler.)
>
>> + //
>> + // This is a UEFI driver -- the arch protocols have been installed
>> + // previously. Among those, the CPU arch protocol has configured the IDT,
>> so
>> + // we can overwrite the IVT used in real mode.
>> + //
>> + Int0x10 = (IVT_ENTRY *)(UINTN)Segment0 + 0x10;
>> +
>> + //
>> + // See SVN r14218.
>> + //
>> + ASSERT (Int0x10->Segment == 0x0000);
>> + ASSERT (Int0x10->Offset == 0x0000);
>> +
>> + Int0x10->Segment = SegmentC >> 4;
>> + Int0x10->Offset = (EFI_PHYSICAL_ADDRESS)(UINTN)(VbeModeInfo + 1) -
>> SegmentC;
>> +
>> + DEBUG ((EFI_D_INFO, "%a: VBE shim installed\n", __FUNCTION__));
>> + return EFI_SUCCESS;
>> +
>> +RestorePam1:
>> + PciWrite8 (Pam1Address, Pam1);
>> +
>> + DEBUG ((EFI_D_ERROR, "%a: VBE shim installation failed: %r\n",
>> __FUNCTION__,
>> + Status));
>> + return Status;
>> +}
>> diff --git a/OvmfPkg/QemuVideoDxe/VbeShim.asm
>> b/OvmfPkg/QemuVideoDxe/VbeShim.asm
>> new file mode 100644
>> index 0000000..bb3d601
>> --- /dev/null
>> +++ b/OvmfPkg/QemuVideoDxe/VbeShim.asm
>> @@ -0,0 +1,302 @@
>> +;------------------------------------------------------------------------------
>> +; @file
>> +; A minimal Int10h stub that allows the Windows 2008 R2 SP1 UEFI guest's
>> buggy,
>> +; default VGA driver to switch to 1024x768x32, on the stdvga and QXL video
>> +; cards of QEMU.
>> +;
>> +; Copyright (C) 2014, Red Hat, Inc.
>> +; Copyright (c) 2013 - 2014, Intel Corporation. All rights reserved.<BR>
>> +;
>> +; This program and the accompanying materials are licensed and made
>> available
>> +; under the terms and conditions of the BSD License which accompanies this
>> +; distribution. The full text of the license may be found at
>> +; http://opensource.org/licenses/bsd-license.php
>> +;
>> +; THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
>> WITHOUT
>> +; WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>> +;
>> +;------------------------------------------------------------------------------
>> +
>> +BITS 16
>> +ORG 0
>> +
>> +VbeInfo:
>> +TIMES 256 nop
>> +
>> +VbeModeInfo:
>> +TIMES 256 nop
>> +
>> +
>> +Handler:
>> + cmp ax, 0x4f00
>> + je GetInfo
>> + cmp ax, 0x4f01
>> + je GetModeInfo
>> + cmp ax, 0x4f02
>> + je SetMode
>> + cmp ax, 0x4f03
>> + je GetMode
>> + cmp ax, 0x4f10
>> + je GetPmCapabilities
>> + cmp ax, 0x4f15
>> + je ReadEdid
>> + cmp ah, 0x00
>> + je SetModeLegacy
>> + mov si, StrUnkownFunction
>> + call PrintStringSi
>> +Hang:
>> + jmp Hang
>> +
>> +
>> +GetInfo:
>> + push es
>> + push di
>> + push ds
>> + push si
>> + push cx
>> +
>> + mov si, StrEnterGetInfo
>> + call PrintStringSi
>> +
>> + ; target (es:di) set on input
>> + push cs
>> + pop ds
>> + mov si, VbeInfo
>> + ; source (ds:si) set now
>> +
>> + mov cx, 256
>> + cld
>> + rep movsb
>> +
>> + pop cx
>> + pop si
>> + pop ds
>> + pop di
>> + pop es
>> + jmp Success
>> +
>> +
>> +GetModeInfo:
>> + push es
>> + push di
>> + push ds
>> + push si
>> + push cx
>> +
>> + mov si, StrEnterGetModeInfo
>> + call PrintStringSi
>> +
>> + and cx, ~0x4000 ; clear potentially set LFB bit in mode number
>> + cmp cx, 0x00f1
>> + je KnownMode1
>> + mov si, StrUnkownMode
>> + call PrintStringSi
>> + jmp Hang
>> +KnownMode1:
>> + ; target (es:di) set on input
>> + push cs
>> + pop ds
>> + mov si, VbeModeInfo
>> + ; source (ds:si) set now
>> +
>> + mov cx, 256
>> + cld
>> + rep movsb
>> +
>> + pop cx
>> + pop si
>> + pop ds
>> + pop di
>> + pop es
>> + jmp Success
>> +
>> +
>> +%define ATT_ADDRESS_REGISTER 0x03c0
>> +%define VBE_DISPI_IOPORT_INDEX 0x01ce
>> +%define VBE_DISPI_IOPORT_DATA 0x01d0
>> +
>> +%define VBE_DISPI_INDEX_XRES 0x1
>> +%define VBE_DISPI_INDEX_YRES 0x2
>> +%define VBE_DISPI_INDEX_BPP 0x3
>> +%define VBE_DISPI_INDEX_ENABLE 0x4
>> +%define VBE_DISPI_INDEX_BANK 0x5
>> +%define VBE_DISPI_INDEX_VIRT_WIDTH 0x6
>> +%define VBE_DISPI_INDEX_VIRT_HEIGHT 0x7
>> +%define VBE_DISPI_INDEX_X_OFFSET 0x8
>> +%define VBE_DISPI_INDEX_Y_OFFSET 0x9
>> +
>> +%define VBE_DISPI_ENABLED 0x01
>> +%define VBE_DISPI_LFB_ENABLED 0x40
>> +
>> +%macro BochsWrite 2
>> + push dx
>> + push ax
>> +
>> + mov dx, VBE_DISPI_IOPORT_INDEX
>> + mov ax, %1
>> + out dx, ax
>> +
>> + mov dx, VBE_DISPI_IOPORT_DATA
>> + mov ax, %2
>> + out dx, ax
>> +
>> + pop ax
>> + pop dx
>> +%endmacro
>> +
>> +SetMode:
>> + push si
>> + push dx
>> + push ax
>> +
>> + mov si, StrEnterSetMode
>> + call PrintStringSi
>> +
>> + cmp bx, 0x40f1
>> + je KnownMode2
>> + mov si, StrUnkownMode
>> + call PrintStringSi
>> + jmp Hang
>> +KnownMode2:
>> +
>> + ; unblank
>> + mov dx, ATT_ADDRESS_REGISTER
>> + mov al, 0x20
>> + out dx, al
>> +
>> + BochsWrite VBE_DISPI_INDEX_ENABLE, 0
>> + BochsWrite VBE_DISPI_INDEX_BANK, 0
>> + BochsWrite VBE_DISPI_INDEX_X_OFFSET, 0
>> + BochsWrite VBE_DISPI_INDEX_Y_OFFSET, 0
>> + BochsWrite VBE_DISPI_INDEX_BPP, 32
>> + BochsWrite VBE_DISPI_INDEX_XRES, 1024
>> + BochsWrite VBE_DISPI_INDEX_VIRT_WIDTH, 1024
>> + BochsWrite VBE_DISPI_INDEX_YRES, 768
>> + BochsWrite VBE_DISPI_INDEX_VIRT_HEIGHT, 768
>> + BochsWrite VBE_DISPI_INDEX_ENABLE, VBE_DISPI_ENABLED |
>> VBE_DISPI_LFB_ENABLED
>> +
>> + pop ax
>> + pop dx
>> + pop si
>> + jmp Success
>> +
>> +
>> +GetMode:
>> + push si
>> + mov si, StrEnterGetMode
>> + call PrintStringSi
>> + mov bx, 0x40f1
>> + pop si
>> + jmp Success
>> +
>> +
>> +GetPmCapabilities:
>> + push si
>> + mov si, StrGetPmCapabilities
>> + call PrintStringSi
>> + pop si
>> + jmp Unsupported
>> +
>> +
>> +ReadEdid:
>> + push si
>> + mov si, StrReadEdid
>> + call PrintStringSi
>> + pop si
>> + jmp Unsupported
>> +
>> +
>> +SetModeLegacy:
>> + push si
>> + mov si, StrEnterSetModeLegacy
>> + call PrintStringSi
>> +
>> + cmp al, 0x03
>> + je KnownMode3
>> + cmp al, 0x12
>> + je KnownMode4
>> + mov si, StrUnkownMode
>> + call PrintStringSi
>> + jmp Hang
>> +KnownMode3:
>> + mov al, 0x30
>> + jmp SetModeLegacyDone
>> +KnownMode4:
>> + mov al, 0x20
>> +SetModeLegacyDone:
>> + mov si, StrExitSuccess
>> + call PrintStringSi
>> + pop si
>> + iret
>> +
>> +
>> +Success:
>> + push si
>> + mov si, StrExitSuccess
>> + call PrintStringSi
>> + pop si
>> +
>> + mov ax, 0x004f
>> + iret
>> +
>> +
>> +Unsupported:
>> + push si
>> + mov si, StrExitUnsupported
>> + call PrintStringSi
>> + pop si
>> +
>> + mov ax, 0x014f
>> + iret
>> +
>> +
>> +PrintStringSi:
>> + pusha
>> + push ds ; save original
>> + push cs
>> + pop ds
>> + mov dx, 0x0402
>> +PrintStringSiLoop:
>> + lodsb
>> + cmp al, 0
>> + je PrintStringSiDone
>> + out dx, al
>> + jmp PrintStringSiLoop
>> +PrintStringSiDone:
>> + pop ds ; restore original
>> + popa
>> + ret
>
> Can you guard this debug with an ifdef, and disable it by default?
The routine is called from several places. Do you want me to make all
the function calls DEBUG-dependent too, or does it suffice if I make
only the body of the function (everything but the "ret" instruction)
build-time conditional, and leave the function calls in place?
>
>> +
>> +
>> +StrExitSuccess:
>> + db 'Exit', 0x0a, 0
>> +
>> +StrExitUnsupported:
>> + db 'Unsupported', 0x0a, 0
>> +
>> +StrUnkownFunction:
>> + db 'Unknown Function', 0x0a, 0
>> +
>> +StrEnterGetInfo:
>> + db 'GetInfo', 0x0a, 0
>> +
>> +StrEnterGetModeInfo:
>> + db 'GetModeInfo', 0x0a, 0
>> +
>> +StrEnterGetMode:
>> + db 'GetMode', 0x0a, 0
>> +
>> +StrEnterSetMode:
>> + db 'SetMode', 0x0a, 0
>> +
>> +StrEnterSetModeLegacy:
>> + db 'SetModeLegacy', 0x0a, 0
>> +
>> +StrUnkownMode:
>> + db 'Unkown Mode', 0x0a, 0
>> +
>> +StrGetPmCapabilities:
>> + db 'GetPmCapabilities', 0x0a, 0
>> +
>> +StrReadEdid:
>> + db 'ReadEdid', 0x0a, 0
>
> Bonus point for dropping these strings when debug is disabled. :)
That seems to imply that I can't load the address of these strings into
SI (when setting up the function calls), littering the code with ifdefs...
>
> Seems like a macro could make that happen.
Ah, you mean the function call should be a macro, which (dependent on
the DEBUG define) would either expand to nothing, or to a function call
(taking the string's label as a parameter). I can try that.
>
>> diff --git a/OvmfPkg/QemuVideoDxe/VbeShim.sh
>> b/OvmfPkg/QemuVideoDxe/VbeShim.sh
>> new file mode 100755
>> index 0000000..37e7586
>> --- /dev/null
>> +++ b/OvmfPkg/QemuVideoDxe/VbeShim.sh
>
> This script is great. :)
>
>> @@ -0,0 +1,82 @@
>> +#!/bin/sh
>> +###
>> +# @file
>> +# Shell script to assemble and dump the fake Int10h handler from NASM
>> source to
>> +# a C array.
>> +#
>> +# Copyright (C) 2014, Red Hat, Inc.
>> +# Copyright (c) 2013 - 2014, Intel Corporation. All rights reserved.<BR>
>> +#
>> +# This program and the accompanying materials are licensed and made
>> available
>> +# under the terms and conditions of the BSD License which accompanies this
>> +# distribution. The full text of the license may be found at
>> +# http://opensource.org/licenses/bsd-license.php
>> +#
>> +# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
>> WITHOUT
>> +# WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>> +#
>> +###
>> +
>> +set -e -u
>> +
>> +STEM=$(dirname -- "$0")/$(basename -- "$0" .sh)
>> +
>> +#
>> +# Install exit handler -- remove temporary files.
>> +#
>> +exit_handler()
>> +{
>> + rm -f "$STEM".bin "$STEM".disasm "$STEM".offsets "$STEM".insns
>> "$STEM".bytes
>> +}
>> +trap exit_handler EXIT
>> +
>> +#
>> +# Assemble the source file.
>> +#
>> +nasm -o "$STEM".bin "$STEM".asm
>> +
>> +#
>> +# Disassemble it, in order to get a binary dump associated with the source.
>> +#
>> +ndisasm "$STEM".bin >"$STEM".disasm
>> +
>> +#
>> +# Create three files, each with one column of the disassembly.
>> +#
>> +# The first column contains the offsets, and it starts the comment.
>> +#
>> +cut -c 1-8 "$STEM".disasm \
>> +| sed -e 's,^, /* ,' >"$STEM".offsets
>> +
>> +#
>> +# The second column contains the assembly-language instructions, and it
>> closes
>> +# the comment. We first pad it to 30 characters.
>> +#
>> +cut -c 29- "$STEM".disasm \
>> +| sed -e 's,$, ,' \
>> + -e 's,^\(.\{30\}\).*$,\1 */,' >"$STEM".insns
>> +
>> +#
>> +# The third column contains the bytes corresponding to the instruction,
>> +# represented as C integer constants. First strip trailing whitespace from
>> the
>> +# middle column of the input disassembly, then process pairs of nibbles.
>> +#
>> +cut -c 11-28 "$STEM".disasm \
>> +| sed -e 's, \+$,,' -e 's/\(..\)/ 0x\1,/g' >"$STEM".bytes
>> +
>> +#
>> +# Write the output file, recombining the columns. The output should have
>> CRLF
>> +# line endings.
>> +#
>> +{
>> + printf '//\n'
>> + printf '// THIS IS A GENERATED FILE. DO NOT EDIT.\n'
>
> How about:
> THIS FILE IS GENERATED BY $STEM
"$STEM" would include the directory name too, but I can certainly do
$(basename -- "$0").
In addition, for v2 I'm thinking about changing the memory allocation
type (of the page at 0) from EfiReservedMemoryType back to
EfiBootServicesCode (same as in your int10-stub branch). The argument
goes like this:
1. correctly written UEFI OSes (ie. all except Windows 2008 R2) don't
care about the Int10h entry, hence they shouldn't lose a page forever
(EfiReservedMemoryType does that)
2.a. "boot services code" vs. "reserved" makes no difference before
ExitBootServices() -- both types protect the IVT from other boot-time
allocations.
2.b. Windows 2008 R2 accesses the IVT regardless of the type of the
memory allocation that we cover it with. It will certainly not free and
then *overwrite* the page at zero (after we've covered it with "boot
services code"), because then it couldn't fetch the Int10h entry from it
(for emulation).
IOW, OSes different from Windows 2008 R2 don't care at all, hence they
shouldn't be penalized; and Windows 2008 R2 doesn't overwrite the IVT
anyway (at runtime either) before fetching the Int10h entry, so we don't
need to protect Win2k8r2 "from itself". Hence we should only protect
(allocate) this page from other UEFI components.
Thanks
Laszlo
------------------------------------------------------------------------------
"Accelerate Dev Cycles with Automated Cross-Browser Testing - For FREE
Instantly run your Selenium tests across 300+ browser/OS combos.
Get unparalleled scalability from the best Selenium testing platform available
Simple to use. Nothing to install. Get started now for free."
http://p.sf.net/sfu/SauceLabs
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel
