I suggest the following: On 11/02/18 04:24, yuchenlin via edk2-devel wrote: > From: yuchenlin <[email protected]> > > BAR | std vga | vmsvga > --------------------------------- > 0 | Framebuffer | I/O space > 1 | Reserved | Framebuffer > 2 | MMIO | FIFO > > Because of the PCI BARs difference between std vga and > vmsvga, we can not simply recognize the "QEMU VMWare SVGA" > as the QEMU_VIDEO_BOCHS_MMIO variant. > > Instead, we remain variant QEMU_VIDEO_VMWARE_SVGA, and use > it for: > > (1) Get framebuffer from correct PCI BAR > (2) Prevent using BAR2 for MMIO
[a] The commit message should be udpated as follows: - We cannot recognize VMW SVGA as BOCHS because that would confuse the IsQxl setting in QemuVideoControllerDriverStart(), - We cannot recognize VMW SVGA as BOCHS_MMIO because BAR2 on VMW SVGA is not the BOCHS MMIO BAR (we can only use port IO). Therefore the list of reasons for which we should introduce QEMU_VIDEO_VMWARE_SVGA should name three reasons: both of the currently listed reasons, plus "prevent mis-recognizing VMW SVGA as QXL" as the third one. > > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: yuchenlin <[email protected]> > --- > OvmfPkg/QemuVideoDxe/Driver.c | 18 ++++++++++++++++-- > OvmfPkg/QemuVideoDxe/Gop.c | 3 ++- > OvmfPkg/QemuVideoDxe/Qemu.h | 2 ++ > 3 files changed, 20 insertions(+), 3 deletions(-) > > diff --git a/OvmfPkg/QemuVideoDxe/Driver.c b/OvmfPkg/QemuVideoDxe/Driver.c > index 2304afd1e6..76d4a2d27e 100644 > --- a/OvmfPkg/QemuVideoDxe/Driver.c > +++ b/OvmfPkg/QemuVideoDxe/Driver.c > @@ -69,6 +69,12 @@ QEMU_VIDEO_CARD gQemuVideoCardList[] = { > 0x1050, > QEMU_VIDEO_BOCHS_MMIO, > L"QEMU VirtIO VGA" > + },{ > + PCI_CLASS_DISPLAY_VGA, > + 0x15ad, > + 0x0405, > + QEMU_VIDEO_VMWARE_SVGA, > + L"QEMU VMWare SVGA" > },{ > 0 /* end of list */ > } > @@ -256,6 +262,12 @@ QemuVideoControllerDriverStart ( > goto ClosePciIo; > } > Private->Variant = Card->Variant; > + if (Private->Variant == QEMU_VIDEO_VMWARE_SVGA) { > + Private->FrameBufferVramBarIndex = PCI_BAR_IDX1; > + } else { > + Private->FrameBufferVramBarIndex = PCI_BAR_IDX0; > + } > + > > // > // IsQxl is based on the detected Card->Variant, which at a later point > might > @@ -320,7 +332,8 @@ QemuVideoControllerDriverStart ( > // Check if accessing the bochs interface works. > // > if (Private->Variant == QEMU_VIDEO_BOCHS_MMIO || > - Private->Variant == QEMU_VIDEO_BOCHS) { > + Private->Variant == QEMU_VIDEO_BOCHS || > + Private->Variant == QEMU_VIDEO_VMWARE_SVGA) { > UINT16 BochsId; > BochsId = BochsRead(Private, VBE_DISPI_INDEX_ID); > if ((BochsId & 0xFFF0) != VBE_DISPI_ID0) { > @@ -383,6 +396,7 @@ QemuVideoControllerDriverStart ( > break; > case QEMU_VIDEO_BOCHS_MMIO: > case QEMU_VIDEO_BOCHS: > + case QEMU_VIDEO_VMWARE_SVGA: > Status = QemuVideoBochsModeSetup (Private, IsQxl); > break; > default: > @@ -764,7 +778,7 @@ ClearScreen ( > Private->PciIo->Mem.Write ( > Private->PciIo, > EfiPciIoWidthFillUint32, > - 0, > + Private->FrameBufferVramBarIndex, > 0, > 0x400000 >> 2, > &Color > diff --git a/OvmfPkg/QemuVideoDxe/Gop.c b/OvmfPkg/QemuVideoDxe/Gop.c > index d490fa7a2e..3abc5eeb36 100644 > --- a/OvmfPkg/QemuVideoDxe/Gop.c > +++ b/OvmfPkg/QemuVideoDxe/Gop.c > @@ -60,7 +60,7 @@ QemuVideoCompleteModeData ( > > Private->PciIo->GetBarAttributes ( > Private->PciIo, > - 0, > + Private->FrameBufferVramBarIndex, > NULL, > (VOID**) &FrameBufDesc > ); > @@ -177,6 +177,7 @@ Routine Description: > break; > case QEMU_VIDEO_BOCHS_MMIO: > case QEMU_VIDEO_BOCHS: > + case QEMU_VIDEO_VMWARE_SVGA: > InitializeBochsGraphicsMode (Private, > &QemuVideoBochsModes[ModeData->InternalModeIndex]); > break; > default: > diff --git a/OvmfPkg/QemuVideoDxe/Qemu.h b/OvmfPkg/QemuVideoDxe/Qemu.h > index d7da761705..3aac9eeca6 100644 > --- a/OvmfPkg/QemuVideoDxe/Qemu.h > +++ b/OvmfPkg/QemuVideoDxe/Qemu.h > @@ -92,6 +92,7 @@ typedef enum { > QEMU_VIDEO_CIRRUS_5446, > QEMU_VIDEO_BOCHS, > QEMU_VIDEO_BOCHS_MMIO, > + QEMU_VIDEO_VMWARE_SVGA, > } QEMU_VIDEO_VARIANT; > > typedef struct { > @@ -120,6 +121,7 @@ typedef struct { > QEMU_VIDEO_VARIANT Variant; > FRAME_BUFFER_CONFIGURE *FrameBufferBltConfigure; > UINTN FrameBufferBltConfigureSize; > + UINT8 FrameBufferVramBarIndex; > } QEMU_VIDEO_PRIVATE_DATA; > > /// > [b] How about the following -- incremental, to be squashed -- patch: > diff --git a/OvmfPkg/QemuVideoDxe/Driver.c b/OvmfPkg/QemuVideoDxe/Driver.c > index 76d4a2d27e7e..8e02700d3960 100644 > --- a/OvmfPkg/QemuVideoDxe/Driver.c > +++ b/OvmfPkg/QemuVideoDxe/Driver.c > @@ -262,12 +262,6 @@ QemuVideoControllerDriverStart ( > goto ClosePciIo; > } > Private->Variant = Card->Variant; > - if (Private->Variant == QEMU_VIDEO_VMWARE_SVGA) { > - Private->FrameBufferVramBarIndex = PCI_BAR_IDX1; > - } else { > - Private->FrameBufferVramBarIndex = PCI_BAR_IDX0; > - } > - > > // > // IsQxl is based on the detected Card->Variant, which at a later point > might > @@ -328,12 +322,19 @@ QemuVideoControllerDriverStart ( > } > } > > + // > + // VMWare SVGA is handled like Bochs (with port IO only). > + // > + if (Private->Variant == QEMU_VIDEO_VMWARE_SVGA) { > + Private->Variant = QEMU_VIDEO_BOCHS; > + Private->FrameBufferVramBarIndex = PCI_BAR_IDX1; > + } > + > // > // Check if accessing the bochs interface works. > // > if (Private->Variant == QEMU_VIDEO_BOCHS_MMIO || > - Private->Variant == QEMU_VIDEO_BOCHS || > - Private->Variant == QEMU_VIDEO_VMWARE_SVGA) { > + Private->Variant == QEMU_VIDEO_BOCHS) { > UINT16 BochsId; > BochsId = BochsRead(Private, VBE_DISPI_INDEX_ID); > if ((BochsId & 0xFFF0) != VBE_DISPI_ID0) { > @@ -396,7 +397,6 @@ QemuVideoControllerDriverStart ( > break; > case QEMU_VIDEO_BOCHS_MMIO: > case QEMU_VIDEO_BOCHS: > - case QEMU_VIDEO_VMWARE_SVGA: > Status = QemuVideoBochsModeSetup (Private, IsQxl); > break; > default: > diff --git a/OvmfPkg/QemuVideoDxe/Gop.c b/OvmfPkg/QemuVideoDxe/Gop.c > index 3abc5eeb36a6..6f542d9eac05 100644 > --- a/OvmfPkg/QemuVideoDxe/Gop.c > +++ b/OvmfPkg/QemuVideoDxe/Gop.c > @@ -177,7 +177,6 @@ Routine Description: > break; > case QEMU_VIDEO_BOCHS_MMIO: > case QEMU_VIDEO_BOCHS: > - case QEMU_VIDEO_VMWARE_SVGA: > InitializeBochsGraphicsMode (Private, > &QemuVideoBochsModes[ModeData->InternalModeIndex]); > break; > default: This would produce the following -- squashed -- patch, for v3: > diff --git a/OvmfPkg/QemuVideoDxe/Qemu.h b/OvmfPkg/QemuVideoDxe/Qemu.h > index d7da761705a1..3aac9eeca687 100644 > --- a/OvmfPkg/QemuVideoDxe/Qemu.h > +++ b/OvmfPkg/QemuVideoDxe/Qemu.h > @@ -92,6 +92,7 @@ typedef enum { > QEMU_VIDEO_CIRRUS_5446, > QEMU_VIDEO_BOCHS, > QEMU_VIDEO_BOCHS_MMIO, > + QEMU_VIDEO_VMWARE_SVGA, > } QEMU_VIDEO_VARIANT; > > typedef struct { > @@ -120,6 +121,7 @@ typedef struct { > QEMU_VIDEO_VARIANT Variant; > FRAME_BUFFER_CONFIGURE *FrameBufferBltConfigure; > UINTN FrameBufferBltConfigureSize; > + UINT8 FrameBufferVramBarIndex; > } QEMU_VIDEO_PRIVATE_DATA; > > /// > diff --git a/OvmfPkg/QemuVideoDxe/Driver.c b/OvmfPkg/QemuVideoDxe/Driver.c > index 2304afd1e686..8e02700d3960 100644 > --- a/OvmfPkg/QemuVideoDxe/Driver.c > +++ b/OvmfPkg/QemuVideoDxe/Driver.c > @@ -69,6 +69,12 @@ QEMU_VIDEO_CARD gQemuVideoCardList[] = { > 0x1050, > QEMU_VIDEO_BOCHS_MMIO, > L"QEMU VirtIO VGA" > + },{ > + PCI_CLASS_DISPLAY_VGA, > + 0x15ad, > + 0x0405, > + QEMU_VIDEO_VMWARE_SVGA, > + L"QEMU VMWare SVGA" > },{ > 0 /* end of list */ > } > @@ -316,6 +322,14 @@ QemuVideoControllerDriverStart ( > } > } > > + // > + // VMWare SVGA is handled like Bochs (with port IO only). > + // > + if (Private->Variant == QEMU_VIDEO_VMWARE_SVGA) { > + Private->Variant = QEMU_VIDEO_BOCHS; > + Private->FrameBufferVramBarIndex = PCI_BAR_IDX1; > + } > + > // > // Check if accessing the bochs interface works. > // > @@ -764,7 +778,7 @@ ClearScreen ( > Private->PciIo->Mem.Write ( > Private->PciIo, > EfiPciIoWidthFillUint32, > - 0, > + Private->FrameBufferVramBarIndex, > 0, > 0x400000 >> 2, > &Color > diff --git a/OvmfPkg/QemuVideoDxe/Gop.c b/OvmfPkg/QemuVideoDxe/Gop.c > index d490fa7a2e6f..6f542d9eac05 100644 > --- a/OvmfPkg/QemuVideoDxe/Gop.c > +++ b/OvmfPkg/QemuVideoDxe/Gop.c > @@ -60,7 +60,7 @@ QemuVideoCompleteModeData ( > > Private->PciIo->GetBarAttributes ( > Private->PciIo, > - 0, > + Private->FrameBufferVramBarIndex, > NULL, > (VOID**) &FrameBufDesc > ); For me this is much easier to understand. ... While we discuss this, I'll go ahead and push the first four patches. The code being reverted is dead anyway. I'll report back about the commit hashes. Thanks, Laszlo _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
