[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH] firmware/vgabios: Port PCI based VBE LFB discovery method from QEMU fork



On 13/06/17 15:04, Andrew Cooper wrote:
> On 10/05/17 15:31, Igor Druzhinin wrote:
>> QEMU-traditional implements non-standard VBE registers for getting LFB
>> physical address from inside of VGA BIOS code. QEMU doesn't have
>> those registers implemented and returns 0 when an HVM guest is trying to
>> access them from the existing ROMBIOS code. This eventually leads to
>> a triple fault inside a guest which happened to use ROMBIOS instead of
>> SeaBIOS when in stdvga mode.
>>
>> QEMU maintains its own fork of VGA BIOS where the VBE LFB discovery is
>> implemented through a regular PCI BAR reading. In order to support that
>> we need to build a PCI compliant VGA BIOS version for stdvga and include
>> it into ROMBIOS instead of the old one.
>>
>> Signed-off-by: Igor Druzhinin <igor.druzhinin@xxxxxxxxxx>
> 
> How much of this is ported from existing changes elsewhere?
> 

Only ASM functions below for PCI conf space accessing are ported from
vgabios fork of QEMU. If I need to incorporate this somehow into the
commit message, could you point me to an example of doing this properly?

>> ---
>> CC: Jan Beulich <jbeulich@xxxxxxxx>
>> CC: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> CC: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
>> CC: Wei Liu <wei.liu2@xxxxxxxxxx>
>> ---
>>  tools/firmware/hvmloader/Makefile |  2 +-
>>  tools/firmware/vgabios/Makefile   | 29 +++++++++++++++--
>>  tools/firmware/vgabios/vbe.c      |  9 ++++++
>>  tools/firmware/vgabios/vgabios.c  | 68 
>> +++++++++++++++++++++++++++++++++++++++
>>  4 files changed, 105 insertions(+), 3 deletions(-)
>>
>> diff --git a/tools/firmware/hvmloader/Makefile 
>> b/tools/firmware/hvmloader/Makefile
>> index 80d7b44..5f6eacd 100644
>> --- a/tools/firmware/hvmloader/Makefile
>> +++ b/tools/firmware/hvmloader/Makefile
>> @@ -45,7 +45,7 @@ CIRRUSVGA_DEBUG ?= n
>>  ROMBIOS_DIR := ../rombios
>>  
>>  ifeq ($(CONFIG_ROMBIOS),y)
>> -STDVGA_ROM    := ../vgabios/VGABIOS-lgpl-latest.bin
>> +STDVGA_ROM    := ../vgabios/VGABIOS-lgpl-latest.stdvga.bin
>>  ifeq ($(CIRRUSVGA_DEBUG),y)
>>  CIRRUSVGA_ROM := ../vgabios/VGABIOS-lgpl-latest.cirrus.debug.bin
>>  else
>> diff --git a/tools/firmware/vgabios/Makefile 
>> b/tools/firmware/vgabios/Makefile
>> index 3284812..0f4026e 100644
>> --- a/tools/firmware/vgabios/Makefile
>> +++ b/tools/firmware/vgabios/Makefile
>> @@ -11,7 +11,7 @@ RELVERS = `pwd | sed "s-.*/--" | sed "s/vgabios//" | sed 
>> "s/-//"`
>>  VGABIOS_DATE = "-DVGABIOS_DATE=\"$(VGABIOS_REL_DATE)\""
>>  
>>  .PHONY: all
>> -all: bios cirrus-bios
>> +all: bios cirrus-bios stdvga-bios
>>  
>>  .PHONY: bios
>>  bios: biossums vgabios.bin vgabios.debug.bin 
>> @@ -19,6 +19,9 @@ bios: biossums vgabios.bin vgabios.debug.bin
>>  .PHONY: cirrus-bios
>>  cirrus-bios: vgabios-cirrus.bin vgabios-cirrus.debug.bin
>>  
>> +.PHONY: stdvga-bios
>> +stdvga-bios: vgabios-stdvga.bin vgabios-stdvga.debug.bin
>> +
>>  .PHONY: clean
>>  clean:
>>      rm -f  biossums vbetables-gen vbetables.h *.o *.s *.ld86 \
>> @@ -30,13 +33,15 @@ distclean: clean
>>  
>>  .PHONY: release
>>  release: 
>> -    VGABIOS_VERS=\"-DVGABIOS_VERS=\\\"$(RELVERS)\\\"\" make bios cirrus-bios
>> +    VGABIOS_VERS=\"-DVGABIOS_VERS=\\\"$(RELVERS)\\\"\" make bios 
>> cirrus-bios stdvga-bios
>>      /bin/rm -f  *.o *.s *.ld86 \
>>            temp.awk.* vgabios.*.orig _vgabios_.*.c core *.bak .#*
>>      cp VGABIOS-lgpl-latest.bin ../$(RELEASE).bin
>>      cp VGABIOS-lgpl-latest.debug.bin ../$(RELEASE).debug.bin
>>      cp VGABIOS-lgpl-latest.cirrus.bin ../$(RELEASE).cirrus.bin
>>      cp VGABIOS-lgpl-latest.cirrus.debug.bin ../$(RELEASE).cirrus.debug.bin
>> +    cp VGABIOS-lgpl-latest.stdvga.bin ../$(RELEASE).stdvga.bin
>> +    cp VGABIOS-lgpl-latest.stdvga.debug.bin ../$(RELEASE).stdvga.debug.bin
>>      tar czvf ../$(RELEASE).tgz --exclude CVS -C .. $(RELEASE)/
>>  
>>  vgabios.bin: biossums vgabios.c vgabios.h vgafonts.h vgatables.h vbe.h 
>> vbe.c vbetables.h
>> @@ -59,6 +64,26 @@ vgabios.debug.bin: biossums vgabios.c vgabios.h 
>> vgafonts.h vgatables.h vbe.h vbe
>>      ./biossums VGABIOS-lgpl-latest.debug.bin
>>      ls -l VGABIOS-lgpl-latest.debug.bin
>>  
>> +vgabios-stdvga.bin: biossums vgabios.c vgabios.h vgafonts.h vgatables.h 
>> vbe.h vbe.c vbetables.h
>> +    $(GCC) -E -P vgabios.c $(VGABIOS_VERS) -DVBE -DPCIBIOS -DPCI_VID=0x1234 
>> -DPCI_DID=0x1111 $(VGABIOS_DATE) > _vgabios-stdvga_.c
> 
> The general makefile-ary around here is in serious need of improvement,
> although it would be better to not merge that with a functional fix. 
> However, given that all the cirrus is behind -DCIRRUS, wouldn't it be
> better to use -DSTDVGA here?
> 

