[RFC][PATCH] pci-assign: Drop support for raw ioport access

[RFC][PATCH] pci-assign: Drop support for raw ioport access

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 2852 bytes --]

From: Jan Kiszka <jan.kiszka@siemens.com>

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 <jan.kiszka@siemens.com>
---

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(-)

diff --git a/hw/device-assignment.c b/hw/device-assignment.c
index 1daadb9..9ad5de5 100644
--- a/hw/device-assignment.c
+++ b/hw/device-assignment.c
@@ -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);
-- 
1.7.3.4


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [RFC][PATCH] pci-assign: Drop support for raw ioport access

[Avi Kivity]

On 05/28/2012 04:03 PM, Jan Kiszka wrote:
> From: Jan Kiszka <jan.kiszka@siemens.com>
> 
> 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 <jan.kiszka@siemens.com>
> ---
> 
> 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?

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

Re: [RFC][PATCH] pci-assign: Drop support for raw ioport access

[Jan Kiszka]

On 2012-05-28 16:06, Avi Kivity wrote:
> On 05/28/2012 04:03 PM, Jan Kiszka wrote:
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> 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 <jan.kiszka@siemens.com>
>> ---
>>
>> 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.

> 
> 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

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [RFC][PATCH] pci-assign: Drop support for raw ioport access

[Avi Kivity]

On 05/29/2012 10:47 AM, Jan Kiszka wrote:
> 
>> 
>> 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?




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

Re: [RFC][PATCH] pci-assign: Drop support for raw ioport access

[Alex Williamson]

On Tue, 2012-05-29 at 12:26 +0300, Avi Kivity wrote:
> On 05/29/2012 10:47 AM, Jan Kiszka wrote:
> > 
> >> 
> >> 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

Re: [RFC][PATCH] pci-assign: Drop support for raw ioport access

[Avi Kivity]

On 05/29/2012 04:47 PM, Alex Williamson wrote:
> On Tue, 2012-05-29 at 12:26 +0300, Avi Kivity wrote:
>> On 05/29/2012 10:47 AM, Jan Kiszka wrote:
>> > 
>> >> 
>> >> 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,
> 

Okay.  Both patches applied, thanks.


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

[PATCH] qemu-kvm: Drop support for raw ioport access

[Jan Kiszka]

No longer needed since device assignment stopped using it.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

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(-)

diff --git a/qemu-kvm.c b/qemu-kvm.c
index 733cd04..133143c 100644
--- a/qemu-kvm.c
+++ b/qemu-kvm.c
@@ -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;
-}
diff --git a/qemu-kvm.h b/qemu-kvm.h
index ddf87a0..3ebbbb6 100644
--- a/qemu-kvm.h
+++ b/qemu-kvm.h
@@ -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
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 554a149..e74a9e4 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -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
-- 
1.7.3.4

Re: [RFC][PATCH] pci-assign: Drop support for raw ioport access

[Alex Williamson]

On Mon, 2012-05-28 at 15:03 +0200, Jan Kiszka wrote:
> From: Jan Kiszka <jan.kiszka@siemens.com>
> 
> 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 <jan.kiszka@siemens.com>
> ---
> 
> 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 <alex.williamson@redhat.com>

>  hw/device-assignment.c |   20 +++-----------------
>  1 files changed, 3 insertions(+), 17 deletions(-)
> 
> diff --git a/hw/device-assignment.c b/hw/device-assignment.c
> index 1daadb9..9ad5de5 100644
> --- a/hw/device-assignment.c
> +++ b/hw/device-assignment.c
> @@ -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);

Re: [RFC][PATCH] pci-assign: Drop support for raw ioport access

[Alex Williamson]

On Mon, 2012-05-28 at 15:03 +0200, Jan Kiszka wrote:
> From: Jan Kiszka <jan.kiszka@siemens.com>
> 
> 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 <jan.kiszka@siemens.com>
> ---
> 
> 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(-)
> 
> diff --git a/hw/device-assignment.c b/hw/device-assignment.c
> index 1daadb9..9ad5de5 100644
> --- a/hw/device-assignment.c
> +++ b/hw/device-assignment.c
> @@ -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;

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

>              }
>  
>              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);

Re: [RFC][PATCH] pci-assign: Drop support for raw ioport access

[Jan Kiszka]

On 2012-05-29 16:41, Alex Williamson wrote:
> On Mon, 2012-05-28 at 15:03 +0200, Jan Kiszka wrote:
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> 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 <jan.kiszka@siemens.com>
>> ---
>>
>> 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(-)
>>
>> diff --git a/hw/device-assignment.c b/hw/device-assignment.c
>> index 1daadb9..9ad5de5 100644
>> --- a/hw/device-assignment.c
>> +++ b/hw/device-assignment.c
>> @@ -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;
> 
> 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

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux