Re: [PATCH][v2] kvm-userspace: Load PCI option ROMs

[Avi Kivity <avi@redhat.com>]

Leendert van Doorn wrote:
> I've been experimenting with passthru graphics for KVM and for that I needed
> to add PCI ROM support to QEMU. I went a slightly different route than
> Haitoa, I enabled the BIOS to do the PCI ROM mapping and initialization and
> added support for QEMU to fully implement the ROM BAR. This patch also uses
> the host BAR addresses in the guest for direct assigned devices, because (as
> Avi already pointed out) this is needed for more complex PCI cards such as
> graphics adapters. My patch also allows you to optionally specify the ROM
> image, which I found very useful for debugging.
>
> My ROMBIOS patches are probably more invasive than necessary. I found it
> cleaner (==easier to understand) to call the rombios from 32-bit mode than
> leave some droppings (option rom address, bdf) in the BIOS extended data
> segment. The other thing you notice is that I initialize the primary VGA ROM
> last. I found that this was necessary when emulating dual headed displays
> (one display on qemu's emulation window, the other the actual device).
>
> I've attached my patches.
>
> Note that I have not included the full graphics passthru patches yet. I have
> that working for the xorg ati drivers (and ATI's ATOMBIOS) but not for the
> closed sourced or Windows drivers yet. Let me know if you are interested in
> those patches.
>
>   

This is very interesting.

> @@ -10194,6 +10195,7 @@ no_serial:
>    pop  dx
>    ret
>  
> +#if !BX_ROMBIOS32
>  rom_checksum:
>    push ax
>    push bx
> @@ -10266,7 +10268,6 @@ block_count_rounded:
>    mov  ax, #0xf000
>    mov  es, ax
>    lea  di, pnp_string
> -
>    mov  bp, sp   ;; Call ROM init routine using seg:off on stack
>    db   0xff     ;; call_far ss:[bp+0]
>    db   0x5e
> @@ -10348,6 +10349,7 @@ rom_scan_increment:
>    xor  ax, ax   ;; Restore DS back to 0000:
>    mov  ds, ax
>    ret
> +#endif /* !BX_ROMBIOS32 */
>   

This is worrying as it will cause us to diverge from upstream bochs bios.

>  
>  post_enable_cache:
>    ;; enable cache
> @@ -10652,12 +10654,6 @@ post_default_ints:
>    ;; PIC
>    call post_init_pic
>  
> -  mov  cx, #0xc000  ;; init vga bios
> -  mov  ax, #0xc780
> -  call rom_scan
> -
> -  call _print_bios_banner
> -
>  #if BX_ROMBIOS32
>    call rombios32_init
>  #else
> @@ -10665,7 +10661,12 @@ post_default_ints:
>    call pcibios_init_iomem_bases
>    call pcibios_init_irqs
>  #endif //BX_PCIBIOS
> -#endif
> +  mov  cx, #0xc000  ;; init vga bios
> +  mov  ax, #0xc780
> +  call rom_scan
> +#endif //BX_ROMBIOS32
> +
> +  call _print_bios_banner
>   

If one assigns a graphics card, it should replace the cirrus vga bios; 
that should make this change unnecessary.

>  
> @@ -953,18 +955,21 @@ static void pci_bios_init_device(PCIDevice *d)
>      default_map:
>          /* default memory mappings */
>          for(i = 0; i < PCI_NUM_REGIONS; i++) {
> +            uint32_t orig, addr, val, size;
>              int ofs;
> -            uint32_t val, size ;
>  
>              if (i == PCI_ROM_SLOT)
>                  ofs = 0x30;
>              else
>                  ofs = 0x10 + i * 4;
> +            orig = pci_config_readl(d, ofs);
>              pci_config_writel(d, ofs, 0xffffffff);
>              val = pci_config_readl(d, ofs);
>              if (val != 0) {
>                  size = (~(val & ~0xf)) + 1;
> -                if (val & PCI_ADDRESS_SPACE_IO)
> +		if (i == PCI_ROM_SLOT)
> +		    paddr = &pci_bios_mem_addr;
> +		else if (val & PCI_ADDRESS_SPACE_IO)
>                      paddr = &pci_bios_io_addr;
>                  else if (size >= 0x04000000)
>                      paddr = &pci_bios_bigmem_addr;
> @@ -972,7 +977,24 @@ static void pci_bios_init_device(PCIDevice *d)
>                      paddr = &pci_bios_mem_addr;
>                  *paddr = (*paddr + size - 1) & ~(size - 1);
>                  pci_set_io_region_addr(d, i, *paddr);
> +		BX_INFO("PCI: region %d: paddr 0x%x, size 0x%x\n",
> +		        i, *paddr, size);
>                  *paddr += size;
> +
> +		/*
> +		 * Some complex PCI devices (such as graphic controllers)
> +		 * keep lots of internal state, including their PCI mappings
> +		 * that were determined at initial PCI configuration.
> +		 * To make them work under QEMU/KVM, we preserve the host's
> +		 * PCI mappings in the guest.
> +		 * (we have to do this twice to ensure we update the
> mappings)
> +		 */
>   

Can you elaborate?  Since we're running the card's bios again, won't it 
initialize correctly?

Keeping the same mapping in the host and guest may not be possible in 
some circumstances (differently sized pci holes, or using a 64-bit BAR 
in a 64-bit host but running a 32-bit guest that cannot utilize 64-bit 
BARs).

Other than that, the patch looks pretty good.  It will need some 
massaging before merging, but nothing serious.

-- 
error compiling committee.c: too many arguments to function