Enable non page boundary BAR device assignment

When using -pcidevice on a device that is already in use by a kernel driver all the user gets is the following (very useful) information:

Failed to assign device "04:00.0" : Device or resource busy Failed to deassign device "04:00.0" : Invalid argument Error initializing device pci-assign

Since I usually prefer to have my computer do the thinking for me, I figured it might be a good idea to check and see if a device is actually used by a driver. If so, tell the user.

So with this patch applied you get the following output:

Failed to assign device "04:00.0" : Device or resource busy *** The driver 'igb' is occupying your device 04:00.0.
***
*** You can try the following commands to free it:
***
*** $ echo "8086 150a" > /sys/bus/pci/drivers/pci-stub/new_id *** $ echo "0000:04:00.0" > /sys/bus/pci/drivers/igb/unbind *** $ echo "0000:04:00.0" > /sys/bus/pci/drivers/pci-stub/bind *** $ echo "8086 150a" > /sys/bus/pci/drivers/pci-stub/remove_id
***
Failed to deassign device "04:00.0" : Invalid argument Error initializing device pci-assign

That should keep people like me from doing the most obvious misuses :-).

CC Daniel P. Berrange <[email protected]>

Signed-off-by Alexander Graf <[email protected]>

---

v1 -> v2:

- add more helpful guidance thanks to Daniel Berrange

v2 -> v3:

- clear name variable before using it, thus 0-terminating the string
- fix region numbers
- use correct unbind/bind names

---
 hw/device-assignment.c |  109 +++++++++++++++++++++++++++++++++++++-----------
 1 files changed, 85 insertions(+), 24 deletions(-)

hw/device-assignment.c +0 -0

index 5cee929..98faa83 100644

hw/device-assignment.c +20 -7

