Skip to contentSkip to conversation

pci-assign: Drop support for raw ioport access

RFC PATCH In review Jan Kiszka 4 participants 10 messages May 28, 2012 – May 29, 2012 KVM

lore
Share & tools
lore

Jan Kiszka

author
#0 lore raw

RFC PATCH 1/1 pci-assign: Drop support for raw ioport access

From: Jan Kiszka <[email protected]>

If the kernel does not support ioport access via sysfs, passthrough can only help if the unlikely case that a port <= 0x3ff is provided by the device. So drop this to simplify the code and to allow dropping the corresponding KVM infrastructure in preparation of upstream merge.

Signed-off-by Jan Kiszka <[email protected]>

---

Does anyone recall the precise use case this was introduced for? It exists since day #1, so commit logs do not help.

 hw/device-assignment.c |   20 +++-----------------
 1 files changed, 3 insertions(+), 17 deletions(-)
hw/device-assignment.c +3 -17
index 1daadb9..9ad5de5 100644
@@ -245,18 +245,8 @@ static void assigned_dev_ioport_setup(PCIDevice *pci_dev, int region_num,
 {
     AssignedDevice *r_dev = DO_UPCAST(AssignedDevice, dev, pci_dev);
     AssignedDevRegion *region = &r_dev->v_addrs[region_num];
-    int r;
 
     region->e_size = size;
-
-    if (region->region->resource_fd < 0) {
-        r = kvm_add_ioport_region(region->u.r_baseport, region->r_size,
-                                  pci_dev->qdev.hotplugged);
-        if (r < 0) {
-            fprintf(stderr, "%s: failed to enable ioport access (%m)\n",
-                    __func__);
-        }
-    }
     memory_region_init(&region->container, "assigned-dev-container", size);
     memory_region_init_io(&region->real_iomem, &assigned_dev_ioport_ops,
                           r_dev->v_addrs + region_num,

@@ -440,10 +430,10 @@ static int assigned_dev_register_regions(PCIRegion *io_regions,
                         ret);
                 abort();
             } else if (errno != EINVAL) {
-                fprintf(stderr, "Using raw in/out ioport access (sysfs - %s)\n",
-                        strerror(errno));
+                fprintf(stderr,
+                        "Kernel doesn't support ioport resource access.\n");
                 close(pci_dev->v_addrs[i].region->resource_fd);
-                pci_dev->v_addrs[i].region->resource_fd = -1;
+                return -1;
             }
 
             pci_dev->v_addrs[i].u.r_baseport = cur_region->base_addr;

@@ -647,10 +637,6 @@ static void free_assigned_device(AssignedDevice *dev)
             continue;
         }
         if (pci_region->type & IORESOURCE_IO) {
-            if (pci_region->resource_fd < 0) {
-                kvm_remove_ioport_region(region->u.r_baseport, region->r_size,
-                                         dev->dev.qdev.hotplugged);
-            }
             memory_region_del_subregion(&region->container,
                                         &region->real_iomem);
             memory_region_destroy(&region->real_iomem);
Signature · 1 line
1.7.3.4
#1 lore raw
Quoted 7 lines · Jan Kiszka

From: Jan Kiszka <[email protected]>

If the kernel does not support ioport access via sysfs, passthrough can only help if the unlikely case that a port <= 0x3ff is provided by the device. So drop this to simplify the code and to allow dropping the corresponding KVM infrastructure in preparation of upstream merge.

Quoted 5 lines
Signed-off-by Jan Kiszka <[email protected]>

---

Does anyone recall the precise use case this was introduced for? It exists since day #1, so commit logs do not help.

At a wild guess, graphics device assignment.

Under what conditions would the kernel not support ioport access via sysfs?

Signature · 1 line
error compiling committee.c: too many arguments to function
#2 lore raw
Quoted 8 lines · Avi Kivity
Quoted 7 lines · Jan Kiszka

From: Jan Kiszka <[email protected]>

If the kernel does not support ioport access via sysfs, passthrough can only help if the unlikely case that a port <= 0x3ff is provided by the device. So drop this to simplify the code and to allow dropping the corresponding KVM infrastructure in preparation of upstream merge.

Quoted 7 lines
Quoted 5 lines
Signed-off-by Jan Kiszka <[email protected]>

---

Does anyone recall the precise use case this was introduced for? It exists since day #1, so commit logs do not help.

At a wild guess, graphics device assignment.

The only explanation now. But that requires more work anyway (e.g. to claim the VGA adapter toward the kernel). And I but we would rather do this on top of VFIO on day.

Quoted 3 lines

Under what conditions would the kernel not support ioport access via sysfs?

No clue. The oldest kernel I checked (2.6.16) does not contain traces it would refuse to provide access.

Jan

Signature · 2 lines
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
#3 lore raw
Quoted 7 lines · Jan Kiszka
Quoted 4 lines

Under what conditions would the kernel not support ioport access via sysfs?

No clue. The oldest kernel I checked (2.6.16) does not contain traces it would refuse to provide access.

I guess this was added first, and sysfs support was added later (9ed83e8eb18b0). Alex, any idea what kernels would fail this?

Signature · 1 line
error compiling committee.c: too many arguments to function
#4 lore raw
Quoted 11 lines · Avi Kivity
Quoted 7 lines · Jan Kiszka
Quoted 4 lines

Under what conditions would the kernel not support ioport access via sysfs?

No clue. The oldest kernel I checked (2.6.16) does not contain traces it would refuse to provide access.

I guess this was added first, and sysfs support was added later (9ed83e8eb18b0). Alex, any idea what kernels would fail this?

It's more recent than that. 8633328b is where we added pci-sysfs ioport access, so 2.6.35 would be the first kernel with it. I don't really know how this ever worked except for a very limited range of io ports addresses before the sysfs access went in, so I'm ok with removing it. Thanks,

Alex

#5 lore raw
Quoted 10 lines · Alex Williamson
Quoted 9 lines · Avi Kivity
Quoted 7 lines · Jan Kiszka
Quoted 4 lines

Under what conditions would the kernel not support ioport access via sysfs?

No clue. The oldest kernel I checked (2.6.16) does not contain traces it would refuse to provide access.

Quoted 9 lines
Quoted 2 lines

I guess this was added first, and sysfs support was added later (9ed83e8eb18b0). Alex, any idea what kernels would fail this?

It's more recent than that. 8633328b is where we added pci-sysfs ioport access, so 2.6.35 would be the first kernel with it. I don't really know how this ever worked except for a very limited range of io ports addresses before the sysfs access went in, so I'm ok with removing it. Thanks,

Okay. Both patches applied, thanks.

Signature · 1 line
error compiling committee.c: too many arguments to function
#6 lore raw

PATCH 1/1 qemu-kvm: Drop support for raw ioport access

In reply to Jan Kiszka

No longer needed since device assignment stopped using it.

Signed-off-by Jan Kiszka <[email protected]>

---

Provided "pci-assign: Drop support for raw ioport access" is applied.

 qemu-kvm.c        |   93 -----------------------------------------------------
 qemu-kvm.h        |    9 -----
 target-i386/kvm.c |   18 ----------
 3 files changed, 0 insertions(+), 120 deletions(-)
qemu-kvm.c +0 -93
index 733cd04..133143c 100644
@@ -315,96 +315,3 @@ void kvm_arch_init_irq_routing(KVMState *s)
 {
 }
 #endif
-
-#ifdef CONFIG_KVM_DEVICE_ASSIGNMENT
-typedef struct KVMIOPortRegion {
-    unsigned long start;
-    unsigned long size;
-    int status;
-    QLIST_ENTRY(KVMIOPortRegion) entry;
-} KVMIOPortRegion;
-
-static QLIST_HEAD(, KVMIOPortRegion) ioport_regions;
-
-static void do_set_ioport_access(void *data)
-{
-    KVMIOPortRegion *region = data;
-    bool enable = region->status > 0;
-    int r;
-
-    r = kvm_arch_set_ioport_access(region->start, region->size, enable);
-    if (r < 0) {
-        region->status = r;
-    } else {
-        region->status = 1;
-    }
-}
-
-int kvm_add_ioport_region(unsigned long start, unsigned long size,
-                          bool is_hot_plug)
-{
-    KVMIOPortRegion *region = g_malloc0(sizeof(KVMIOPortRegion));
-    CPUArchState *env;
-    int r = 0;
-
-    region->start = start;
-    region->size = size;
-    region->status = 1;
-    QLIST_INSERT_HEAD(&ioport_regions, region, entry);
-
-    if (is_hot_plug) {
-        for (env = first_cpu; env != NULL; env = env->next_cpu) {
-            run_on_cpu(env, do_set_ioport_access, region);
-            if (region->status < 0) {
-                r = region->status;
-                kvm_remove_ioport_region(start, size, is_hot_plug);
-                break;
-            }
-        }
-    }
-    return r;
-}
-
-int kvm_remove_ioport_region(unsigned long start, unsigned long size,
-                             bool is_hot_unplug)
-{
-    KVMIOPortRegion *region, *tmp;
-    CPUArchState *env;
-    int r = -ENOENT;
-
-    QLIST_FOREACH_SAFE(region, &ioport_regions, entry, tmp) {
-        if (region->start == start && region->size == size) {
-            region->status = 0;
-        }
-        if (is_hot_unplug) {
-            for (env = first_cpu; env != NULL; env = env->next_cpu) {
-                run_on_cpu(env, do_set_ioport_access, region);
-            }
-        }
-        QLIST_REMOVE(region, entry);
-        g_free(region);
-        r = 0;
-    }
-    return r;
-}
-#endif /* CONFIG_KVM_DEVICE_ASSIGNMENT */
-
-int kvm_update_ioport_access(CPUArchState *env)
-{
-#ifdef CONFIG_KVM_DEVICE_ASSIGNMENT
-    KVMIOPortRegion *region;
-    int r;
-
-    assert(qemu_cpu_is_self(env));
-
-    QLIST_FOREACH(region, &ioport_regions, entry) {
-        bool enable = region->status > 0;
-
-        r = kvm_arch_set_ioport_access(region->start, region->size, enable);
-        if (r < 0) {
-            return r;
-        }
-    }
-#endif /* CONFIG_KVM_DEVICE_ASSIGNMENT */
-    return 0;
-}
qemu-kvm.h +0 -9
index ddf87a0..3ebbbb6 100644
@@ -109,13 +109,4 @@ int kvm_assign_set_msix_entry(KVMState *s,
 
 #endif /* CONFIG_KVM */
 
-int kvm_add_ioport_region(unsigned long start, unsigned long size,
-                          bool is_hot_plug);
-int kvm_remove_ioport_region(unsigned long start, unsigned long size,
-                             bool is_hot_unplug);
-
-int kvm_update_ioport_access(CPUArchState *env);
-int kvm_arch_set_ioport_access(unsigned long start, unsigned long size,
-                               bool enable);
-
 #endif
target-i386/kvm.c +0 -18
index 554a149..e74a9e4 100644
@@ -358,11 +358,6 @@ int kvm_arch_init_vcpu(CPUX86State *env)
     uint32_t signature[3];
     int r;
 
-    r = kvm_update_ioport_access(env);
-    if (r < 0) {
-        return r;
-    }
-
     env->cpuid_features &= kvm_arch_get_supported_cpuid(s, 1, 0, R_EDX);
 
     i = env->cpuid_ext_features & CPUID_EXT_HYPERVISOR;

@@ -2034,16 +2029,3 @@ void kvm_arch_init_irq_routing(KVMState *s)
         no_hpet = 1;
     }
 }
-
-#ifdef CONFIG_KVM_DEVICE_ASSIGNMENT
-#include <sys/io.h>
-
-int kvm_arch_set_ioport_access(unsigned long start, unsigned long size,
-                               bool enable)
-{
-    if (ioperm(start, size, enable) < 0) {
-        return -errno;
-    }
-    return 0;
-}
-#endif
Signature · 1 line
1.7.3.4
Acked-by on v1 1/1
#7 lore raw
In reply to Jan Kiszka
Quoted 7 lines · Jan Kiszka

From: Jan Kiszka <[email protected]>

If the kernel does not support ioport access via sysfs, passthrough can only help if the unlikely case that a port <= 0x3ff is provided by the device. So drop this to simplify the code and to allow dropping the corresponding KVM infrastructure in preparation of upstream merge.

Quoted 5 lines
Signed-off-by Jan Kiszka <[email protected]>

---

Does anyone recall the precise use case this was introduced for? It exists since day #1, so commit logs do not help.

Nope, I don't know how this ever worked except for very limited address ranges.

Acked-by Alex Williamson <[email protected]>
Quoted 51 lines
 hw/device-assignment.c |   20 +++-----------------
 1 files changed, 3 insertions(+), 17 deletions(-)
hw/device-assignment.c +3 -17
index 1daadb9..9ad5de5 100644
@@ -245,18 +245,8 @@ static void assigned_dev_ioport_setup(PCIDevice *pci_dev, int region_num,
 {
     AssignedDevice *r_dev = DO_UPCAST(AssignedDevice, dev, pci_dev);
     AssignedDevRegion *region = &r_dev->v_addrs[region_num];
-    int r;
 
     region->e_size = size;
-
-    if (region->region->resource_fd < 0) {
-        r = kvm_add_ioport_region(region->u.r_baseport, region->r_size,
-                                  pci_dev->qdev.hotplugged);
-        if (r < 0) {
-            fprintf(stderr, "%s: failed to enable ioport access (%m)\n",
-                    __func__);
-        }
-    }
     memory_region_init(&region->container, "assigned-dev-container", size);
     memory_region_init_io(&region->real_iomem, &assigned_dev_ioport_ops,
                           r_dev->v_addrs + region_num,

@@ -440,10 +430,10 @@ static int assigned_dev_register_regions(PCIRegion *io_regions,
                         ret);
                 abort();
             } else if (errno != EINVAL) {
-                fprintf(stderr, "Using raw in/out ioport access (sysfs - %s)\n",
-                        strerror(errno));
+                fprintf(stderr,
+                        "Kernel doesn't support ioport resource access.\n");
                 close(pci_dev->v_addrs[i].region->resource_fd);
-                pci_dev->v_addrs[i].region->resource_fd = -1;
+                return -1;
             }
 
             pci_dev->v_addrs[i].u.r_baseport = cur_region->base_addr;

@@ -647,10 +637,6 @@ static void free_assigned_device(AssignedDevice *dev)
             continue;
         }
         if (pci_region->type & IORESOURCE_IO) {
-            if (pci_region->resource_fd < 0) {
-                kvm_remove_ioport_region(region->u.r_baseport, region->r_size,
-                                         dev->dev.qdev.hotplugged);
-            }
             memory_region_del_subregion(&region->container,
                                         &region->real_iomem);
             memory_region_destroy(&region->real_iomem);
#8 lore raw
In reply to Jan Kiszka
Quoted 40 lines · Jan Kiszka

From: Jan Kiszka <[email protected]>

If the kernel does not support ioport access via sysfs, passthrough can only help if the unlikely case that a port <= 0x3ff is provided by the device. So drop this to simplify the code and to allow dropping the corresponding KVM infrastructure in preparation of upstream merge.

Signed-off-by Jan Kiszka <[email protected]>

---

Does anyone recall the precise use case this was introduced for? It exists since day #1, so commit logs do not help.

 hw/device-assignment.c |   20 +++-----------------
 1 files changed, 3 insertions(+), 17 deletions(-)
hw/device-assignment.c +0 -10
index 1daadb9..9ad5de5 100644
@@ -245,18 +245,8 @@ static void assigned_dev_ioport_setup(PCIDevice *pci_dev, int region_num,
 {
     AssignedDevice *r_dev = DO_UPCAST(AssignedDevice, dev, pci_dev);
     AssignedDevRegion *region = &r_dev->v_addrs[region_num];
-    int r;
 
     region->e_size = size;
-
-    if (region->region->resource_fd < 0) {
-        r = kvm_add_ioport_region(region->u.r_baseport, region->r_size,
-                                  pci_dev->qdev.hotplugged);
-        if (r < 0) {
-            fprintf(stderr, "%s: failed to enable ioport access (%m)\n",
-                    __func__);
-        }
-    }
     memory_region_init(&region->container, "assigned-dev-container", size);
     memory_region_init_io(&region->real_iomem, &assigned_dev_ioport_ops,
                           r_dev->v_addrs + region_num,

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

Quoted 10 lines
hw/device-assignment.c +3 -3
@@ -440,10 +430,10 @@ static int assigned_dev_register_regions(PCIRegion *io_regions,
                         ret);
                 abort();
             } else if (errno != EINVAL) {
-                fprintf(stderr, "Using raw in/out ioport access (sysfs - %s)\n",
-                        strerror(errno));
+                fprintf(stderr,
+                        "Kernel doesn't support ioport resource access.\n");
                 close(pci_dev->v_addrs[i].region->resource_fd);
-                pci_dev->v_addrs[i].region->resource_fd = -1;
+                return -1;

Jan, I think we could probably get away with making this non-fatal. Quite a few cards include an I/O port range that's never used. Can we follow-up with a patch that just hides the I/O port BAR if we don't have sysfs access and print and error so we have a breadcrumb if the device then fails to work? If someone is using a pre-sysfs-ioport kernel that would at least be a little more friendly and probably the same level of functionality they have now. Thanks,

Alex

Quoted 14 lines
hw/device-assignment.c +0 -4
             }
 
             pci_dev->v_addrs[i].u.r_baseport = cur_region->base_addr;

@@ -647,10 +637,6 @@ static void free_assigned_device(AssignedDevice *dev)
             continue;
         }
         if (pci_region->type & IORESOURCE_IO) {
-            if (pci_region->resource_fd < 0) {
-                kvm_remove_ioport_region(region->u.r_baseport, region->r_size,
-                                         dev->dev.qdev.hotplugged);
-            }
             memory_region_del_subregion(&region->container,
                                         &region->real_iomem);
             memory_region_destroy(&region->real_iomem);
#9 lore raw
Quoted 52 lines · Alex Williamson
Quoted 40 lines · Jan Kiszka

From: Jan Kiszka <[email protected]>

If the kernel does not support ioport access via sysfs, passthrough can only help if the unlikely case that a port <= 0x3ff is provided by the device. So drop this to simplify the code and to allow dropping the corresponding KVM infrastructure in preparation of upstream merge.

Signed-off-by Jan Kiszka <[email protected]>

---

Does anyone recall the precise use case this was introduced for? It exists since day #1, so commit logs do not help.

 hw/device-assignment.c |   20 +++-----------------
 1 files changed, 3 insertions(+), 17 deletions(-)
hw/device-assignment.c +0 -10
index 1daadb9..9ad5de5 100644
@@ -245,18 +245,8 @@ static void assigned_dev_ioport_setup(PCIDevice *pci_dev, int region_num,
 {
     AssignedDevice *r_dev = DO_UPCAST(AssignedDevice, dev, pci_dev);
     AssignedDevRegion *region = &r_dev->v_addrs[region_num];
-    int r;
 
     region->e_size = size;
-
-    if (region->region->resource_fd < 0) {
-        r = kvm_add_ioport_region(region->u.r_baseport, region->r_size,
-                                  pci_dev->qdev.hotplugged);
-        if (r < 0) {
-            fprintf(stderr, "%s: failed to enable ioport access (%m)\n",
-                    __func__);
-        }
-    }
     memory_region_init(&region->container, "assigned-dev-container", size);
     memory_region_init_io(&region->real_iomem, &assigned_dev_ioport_ops,
                           r_dev->v_addrs + region_num,

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

Quoted 10 lines
hw/device-assignment.c +3 -3
@@ -440,10 +430,10 @@ static int assigned_dev_register_regions(PCIRegion *io_regions,
                         ret);
                 abort();
             } else if (errno != EINVAL) {
-                fprintf(stderr, "Using raw in/out ioport access (sysfs - %s)\n",
-                        strerror(errno));
+                fprintf(stderr,
+                        "Kernel doesn't support ioport resource access.\n");
                 close(pci_dev->v_addrs[i].region->resource_fd);
-                pci_dev->v_addrs[i].region->resource_fd = -1;
+                return -1;
Quoted 7 lines

Jan, I think we could probably get away with making this non-fatal. Quite a few cards include an I/O port range that's never used. Can we follow-up with a patch that just hides the I/O port BAR if we don't have sysfs access and print and error so we have a breadcrumb if the device then fails to work? If someone is using a pre-sysfs-ioport kernel that would at least be a little more friendly and probably the same level of functionality they have now. Thanks,

Sounds reasonable. Will have a look.

Jan

Signature · 2 lines
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux