Re: [PATCH] kvm: device-assignment: Add PCI option ROM support

["Yang, Sheng" <sheng.yang@intel.com>]

On Tuesday 16 June 2009 00:29:17 Alex Williamson wrote:
> The PCI code already knows about option ROMs, so we just need to
> mmap some space for it, load it with a copy of the contents, and
> complete the plubming to the generic code.
>
> With this a Linux guest can get access to the ROM contents via
> /sys/bus/pci/devices/<dev>/rom.  This might also enable the BIOS
> to execute ROMs by loading them dynamically from the device
> rather than hoping they all fit into RAM.
>
> Signed-off-by: Alex Williamson <alex.williamson@hp.com>

Hi Alex

The patch looks fine. One question: if guest write to the ROM, I think the 
guest would be killed for QEmu would receive a SIGSEGV? I am not sure if it's 
too severe...

-- 
regards
Yang, Sheng

> ---
>
>  hw/device-assignment.c |   60
> ++++++++++++++++++++++++++++++++++++------------ hw/device-assignment.h |  
>  5 +---
>  2 files changed, 46 insertions(+), 19 deletions(-)
>
> diff --git a/hw/device-assignment.c b/hw/device-assignment.c
> index 65920d0..dfcd670 100644
> --- a/hw/device-assignment.c
> +++ b/hw/device-assignment.c
> @@ -286,8 +286,8 @@ static void assigned_dev_pci_write_config(PCIDevice *d,
> uint32_t address, /* Continue to program the card */
>      }
>
> -    if ((address >= 0x10 && address <= 0x24) || address == 0x34 ||
> -        address == 0x3c || address == 0x3d ||
> +    if ((address >= 0x10 && address <= 0x24) || address == 0x30 ||
> +        address == 0x34 || address == 0x3c || address == 0x3d ||
>          pci_access_cap_config(d, address, len)) {
>          /* used for update-mappings (BAR emulation) */
>          pci_default_write_config(d, address, val, len);
> @@ -322,8 +322,8 @@ static uint32_t assigned_dev_pci_read_config(PCIDevice
> *d, uint32_t address, AssignedDevice *pci_dev = container_of(d,
> AssignedDevice, dev);
>
>      if (address < 0x4 || (pci_dev->need_emulate_cmd && address == 0x4) ||
> -	(address >= 0x10 && address <= 0x24) || address == 0x34 ||
> -        address == 0x3c || address == 0x3d ||
> +	(address >= 0x10 && address <= 0x24) || address == 0x30 ||
> +        address == 0x34 || address == 0x3c || address == 0x3d ||
>          pci_access_cap_config(d, address, len)) {
>          val = pci_default_read_config(d, address, len);
>          DEBUG("(%x.%x): address=%04x val=0x%08x len=%d\n",
> @@ -384,11 +384,20 @@ static int assigned_dev_register_regions(PCIRegion
> *io_regions,
>
>              /* map physical memory */
>              pci_dev->v_addrs[i].e_physbase = cur_region->base_addr;
> -            pci_dev->v_addrs[i].u.r_virtbase =
> -                mmap(NULL,
> -                     (cur_region->size + 0xFFF) & 0xFFFFF000,
> -                     PROT_WRITE | PROT_READ, MAP_SHARED,
> -                     cur_region->resource_fd, (off_t) 0);
> +            if (i == PCI_ROM_SLOT) {
> +                pci_dev->v_addrs[i].u.r_virtbase =
> +                    mmap(NULL,
> +                         (cur_region->size + 0xFFF) & 0xFFFFF000,
> +                         PROT_WRITE | PROT_READ, MAP_ANONYMOUS |
> MAP_PRIVATE, +                         0, (off_t) 0);
> +
> +            } else {
> +                pci_dev->v_addrs[i].u.r_virtbase =
> +                    mmap(NULL,
> +                         (cur_region->size + 0xFFF) & 0xFFFFF000,
> +                         PROT_WRITE | PROT_READ, MAP_SHARED,
> +                         cur_region->resource_fd, (off_t) 0);
> +            }
>
>              if (pci_dev->v_addrs[i].u.r_virtbase == MAP_FAILED) {
>                  pci_dev->v_addrs[i].u.r_virtbase = NULL;
> @@ -397,6 +406,14 @@ static int assigned_dev_register_regions(PCIRegion
> *io_regions, (uint32_t) (cur_region->base_addr));
>                  return -1;
>              }
> +
> +            if (i == PCI_ROM_SLOT) {
> +                memset(pci_dev->v_addrs[i].u.r_virtbase, 0,
> +                       (cur_region->size + 0xFFF) & 0xFFFFF000);
> +                mprotect(pci_dev->v_addrs[PCI_ROM_SLOT].u.r_virtbase,
> +                         (cur_region->size + 0xFFF) & 0xFFFFF000,
> PROT_READ); +            }
> +
>              pci_dev->v_addrs[i].r_size = cur_region->size;
>              pci_dev->v_addrs[i].e_size = 0;
>
> @@ -468,7 +485,7 @@ again:
>          return 1;
>      }
>
> -    for (r = 0; r < MAX_IO_REGIONS; r++) {
> +    for (r = 0; r < PCI_NUM_REGIONS; r++) {
>  	if (fscanf(f, "%lli %lli %lli\n", &start, &end, &flags) != 3)
>  	    break;
>
> @@ -480,11 +497,13 @@ again:
>              continue;
>          if (flags & IORESOURCE_MEM) {
>              flags &= ~IORESOURCE_IO;
> -	    snprintf(name, sizeof(name), "%sresource%d", dir, r);
> -            fd = open(name, O_RDWR);
> -            if (fd == -1)
> -                continue;       /* probably ROM */
> -            rp->resource_fd = fd;
> +            if (r != PCI_ROM_SLOT) {
> +                snprintf(name, sizeof(name), "%sresource%d", dir, r);
> +                fd = open(name, O_RDWR);
> +                if (fd == -1)
> +                    continue;
> +                rp->resource_fd = fd;
> +            }
>          } else
>              flags &= ~IORESOURCE_PREFETCH;
>
> @@ -1390,6 +1409,17 @@ ram_addr_t assigned_dev_load_option_roms(ram_addr_t
> rom_base_offset) continue;
>          }
>
> +        /* Copy ROM contents into the space backing the ROM BAR */
> +        if (adev->assigned_dev->v_addrs[PCI_ROM_SLOT].r_size >= size &&
> +            adev->assigned_dev->v_addrs[PCI_ROM_SLOT].u.r_virtbase) {
> +           
> mprotect(adev->assigned_dev->v_addrs[PCI_ROM_SLOT].u.r_virtbase, +         
>            size, PROT_READ | PROT_WRITE);
> +            memcpy(adev->assigned_dev->v_addrs[PCI_ROM_SLOT].u.r_virtbase,
> +                   buf, size);
> +           
> mprotect(adev->assigned_dev->v_addrs[PCI_ROM_SLOT].u.r_virtbase, +         
>            size, PROT_READ);
> +        }
> +
>          /* Scan the buffer for suitable ROMs and increase the offset */
>          offset += scan_option_rom(adev->assigned_dev->dev.devfn, buf,
> offset);
>
> diff --git a/hw/device-assignment.h b/hw/device-assignment.h
> index c691e11..713f9b7 100644
> --- a/hw/device-assignment.h
> +++ b/hw/device-assignment.h
> @@ -36,9 +36,6 @@
>  /* From include/linux/pci.h in the kernel sources */
>  #define PCI_DEVFN(slot, func)   ((((slot) & 0x1f) << 3) | ((func) & 0x07))
>
> -/* The number of BARs in the config space header */
> -#define MAX_IO_REGIONS (6)
> -
>  typedef struct {
>      int type;           /* Memory or port I/O */
>      int valid;
> @@ -53,7 +50,7 @@ typedef struct {
>      uint16_t region_number; /* number of active regions */
>
>      /* Port I/O or MMIO Regions */
> -    PCIRegion regions[MAX_IO_REGIONS];
> +    PCIRegion regions[PCI_NUM_REGIONS];
>      int config_fd;
>  } PCIDevRegions;