[PATCH 0/3] device-assignment: PCI option ROM fixes

[PATCH 0/3] device-assignment: PCI option ROM fixes

[Alex Williamson]

Changeset b4f8c249 in kvm.git makes the mprotects in device assignment
produce a "Bad address" hang when a device with an option ROM is
assigned.  We can avoid this by just using the slow mapping path since
ROM access doesn't need to be fast.  Apparently nobody has ever mapped
a ROM via this path, because passing NULL to cpu_register_io_memory()
doesn't work.  I also found we're overly restrictive in copying the
ROM from the host, I must have been lucky and had a ROM that matched
the BAR size when I added this.

Alex

---

Alex Williamson (3):
      device-assignment: Byte-wise ROM read
      device-assignment: Always use slow mapping for PCI option ROM
      device-assignment: Fix slow option ROM mapping


 hw/device-assignment.c |   21 ++++++++++-----------
 1 files changed, 10 insertions(+), 11 deletions(-)

[PATCH 1/3] device-assignment: Fix slow option ROM mapping

[Alex Williamson]

cpu_register_io_memory() supports individual function pointers
being NULL, not the structure itself.  Create and pass the
right thing.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---

 hw/device-assignment.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/hw/device-assignment.c b/hw/device-assignment.c
index c56870e..c26ff6d 100644
--- a/hw/device-assignment.c
+++ b/hw/device-assignment.c
@@ -233,6 +233,8 @@ static CPUReadMemoryFunc * const slow_bar_read[] = {
     &slow_bar_readl
 };
 
+static CPUWriteMemoryFunc * const slow_bar_null_write[] = {NULL, NULL, NULL};
+
 static void assigned_dev_iomem_map_slow(PCIDevice *pci_dev, int region_num,
                                         pcibus_t e_phys, pcibus_t e_size,
                                         int type)
