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

[Alex Williamson <alex.williamson@redhat.com>]

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