Right, probably worth it. Also, I can remove the old vgabios binary from
building since it seems we don't need this anymore.

>> +    $(BCC) -o vgabios-stdvga.s -C-c -D__i86__ -S -0 _vgabios-stdvga_.c
>> +    sed -e 's/^\.text//' -e 's/^\.data//' vgabios-stdvga.s > 
>> _vgabios-stdvga_.s
>> +    $(AS86) _vgabios-stdvga_.s -b vgabios-stdvga.bin -u -w- -g -0 -j -O -l 
>> vgabios-stdvga.txt
>> +    rm -f _vgabios-stdvga_.s _vgabios-stdvga_.c vgabios-stdvga.s
>> +    cp vgabios-stdvga.bin VGABIOS-lgpl-latest.stdvga.bin
>> +    ./biossums VGABIOS-lgpl-latest.stdvga.bin
>> +    ls -l VGABIOS-lgpl-latest.stdvga.bin
>> +
>> +vgabios-stdvga.debug.bin: biossums vgabios.c vgabios.h vgafonts.h 
>> vgatables.h vbe.h vbe.c vbetables.h
>> +    $(GCC) -E -P vgabios.c $(VGABIOS_VERS) -DVBE -DPCIBIOS -DPCI_VID=0x1234 
>> -DPCI_DID=0x1111 -DDEBUG $(VGABIOS_DATE) > _vgabios-stdvga-debug_.c
>> +    $(BCC) -o vgabios-stdvga-debug.s -C-c -D__i86__ -S -0 
>> _vgabios-stdvga-debug_.c
>> +    sed -e 's/^\.text//' -e 's/^\.data//' vgabios-stdvga-debug.s > 
>> _vgabios-stdvga-debug_.s
>> +    $(AS86) _vgabios-stdvga-debug_.s -b vgabios-stdvga-debug.bin -u -w- -g 
>> -0 -j -O -l vgabios-stdvga-debug.txt
>> +    rm -f _vgabios-stdvga-debug_.s _vgabios-stdvga-debug_.c 
>> vgabios-stdvga-debug.s
>> +    cp vgabios-stdvga-debug.bin VGABIOS-lgpl-latest.stdvga.debug.bin
>> +    ./biossums VGABIOS-lgpl-latest.stdvga.debug.bin
>> +    ls -l VGABIOS-lgpl-latest.stdvga.debug.bin
>> +
>>  vgabios-cirrus.bin: biossums vgabios.c vgabios.h vgafonts.h vgatables.h 
>> clext.c
>>      $(GCC) -E -P vgabios.c $(VGABIOS_VERS) -DCIRRUS -DPCIBIOS 
>> $(VGABIOS_DATE) > _vgabios-cirrus_.c
>>      $(BCC) -o vgabios-cirrus.s -C-c -D__i86__ -S -0 _vgabios-cirrus_.c
>> diff --git a/tools/firmware/vgabios/vbe.c b/tools/firmware/vgabios/vbe.c
>> index c506690..d7706f5 100644
>> --- a/tools/firmware/vgabios/vbe.c
>> +++ b/tools/firmware/vgabios/vbe.c
>> @@ -914,6 +914,7 @@ Bit16u *AX;Bit16u CX; Bit16u ES;Bit16u DI;
>>          ModeInfoListItem  *cur_info;
>>          Boolean           using_lfb;
>>          ModeInfoBlockCompact   info;
>> +        Bit16u            lfb_addr=0;
>>  
>>  #ifdef DEBUG
>>          printf("VBE vbe_biosfn_return_mode_information ES%x DI%x 
>> CX%x\n",ES,DI,CX);
>> @@ -957,6 +958,14 @@ Bit16u *AX;Bit16u CX; Bit16u ES;Bit16u DI;
>>                  outw(VBE_DISPI_IOPORT_INDEX,VBE_DISPI_INDEX_LFB_ADDRESS_L);
>>                  info.PhysBasePtr |= inw(VBE_DISPI_IOPORT_DATA);
>>  #endif                                                      
>> +#ifdef PCI_VID
>> +                if ((Bit16u)(info.PhysBasePtr >> 16) == 0 &&
>> +                    (Bit16u)info.PhysBasePtr == 0)
>> +                  lfb_addr = pci_get_lfb_addr(PCI_VID);
>> +
>> +                if (lfb_addr > 0)
>> +                  info.PhysBasePtr = ((Bit32u)lfb_addr << 16);
>> +#endif
>>                  result = 0x4f;
>>  
>>                  // copy updates in mode_info_block back
>> diff --git a/tools/firmware/vgabios/vgabios.c 
>> b/tools/firmware/vgabios/vgabios.c
>> index 1c75b7d..22471c5 100644
>> --- a/tools/firmware/vgabios/vgabios.c
>> +++ b/tools/firmware/vgabios/vgabios.c
>> @@ -209,8 +209,13 @@ vgabios_pci_data:
>>  .word 0x1013
>>  .word 0x00b8 // CLGD5446
>>  #else
>> +#ifdef PCI_VID
> 
> This probably wants to be an #elif defined(STDVGA) to avoid the repeated
> #endif's after the error.

This is a side effect of having the old and the new vgabios binaries
building.

Igor

> 
> ~Andrew
> 
>> +.word PCI_VID
>> +.word PCI_DID
>> +#else
>>  #error "Unknown PCI vendor and device id"
>>  #endif
>> +#endif
>>  .word 0 // reserved
>>  .word 0x18 // dlen
>>  .byte 0 // revision
>> @@ -3829,6 +3834,69 @@ void printf(s)
>>  }
>>  #endif
>>  
>> +ASM_START
>> +  ; get LFB address from PCI
>> +  ; in - ax: PCI device vendor
>> +  ; out - ax: LFB address (high 16 bit)
>> +  ;; NOTE - may be called in protected mode
>> +_pci_get_lfb_addr:
>> +  push bx
>> +  push cx
>> +  push dx
>> +  push eax
>> +    mov bx, ax
>> +    xor cx, cx
>> +    mov dl, #0x00
>> +    call pci_read_reg
>> +    cmp ax, #0xffff
>> +    jz pci_get_lfb_addr_fail
>> + pci_get_lfb_addr_next_dev:
>> +    mov dl, #0x00
>> +    call pci_read_reg
>> +    cmp ax, bx ;; check vendor
>> +    jz pci_get_lfb_addr_found
>> +    add cx, #0x8
>> +    cmp cx, #0x200 ;; search bus #0 and #1
>> +    jb pci_get_lfb_addr_next_dev
>> + pci_get_lfb_addr_fail:
>> +    xor dx, dx ;; no LFB
>> +    jmp pci_get_lfb_addr_return
>> + pci_get_lfb_addr_found:
>> +    mov dl, #0x10 ;; I/O space #0
>> +    call pci_read_reg
>> +    test ax, #0xfff1
>> +    jz pci_get_lfb_addr_success
>> +    mov dl, #0x14 ;; I/O space #1
>> +    call pci_read_reg
>> +    test ax, #0xfff1
>> +    jnz pci_get_lfb_addr_fail
>> + pci_get_lfb_addr_success:
>> +    shr eax, #16
>> +    mov dx, ax ;; LFB address
>> + pci_get_lfb_addr_return:
>> +  pop eax
>> +  mov ax, dx
>> +  pop dx
>> +  pop cx
>> +  pop bx
>> +  ret
>> +
>> +  ; read PCI register
>> +  ; in - cx: device/function
>> +  ; in - dl: register
>> +  ; out - eax: value
>> +pci_read_reg:
>> +  mov eax, #0x00800000
>> +  mov ax, cx
>> +  shl eax, #8
>> +  mov al, dl
>> +  mov dx, #0xcf8
>> +  out dx, eax
>> +  add dl, #4
>> +  in  eax, dx
>> +  ret
>> +ASM_END
>> +
>>  #ifdef VBE
>>  #include "vbe.c"
>>  #endif
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.