@@ -244,7 +246,7 @@ static void assigned_dev_iomem_map_slow(PCIDevice *pci_dev, int region_num,
 
     DEBUG("%s", "slow map\n");
     if (region_num == PCI_ROM_SLOT)
-        m = cpu_register_io_memory(slow_bar_read, NULL, region);
+        m = cpu_register_io_memory(slow_bar_read, slow_bar_null_write, region);
     else
         m = cpu_register_io_memory(slow_bar_read, slow_bar_write, region);
     cpu_register_physical_memory(e_phys, e_size, m);

Re: [PATCH 1/3] device-assignment: Fix slow option ROM mapping

[Chris Wright]

* Alex Williamson (alex.williamson@redhat.com) wrote:
> cpu_register_io_memory() supports individual function pointers
> being NULL, not the structure itself.  Create and pass the
> right thing.

Acked-by: Chris Wright <chrisw@redhat.com>

[PATCH 2/3] device-assignment: Always use slow mapping for PCI option ROM

[Alex Williamson]

KVM doesn't support read-only mappings for MMIO space.  Performance isn't
an issue for the option ROM mapping, so always use slow mapping.  kvm.git
cset b4f8c249 will make kvm hang with a "Bad address" fault without this.
We can also then drop the extraneous mprotects since the guest has no way
to write to these regions.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---

 hw/device-assignment.c |    8 ++------
 1 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/hw/device-assignment.c b/hw/device-assignment.c
index c26ff6d..0e82a16 100644
--- a/hw/device-assignment.c
+++ b/hw/device-assignment.c
@@ -541,6 +541,8 @@ static int assigned_dev_register_regions(PCIRegion *io_regions,
             /* map physical memory */
             pci_dev->v_addrs[i].e_physbase = cur_region->base_addr;
             if (i == PCI_ROM_SLOT) {
+                /* KVM doesn't support read-only mappings, use slow map */
+                slow_map = 1;
                 pci_dev->v_addrs[i].u.r_virtbase =
                     mmap(NULL,
                          cur_region->size,
@@ -566,8 +568,6 @@ static int assigned_dev_register_regions(PCIRegion *io_regions,
             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;
@@ -1691,12 +1691,8 @@ static void assigned_dev_load_option_rom(AssignedDevice *dev)
     /* Copy ROM contents into the space backing the ROM BAR */
     if (dev->v_addrs[PCI_ROM_SLOT].r_size >= size &&
         dev->v_addrs[PCI_ROM_SLOT].u.r_virtbase) {
-        mprotect(dev->v_addrs[PCI_ROM_SLOT].u.r_virtbase,
-                 size, PROT_READ | PROT_WRITE);
         memcpy(dev->v_addrs[PCI_ROM_SLOT].u.r_virtbase,
                buf, size);
-        mprotect(dev->v_addrs[PCI_ROM_SLOT].u.r_virtbase,
-                 size, PROT_READ);
     }
 
     free(buf);

Re: [PATCH 2/3] device-assignment: Always use slow mapping for PCI option ROM

[Chris Wright]

* Alex Williamson (alex.williamson@redhat.com) wrote:
> KVM doesn't support read-only mappings for MMIO space.  Performance isn't
> an issue for the option ROM mapping, so always use slow mapping.  kvm.git
> cset b4f8c249 will make kvm hang with a "Bad address" fault without this.
> We can also then drop the extraneous mprotects since the guest has no way
> to write to these regions.

Acked-by: Chris Wright <chrisw@redhat.com>

[PATCH 3/3] device-assignment: Byte-wise ROM read

[Alex Williamson]

The host kernel filters the PCI option ROM, returning only bytes for
the actual ROM size, not for the whole BAR.  That means we typically
do a short read of the PCI sysfs ROM file.  Read it a byte at a time
so we know how much to actually copy and only skip the copy if we
get nothing.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---

 hw/device-assignment.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/hw/device-assignment.c b/hw/device-assignment.c
index 0e82a16..3bb7f0b 100644
--- a/hw/device-assignment.c
+++ b/hw/device-assignment.c
@@ -1680,19 +1680,20 @@ static void assigned_dev_load_option_rom(AssignedDevice *dev)
         return;
     }
 
-    ret = fread(buf, size, 1, fp);
-    if (!feof(fp) || ferror(fp) || ret != 1) {
+    if (!(ret = fread(buf, 1, size, fp))) {
         free(buf);
         fclose(fp);
         return;
     }
     fclose(fp);
 
+    /* The number of bytes read is often much smaller than the BAR size */
+    size = ret;
+
     /* Copy ROM contents into the space backing the ROM BAR */
     if (dev->v_addrs[PCI_ROM_SLOT].r_size >= size &&
         dev->v_addrs[PCI_ROM_SLOT].u.r_virtbase) {
-        memcpy(dev->v_addrs[PCI_ROM_SLOT].u.r_virtbase,
-               buf, size);
+        memcpy(dev->v_addrs[PCI_ROM_SLOT].u.r_virtbase, buf, size);
     }
 
     free(buf);

Re: [PATCH 3/3] device-assignment: Byte-wise ROM read

[Chris Wright]

* Alex Williamson (alex.williamson@redhat.com) wrote:
> The host kernel filters the PCI option ROM, returning only bytes for
> the actual ROM size, not for the whole BAR.  That means we typically
> do a short read of the PCI sysfs ROM file.  Read it a byte at a time
> so we know how much to actually copy and only skip the copy if we
> get nothing.

Acked-by: Chris Wright <chrisw@redhat.com>

Re: [PATCH 0/3] device-assignment: PCI option ROM fixes

[Avi Kivity]

  On 07/30/2010 10:40 PM, Alex Williamson wrote:
> Changeset b4f8c249 in kvm.git makes the mprotects in device assignment
> produce a "Bad address" hang when a device with an option ROM is
> assigned.  We can avoid this by just using the slow mapping path since
> ROM access doesn't need to be fast.  Apparently nobody has ever mapped
> a ROM via this path, because passing NULL to cpu_register_io_memory()
> doesn't work.  I also found we're overly restrictive in copying the
> ROM from the host, I must have been lucky and had a ROM that matched
> the BAR size when I added this.

Applied, thanks.

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