+    fclose(f);
+
+    return 0;
+}
+
+static int get_real_vendor_id(const char *devpath, uint16_t *val)
+{
+    return get_real_id(devpath, "vendor", val);
+}
+
+static int get_real_device_id(const char *devpath, uint16_t *val)
+{
+    return get_real_id(devpath, "device", val);
+}
+
 static int get_real_device(AssignedDevice *pci_dev, uint8_t r_bus,
                            uint8_t r_dev, uint8_t r_func)
 {
     char dir[128], name[128];
-    int fd, r = 0;
+    int fd, r = 0, v;
     FILE *f;
     unsigned long long start, end, size, flags;
-    unsigned long id;
+    uint16_t id;
     struct stat statbuf;
     PCIRegion *rp;
     PCIDevRegions *dev = &pci_dev->real_device;

@@ -637,31 +667,21 @@ again:
 
     fclose(f);
 
-    /* read and fill device ID */
-    snprintf(name, sizeof(name), "%svendor", dir);
-    f = fopen(name, "r");
-    if (f == NULL) {
-        fprintf(stderr, "%s: %s: %m\n", __func__, name);
+    /* read and fill vendor ID */
+    v = get_real_vendor_id(dir, &id);
+    if (v) {

hw/device-assignment.c +5 -10

-    /* read and fill vendor ID */
-    snprintf(name, sizeof(name), "%sdevice", dir);
-    f = fopen(name, "r");
-    if (f == NULL) {
-        fprintf(stderr, "%s: %s: %m\n", __func__, name);
+    /* read and fill device ID */
+    v = get_real_device_id(dir, &id);
+    if (v) {
         return 1;
     }
-    if (fscanf(f, "%li\n", &id) == 1) {
-	pci_dev->dev.config[2] = id & 0xff;
-	pci_dev->dev.config[3] = (id & 0xff00) >> 8;
-    }
-    fclose(f);
+    pci_dev->dev.config[2] = id & 0xff;
+    pci_dev->dev.config[3] = (id & 0xff00) >> 8;
 
     /* dealing with virtual function device */
     snprintf(name, sizeof(name), "%sphysfn/", dir);

hw/device-assignment.c +10 -0

+        }
+
+        fprintf(stderr, "*** The driver '%s' is occupying your device "
+                        "%02x:%02x.%x.\n",
+                ns, dev->host.bus, dev->host.dev, dev->host.func);
+        fprintf(stderr, "***\n");
+        fprintf(stderr, "*** You can try the following commands to free "
+                        "it:\n");
+        fprintf(stderr, "***\n");
+        fprintf(stderr, "*** $ echo \"%04x %04x\" > /sys/bus/pci/drivers/"

hw/device-assignment.c +1 -0

+    }
     return r;
 }
 

Signature · 1 line

1.6.0.2

Quoted 26 lines · Alexander Graf

When using -pcidevice on a device that is already in use by a kernel driver all the user gets is the following (very useful) information:

Failed to assign device "04:00.0" : Device or resource busy Failed to deassign device "04:00.0" : Invalid argument Error initializing device pci-assign

Since I usually prefer to have my computer do the thinking for me, I figured it might be a good idea to check and see if a device is actually used by a driver. If so, tell the user.

So with this patch applied you get the following output:

Failed to assign device "04:00.0" : Device or resource busy *** The driver 'igb' is occupying your device 04:00.0.
***
*** You can try the following commands to free it:
***
*** $ echo "8086 150a" > /sys/bus/pci/drivers/pci-stub/new_id *** $ echo "0000:04:00.0" > /sys/bus/pci/drivers/igb/unbind *** $ echo "0000:04:00.0" > /sys/bus/pci/drivers/pci-stub/bind *** $ echo "8086 150a" > /sys/bus/pci/drivers/pci-stub/remove_id
***
Failed to deassign device "04:00.0" : Invalid argument Error initializing device pci-assign

Quoted 5 lines

That should keep people like me from doing the most obvious misuses :-).

CC Daniel P. Berrange <[email protected]>

Signed-off-by Alexander Graf <[email protected]>

Minor nits and a bug.

Quoted 23 lines

---

v1 -> v2:

- add more helpful guidance thanks to Daniel Berrange

v2 -> v3:

- clear name variable before using it, thus 0-terminating the string
- fix region numbers
- use correct unbind/bind names

---
 hw/device-assignment.c |  109 +++++++++++++++++++++++++++++++++++++-----------
 1 files changed, 85 insertions(+), 24 deletions(-)

hw/device-assignment.c +0 -0

index 5cee929..98faa83 100644
@@ -564,14 +564,44 @@ static int assigned_dev_register_regions(PCIRegion *io_regions,
     return 0;
 }
 

hw/device-assignment.c static int assigned_dev_register_regions(PCIRegion *io_regions,

Quoted 5 lines

hw/device-assignment.c +4 -0

+static int get_real_id(const char *devpath, const char *idname, uint16_t *val)
+{
+    FILE *f;
+    char name[128];
 

let's not introduce arbitraty file name length limitations. strlen is not hard to use. I know all this module is broken this way, but let's not add more.

hw/device-assignment.c static int assigned_dev_register_regions(PCIRegion *io_regions,

Quoted 13 lines

hw/device-assignment.c +11 -0

+    long id;
+
+    snprintf(name, sizeof(name), "%s%s", devpath, idname);
+    f = fopen(name, "r");
+    if (f == NULL) {
+        fprintf(stderr, "%s: %s: %m\n", __func__, name);
+        return -1;
+    }
+    if (fscanf(f, "%li\n", &id) == 1) {
+        *val = id;
+    }
 

handle fscanf error?

Quoted 47 lines

hw/device-assignment.c +17 -2

+    fclose(f);
+
+    return 0;
+}
+
+static int get_real_vendor_id(const char *devpath, uint16_t *val)
+{
+    return get_real_id(devpath, "vendor", val);
+}
+
+static int get_real_device_id(const char *devpath, uint16_t *val)
+{
+    return get_real_id(devpath, "device", val);
+}
+
 static int get_real_device(AssignedDevice *pci_dev, uint8_t r_bus,
                            uint8_t r_dev, uint8_t r_func)
 {
     char dir[128], name[128];
-    int fd, r = 0;
+    int fd, r = 0, v;
     FILE *f;
     unsigned long long start, end, size, flags;
-    unsigned long id;
+    uint16_t id;
     struct stat statbuf;

     PCIRegion *rp;
     PCIDevRegions *dev = &pci_dev->real_device;

hw/device-assignment.c +3 -9

@@ -637,31 +667,21 @@ again:
 
     fclose(f);
 
-    /* read and fill device ID */
-    snprintf(name, sizeof(name), "%svendor", dir);
-    f = fopen(name, "r");
-    if (f == NULL) {
-        fprintf(stderr, "%s: %s: %m\n", __func__, name);
+    /* read and fill vendor ID */
+    v = get_real_vendor_id(dir, &id);
+    if (v) {
         return 1;
     }
-    if (fscanf(f, "%li\n", &id) == 1) {
-	pci_dev->dev.config[0] = id & 0xff;
-	pci_dev->dev.config[1] = (id & 0xff00) >> 8;
-    }

hw/device-assignment.c again: · line 685

Quoted 5 lines

hw/device-assignment.c +2 -1

-    fclose(f);
+    pci_dev->dev.config[0] = id & 0xff;
+    pci_dev->dev.config[1] = (id & 0xff00) >> 8;
 
 

this seems an unrelated cleanup?
If so better as a separate patch?

Quoted 23 lines

hw/device-assignment.c +5 -10

-    /* read and fill vendor ID */
-    snprintf(name, sizeof(name), "%sdevice", dir);
-    f = fopen(name, "r");
-    if (f == NULL) {
-        fprintf(stderr, "%s: %s: %m\n", __func__, name);
+    /* read and fill device ID */
+    v = get_real_device_id(dir, &id);
+    if (v) {
         return 1;
     }
-    if (fscanf(f, "%li\n", &id) == 1) {
-	pci_dev->dev.config[2] = id & 0xff;
-	pci_dev->dev.config[3] = (id & 0xff00) >> 8;
-    }
-    fclose(f);
+    pci_dev->dev.config[2] = id & 0xff;
+    pci_dev->dev.config[3] = (id & 0xff00) >> 8;
 
     /* dealing with virtual function device */

     snprintf(name, sizeof(name), "%sphysfn/", dir);

hw/device-assignment.c static uint32_t calc_assigned_dev_id(uint8_t bus, uint8_t devfn) · line 763

Quoted 6 lines

hw/device-assignment.c +1 -1

@@ -739,7 +759,9 @@ static uint32_t calc_assigned_dev_id(uint8_t bus, uint8_t devfn)
 static int assign_device(AssignedDevice *dev)
 {
     struct kvm_assigned_pci_dev assigned_dev_data;
-    int r;
+    char name[128], dir[128], driver[128], *ns;
 

Yes 128 will be enough for now. But it's pretty ugly. In this case, something like

    char dir[] = "/sys/bus/pci/devices/0000:00:00.0/";

will allocate just enough memory.
Or use MAX PATH.

Quoted 9 lines

hw/device-assignment.c +2 -0

+    uint16_t vendor_id, device_id;
+    int r, v;
 
     memset(&assigned_dev_data, 0, sizeof(assigned_dev_data));

assigned_dev_data.assigned_dev_id =

hw/device-assignment.c +0 -0

@@ -761,9 +783,48 @@ static int assign_device(AssignedDevice *dev)
 #endif
 

hw/device-assignment.c static int assign_device(AssignedDevice *dev) · line 830

Quoted 4 lines

hw/device-assignment.c +1 -1

     r = kvm_assign_pci_device(kvm_context, &assigned_dev_data);
-    if (r < 0)
+    if (r < 0) {
 

Please put all of the below in a separate function.

hw/device-assignment.c static int assign_device(AssignedDevice *dev) · line 830

Quoted 6 lines

hw/device-assignment.c +2 -0

 	fprintf(stderr, "Failed to assign device \"%s\" : %s\n",
                 dev->dev.qdev.id, strerror(-r));
+
+        snprintf(dir, sizeof(dir),
 

snprintf? So you worry about overflowing dir?
But dir will not be 0 terminated on overflow,
so use of %s below would crash anyway.
As in fact we know this can not overflow, just use sprintf.

hw/device-assignment.c static int assign_device(AssignedDevice *dev) · line 830

Quoted 4 lines

hw/device-assignment.c +2 -0

+                 "/sys/bus/pci/devices/0000:%02x:%02x.%x/",
+	         dev->host.bus, dev->host.dev, dev->host.func);
 

This assumes domain 0. I know multidomain is broken with device assignment, but pls add TOIDO here so we don't forget to fix it.

hw/device-assignment.c static int assign_device(AssignedDevice *dev) · line 797

Quoted 4 lines

hw/device-assignment.c +2 -0

+
+        snprintf(name, sizeof(name), "%sdriver", dir);

So why do sprintf twice? Just put "driver" as part of the template above.

hw/device-assignment.c static int assign_device(AssignedDevice *dev) · line 799

Quoted 4 lines

hw/device-assignment.c +2 -0

+
+        memset(driver, 0, sizeof(driver));

just initialize driver to 0 by = {};

hw/device-assignment.c static int assign_device(AssignedDevice *dev) · line 797

Quoted 2 lines

hw/device-assignment.c +1 -0

+        v = readlink(name, driver, sizeof(driver));

So if readlink fills up all of driver, strrchr below will cause coredump, right? Better check v against sizeof driver.

hw/device-assignment.c static int assign_device(AssignedDevice *dev) · line 802

Quoted 4 lines

hw/device-assignment.c +2 -0

+        if ((v <= 0) || !(ns = strrchr(driver, '/'))) {
+            return r;

Add some fprintf here. Maybe report errno as well.

hw/device-assignment.c static int assign_device(AssignedDevice *dev) · line 806

Quoted 9 lines

hw/device-assignment.c +7 -0

+        }
+
+        ns++;
+
+        if (get_real_vendor_id(dir, &vendor_id) ||
+            get_real_device_id(dir, &device_id)) {
+            return r;

And here.

Quoted 12 lines

hw/device-assignment.c +10 -0

+        }
+
+        fprintf(stderr, "*** The driver '%s' is occupying your device "
+                        "%02x:%02x.%x.\n",
+                ns, dev->host.bus, dev->host.dev, dev->host.func);
+        fprintf(stderr, "***\n");
+        fprintf(stderr, "*** You can try the following commands to free "
+                        "it:\n");
+        fprintf(stderr, "***\n");
+        fprintf(stderr, "*** $ echo \"%04x %04x\" > /sys/bus/pci/drivers/"

hw/device-assignment.c static int assign_device(AssignedDevice *dev) · line 826

Quoted 10 lines

hw/device-assignment.c +10 -0

+                        "pci-stub/new_id\n", vendor_id, device_id);
+        fprintf(stderr, "*** $ echo \"0000:%02x:%02x.%x\" > /sys/bus/pci"
+                        "/drivers/%s /unbind\n",
+                dev->host.bus, dev->host.dev, dev->host.func, ns);
+        fprintf(stderr, "*** $ echo \"0000:%02x:%02x.%x\" > /sys/bus/pci"
+                        "/drivers/ pci-stub/bind\n",
+                dev->host.bus, dev->host.dev, dev->host.func);
+        fprintf(stderr, "*** $ echo \"%x %x\" > /sys/bus/pci/drivers/pci-stub"
+                        "/remove_id\n", vendor_id, device_id);
+        fprintf(stderr, "***\n");

above assumes domain zero. Please add a TODO to fix.

Quoted 60 lines · Michael S. Tsirkin

Quoted 26 lines · Alexander Graf

When using -pcidevice on a device that is already in use by a kernel driver all the user gets is the following (very useful) information:

Failed to assign device "04:00.0" : Device or resource busy Failed to deassign device "04:00.0" : Invalid argument Error initializing device pci-assign

Since I usually prefer to have my computer do the thinking for me, I figured it might be a good idea to check and see if a device is actually used by a driver. If so, tell the user.

So with this patch applied you get the following output:

Failed to assign device "04:00.0" : Device or resource busy *** The driver 'igb' is occupying your device 04:00.0.
***
*** You can try the following commands to free it:
***
*** $ echo "8086 150a" > /sys/bus/pci/drivers/pci-stub/new_id *** $ echo "0000:04:00.0" > /sys/bus/pci/drivers/igb/unbind *** $ echo "0000:04:00.0" > /sys/bus/pci/drivers/pci-stub/bind *** $ echo "8086 150a" > /sys/bus/pci/drivers/pci-stub/remove_id
***
Failed to deassign device "04:00.0" : Invalid argument Error initializing device pci-assign

Quoted 5 lines

That should keep people like me from doing the most obvious misuses :-).

CC Daniel P. Berrange <[email protected]>

Signed-off-by Alexander Graf <[email protected]>

Minor nits and a bug.

Quoted 23 lines

---

v1 -> v2:

- add more helpful guidance thanks to Daniel Berrange

v2 -> v3:

- clear name variable before using it, thus 0-terminating the string
- fix region numbers
- use correct unbind/bind names

---
 hw/device-assignment.c |  109 +++++++++++++++++++++++++++++++++++++-----------
 1 files changed, 85 insertions(+), 24 deletions(-)

hw/device-assignment.c +0 -0

index 5cee929..98faa83 100644
@@ -564,14 +564,44 @@ static int assigned_dev_register_regions(PCIRegion *io_regions,
     return 0;
 }
 

Quoted 10 lines

hw/device-assignment.c static int assigned_dev_register_regions(PCIRegion *io_regions,

Quoted 5 lines

hw/device-assignment.c +4 -0

+static int get_real_id(const char *devpath, const char *idname, uint16_t *val)
+{
+    FILE *f;
+    char name[128];
 

let's not introduce arbitraty file name length limitations. strlen is not hard to use. I know all this module is broken this way, but let's not add more.

It's just a move of existing code. I tried to change it as little as possible. Cleanups for that are welcome for later.

Quoted 14 lines

hw/device-assignment.c static int assigned_dev_register_regions(PCIRegion *io_regions,

Quoted 13 lines

hw/device-assignment.c +11 -0

+    long id;
+
+    snprintf(name, sizeof(name), "%s%s", devpath, idname);
+    f = fopen(name, "r");
+    if (f == NULL) {
+        fprintf(stderr, "%s: %s: %m\n", __func__, name);
+        return -1;
+    }
+    if (fscanf(f, "%li\n", &id) == 1) {
+        *val = id;
+    }
 

Quoted 2 lines

handle fscanf error?

Interesting. I don't think it was done before, but I can put it in.

Quoted 47 lines

Quoted 47 lines

hw/device-assignment.c +17 -2

+    fclose(f);
+
+    return 0;
+}
+
+static int get_real_vendor_id(const char *devpath, uint16_t *val)
+{
+    return get_real_id(devpath, "vendor", val);
+}
+
+static int get_real_device_id(const char *devpath, uint16_t *val)
+{
+    return get_real_id(devpath, "device", val);
+}
+
 static int get_real_device(AssignedDevice *pci_dev, uint8_t r_bus,
                            uint8_t r_dev, uint8_t r_func)
 {
     char dir[128], name[128];
-    int fd, r = 0;
+    int fd, r = 0, v;
     FILE *f;
     unsigned long long start, end, size, flags;
-    unsigned long id;
+    uint16_t id;
     struct stat statbuf;

     PCIRegion *rp;
     PCIDevRegions *dev = &pci_dev->real_device;

hw/device-assignment.c +3 -9

@@ -637,31 +667,21 @@ again:
 
     fclose(f);
 
-    /* read and fill device ID */
-    snprintf(name, sizeof(name), "%svendor", dir);
-    f = fopen(name, "r");
-    if (f == NULL) {
-        fprintf(stderr, "%s: %s: %m\n", __func__, name);
+    /* read and fill vendor ID */
+    v = get_real_vendor_id(dir, &id);
+    if (v) {
         return 1;
     }
-    if (fscanf(f, "%li\n", &id) == 1) {
-	pci_dev->dev.config[0] = id & 0xff;
-	pci_dev->dev.config[1] = (id & 0xff00) >> 8;
-    }

Quoted 9 lines

hw/device-assignment.c again: · line 685

Quoted 5 lines

hw/device-assignment.c +2 -1

-    fclose(f);
+    pci_dev->dev.config[0] = id & 0xff;
+    pci_dev->dev.config[1] = (id & 0xff00) >> 8;
 
 

this seems an unrelated cleanup?
If so better as a separate patch?

It's the code move. I split it now.

Quoted 32 lines

Quoted 23 lines

hw/device-assignment.c +5 -10

-    /* read and fill vendor ID */
-    snprintf(name, sizeof(name), "%sdevice", dir);
-    f = fopen(name, "r");
-    if (f == NULL) {
-        fprintf(stderr, "%s: %s: %m\n", __func__, name);
+    /* read and fill device ID */
+    v = get_real_device_id(dir, &id);
+    if (v) {
         return 1;
     }
-    if (fscanf(f, "%li\n", &id) == 1) {
-	pci_dev->dev.config[2] = id & 0xff;
-	pci_dev->dev.config[3] = (id & 0xff00) >> 8;
-    }
-    fclose(f);
+    pci_dev->dev.config[2] = id & 0xff;
+    pci_dev->dev.config[3] = (id & 0xff00) >> 8;
 
     /* dealing with virtual function device */

     snprintf(name, sizeof(name), "%sphysfn/", dir);

hw/device-assignment.c static uint32_t calc_assigned_dev_id(uint8_t bus, uint8_t devfn) · line 763

Quoted 6 lines

hw/device-assignment.c +1 -1

@@ -739,7 +759,9 @@ static uint32_t calc_assigned_dev_id(uint8_t bus, uint8_t devfn)
 static int assign_device(AssignedDevice *dev)
 {
     struct kvm_assigned_pci_dev assigned_dev_data;
-    int r;
+    char name[128], dir[128], driver[128], *ns;
 

Yes 128 will be enough for now. But it's pretty ugly. In this case, something like

Quoted 4 lines

    char dir[] = "/sys/bus/pci/devices/0000:00:00.0/";

will allocate just enough memory.
Or use MAX PATH.

Used MAX_PATH now.

Quoted 9 lines

Quoted 9 lines

hw/device-assignment.c +2 -0

+    uint16_t vendor_id, device_id;
+    int r, v;
 
     memset(&assigned_dev_data, 0, sizeof(assigned_dev_data));

assigned_dev_data.assigned_dev_id =

hw/device-assignment.c +0 -0

@@ -761,9 +783,48 @@ static int assign_device(AssignedDevice *dev)
 #endif
 

Quoted 8 lines

hw/device-assignment.c static int assign_device(AssignedDevice *dev) · line 830

Quoted 4 lines

hw/device-assignment.c +1 -1

     r = kvm_assign_pci_device(kvm_context, &assigned_dev_data);
-    if (r < 0)
+    if (r < 0) {
 

Please put all of the below in a separate function.

Ok.

Quoted 7 lines

hw/device-assignment.c static int assign_device(AssignedDevice *dev) · line 830

Quoted 6 lines

hw/device-assignment.c +2 -0

 	fprintf(stderr, "Failed to assign device \"%s\" : %s\n",
                 dev->dev.qdev.id, strerror(-r));
+
+        snprintf(dir, sizeof(dir),
 

Quoted 5 lines

snprintf? So you worry about overflowing dir?
But dir will not be 0 terminated on overflow,
so use of %s below would crash anyway.
As in fact we know this can not overflow, just use sprintf.

Ok.

Quoted 9 lines

hw/device-assignment.c static int assign_device(AssignedDevice *dev) · line 830

Quoted 4 lines

hw/device-assignment.c +2 -0

+                 "/sys/bus/pci/devices/0000:%02x:%02x.%x/",
+	         dev->host.bus, dev->host.dev, dev->host.func);
 

This assumes domain 0. I know multidomain is broken with device assignment, but pls add TOIDO here so we don't forget to fix it.

Ok.

Quoted 8 lines

hw/device-assignment.c static int assign_device(AssignedDevice *dev) · line 797

Quoted 4 lines

hw/device-assignment.c +2 -0

+
+        snprintf(name, sizeof(name), "%sdriver", dir);

So why do sprintf twice? Just put "driver" as part of the template above.

We're using dir later in the code.

Quoted 5 lines

Quoted 4 lines

hw/device-assignment.c +2 -0

+
+        memset(driver, 0, sizeof(driver));

Quoted 3 lines

just initialize driver to 0 by = {};

That initializes it to 0? I mean, all elements?

Quoted 7 lines

hw/device-assignment.c static int assign_device(AssignedDevice *dev) · line 797

Quoted 2 lines

hw/device-assignment.c +1 -0

+        v = readlink(name, driver, sizeof(driver));

So if readlink fills up all of driver, strrchr below will cause coredump, right? Better check v against sizeof driver.

Ok.

Quoted 7 lines

hw/device-assignment.c static int assign_device(AssignedDevice *dev) · line 802

Quoted 4 lines

hw/device-assignment.c +2 -0

+        if ((v <= 0) || !(ns = strrchr(driver, '/'))) {
+            return r;

Add some fprintf here. Maybe report errno as well.

Ok.

Quoted 12 lines

hw/device-assignment.c static int assign_device(AssignedDevice *dev) · line 806

Quoted 9 lines

hw/device-assignment.c +7 -0

+        }
+
+        ns++;
+
+        if (get_real_vendor_id(dir, &vendor_id) ||
+            get_real_device_id(dir, &device_id)) {
+            return r;

And here.

Yep.

Quoted 23 lines

Quoted 22 lines

hw/device-assignment.c +20 -0

+        }
+
+        fprintf(stderr, "*** The driver '%s' is occupying your device "
+                        "%02x:%02x.%x.\n",
+                ns, dev->host.bus, dev->host.dev, dev->host.func);
+        fprintf(stderr, "***\n");
+        fprintf(stderr, "*** You can try the following commands to free "
+                        "it:\n");
+        fprintf(stderr, "***\n");
+        fprintf(stderr, "*** $ echo \"%04x %04x\" > /sys/bus/pci/drivers/"
+                        "pci-stub/new_id\n", vendor_id, device_id);
+        fprintf(stderr, "*** $ echo \"0000:%02x:%02x.%x\" > /sys/bus/pci"
+                        "/drivers/%s /unbind\n",
+                dev->host.bus, dev->host.dev, dev->host.func, ns);
+        fprintf(stderr, "*** $ echo \"0000:%02x:%02x.%x\" > /sys/bus/pci"
+                        "/drivers/ pci-stub/bind\n",
+                dev->host.bus, dev->host.dev, dev->host.func);
+        fprintf(stderr, "*** $ echo \"%x %x\" > /sys/bus/pci/drivers/pci-stub"
+                        "/remove_id\n", vendor_id, device_id);
+        fprintf(stderr, "***\n");

Quoted 2 lines

above assumes domain zero. Please add a TODO to fix.

Same as above, right? In fact, a lot of the code assumes that so it's more of a generic TODO :-(.

Alex

While trying to get device passthrough working with an emulex hba, kvm refused to pass it through because it has a BAR of 256 bytes:

        Region 0: Memory at d2100000 (64-bit, non-prefetchable) [size=4K]
        Region 2: Memory at d2101000 (64-bit, non-prefetchable) [size=256]
        Region 4: I/O ports at b100 [size=256]

Since the page boundary is an arbitrary optimization to allow 1:1 mapping of physical to virtual addresses, we can still take the old MMIO callback route.

So let's add a second code path that allows for size & 0xFFF != 0 sized regions by looping it through userspace.

I verified that it works by passing through an e1000 with this additional patch applied and the card acted the same way it did without this patch:

             map_func = assigned_dev_iomem_map;

Signed-off-by Alexander Graf <[email protected]>

---

v1 -> v2:

- don't use map_func function pointer
- use the same code for mmap on fast and slow path

---
 hw/device-assignment.c |  123 +++++++++++++++++++++++++++++++++++++++++++++---
 1 files changed, 116 insertions(+), 7 deletions(-)

hw/device-assignment.c +0 -0

index 13a86bb..5cee929 100644

hw/device-assignment.c +7 -0

+
+    r = *in;
+    DEBUG("slow_bar_readl addr=0x" TARGET_FMT_plx " val=0x%08x\n", addr, r);
+
+    return r;
+}
+

hw/device-assignment.c +8 -0

+
+    DEBUG("slow_bar_writeb addr=0x" TARGET_FMT_plx " val=0x%02x\n", addr, val);
+    *out = val;
+}
+
+static void slow_bar_writew(void *opaque, target_phys_addr_t addr, uint32_t val)
+{
+    AssignedDevRegion *d = opaque;

hw/device-assignment.c +21 -0

+
+    DEBUG("slow_bar_writel addr=0x" TARGET_FMT_plx " val=0x%08x\n", addr, val);
+    *out = val;
+}
+
+static CPUWriteMemoryFunc * const slow_bar_write[] = {
+    &slow_bar_writeb,
+    &slow_bar_writew,
+    &slow_bar_writel
+};
+
+static CPUReadMemoryFunc * const slow_bar_read[] = {
+    &slow_bar_readb,
+    &slow_bar_readw,
+    &slow_bar_readl
+};
+
+static void assigned_dev_iomem_map_slow(PCIDevice *pci_dev, int region_num,
+                                        pcibus_t e_phys, pcibus_t e_size,
+                                        int type)
+{

hw/device-assignment.c +4 -0

+    }
+}
+
 static void assigned_dev_iomem_map(PCIDevice *pci_dev, int region_num,
                                    pcibus_t e_phys, pcibus_t e_size, int type)
 {

@@ -381,15 +480,22 @@ static int assigned_dev_register_regions(PCIRegion *io_regions,
 
         /* handle memory io regions */
         if (cur_region->type & IORESOURCE_MEM) {
+            int slow_map = 0;
             int t = cur_region->type & IORESOURCE_PREFETCH

hw/device-assignment.c +6 -1

                         i, (unsigned long long)cur_region->base_addr,
                         cur_region->size);
+                slow_map = 1;
+            }
+
+            if (slow_map && (i == PCI_ROM_SLOT)) {
+                fprintf(stderr, "ROM not aligned - can't continue\n");
                 return -1;
             }
 

@@ -405,7 +511,7 @@ static int assigned_dev_register_regions(PCIRegion *io_regions,
             } else {
                 pci_dev->v_addrs[i].u.r_virtbase =
                     mmap(NULL,
-                         (cur_region->size + 0xFFF) & 0xFFFFF000,
+                         cur_region->size,
                          PROT_WRITE | PROT_READ, MAP_SHARED,
                          cur_region->resource_fd, (off_t) 0);
             }

hw/device-assignment.c +2 -1

             pci_register_bar((PCIDevice *) pci_dev, i,
                              cur_region->size, t,
-                             assigned_dev_iomem_map);
+                             slow_map ? assigned_dev_iomem_map_slow
+                                      : assigned_dev_iomem_map);
             continue;
         }
         /* handle port io regions */

Signature · 1 line

1.6.0.2

Quoted 18 lines · Alexander Graf

While trying to get device passthrough working with an emulex hba, kvm refused to pass it through because it has a BAR of 256 bytes:

        Region 0: Memory at d2100000 (64-bit, non-prefetchable) [size=4K]
        Region 2: Memory at d2101000 (64-bit, non-prefetchable) [size=256]
        Region 4: I/O ports at b100 [size=256]

Since the page boundary is an arbitrary optimization to allow 1:1 mapping of physical to virtual addresses, we can still take the old MMIO callback route.

So let's add a second code path that allows for size & 0xFFF != 0 sized regions by looping it through userspace.

I verified that it works by passing through an e1000 with this additional patch applied and the card acted the same way it did without this patch:

             map_func = assigned_dev_iomem_map;

Review

Quoted 3 lines

(quoted diff) +1 -1

-            if (cur_region->size & 0xFFF) {
+            if (i != PCI_ROM_SLOT){
                 fprintf(stderr, "PCI region %d at address 0x%llx "

The patch is pretty clean. However, I think I see a bug below. Did you also try a device with sub-page BAR?
If yes, did the bar get non page aligned value?

Quoted 17 lines

Signed-off-by Alexander Graf <[email protected]>

---

v1 -> v2:

- don't use map_func function pointer
- use the same code for mmap on fast and slow path

---
 hw/device-assignment.c |  123 +++++++++++++++++++++++++++++++++++++++++++++---
 1 files changed, 116 insertions(+), 7 deletions(-)

hw/device-assignment.c +0 -0

index 13a86bb..5cee929 100644

hw/device-assignment.c static uint32_t assigned_dev_ioport_readl(void *opaque, uint32_t addr) · line 155

Quoted 8 lines

hw/device-assignment.c +4 -0

@@ -148,6 +148,105 @@ static uint32_t assigned_dev_ioport_readl(void *opaque, uint32_t addr)
     return value;
 }
 
+static uint32_t slow_bar_readb(void *opaque, target_phys_addr_t addr)
+{
+    AssignedDevRegion *d = opaque;
+    uint8_t *in = (uint8_t*)(d->u.r_virtbase + addr);
 

this is a void* pointer, no need to cast

hw/device-assignment.c static uint32_t assigned_dev_ioport_readl(void *opaque, uint32_t addr) · line 157

Quoted 2 lines

hw/device-assignment.c +1 -0

+    uint32_t r = -1;
 

don't initialize r here as you override 1 line below.

Quoted 8 lines

hw/device-assignment.c +8 -0

+
+    r = *in;
+    DEBUG("slow_bar_readl addr=0x" TARGET_FMT_plx " val=0x%08x\n", addr, r);
+
+    return r;
+}
+
+static uint32_t slow_bar_readw(void *opaque, target_phys_addr_t addr)

hw/device-assignment.c static uint32_t assigned_dev_ioport_readl(void *opaque, uint32_t addr)

Quoted 10 lines

hw/device-assignment.c +9 -0

+{
+    AssignedDevRegion *d = opaque;
+    uint16_t *in = (uint16_t*)(d->u.r_virtbase + addr);
+    uint32_t r = -1;
+
+    r = *in;
+    DEBUG("slow_bar_readl addr=0x" TARGET_FMT_plx " val=0x%08x\n", addr, r);
+
+    return r;
 

same as above

hw/device-assignment.c static uint32_t assigned_dev_ioport_readl(void *opaque, uint32_t addr) · line 184

Quoted 13 lines

hw/device-assignment.c +12 -0

+}
+
+static uint32_t slow_bar_readl(void *opaque, target_phys_addr_t addr)
+{
+    AssignedDevRegion *d = opaque;
+    uint32_t *in = (uint32_t*)(d->u.r_virtbase + addr);
+    uint32_t r = -1;
+
+    r = *in;
+    DEBUG("slow_bar_readl addr=0x" TARGET_FMT_plx " val=0x%08x\n", addr, r);
+
+    return r;

same as above

hw/device-assignment.c static uint32_t assigned_dev_ioport_readl(void *opaque, uint32_t addr) · line 190

Quoted 7 lines

hw/device-assignment.c +6 -0

+}
+
+static void slow_bar_writeb(void *opaque, target_phys_addr_t addr, uint32_t val)
+{
+    AssignedDevRegion *d = opaque;
+    uint8_t *out = (uint8_t*)(d->u.r_virtbase + addr);

no need for cast as r_virtbase is a void pointer.

Quoted 9 lines

hw/device-assignment.c +8 -0

+
+    DEBUG("slow_bar_writeb addr=0x" TARGET_FMT_plx " val=0x%02x\n", addr, val);
+    *out = val;
+}
+
+static void slow_bar_writew(void *opaque, target_phys_addr_t addr, uint32_t val)
+{
+    AssignedDevRegion *d = opaque;

hw/device-assignment.c static uint32_t assigned_dev_ioport_readl(void *opaque, uint32_t addr) · line 208

Quoted 10 lines

hw/device-assignment.c +10 -0

+    uint16_t *out = (uint16_t*)(d->u.r_virtbase + addr);
+
+    DEBUG("slow_bar_writew addr=0x" TARGET_FMT_plx " val=0x%04x\n", addr, val);
+    *out = val;
+}
+
+static void slow_bar_writel(void *opaque, target_phys_addr_t addr, uint32_t val)
+{
+    AssignedDevRegion *d = opaque;
+    uint32_t *out = (uint32_t*)(d->u.r_virtbase + addr);

same as above

Quoted 26 lines

hw/device-assignment.c +26 -0

+
+    DEBUG("slow_bar_writel addr=0x" TARGET_FMT_plx " val=0x%08x\n", addr, val);
+    *out = val;
+}
+
+static CPUWriteMemoryFunc * const slow_bar_write[] = {
+    &slow_bar_writeb,
+    &slow_bar_writew,
+    &slow_bar_writel
+};
+
+static CPUReadMemoryFunc * const slow_bar_read[] = {
+    &slow_bar_readb,
+    &slow_bar_readw,
+    &slow_bar_readl
+};
+
+static void assigned_dev_iomem_map_slow(PCIDevice *pci_dev, int region_num,
+                                        pcibus_t e_phys, pcibus_t e_size,
+                                        int type)
+{
+    AssignedDevice *r_dev = container_of(pci_dev, AssignedDevice, dev);
+    AssignedDevRegion *region = &r_dev->v_addrs[region_num];
+    PCIRegion *real_region = &r_dev->real_device.regions[region_num];
+    int m;
+

hw/device-assignment.c static uint32_t assigned_dev_ioport_readl(void *opaque, uint32_t addr) · line 239

Quoted 6 lines

hw/device-assignment.c +5 -0

+    DEBUG("slow map\n");
+    m = cpu_register_io_memory(slow_bar_read, slow_bar_write, region);
+    cpu_register_physical_memory(e_phys, e_size, m);
+
+    /* MSI-X MMIO page */

some code duplication ... use a common function?

hw/device-assignment.c static uint32_t assigned_dev_ioport_readl(void *opaque, uint32_t addr) · line 246

Quoted 9 lines

hw/device-assignment.c +7 -0

+    if ((e_size > 0) &&
+        real_region->base_addr <= r_dev->msix_table_addr &&
+        real_region->base_addr + real_region->size >= r_dev->msix_table_addr) {
+        int offset = r_dev->msix_table_addr - real_region->base_addr;
+
+        cpu_register_physical_memory(e_phys + offset,
+                TARGET_PAGE_SIZE, r_dev->mmio_index);

How does this work? Does the last registered callback win or are both called for MSIX page?

Quoted 19 lines

hw/device-assignment.c +3 -0

+    }
+}
+

static void assigned_dev_iomem_map(PCIDevice *pci_dev, int region_num,
pcibus_t e_phys, pcibus_t e_size, int type)
{

hw/device-assignment.c +2 -2

@@ -381,15 +480,22 @@ static int assigned_dev_register_regions(PCIRegion *io_regions,
 
         /* handle memory io regions */
         if (cur_region->type & IORESOURCE_MEM) {
+            int slow_map = 0;
             int t = cur_region->type & IORESOURCE_PREFETCH
                 ? PCI_BASE_ADDRESS_MEM_PREFETCH
                 : PCI_BASE_ADDRESS_SPACE_MEMORY;
+
             if (cur_region->size & 0xFFF) {
-                fprintf(stderr, "Unable to assign device: PCI region %d "
-                        "at address 0x%llx has size 0x%x, "

hw/device-assignment.c static int assigned_dev_register_regions(PCIRegion *io_regions, · line 501

Quoted 5 lines

hw/device-assignment.c +3 -1

-                        " which is not a multiple of 4K\n",
+                fprintf(stderr, "PCI region %d at address 0x%llx "
+                        "has size 0x%x, which is not a multiple of 4K. "
+                        "Using slow map\n",
 

I still think "Using slow map" tells the user nothing. How about "Disabling direct guest access, device will be slow"?

Quoted 23 lines

hw/device-assignment.c +6 -1

                         i, (unsigned long long)cur_region->base_addr,
                         cur_region->size);
+                slow_map = 1;
+            }
+
+            if (slow_map && (i == PCI_ROM_SLOT)) {
+                fprintf(stderr, "ROM not aligned - can't continue\n");
                 return -1;
             }

@@ -405,7 +511,7 @@ static int assigned_dev_register_regions(PCIRegion *io_regions,
             } else {
                 pci_dev->v_addrs[i].u.r_virtbase =
                     mmap(NULL,
-                         (cur_region->size + 0xFFF) & 0xFFFFF000,
+                         cur_region->size,
                          PROT_WRITE | PROT_READ, MAP_SHARED,
                          cur_region->resource_fd, (off_t) 0);
             }

@@ -429,12 +535,15 @@ static int assigned_dev_register_regions(PCIRegion *io_regions,
             pci_dev->v_addrs[i].e_size = 0;
 

hw/device-assignment.c static int assigned_dev_register_regions(PCIRegion *io_regions, · line 542

Quoted 8 lines

hw/device-assignment.c +4 -2

             /* add offset */
-            pci_dev->v_addrs[i].u.r_virtbase +=
-                (cur_region->base_addr & 0xFFF);
+            if (!slow_map) {
+                pci_dev->v_addrs[i].u.r_virtbase +=
+                    (cur_region->base_addr & 0xFFF);
+            }
 

This looks wrong. I think mmap returns a page aligned address, that's why we did this offset math. No?
And if not, there was no reason for this code in the first place.

Quoted 162 lines · Michael S. Tsirkin

Quoted 18 lines · Alexander Graf

While trying to get device passthrough working with an emulex hba, kvm refused to pass it through because it has a BAR of 256 bytes:

        Region 0: Memory at d2100000 (64-bit, non-prefetchable) [size=4K]
        Region 2: Memory at d2101000 (64-bit, non-prefetchable) [size=256]
        Region 4: I/O ports at b100 [size=256]

Since the page boundary is an arbitrary optimization to allow 1:1 mapping of physical to virtual addresses, we can still take the old MMIO callback route.

So let's add a second code path that allows for size & 0xFFF != 0 sized regions by looping it through userspace.

I verified that it works by passing through an e1000 with this additional patch applied and the card acted the same way it did without this patch:

             map_func = assigned_dev_iomem_map;

Review

Quoted 3 lines

(quoted diff) +1 -1

-            if (cur_region->size & 0xFFF) {
+            if (i != PCI_ROM_SLOT){
                 fprintf(stderr, "PCI region %d at address 0x%llx "

The patch is pretty clean. However, I think I see a bug below. Did you also try a device with sub-page BAR?
If yes, did the bar get non page aligned value?

Quoted 17 lines

Signed-off-by Alexander Graf <[email protected]>

---

v1 -> v2:

- don't use map_func function pointer
- use the same code for mmap on fast and slow path

---
 hw/device-assignment.c |  123 +++++++++++++++++++++++++++++++++++++++++++++---
 1 files changed, 116 insertions(+), 7 deletions(-)

hw/device-assignment.c +0 -0

index 13a86bb..5cee929 100644

hw/device-assignment.c static uint32_t assigned_dev_ioport_readl(void *opaque, uint32_t addr) · line 155

Quoted 8 lines

hw/device-assignment.c +4 -0

@@ -148,6 +148,105 @@ static uint32_t assigned_dev_ioport_readl(void *opaque, uint32_t addr)
     return value;
 }
 
+static uint32_t slow_bar_readb(void *opaque, target_phys_addr_t addr)
+{
+    AssignedDevRegion *d = opaque;
+    uint8_t *in = (uint8_t*)(d->u.r_virtbase + addr);
 

this is a void* pointer, no need to cast

hw/device-assignment.c static uint32_t assigned_dev_ioport_readl(void *opaque, uint32_t addr) · line 157

Quoted 2 lines

hw/device-assignment.c +1 -0

+    uint32_t r = -1;
 

don't initialize r here as you override 1 line below.

Quoted 8 lines

hw/device-assignment.c +8 -0

+
+    r = *in;
+    DEBUG("slow_bar_readl addr=0x" TARGET_FMT_plx " val=0x%08x\n", addr, r);
+
+    return r;
+}
+
+static uint32_t slow_bar_readw(void *opaque, target_phys_addr_t addr)

hw/device-assignment.c static uint32_t assigned_dev_ioport_readl(void *opaque, uint32_t addr)

Quoted 10 lines

hw/device-assignment.c +9 -0

+{
+    AssignedDevRegion *d = opaque;
+    uint16_t *in = (uint16_t*)(d->u.r_virtbase + addr);
+    uint32_t r = -1;
+
+    r = *in;
+    DEBUG("slow_bar_readl addr=0x" TARGET_FMT_plx " val=0x%08x\n", addr, r);
+
+    return r;
 

same as above

hw/device-assignment.c static uint32_t assigned_dev_ioport_readl(void *opaque, uint32_t addr) · line 184

Quoted 13 lines

hw/device-assignment.c +12 -0

+}
+
+static uint32_t slow_bar_readl(void *opaque, target_phys_addr_t addr)
+{
+    AssignedDevRegion *d = opaque;
+    uint32_t *in = (uint32_t*)(d->u.r_virtbase + addr);
+    uint32_t r = -1;
+
+    r = *in;
+    DEBUG("slow_bar_readl addr=0x" TARGET_FMT_plx " val=0x%08x\n", addr, r);
+
+    return r;

same as above

hw/device-assignment.c static uint32_t assigned_dev_ioport_readl(void *opaque, uint32_t addr) · line 190

Quoted 7 lines

hw/device-assignment.c +6 -0

+}
+
+static void slow_bar_writeb(void *opaque, target_phys_addr_t addr, uint32_t val)
+{
+    AssignedDevRegion *d = opaque;
+    uint8_t *out = (uint8_t*)(d->u.r_virtbase + addr);

no need for cast as r_virtbase is a void pointer.

Quoted 9 lines

hw/device-assignment.c +8 -0

+
+    DEBUG("slow_bar_writeb addr=0x" TARGET_FMT_plx " val=0x%02x\n", addr, val);
+    *out = val;
+}
+
+static void slow_bar_writew(void *opaque, target_phys_addr_t addr, uint32_t val)
+{
+    AssignedDevRegion *d = opaque;

hw/device-assignment.c static uint32_t assigned_dev_ioport_readl(void *opaque, uint32_t addr) · line 208

Quoted 10 lines

hw/device-assignment.c +10 -0

+    uint16_t *out = (uint16_t*)(d->u.r_virtbase + addr);
+
+    DEBUG("slow_bar_writew addr=0x" TARGET_FMT_plx " val=0x%04x\n", addr, val);
+    *out = val;
+}
+
+static void slow_bar_writel(void *opaque, target_phys_addr_t addr, uint32_t val)
+{
+    AssignedDevRegion *d = opaque;
+    uint32_t *out = (uint32_t*)(d->u.r_virtbase + addr);

same as above

Quoted 26 lines

hw/device-assignment.c +26 -0

+
+    DEBUG("slow_bar_writel addr=0x" TARGET_FMT_plx " val=0x%08x\n", addr, val);
+    *out = val;
+}
+
+static CPUWriteMemoryFunc * const slow_bar_write[] = {
+    &slow_bar_writeb,
+    &slow_bar_writew,
+    &slow_bar_writel
+};
+
+static CPUReadMemoryFunc * const slow_bar_read[] = {
+    &slow_bar_readb,
+    &slow_bar_readw,
+    &slow_bar_readl
+};
+
+static void assigned_dev_iomem_map_slow(PCIDevice *pci_dev, int region_num,
+                                        pcibus_t e_phys, pcibus_t e_size,
+                                        int type)
+{
+    AssignedDevice *r_dev = container_of(pci_dev, AssignedDevice, dev);
+    AssignedDevRegion *region = &r_dev->v_addrs[region_num];
+    PCIRegion *real_region = &r_dev->real_device.regions[region_num];
+    int m;
+

Quoted 9 lines

hw/device-assignment.c static uint32_t assigned_dev_ioport_readl(void *opaque, uint32_t addr) · line 239

Quoted 6 lines

hw/device-assignment.c +5 -0

+    DEBUG("slow map\n");
+    m = cpu_register_io_memory(slow_bar_read, slow_bar_write, region);
+    cpu_register_physical_memory(e_phys, e_size, m);
+
+    /* MSI-X MMIO page */

some code duplication ... use a common function?

I'm not sure. It's pretty little duplication. If you feel like this should be merged, feel free to make a cleanup patch afterwards :-).

Quoted 10 lines

hw/device-assignment.c static uint32_t assigned_dev_ioport_readl(void *opaque, uint32_t addr) · line 246

Quoted 9 lines

hw/device-assignment.c +7 -0

+    if ((e_size > 0) &&
+        real_region->base_addr <= r_dev->msix_table_addr &&
+        real_region->base_addr + real_region->size >= r_dev->msix_table_addr) {
+        int offset = r_dev->msix_table_addr - real_region->base_addr;
+
+        cpu_register_physical_memory(e_phys + offset,
+                TARGET_PAGE_SIZE, r_dev->mmio_index);

Quoted 3 lines

How does this work? Does the last registered callback win or are both called for MSIX page?

In qemu memory allocations the last one always wins.

Quoted 19 lines

Quoted 19 lines

hw/device-assignment.c +3 -0

+    }
+}
+

static void assigned_dev_iomem_map(PCIDevice *pci_dev, int region_num,
pcibus_t e_phys, pcibus_t e_size, int type)
{

hw/device-assignment.c +2 -2

@@ -381,15 +480,22 @@ static int assigned_dev_register_regions(PCIRegion *io_regions,
 
         /* handle memory io regions */
         if (cur_region->type & IORESOURCE_MEM) {
+            int slow_map = 0;
             int t = cur_region->type & IORESOURCE_PREFETCH
                 ? PCI_BASE_ADDRESS_MEM_PREFETCH
                 : PCI_BASE_ADDRESS_SPACE_MEMORY;
+
             if (cur_region->size & 0xFFF) {
-                fprintf(stderr, "Unable to assign device: PCI region %d "
-                        "at address 0x%llx has size 0x%x, "

Quoted 9 lines

hw/device-assignment.c static int assigned_dev_register_regions(PCIRegion *io_regions, · line 501

Quoted 5 lines

hw/device-assignment.c +3 -1

-                        " which is not a multiple of 4K\n",
+                fprintf(stderr, "PCI region %d at address 0x%llx "
+                        "has size 0x%x, which is not a multiple of 4K. "
+                        "Using slow map\n",
 

I still think "Using slow map" tells the user nothing. How about "Disabling direct guest access, device will be slow"?

Changed.

Quoted 23 lines

Quoted 23 lines

hw/device-assignment.c +6 -1

                         i, (unsigned long long)cur_region->base_addr,
                         cur_region->size);
+                slow_map = 1;
+            }
+
+            if (slow_map && (i == PCI_ROM_SLOT)) {
+                fprintf(stderr, "ROM not aligned - can't continue\n");
                 return -1;
             }

@@ -405,7 +511,7 @@ static int assigned_dev_register_regions(PCIRegion *io_regions,
             } else {
                 pci_dev->v_addrs[i].u.r_virtbase =
                     mmap(NULL,
-                         (cur_region->size + 0xFFF) & 0xFFFFF000,
+                         cur_region->size,
                          PROT_WRITE | PROT_READ, MAP_SHARED,
                          cur_region->resource_fd, (off_t) 0);
             }

@@ -429,12 +535,15 @@ static int assigned_dev_register_regions(PCIRegion *io_regions,
             pci_dev->v_addrs[i].e_size = 0;
 

Quoted 13 lines

hw/device-assignment.c static int assigned_dev_register_regions(PCIRegion *io_regions, · line 542

Quoted 8 lines

hw/device-assignment.c +4 -2

             /* add offset */
-            pci_dev->v_addrs[i].u.r_virtbase +=
-                (cur_region->base_addr & 0xFFF);
+            if (!slow_map) {
+                pci_dev->v_addrs[i].u.r_virtbase +=
+                    (cur_region->base_addr & 0xFFF);
+            }
 

This looks wrong. I think mmap returns a page aligned address, that's why we did this offset math. No?
And if not, there was no reason for this code in the first place.

Yeah, I guess you're right.

Alex

I guess this means you'll have to find a device with a sub-page BAR to test this on, instead of hacking driver for a device with full page BAR. Did anyone ever try doing passthrough on an emulated device in nested virt?

Quoted 4 lines · Michael S. Tsirkin

I guess this means you'll have to find a device with a sub-page BAR to test this on, instead of hacking driver for a device with full page BAR. Did anyone ever try doing passthrough on an emulated device in nested virt?

We don't emulate an IOMMU, so no.