Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1239211 > unrolled thread

[PATCH v3 0/3] uio: add MSI/MSI-X support to uio_pci_generic driver

Back to article view | Back to linux.kernel

Contents

  [PATCH v3 0/3] uio: add MSI/MSI-X support to uio_pci_generic driver Vlad Zolotarov <vladz@cloudius-systems.com> - 2015-10-04 22:50 +0200
    [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Vlad Zolotarov <vladz@cloudius-systems.com> - 2015-10-04 22:50 +0200
      Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Greg KH <gregkh@linuxfoundation.org> - 2015-10-05 05:20 +0200
        Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Vlad Zolotarov <vladz@cloudius-systems.com> - 2015-10-05 09:50 +0200
          Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Greg KH <gregkh@linuxfoundation.org> - 2015-10-05 11:50 +0200
            Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Vlad Zolotarov <vladz@cloudius-systems.com> - 2015-10-05 12:50 +0200
              Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Avi Kivity <avi@scylladb.com> - 2015-10-05 13:10 +0200
                Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Greg KH <gregkh@linuxfoundation.org> - 2015-10-05 15:10 +0200
              Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Greg KH <gregkh@linuxfoundation.org> - 2015-10-05 13:10 +0200
                Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Vlad Zolotarov <vladz@cloudius-systems.com> - 2015-10-05 13:50 +0200
                  Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Avi Kivity <avi@scylladb.com> - 2015-10-05 13:50 +0200
                    Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Vlad Zolotarov <vladz@cloudius-systems.com> - 2015-10-05 14:00 +0200
        Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Avi Kivity <avi@scylladb.com> - 2015-10-05 10:30 +0200
          Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Greg KH <gregkh@linuxfoundation.org> - 2015-10-05 11:50 +0200
            Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Avi Kivity <avi@scylladb.com> - 2015-10-05 12:30 +0200
              Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support "Michael S. Tsirkin" <mst@redhat.com> - 2015-10-06 16:40 +0200
                Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Vlad Zolotarov <vladz@cloudius-systems.com> - 2015-10-06 16:50 +0200
                  Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support "Michael S. Tsirkin" <mst@redhat.com> - 2015-10-06 17:00 +0200
                    Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Vlad Zolotarov <vladz@cloudius-systems.com> - 2015-10-06 17:30 +0200
                    Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Avi Kivity <avi@scylladb.com> - 2015-10-06 17:30 +0200
                      Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Alex Williamson <alex.williamson@redhat.com> - 2015-10-06 21:00 +0200
                        Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Stephen Hemminger <stephen@networkplumber.org> - 2015-10-06 23:40 +0200
                          Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Alex Williamson <alex.williamson@redhat.com> - 2015-10-06 23:50 +0200
                            Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Vlad Zolotarov <vladz@cloudius-systems.com> - 2015-10-07 10:00 +0200
                            Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Vlad Zolotarov <vladz@cloudius-systems.com> - 2015-10-07 10:10 +0200
                              Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Vlad Zolotarov <vladz@cloudius-systems.com> - 2015-10-07 10:10 +0200
                        Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Avi Kivity <avi@scylladb.com> - 2015-10-07 09:00 +0200
                          Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Avi Kivity <avi@scylladb.com> - 2015-10-07 18:40 +0200
                            Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support "Michael S. Tsirkin" <mst@redhat.com> - 2015-10-07 23:10 +0200
                              Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Gleb Natapov <gleb@scylladb.com> - 2015-10-08 06:20 +0200
                                Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support "Michael S. Tsirkin" <mst@redhat.com> - 2015-10-08 09:50 +0200
                                  Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Gleb Natapov <gleb@scylladb.com> - 2015-10-08 10:00 +0200
                                    Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support "Michael S. Tsirkin" <mst@redhat.com> - 2015-10-08 11:40 +0200
                                      Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Gleb Natapov <gleb@scylladb.com> - 2015-10-08 11:50 +0200
                                        Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support "Michael S. Tsirkin" <mst@redhat.com> - 2015-10-08 14:20 +0200
                              Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Avi Kivity <avi@scylladb.com> - 2015-10-08 07:40 +0200
                                Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support "Michael S. Tsirkin" <mst@redhat.com> - 2015-10-08 09:40 +0200
                                  Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Avi Kivity <avi@scylladb.com> - 2015-10-08 10:50 +0200
                                    Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support "Michael S. Tsirkin" <mst@redhat.com> - 2015-10-08 11:20 +0200
                                      Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Avi Kivity <avi@scylladb.com> - 2015-10-08 11:50 +0200
                                        Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support "Michael S. Tsirkin" <mst@redhat.com> - 2015-10-08 14:10 +0200
                                          Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Gleb Natapov <gleb@scylladb.com> - 2015-10-08 14:30 +0200
                                            Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support "Michael S. Tsirkin" <mst@redhat.com> - 2015-10-08 15:30 +0200
                                              Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Gleb Natapov <gleb@scylladb.com> - 2015-10-08 15:30 +0200
                                                Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support "Michael S. Tsirkin" <mst@redhat.com> - 2015-10-08 18:50 +0200
                                                  Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Gleb Natapov <gleb@scylladb.com> - 2015-10-08 19:10 +0200
                                                    Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support "Michael S. Tsirkin" <mst@redhat.com> - 2015-10-08 19:40 +0200
                                                      Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Gleb Natapov <gleb@scylladb.com> - 2015-10-08 20:00 +0200
                                                      Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Greg KH <gregkh@linuxfoundation.org> - 2015-10-08 20:40 +0200
                                Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support "Michael S. Tsirkin" <mst@redhat.com> - 2015-10-08 10:40 +0200
                                  Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Gleb Natapov <gleb@scylladb.com> - 2015-10-08 11:00 +0200
                                  Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Avi Kivity <avi@scylladb.com> - 2015-10-08 11:20 +0200
                                    Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support "Michael S. Tsirkin" <mst@redhat.com> - 2015-10-08 12:30 +0200
                                      Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Avi Kivity <avi@scylladb.com> - 2015-10-08 15:30 +0200
                                        Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support "Michael S. Tsirkin" <mst@redhat.com> - 2015-10-08 16:20 +0200
                                        Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Alex Williamson <alex.williamson@redhat.com> - 2015-10-08 17:40 +0200
                          Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Alex Williamson <alex.williamson@redhat.com> - 2015-10-07 18:40 +0200
                            Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support "Michael S. Tsirkin" <mst@redhat.com> - 2015-10-07 22:10 +0200
                        Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Vlad Zolotarov <vladz@cloudius-systems.com> - 2015-10-07 10:00 +0200
                          Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support "Michael S. Tsirkin" <mst@redhat.com> - 2015-10-08 10:50 +0200
          Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support "Michael S. Tsirkin" <mst@redhat.com> - 2015-10-06 16:50 +0200
            Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Avi Kivity <avi@scylladb.com> - 2015-10-06 17:30 +0200
      Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Stephen Hemminger <stephen@networkplumber.org> - 2015-10-05 10:50 +0200
        Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Vlad Zolotarov <vladz@cloudius-systems.com> - 2015-10-05 11:10 +0200
          Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Vlad Zolotarov <vladz@cloudius-systems.com> - 2015-10-05 12:10 +0200
            Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support "Michael S. Tsirkin" <mst@redhat.com> - 2015-10-05 22:20 +0200
        Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support Vlad Zolotarov <vladz@cloudius-systems.com> - 2015-10-05 11:20 +0200
      Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support "Michael S. Tsirkin" <mst@redhat.com> - 2015-10-05 21:20 +0200
    Re: [PATCH v3 0/3] uio: add MSI/MSI-X support to uio_pci_generic  driver Vlad Zolotarov <vladz@cloudius-systems.com> - 2015-10-04 22:50 +0200
    Re: [PATCH v3 1/3] uio: add ioctl support Greg KH <gregkh@linuxfoundation.org> - 2015-10-05 05:10 +0200
      Re: [PATCH v3 1/3] uio: add ioctl support Vlad Zolotarov <vladz@cloudius-systems.com> - 2015-10-05 09:40 +0200
        Re: [PATCH v3 1/3] uio: add ioctl support Greg KH <gregkh@linuxfoundation.org> - 2015-10-05 11:50 +0200
          Re: [PATCH v3 1/3] uio: add ioctl support Vlad Zolotarov <vladz@cloudius-systems.com> - 2015-10-05 12:40 +0200
            Re: [PATCH v3 1/3] uio: add ioctl support "Michael S. Tsirkin" <mst@redhat.com> - 2015-10-05 22:10 +0200
              Re: [PATCH v3 1/3] uio: add ioctl support "Michael S. Tsirkin" <mst@redhat.com> - 2015-10-06 00:30 +0200
                Re: [PATCH v3 1/3] uio: add ioctl support Vlad Zolotarov <vladz@cloudius-systems.com> - 2015-10-06 10:40 +0200
                  Re: [PATCH v3 1/3] uio: add ioctl support "Michael S. Tsirkin" <mst@redhat.com> - 2015-10-06 16:20 +0200
                    Re: [PATCH v3 1/3] uio: add ioctl support Gleb Natapov <gleb@scylladb.com> - 2015-10-06 16:40 +0200
                      Re: [PATCH v3 1/3] uio: add ioctl support "Michael S. Tsirkin" <mst@redhat.com> - 2015-10-06 17:30 +0200
                        Re: [PATCH v3 1/3] uio: add ioctl support Vlad Zolotarov <vladz@cloudius-systems.com> - 2015-10-06 17:40 +0200
                        Re: [PATCH v3 1/3] uio: add ioctl support Gleb Natapov <gleb@scylladb.com> - 2015-10-06 18:00 +0200
    Re: [PATCH v3 0/3] uio: add MSI/MSI-X support to uio_pci_generic  driver "Michael S. Tsirkin" <mst@redhat.com> - 2015-10-05 22:00 +0200
      Re: [PATCH v3 0/3] uio: add MSI/MSI-X support to uio_pci_generic  driver Vlad Zolotarov <vladz@cloudius-systems.com> - 2015-10-06 10:40 +0200
        Re: [PATCH v3 0/3] uio: add MSI/MSI-X support to uio_pci_generic  driver "Michael S. Tsirkin" <mst@redhat.com> - 2015-10-06 16:40 +0200
          Re: [PATCH v3 0/3] uio: add MSI/MSI-X support to uio_pci_generic  driver Vlad Zolotarov <vladz@cloudius-systems.com> - 2015-10-06 16:50 +0200
            Re: [PATCH v3 0/3] uio: add MSI/MSI-X support to uio_pci_generic  driver "Michael S. Tsirkin" <mst@redhat.com> - 2015-10-06 17:20 +0200
              Re: [PATCH v3 0/3] uio: add MSI/MSI-X support to uio_pci_generic  driver Vlad Zolotarov <vladz@cloudius-systems.com> - 2015-10-06 18:40 +0200
          Re: [PATCH v3 0/3] uio: add MSI/MSI-X support to uio_pci_generic  driver Avi Kivity <avi@cloudius-systems.com> - 2015-10-06 17:20 +0200
            Re: [PATCH v3 0/3] uio: add MSI/MSI-X support to uio_pci_generic  driver "Michael S. Tsirkin" <mst@redhat.com> - 2015-10-06 17:20 +0200
              Re: [PATCH v3 0/3] uio: add MSI/MSI-X support to uio_pci_generic  driver Gleb Natapov <gleb@scylladb.com> - 2015-10-06 18:10 +0200
              Re: [PATCH v3 0/3] uio: add MSI/MSI-X support to uio_pci_generic  driver Avi Kivity <avi@scylladb.com> - 2015-10-06 18:10 +0200
                Re: [PATCH v3 0/3] uio: add MSI/MSI-X support to uio_pci_generic  driver Avi Kivity <avi@scylladb.com> - 2015-10-07 12:30 +0200
                Re: [PATCH v3 0/3] uio: add MSI/MSI-X support to uio_pci_generic  driver "Michael S. Tsirkin" <mst@redhat.com> - 2015-10-07 12:30 +0200

Page 4 of 5 — ← Prev page 1 2 3 [4] 5  Next page →

#1240514 — Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support

On Mon, Oct 05, 2015 at 11:28:03AM +0300, Avi Kivity wrote:
> Eventfd is a natural enough representation of an interrupt; both kvm and
> vfio use it, and are also able to share the eventfd, allowing a vfio
> interrupt to generate a kvm interrupt, without userspace intervention, and
> one day without even kernel intervention.

eventfd without kernel intervention sounds unlikely.

kvm might configure the cpu such that an interrupt will not trigger a
vmexit.  eventfd seems like an unlikely interface to do that: with the
eventfd, device triggering it has no info about the interrupt so it
can't send it to the correct VM.

-- 
MST
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1240569 — Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support

On 10/06/2015 05:46 PM, Michael S. Tsirkin wrote:
> On Mon, Oct 05, 2015 at 11:28:03AM +0300, Avi Kivity wrote:
>> Eventfd is a natural enough representation of an interrupt; both kvm and
>> vfio use it, and are also able to share the eventfd, allowing a vfio
>> interrupt to generate a kvm interrupt, without userspace intervention, and
>> one day without even kernel intervention.
> eventfd without kernel intervention sounds unlikely.
>
> kvm might configure the cpu such that an interrupt will not trigger a
> vmexit.  eventfd seems like an unlikely interface to do that: with the
> eventfd, device triggering it has no info about the interrupt so it
> can't send it to the correct VM.

https://lwn.net/Articles/650863/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1239361 — Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support

On Sun,  4 Oct 2015 23:43:17 +0300
Vlad Zolotarov <vladz@cloudius-systems.com> wrote:

> +static int setup_maps(struct pci_dev *pdev, struct uio_info *info)
> +{
> +	int i, m = 0, p = 0, err;
> +	static const char * const bar_names[] = {
> +		"BAR0",	"BAR1",	"BAR2",	"BAR3",	"BAR4",	"BAR5",
> +	};
> +
> +	for (i = 0; i < ARRAY_SIZE(bar_names); i++) {
> +		unsigned long start = pci_resource_start(pdev, i);
> +		unsigned long flags = pci_resource_flags(pdev, i);
> +		unsigned long len = pci_resource_len(pdev, i);
> +
> +		if (start == 0 || len == 0)
> +			continue;
> +
> +		if (flags & IORESOURCE_MEM) {
> +			void __iomem *addr;
> +
> +			if (m >= MAX_UIO_MAPS)
> +				continue;
> +
> +			addr = ioremap(start, len);
> +			if (addr == NULL) {
> +				err = -EINVAL;
> +				goto fail;
> +			}
> +
> +			info->mem[m].name = bar_names[i];
> +			info->mem[m].addr = start;
> +			info->mem[m].internal_addr = addr;
> +			info->mem[m].size = len;
> +			info->mem[m].memtype = UIO_MEM_PHYS;
> +			++m;
> +		} else if (flags & IORESOURCE_IO) {
> +			if (p >= MAX_UIO_PORT_REGIONS)
> +				continue;
> +
> +			info->port[p].name = bar_names[i];
> +			info->port[p].start = start;
> +			info->port[p].size = len;
> +			info->port[p].porttype = UIO_PORT_X86;
> +			++p;
> +		}
> +	}
> +
> +	return 0;
> +fail:
> +	for (i = 0; i < m; i++) {
> +		iounmap(info->mem[i].internal_addr);
> +		info->mem[i].internal_addr = NULL;
> +	}
> +
> +	return err;
> +

I wonder do we really have to setup all the BAR's in uio_pci_generic?
The DPDK code works with uio_pci_generic already, and it didn't setup the BAR's.
One possible issue is that without that maybe kernel would not know about the
region used for MSI-X vectors table.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1239367 — Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support

On 10/05/15 11:41, Stephen Hemminger wrote:
> On Sun,  4 Oct 2015 23:43:17 +0300
> Vlad Zolotarov <vladz@cloudius-systems.com> wrote:
>
>> +static int setup_maps(struct pci_dev *pdev, struct uio_info *info)
>> +{
>> +	int i, m = 0, p = 0, err;
>> +	static const char * const bar_names[] = {
>> +		"BAR0",	"BAR1",	"BAR2",	"BAR3",	"BAR4",	"BAR5",
>> +	};
>> +
>> +	for (i = 0; i < ARRAY_SIZE(bar_names); i++) {
>> +		unsigned long start = pci_resource_start(pdev, i);
>> +		unsigned long flags = pci_resource_flags(pdev, i);
>> +		unsigned long len = pci_resource_len(pdev, i);
>> +
>> +		if (start == 0 || len == 0)
>> +			continue;
>> +
>> +		if (flags & IORESOURCE_MEM) {
>> +			void __iomem *addr;
>> +
>> +			if (m >= MAX_UIO_MAPS)
>> +				continue;
>> +
>> +			addr = ioremap(start, len);
>> +			if (addr == NULL) {
>> +				err = -EINVAL;
>> +				goto fail;
>> +			}
>> +
>> +			info->mem[m].name = bar_names[i];
>> +			info->mem[m].addr = start;
>> +			info->mem[m].internal_addr = addr;
>> +			info->mem[m].size = len;
>> +			info->mem[m].memtype = UIO_MEM_PHYS;
>> +			++m;
>> +		} else if (flags & IORESOURCE_IO) {
>> +			if (p >= MAX_UIO_PORT_REGIONS)
>> +				continue;
>> +
>> +			info->port[p].name = bar_names[i];
>> +			info->port[p].start = start;
>> +			info->port[p].size = len;
>> +			info->port[p].porttype = UIO_PORT_X86;
>> +			++p;
>> +		}
>> +	}
>> +
>> +	return 0;
>> +fail:
>> +	for (i = 0; i < m; i++) {
>> +		iounmap(info->mem[i].internal_addr);
>> +		info->mem[i].internal_addr = NULL;
>> +	}
>> +
>> +	return err;
>> +
> I wonder do we really have to setup all the BAR's in uio_pci_generic?
> The DPDK code works with uio_pci_generic already, and it didn't setup the BAR's.

DPDK never used uio_pci_generic with MSI-X support so far and all MSI-X 
capable UIO DPDK drivers like igb_uio and your newly proposed uio_msi do 
map them all.
U also mentioned in the other thread that virtio requires portio bars too.
The thing is that generally bars are needed for programming the device 
therefore it's logical to have them exposed. In general different 
devices have different registers layout therefore a general driver may 
not know which bar exactly is going to be required for a specific device 
and for a specific usage. That's why I think that "map them all" 
approach is rather generic and appropriate. However if there are other 
motives not to do so that I'm missing, pls., let me know.

thanks,
vlad

> One possible issue is that without that maybe kernel would not know about the
> region used for MSI-X vectors table.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1239402 — Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support

On 10/05/15 12:08, Vlad Zolotarov wrote:
>
>
> On 10/05/15 11:41, Stephen Hemminger wrote:
>> On Sun,  4 Oct 2015 23:43:17 +0300
>> Vlad Zolotarov <vladz@cloudius-systems.com> wrote:
>>
>>> +static int setup_maps(struct pci_dev *pdev, struct uio_info *info)
>>> +{
>>> +    int i, m = 0, p = 0, err;
>>> +    static const char * const bar_names[] = {
>>> +        "BAR0",    "BAR1",    "BAR2",    "BAR3", "BAR4",    "BAR5",
>>> +    };
>>> +
>>> +    for (i = 0; i < ARRAY_SIZE(bar_names); i++) {
>>> +        unsigned long start = pci_resource_start(pdev, i);
>>> +        unsigned long flags = pci_resource_flags(pdev, i);
>>> +        unsigned long len = pci_resource_len(pdev, i);
>>> +
>>> +        if (start == 0 || len == 0)
>>> +            continue;
>>> +
>>> +        if (flags & IORESOURCE_MEM) {
>>> +            void __iomem *addr;
>>> +
>>> +            if (m >= MAX_UIO_MAPS)
>>> +                continue;
>>> +
>>> +            addr = ioremap(start, len);
>>> +            if (addr == NULL) {
>>> +                err = -EINVAL;
>>> +                goto fail;
>>> +            }
>>> +
>>> +            info->mem[m].name = bar_names[i];
>>> +            info->mem[m].addr = start;
>>> +            info->mem[m].internal_addr = addr;
>>> +            info->mem[m].size = len;
>>> +            info->mem[m].memtype = UIO_MEM_PHYS;
>>> +            ++m;
>>> +        } else if (flags & IORESOURCE_IO) {
>>> +            if (p >= MAX_UIO_PORT_REGIONS)
>>> +                continue;
>>> +
>>> +            info->port[p].name = bar_names[i];
>>> +            info->port[p].start = start;
>>> +            info->port[p].size = len;
>>> +            info->port[p].porttype = UIO_PORT_X86;
>>> +            ++p;
>>> +        }
>>> +    }
>>> +
>>> +    return 0;
>>> +fail:
>>> +    for (i = 0; i < m; i++) {
>>> +        iounmap(info->mem[i].internal_addr);
>>> +        info->mem[i].internal_addr = NULL;
>>> +    }
>>> +
>>> +    return err;
>>> +
>> I wonder do we really have to setup all the BAR's in uio_pci_generic?
>> The DPDK code works with uio_pci_generic already, and it didn't setup 
>> the BAR's.
>
> DPDK never used uio_pci_generic with MSI-X support so far and all 
> MSI-X capable UIO DPDK drivers like igb_uio and your newly proposed 
> uio_msi do map them all.
> U also mentioned in the other thread that virtio requires portio bars 
> too.
> The thing is that generally bars are needed for programming the device 
> therefore it's logical to have them exposed. In general different 
> devices have different registers layout therefore a general driver may 
> not know which bar exactly is going to be required for a specific 
> device and for a specific usage. That's why I think that "map them 
> all" approach is rather generic and appropriate. However if there are 
> other motives not to do so that I'm missing, pls., let me know.

Having said all that however I'd agree if someone would say that 
mappings setting would rather come as a separate patch in this series... ;)
it will in v4...


>
> thanks,
> vlad
>
>> One possible issue is that without that maybe kernel would not know 
>> about the
>> region used for MSI-X vectors table.
>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1239929 — Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support

On Mon, Oct 05, 2015 at 01:06:09PM +0300, Vlad Zolotarov wrote:
> Having said all that however I'd agree if someone would say that mappings
> setting would rather come as a separate patch in this series... ;)
> it will in v4...

Just drop this is my advice. There are enough controversial things here
as it is.

-- 
MST
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1239379 — Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support

On 10/05/15 11:41, Stephen Hemminger wrote:
> On Sun,  4 Oct 2015 23:43:17 +0300
> Vlad Zolotarov <vladz@cloudius-systems.com> wrote:
>
>> +static int setup_maps(struct pci_dev *pdev, struct uio_info *info)
>> +{
>> +	int i, m = 0, p = 0, err;
>> +	static const char * const bar_names[] = {
>> +		"BAR0",	"BAR1",	"BAR2",	"BAR3",	"BAR4",	"BAR5",
>> +	};
>> +
>> +	for (i = 0; i < ARRAY_SIZE(bar_names); i++) {
>> +		unsigned long start = pci_resource_start(pdev, i);
>> +		unsigned long flags = pci_resource_flags(pdev, i);
>> +		unsigned long len = pci_resource_len(pdev, i);
>> +
>> +		if (start == 0 || len == 0)
>> +			continue;
>> +
>> +		if (flags & IORESOURCE_MEM) {
>> +			void __iomem *addr;
>> +
>> +			if (m >= MAX_UIO_MAPS)
>> +				continue;
>> +
>> +			addr = ioremap(start, len);
>> +			if (addr == NULL) {
>> +				err = -EINVAL;
>> +				goto fail;
>> +			}
>> +
>> +			info->mem[m].name = bar_names[i];
>> +			info->mem[m].addr = start;
>> +			info->mem[m].internal_addr = addr;
>> +			info->mem[m].size = len;
>> +			info->mem[m].memtype = UIO_MEM_PHYS;
>> +			++m;
>> +		} else if (flags & IORESOURCE_IO) {
>> +			if (p >= MAX_UIO_PORT_REGIONS)
>> +				continue;
>> +
>> +			info->port[p].name = bar_names[i];
>> +			info->port[p].start = start;
>> +			info->port[p].size = len;
>> +			info->port[p].porttype = UIO_PORT_X86;
>> +			++p;
>> +		}
>> +	}
>> +
>> +	return 0;
>> +fail:
>> +	for (i = 0; i < m; i++) {
>> +		iounmap(info->mem[i].internal_addr);
>> +		info->mem[i].internal_addr = NULL;
>> +	}
>> +
>> +	return err;
>> +
> I wonder do we really have to setup all the BAR's in uio_pci_generic?
> The DPDK code works with uio_pci_generic already, and it didn't setup the BAR's.
> One possible issue is that without that maybe kernel would not know about the
> region used for MSI-X vectors table.

So, what's your point? It sounds like u are for setting the mappings in 
the uio_pci_generic after all, aren't u? ;)


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1239858 — Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support

On Sun, Oct 04, 2015 at 11:43:17PM +0300, Vlad Zolotarov wrote:
> Add support for MSI and MSI-X interrupt modes:
>    - Interrupt mode selection order is:
>         INT#X (for backward compatibility) -> MSI-X -> MSI.
>    - Add ioctl() commands:
>       - UIO_PCI_GENERIC_INT_MODE_GET: query the current interrupt mode.
>       - UIO_PCI_GENERIC_IRQ_NUM_GET: query the maximum number of IRQs.

This might be something that humans might want to
read or configure, as well.
In fact, # of interrupts is a limited resource
so it seems very reasonable to have a system
admin configure it rather than the application.
So why not use sysfs attributes for this?

>       - UIO_PCI_GENERIC_IRQ_SET: bind the IRQ to eventfd (similar to vfio).

I think that as a first step, you should just use the regular
uio sysfs interface to report interrupts.
Adding eventfd support to uio sounds like a reasonable
extension but I don't see why it needs to be done
as part of the same patch.

>    - Add mappings to all bars (memory and portio):

I don't think it's a good idea.
People already can poke at BARs through device sysfs
(and already abuse this interface).
We don't want to add another API to do exactly the same thing.
In any case, this should be a separate patch if necessary.

> 	some devices have
>      registers related to MSI/MSI-X handling outside BAR0.

This is very vague. Definitely not enough to justify a new
kernel/userspace API.


> 
> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
> ---
> New in v3:
>    - Add __iomem qualifier to temp buffer receiving ioremap value.
> 
> New in v2:
>    - Added #include <linux/uaccess.h> to uio_pci_generic.c
> 
> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
> ---
>  drivers/uio/uio_pci_generic.c   | 410 +++++++++++++++++++++++++++++++++++++---
>  include/linux/uio_pci_generic.h |  36 ++++
>  2 files changed, 423 insertions(+), 23 deletions(-)
>  create mode 100644 include/linux/uio_pci_generic.h
> 
> diff --git a/drivers/uio/uio_pci_generic.c b/drivers/uio/uio_pci_generic.c
> index d0b508b..6b8b1789 100644
> --- a/drivers/uio/uio_pci_generic.c
> +++ b/drivers/uio/uio_pci_generic.c
> @@ -22,16 +22,32 @@
>  #include <linux/device.h>
>  #include <linux/module.h>
>  #include <linux/pci.h>
> +#include <linux/msi.h>
>  #include <linux/slab.h>
>  #include <linux/uio_driver.h>
> +#include <linux/uio_pci_generic.h>
> +#include <linux/eventfd.h>
> +#include <linux/uaccess.h>
>  
>  #define DRIVER_VERSION	"0.01.0"
>  #define DRIVER_AUTHOR	"Michael S. Tsirkin <mst@redhat.com>"
>  #define DRIVER_DESC	"Generic UIO driver for PCI 2.3 devices"
>  
> +struct msix_info {
> +	int num_irqs;
> +	struct msix_entry *table;
> +	struct uio_msix_irq_ctx {
> +		struct eventfd_ctx *trigger;	/* MSI-x vector to eventfd */
> +		char *name;			/* name in /proc/interrupts */
> +	} *ctx;
> +};
> +
>  struct uio_pci_generic_dev {
>  	struct uio_info info;
>  	struct pci_dev *pdev;
> +	struct mutex msix_state_lock;		/* ioctl mutex */
> +	enum uio_int_mode int_mode;
> +	struct msix_info msix;
>  };
>  
>  static inline struct uio_pci_generic_dev *
> @@ -40,9 +56,177 @@ to_uio_pci_generic_dev(struct uio_info *info)
>  	return container_of(info, struct uio_pci_generic_dev, info);
>  }
>  
> -/* Interrupt handler. Read/modify/write the command register to disable
> - * the interrupt. */
> -static irqreturn_t irqhandler(int irq, struct uio_info *info)
> +/* Unmap previously ioremap'd resources */
> +static void release_iomaps(struct uio_pci_generic_dev *gdev)
> +{
> +	int i;
> +	struct uio_mem *mem = gdev->info.mem;
> +
> +	for (i = 0; i < MAX_UIO_MAPS; i++, mem++) {
> +		if (mem->internal_addr) {
> +			iounmap(mem->internal_addr);
> +			mem->internal_addr = NULL;
> +		}
> +	}
> +}
> +
> +static int setup_maps(struct pci_dev *pdev, struct uio_info *info)
> +{
> +	int i, m = 0, p = 0, err;
> +	static const char * const bar_names[] = {
> +		"BAR0",	"BAR1",	"BAR2",	"BAR3",	"BAR4",	"BAR5",
> +	};
> +
> +	for (i = 0; i < ARRAY_SIZE(bar_names); i++) {
> +		unsigned long start = pci_resource_start(pdev, i);
> +		unsigned long flags = pci_resource_flags(pdev, i);
> +		unsigned long len = pci_resource_len(pdev, i);
> +
> +		if (start == 0 || len == 0)
> +			continue;
> +
> +		if (flags & IORESOURCE_MEM) {
> +			void __iomem *addr;
> +
> +			if (m >= MAX_UIO_MAPS)
> +				continue;
> +
> +			addr = ioremap(start, len);
> +			if (addr == NULL) {
> +				err = -EINVAL;
> +				goto fail;
> +			}
> +
> +			info->mem[m].name = bar_names[i];
> +			info->mem[m].addr = start;
> +			info->mem[m].internal_addr = addr;
> +			info->mem[m].size = len;
> +			info->mem[m].memtype = UIO_MEM_PHYS;
> +			++m;
> +		} else if (flags & IORESOURCE_IO) {
> +			if (p >= MAX_UIO_PORT_REGIONS)
> +				continue;
> +
> +			info->port[p].name = bar_names[i];
> +			info->port[p].start = start;
> +			info->port[p].size = len;
> +			info->port[p].porttype = UIO_PORT_X86;
> +			++p;
> +		}
> +	}
> +
> +	return 0;
> +fail:
> +	for (i = 0; i < m; i++) {
> +		iounmap(info->mem[i].internal_addr);
> +		info->mem[i].internal_addr = NULL;
> +	}
> +
> +	return err;
> +}
> +
> +static irqreturn_t msix_irqhandler(int irq, void *arg);
> +
> +/* set the mapping between vector # and existing eventfd. */
> +static int set_irq_eventfd(struct uio_pci_generic_dev *gdev, int vec, int fd)
> +{
> +	struct uio_msix_irq_ctx *ctx;
> +	struct eventfd_ctx *trigger;
> +	struct pci_dev *pdev = gdev->pdev;
> +	int irq, err;
> +
> +	if (vec >= gdev->msix.num_irqs) {
> +		dev_notice(&gdev->pdev->dev, "vec %u >= num_vec %u\n",
> +			   vec, gdev->msix.num_irqs);
> +		return -ERANGE;
> +	}
> +
> +	irq = gdev->msix.table[vec].vector;
> +
> +	/* Cleanup existing irq mapping */
> +	ctx = &gdev->msix.ctx[vec];
> +	if (ctx->trigger) {
> +		free_irq(irq, ctx->trigger);
> +		eventfd_ctx_put(ctx->trigger);
> +		ctx->trigger = NULL;
> +	}
> +
> +	/* Passing -1 is used to disable interrupt */
> +	if (fd < 0)
> +		return 0;
> +
> +
> +	trigger = eventfd_ctx_fdget(fd);
> +	if (IS_ERR(trigger)) {
> +		err = PTR_ERR(trigger);
> +		dev_notice(&gdev->pdev->dev,
> +			   "eventfd ctx get failed: %d\n", err);
> +		return err;
> +	}
> +
> +	err = request_irq(irq, msix_irqhandler, 0, ctx->name, trigger);
> +	if (err) {
> +		dev_notice(&pdev->dev, "request irq failed: %d\n", err);
> +		eventfd_ctx_put(trigger);
> +		return err;
> +	}
> +
> +	dev_dbg(&pdev->dev, "map vector %u to fd %d trigger %p\n",
> +		vec, fd, trigger);
> +	ctx->trigger = trigger;
> +
> +	return 0;
> +}
> +
> +static int uio_pci_generic_ioctl(struct uio_info *info, unsigned int cmd,
> +				 unsigned long arg)
> +{
> +	struct uio_pci_generic_dev *gdev = to_uio_pci_generic_dev(info);
> +	struct uio_pci_generic_irq_set hdr;
> +	int err;
> +
> +	switch (cmd) {
> +	case UIO_PCI_GENERIC_IRQ_SET:
> +		if (copy_from_user(&hdr, (void __user *)arg, sizeof(hdr)))
> +			return -EFAULT;
> +
> +		/* Locking is needed to ensure two things:
> +		 *  1) Two IRQ_SET ioctl()'s are not running in parallel.
> +		 *  2) IRQ_SET ioctl() is not running in parallel with remove().
> +		 */
> +		mutex_lock(&gdev->msix_state_lock);
> +		if (gdev->int_mode != UIO_INT_MODE_MSIX) {
> +			mutex_unlock(&gdev->msix_state_lock);
> +			return -EOPNOTSUPP;
> +		}
> +
> +		err = set_irq_eventfd(gdev, hdr.vec, hdr.fd);
> +		mutex_unlock(&gdev->msix_state_lock);
> +
> +		break;
> +	case UIO_PCI_GENERIC_IRQ_NUM_GET:
> +		if (gdev->int_mode == UIO_INT_MODE_NONE)
> +			err = put_user(0, (u32 __user *)arg);
> +		else if (gdev->int_mode != UIO_INT_MODE_MSIX)
> +			err = put_user(1, (u32 __user *)arg);
> +		else
> +			err = put_user(gdev->msix.num_irqs,
> +				       (u32 __user *)arg);
> +
> +		break;
> +	case UIO_PCI_GENERIC_INT_MODE_GET:
> +		err = put_user(gdev->int_mode, (u32 __user *)arg);
> +
> +		break;
> +	default:
> +		err = -EOPNOTSUPP;
> +	}
> +
> +	return err;
> +}
> +
> +/* INT#X interrupt handler. */
> +static irqreturn_t intx_irqhandler(int irq, struct uio_info *info)
>  {
>  	struct uio_pci_generic_dev *gdev = to_uio_pci_generic_dev(info);
>  
> @@ -53,8 +237,162 @@ static irqreturn_t irqhandler(int irq, struct uio_info *info)
>  	return IRQ_HANDLED;
>  }
>  
> -static int probe(struct pci_dev *pdev,
> -			   const struct pci_device_id *id)
> +/* MSI interrupt handler. */
> +static irqreturn_t msi_irqhandler(int irq, struct uio_info *info)
> +{
> +	/* UIO core will signal the user process. */
> +	return IRQ_HANDLED;
> +}
> +
> +/* MSI-X interrupt handler. */
> +static irqreturn_t msix_irqhandler(int irq, void *arg)
> +{
> +	struct eventfd_ctx *trigger = arg;
> +
> +	pr_devel("irq %u trigger %p\n", irq, trigger);
> +
> +	eventfd_signal(trigger, 1);
> +	return IRQ_HANDLED;
> +}
> +
> +static bool enable_intx(struct uio_pci_generic_dev *gdev)
> +{
> +	struct pci_dev *pdev = gdev->pdev;
> +
> +	if (!pdev->irq || !pci_intx_mask_supported(pdev))
> +		return false;
> +
> +	gdev->int_mode = UIO_INT_MODE_INTX;
> +	gdev->info.irq = pdev->irq;
> +	gdev->info.irq_flags = IRQF_SHARED;
> +	gdev->info.handler = intx_irqhandler;
> +
> +	return true;
> +}
> +
> +static void set_pci_master(struct pci_dev *pdev)
> +{
> +	pci_set_master(pdev);
> +	dev_warn(&pdev->dev, "Enabling PCI bus mastering. Bogus userspace application is able to trash kernel memory using DMA");
> +	add_taint(TAINT_USER, LOCKDEP_STILL_OK);

Basically, users won't notice this until their kernel
crashes, and then it's too late.

Further, this happens silently: it's not an admin doing
something insecure intentionally.
It's the driver doing something insecure.

This doesn't make sense.

IIUC, the issue is that userspace can reprogram MSI/MSI-X
configuration, causing it to write into kernel memory.

So IMHO the right thing to do is to prevent userspace from
poking at MSI/MSI-X configuration when this is enabled.

This includes the MSI/MSI-X capability in config space
and the MSI-X table in the relevant BAR.

For MSI-X it's especially tricky.
Ideally, we'd map a zero page over that region, because
existing applications expect to be able to map the whole
BAR.

This will need some infrastructure work.

> +}
> +
> +static bool enable_msi(struct uio_pci_generic_dev *gdev)
> +{
> +	struct pci_dev *pdev = gdev->pdev;
> +
> +	set_pci_master(pdev);
> +
> +	if (pci_enable_msi(pdev))
> +		return false;
> +
> +	gdev->int_mode = UIO_INT_MODE_MSI;
> +	gdev->info.irq = pdev->irq;
> +	gdev->info.irq_flags = 0;
> +	gdev->info.handler = msi_irqhandler;
> +
> +	return true;
> +}
> +
> +static bool enable_msix(struct uio_pci_generic_dev *gdev)
> +{
> +	struct pci_dev *pdev = gdev->pdev;
> +	int i, vectors = pci_msix_vec_count(pdev);
> +
> +	if (vectors <= 0)
> +		return false;
> +
> +	gdev->msix.table = kcalloc(vectors, sizeof(struct msix_entry),
> +				   GFP_KERNEL);
> +	if (!gdev->msix.table) {
> +		dev_err(&pdev->dev, "Failed to allocate memory for MSI-X table");
> +		return false;
> +	}
> +
> +	gdev->msix.ctx = kcalloc(vectors, sizeof(struct uio_msix_irq_ctx),
> +				 GFP_KERNEL);
> +	if (!gdev->msix.ctx) {
> +		dev_err(&pdev->dev, "Failed to allocate memory for MSI-X contexts");
> +		goto err_ctx_alloc;
> +	}
> +
> +	for (i = 0; i < vectors; i++) {
> +		gdev->msix.table[i].entry = i;
> +		gdev->msix.ctx[i].name = kasprintf(GFP_KERNEL,
> +						   KBUILD_MODNAME "[%d](%s)",
> +						   i, pci_name(pdev));
> +		if (!gdev->msix.ctx[i].name)
> +			goto err_name_alloc;
> +	}
> +
> +	set_pci_master(pdev);
> +
> +	if (pci_enable_msix(pdev, gdev->msix.table, vectors))
> +		goto err_msix_enable;
> +
> +	gdev->int_mode = UIO_INT_MODE_MSIX;
> +	gdev->info.irq = UIO_IRQ_CUSTOM;
> +	gdev->msix.num_irqs = vectors;
> +
> +	return true;
> +
> +err_msix_enable:
> +	pci_clear_master(pdev);
> +err_name_alloc:
> +	for (i = 0; i < vectors; i++)
> +		kfree(gdev->msix.ctx[i].name);
> +
> +	kfree(gdev->msix.ctx);
> +err_ctx_alloc:
> +	kfree(gdev->msix.table);
> +
> +	return false;
> +}
> +
> +/**
> + * Disable interrupts and free related resources.
> + *
> + * @gdev device handle
> + *
> + * This function should be called after the corresponding UIO device has been
> + * unregistered. This will ensure that there are no currently running ioctl()s
> + * and there won't be any new ones until next probe() call.
> + */
> +static void disable_intr(struct uio_pci_generic_dev *gdev)
> +{
> +	struct pci_dev *pdev = gdev->pdev;
> +	int i;
> +
> +	switch (gdev->int_mode) {
> +	case UIO_INT_MODE_MSI:
> +		pci_disable_msi(pdev);
> +		pci_clear_master(pdev);
> +
> +		break;
> +	case UIO_INT_MODE_MSIX:
> +		/* No need for locking here since there shouldn't be any
> +		 * ioctl()s running by now.
> +		 */
> +		for (i = 0; i < gdev->msix.num_irqs; i++) {
> +			if (gdev->msix.ctx[i].trigger)
> +				set_irq_eventfd(gdev, i, -1);
> +
> +			kfree(gdev->msix.ctx[i].name);
> +		}
> +
> +		pci_disable_msix(pdev);
> +		pci_clear_master(pdev);
> +		kfree(gdev->msix.ctx);
> +		kfree(gdev->msix.table);
> +
> +		break;
> +	default:
> +		break;
> +	}
> +}
> +
> +
> +static int probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  {
>  	struct uio_pci_generic_dev *gdev;
>  	int err;
> @@ -66,42 +404,64 @@ static int probe(struct pci_dev *pdev,
>  		return err;
>  	}
>  
> -	if (!pdev->irq) {
> -		dev_warn(&pdev->dev, "No IRQ assigned to device: "
> -			 "no support for interrupts?\n");
> -		pci_disable_device(pdev);
> -		return -ENODEV;
> -	}
> -
> -	if (!pci_intx_mask_supported(pdev)) {
> -		err = -ENODEV;
> -		goto err_verify;
> -	}
> -
>  	gdev = kzalloc(sizeof(struct uio_pci_generic_dev), GFP_KERNEL);
>  	if (!gdev) {
>  		err = -ENOMEM;
>  		goto err_alloc;
>  	}
>  
> +	gdev->pdev = pdev;
>  	gdev->info.name = "uio_pci_generic";
>  	gdev->info.version = DRIVER_VERSION;
> -	gdev->info.irq = pdev->irq;
> -	gdev->info.irq_flags = IRQF_SHARED;
> -	gdev->info.handler = irqhandler;
> -	gdev->pdev = pdev;
> +	gdev->info.ioctl = uio_pci_generic_ioctl;
> +	mutex_init(&gdev->msix_state_lock);
> +
> +	err = pci_request_regions(pdev, "uio_pci_generic");
> +	if (err != 0) {
> +		dev_err(&pdev->dev, "Cannot request regions\n");
> +		goto err_request_regions;
> +	}
> +
> +	/* Enable the corresponding interrupt mode. Try to enable INT#X first
> +	 * for backward compatibility.
> +	 */
> +	if (enable_intx(gdev))
> +		dev_info(&pdev->dev, "Using INT#x mode: IRQ %ld",
> +			 gdev->info.irq);
> +	else if (enable_msix(gdev))
> +		dev_info(&pdev->dev, "Using MSI-X mode: number of IRQs %d",
> +			 gdev->msix.num_irqs);
> +	else if (enable_msi(gdev))
> +		dev_info(&pdev->dev, "Using MSI mode: IRQ %ld", gdev->info.irq);
> +	else {
> +		err = -ENODEV;
> +		goto err_verify;
> +	}
> +
> +	/* remap resources */
> +	err = setup_maps(pdev, &gdev->info);
> +	if (err)
> +		goto err_maps;
>  
>  	err = uio_register_device(&pdev->dev, &gdev->info);
>  	if (err)
>  		goto err_register;
> +
>  	pci_set_drvdata(pdev, gdev);
>  
>  	return 0;
> +
>  err_register:
> +	release_iomaps(gdev);
> +err_maps:
> +	disable_intr(gdev);
> +err_verify:
> +	pci_release_regions(pdev);
> +err_request_regions:
>  	kfree(gdev);
>  err_alloc:
> -err_verify:
>  	pci_disable_device(pdev);
> +
>  	return err;
>  }
>  
> @@ -110,8 +470,12 @@ static void remove(struct pci_dev *pdev)
>  	struct uio_pci_generic_dev *gdev = pci_get_drvdata(pdev);
>  
>  	uio_unregister_device(&gdev->info);
> -	pci_disable_device(pdev);
> +	disable_intr(gdev);
> +	release_iomaps(gdev);
> +	pci_release_regions(pdev);
>  	kfree(gdev);
> +	pci_disable_device(pdev);
> +	pci_set_drvdata(pdev, NULL);
>  }
>  
>  static struct pci_driver uio_pci_driver = {
> diff --git a/include/linux/uio_pci_generic.h b/include/linux/uio_pci_generic.h
> new file mode 100644
> index 0000000..10716fc
> --- /dev/null
> +++ b/include/linux/uio_pci_generic.h
> @@ -0,0 +1,36 @@
> +/*
> + * include/linux/uio_pci_generic.h
> + *
> + * Userspace generic PCI IO driver.
> + *
> + * Licensed under the GPLv2 only.
> + */
> +
> +#ifndef _UIO_PCI_GENERIC_H_
> +#define _UIO_PCI_GENERIC_H_
> +
> +#include <linux/ioctl.h>
> +
> +enum uio_int_mode {
> +	UIO_INT_MODE_NONE,
> +	UIO_INT_MODE_INTX,
> +	UIO_INT_MODE_MSI,
> +	UIO_INT_MODE_MSIX
> +};
> +
> +/* bind the requested IRQ to the given eventfd */
> +struct uio_pci_generic_irq_set {
> +	int vec; /* index of the IRQ to connect to starting from 0 */
> +	int fd;
> +};
> +
> +#define UIO_PCI_GENERIC_BASE		0x86
> +
> +#define UIO_PCI_GENERIC_IRQ_SET		_IOW('I', UIO_PCI_GENERIC_BASE + 1, \
> +		struct uio_pci_generic_irq_set)
> +#define UIO_PCI_GENERIC_IRQ_NUM_GET	_IOW('I', UIO_PCI_GENERIC_BASE + 2, \
> +		uint32_t)
> +#define UIO_PCI_GENERIC_INT_MODE_GET	_IOW('I', UIO_PCI_GENERIC_BASE + 3, \
> +		uint32_t)
> +
> +#endif /* _UIO_PCI_GENERIC_H_ */
> -- 
> 2.1.0
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1239215 — Re: [PATCH v3 0/3] uio: add MSI/MSI-X support to uio_pci_generic driver

This is the same v3 but with the correct email address of Greg. In the 
first iteration the first letter of the email was missing... ;)

On 10/04/15 23:43, Vlad Zolotarov wrote:
> This series add support for MSI and MSI-X interrupts to uio_pci_generic driver.
>   
> Currently uio_pci_generic supports only legacy INT#x interrupts source. However
> there are situations when this is not enough, for instance SR-IOV VF devices that
> simply don't have INT#x capability. For such devices uio_pci_generic will simply
> fail (more specifically probe() will fail).
>   
> When IOMMU is either not available (e.g. Amazon EC2) or not acceptable due to performance
> overhead and thus VFIO is not an option users that develop user-space drivers are left
> without any option but to develop some proprietary UIO drivers (e.g. igb_uio driver in Intel's
> DPDK) just to be able to use UIO infrastructure.
>   
> This series provides a generic solution for this problem while preserving the original behaviour
> for devices for which the original uio_pci_generic had worked before (i.e. INT#x will be used by default).
>
> New in v3:
>     - Add __iomem qualifier to temp buffer receiving ioremap value.
>
> New in v2:
>     - Added #include <linux/uaccess.h> to uio_pci_generic.c
>
> Vlad Zolotarov (3):
>    uio: add ioctl support
>    uio_pci_generic: add MSI/MSI-X support
>    Documentation: update uio-howto
>
>   Documentation/DocBook/uio-howto.tmpl |  29 ++-
>   drivers/uio/uio.c                    |  15 ++
>   drivers/uio/uio_pci_generic.c        | 410 +++++++++++++++++++++++++++++++++--
>   include/linux/uio_driver.h           |   3 +
>   include/linux/uio_pci_generic.h      |  36 +++
>   5 files changed, 467 insertions(+), 26 deletions(-)
>   create mode 100644 include/linux/uio_pci_generic.h
>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1239263 — Re: [PATCH v3 1/3] uio: add ioctl support

On Sun, Oct 04, 2015 at 11:43:16PM +0300, Vlad Zolotarov wrote:
> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
> ---
>  drivers/uio/uio.c          | 15 +++++++++++++++
>  include/linux/uio_driver.h |  3 +++
>  2 files changed, 18 insertions(+)

You add an ioctl yet fail to justify _why_ you need/want that ioctl, and
you don't document it at all?  Come on, you know better than that, no
one can take a patch that has no changelog comments at all like this :(

Also, I _REALLY_ don't want to add any ioctls to the UIO interface, so
you had better have a really compelling argument as to why this is the
_ONLY_ way you can solve this unknown problem by using such a horrid
thing...

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1239323 — Re: [PATCH v3 1/3] uio: add ioctl support

On 10/05/15 06:03, Greg KH wrote:
> On Sun, Oct 04, 2015 at 11:43:16PM +0300, Vlad Zolotarov wrote:
>> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
>> ---
>>   drivers/uio/uio.c          | 15 +++++++++++++++
>>   include/linux/uio_driver.h |  3 +++
>>   2 files changed, 18 insertions(+)
> You add an ioctl yet fail to justify _why_ you need/want that ioctl, and
> you don't document it at all?  Come on, you know better than that, no
> one can take a patch that has no changelog comments at all like this :(

My bad. U are absolutely right here - it was late and I was tired that I 
missed that to someone it may not be so "crystal clear" like it is to 
me... :)
Again, my bad - let me clarify it here and if we agree I'll respin the 
series with all relevant updates including the changelog.

>
> Also, I _REALLY_ don't want to add any ioctls to the UIO interface, so
> you had better have a really compelling argument as to why this is the
> _ONLY_ way you can solve this unknown problem by using such a horrid
> thing...

Pls., note that this doesn't _ADD_ any ioctls directly to UIO driver, 
but only lets the underlying PCI drivers to have them. UIO in this case 
is only a proxy.

The main idea of this series is, as mentioned in PATCH0, to add the MSI 
and MSI-X support for uio_pci_generic driver.
While with MSI the things are quite simple and we may just ride the 
existing infrastructure, with the MSI-X the things get a bit more 
complicated since we may have more than one interrupt vector. Therefore 
we have to decide which interface we want to give to the user.

One option could be to make all existing interrupts trigger the same 
objects in UIO as the current single interrupt does, however this would 
create an awkward, quite not-flexible semantics. For instance a regular 
(kernel) driver has a separate state machine for each interrupt line, 
which sometimes runs on a separate CPU, etc. This way we get to the 
second option - allow indication for each separate interrupt vector. And 
for obvious reasons (mentioned above) we (Stephen has sent a similar 
series on a dpdk-dev list) chose a second approach.

In order not to invent the wheel we mimicked the VFIO approach, which 
allows to bind the pre-allocated  eventfd descriptor to the specific 
interrupt vector using the ioctl().

The interface is simple. The UIO_PCI_GENERIC_IRQ_SET ioctl() data is:

struct uio_pci_generic_irq_set {
	int vec; /* index of the IRQ to connect to starting from 0 */
	int fd;
};


where "vec" is an index of the IRQ starting from 0 and "fd" is an 
eventfd file descriptor a user wants to poll() for in order to get the 
interrupt indications. If "fd" is less than 0, ioctl() will unbind the 
interrupt from the previously bound eventfd descriptor.

This way a user may poll() for any IRQ it wants separately, or epoll() 
for any subset of them, or do whatever he/she wants to do.

That's why we needed the ioctl(). I admit that it may not be the _ONLY_ 
way to achieve the goal described above but again we took VFIO approach 
as a template for a solution and just followed it. If u think there is 
more elegant/robust/better way to do so, pls., share. :)

thanks,
vlad


>
> thanks,
>
> greg k-h


On 10/05/15 06:03, Greg KH wrote:
> On Sun, Oct 04, 2015 at 11:43:16PM +0300, Vlad Zolotarov wrote:
>> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
>> ---
>>   drivers/uio/uio.c          | 15 +++++++++++++++
>>   include/linux/uio_driver.h |  3 +++
>>   2 files changed, 18 insertions(+)
> You add an ioctl yet fail to justify _why_ you need/want that ioctl, and
> you don't document it at all?  Come on, you know better than that, no
> one can take a patch that has no changelog comments at all like this :(
>
> Also, I _REALLY_ don't want to add any ioctls to the UIO interface, so
> you had better have a really compelling argument as to why this is the
> _ONLY_ way you can solve this unknown problem by using such a horrid
> thing...
>
> thanks,
>
> greg k-h

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1239394 — Re: [PATCH v3 1/3] uio: add ioctl support

On Mon, Oct 05, 2015 at 10:33:20AM +0300, Vlad Zolotarov wrote:
> On 10/05/15 06:03, Greg KH wrote:
> >On Sun, Oct 04, 2015 at 11:43:16PM +0300, Vlad Zolotarov wrote:
> >>Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
> >>---
> >>  drivers/uio/uio.c          | 15 +++++++++++++++
> >>  include/linux/uio_driver.h |  3 +++
> >>  2 files changed, 18 insertions(+)
> >You add an ioctl yet fail to justify _why_ you need/want that ioctl, and
> >you don't document it at all?  Come on, you know better than that, no
> >one can take a patch that has no changelog comments at all like this :(
> 
> My bad. U are absolutely right here - it was late and I was tired that I
> missed that to someone it may not be so "crystal clear" like it is to me...
> :)
> Again, my bad - let me clarify it here and if we agree I'll respin the
> series with all relevant updates including the changelog.
> 
> >
> >Also, I _REALLY_ don't want to add any ioctls to the UIO interface, so
> >you had better have a really compelling argument as to why this is the
> >_ONLY_ way you can solve this unknown problem by using such a horrid
> >thing...
> 
> Pls., note that this doesn't _ADD_ any ioctls directly to UIO driver, but
> only lets the underlying PCI drivers to have them. UIO in this case is only
> a proxy.

Exactly, and I don't want to provide an ioctl "proxy" for UIO drivers.
That way lies madness and horrid code, and other nasty things (hint,
each ioctl is a custom syscall, so you are opening up the box for all
sorts of bad things to happen in drivers...)

For example, your ioctl you use here is incorrect, and will fail
horribly on a large majority of systems.  I don't want to open up the
requirements that more people have to know how to "do it right" in order
to use the UIO interface for their drivers, as people will get it wrong
(as this patch series shows...)

> The main idea of this series is, as mentioned in PATCH0, to add the MSI and
> MSI-X support for uio_pci_generic driver.

Yes, I know that, but I don't see anything that shows _how_ to use this
api.  And then there's the issue of why we even need this, why not just
write a whole new driver for this, like the previous driver did (which
also used ioctls, yes, I didn't have the chance to object to that before
everyone else did...)

> While with MSI the things are quite simple and we may just ride the existing
> infrastructure, with the MSI-X the things get a bit more complicated since
> we may have more than one interrupt vector. Therefore we have to decide
> which interface we want to give to the user.
> 
> One option could be to make all existing interrupts trigger the same objects
> in UIO as the current single interrupt does, however this would create an
> awkward, quite not-flexible semantics. For instance a regular (kernel)
> driver has a separate state machine for each interrupt line, which sometimes
> runs on a separate CPU, etc. This way we get to the second option - allow
> indication for each separate interrupt vector. And for obvious reasons
> (mentioned above) we (Stephen has sent a similar series on a dpdk-dev list)
> chose a second approach.
> 
> In order not to invent the wheel we mimicked the VFIO approach, which allows
> to bind the pre-allocated  eventfd descriptor to the specific interrupt
> vector using the ioctl().
> 
> The interface is simple. The UIO_PCI_GENERIC_IRQ_SET ioctl() data is:
> 
> struct uio_pci_generic_irq_set {
> 	int vec; /* index of the IRQ to connect to starting from 0 */
> 	int fd;
> };

And that's broken :(

NEVER use an "int" for an ioctl, it is wrong and will cause horrible
issues on a large number of systems.  That is what the __u16 and friends
variable types are for.  You know better than this :)

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1239434 — Re: [PATCH v3 1/3] uio: add ioctl support

On 10/05/15 11:01, Greg KH wrote:
> On Mon, Oct 05, 2015 at 10:33:20AM +0300, Vlad Zolotarov wrote:
>> On 10/05/15 06:03, Greg KH wrote:
>>> On Sun, Oct 04, 2015 at 11:43:16PM +0300, Vlad Zolotarov wrote:
>>>> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
>>>> ---
>>>>   drivers/uio/uio.c          | 15 +++++++++++++++
>>>>   include/linux/uio_driver.h |  3 +++
>>>>   2 files changed, 18 insertions(+)
>>> You add an ioctl yet fail to justify _why_ you need/want that ioctl, and
>>> you don't document it at all?  Come on, you know better than that, no
>>> one can take a patch that has no changelog comments at all like this :(
>> My bad. U are absolutely right here - it was late and I was tired that I
>> missed that to someone it may not be so "crystal clear" like it is to me...
>> :)
>> Again, my bad - let me clarify it here and if we agree I'll respin the
>> series with all relevant updates including the changelog.
>>
>>> Also, I _REALLY_ don't want to add any ioctls to the UIO interface, so
>>> you had better have a really compelling argument as to why this is the
>>> _ONLY_ way you can solve this unknown problem by using such a horrid
>>> thing...
>> Pls., note that this doesn't _ADD_ any ioctls directly to UIO driver, but
>> only lets the underlying PCI drivers to have them. UIO in this case is only
>> a proxy.
> Exactly, and I don't want to provide an ioctl "proxy" for UIO drivers.
> That way lies madness and horrid code, and other nasty things (hint,
> each ioctl is a custom syscall, so you are opening up the box for all
> sorts of bad things to happen in drivers...)
>
> For example, your ioctl you use here is incorrect, and will fail
> horribly on a large majority of systems.  I don't want to open up the
> requirements that more people have to know how to "do it right" in order
> to use the UIO interface for their drivers, as people will get it wrong
> (as this patch series shows...)

Sometimes there is no other (better) way to get things done. And bugs - 
isn't it what code review is for? ;)
I'll fix the "int" issue.

>
>> The main idea of this series is, as mentioned in PATCH0, to add the MSI and
>> MSI-X support for uio_pci_generic driver.
> Yes, I know that, but I don't see anything that shows _how_ to use this
> api.

I get that, i'll extend PATCH3 of this series with a detailed 
description in v4.

U use it as follows:

 1. Bind the PCI function to uio_pci_generic.
 2. Query for its interrupt mode with UIO_PCI_GENERIC_INT_MODE_GET ioctl.
 3. If interrupt mode is INT#x or MSI - use the current UIO interface
    for polling, namely use the UIO file descriptor.
 4. Else
     1. Query for the number of MSI-X vectors with
        UIO_PCI_GENERIC_IRQ_NUM_GET ioctl.
     2. Allocate the required number of eventfd descriptors using
        eventfd() from sys/eventfd.h.
     3. Bind them to the required IRQs with UIO_PCI_GENERIC_IRQ_SET ioctl.
 5. When done, just unbind the PCI function from the uio_pci_generic.


> And then there's the issue of why we even need this, why not just
> write a whole new driver for this, like the previous driver did (which
> also used ioctls, yes, I didn't have the chance to object to that before
> everyone else did...)

Which "previous driver" do u refer here?
IMHO writing something instead of UIO (not just uio_pci_generic) seems 
like an overkill for solving this issue. Supporting MSI-X interrupts 
seem like a very beneficial feature for uio_pci_generic and it's really 
not _THAT_ complicated API - just look at VFIO for a comparison... ;)
uio_pci_generic is clearly missing this important feature. And creating 
another user space driver infrastructure just to add it seems extremely 
unjustified.

>
>> While with MSI the things are quite simple and we may just ride the existing
>> infrastructure, with the MSI-X the things get a bit more complicated since
>> we may have more than one interrupt vector. Therefore we have to decide
>> which interface we want to give to the user.
>>
>> One option could be to make all existing interrupts trigger the same objects
>> in UIO as the current single interrupt does, however this would create an
>> awkward, quite not-flexible semantics. For instance a regular (kernel)
>> driver has a separate state machine for each interrupt line, which sometimes
>> runs on a separate CPU, etc. This way we get to the second option - allow
>> indication for each separate interrupt vector. And for obvious reasons
>> (mentioned above) we (Stephen has sent a similar series on a dpdk-dev list)
>> chose a second approach.
>>
>> In order not to invent the wheel we mimicked the VFIO approach, which allows
>> to bind the pre-allocated  eventfd descriptor to the specific interrupt
>> vector using the ioctl().
>>
>> The interface is simple. The UIO_PCI_GENERIC_IRQ_SET ioctl() data is:
>>
>> struct uio_pci_generic_irq_set {
>> 	int vec; /* index of the IRQ to connect to starting from 0 */
>> 	int fd;
>> };
> And that's broken :(

Good catch. Thanks. Will fix.
I'm not a big ioctl fan myself but unfortunately I don't see a good 
alternative here. proc? Would it make it cleaner?

>
> NEVER use an "int" for an ioctl, it is wrong and will cause horrible
> issues on a large number of systems.  That is what the __u16 and friends
> variable types are for.  You know better than this :)
>
> greg k-h

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1239916 — Re: [PATCH v3 1/3] uio: add ioctl support

On Mon, Oct 05, 2015 at 01:36:35PM +0300, Vlad Zolotarov wrote:
> >And then there's the issue of why we even need this, why not just
> >write a whole new driver for this, like the previous driver did (which
> >also used ioctls, yes, I didn't have the chance to object to that before
> >everyone else did...)
> 
> Which "previous driver" do u refer here?
> IMHO writing something instead of UIO (not just uio_pci_generic) seems like
> an overkill for solving this issue. Supporting MSI-X interrupts seem like a
> very beneficial feature for uio_pci_generic and it's really not _THAT_
> complicated API - just look at VFIO for a comparison... ;)

Except most things VFIO does is actually there for security.
Which, for a device that can do DMA and isn't even behind an IOMMU,
sounds like a pretty big deal actually.

> uio_pci_generic is clearly missing this important feature. And creating
> another user space driver infrastructure just to add it seems extremely
> unjustified.

uio_pci_generic was always intended to be used with extremely simple
devices which don't do DMA, or where someone else has set up the IOMMU
(like kvm does with CONFIG_KVM_DEVICE_ASSIGNMENT).

We need to be much more careful with MSI if there's no IOMMU.

-- 
MST
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1239998 — Re: [PATCH v3 1/3] uio: add ioctl support

On Tue, Oct 06, 2015 at 12:43:45AM +0300, Vladislav Zolotarov wrote:
> So, like it has already been asked in a different thread I'm going to
> ask a rhetorical question: what adding an MSI and MSI-X interrupts support to
> uio_pci_generic has to do with security?

memory protection is a better term than security.

It's very simple: you enable bus mastering and you ask userspace to map
all device BARs. One of these BARs holds the address to which device
writes to trigger MSI-X interrupt.

This is how MSI-X works, internally: from the point of view of
PCI it's a memory write. It just so happens that the destination
address is in the interrupt controller, that triggers an interrupt.

But a bug in this userspace application can corrupt the MSI-X table,
which in turn can easily corrupt kernel memory, or unrelated processes's
memory.  This is in my opinion unacceptable.

So you need to be very careful
- probably need to reset device before you even enable bus master
- prevent userspace from touching msi config
- prevent userspace from moving BARs since msi-x config is within a BAR
- detect reset and prevent linux from touching device while it's under
  reset

The list goes on and on.

This is pretty much what VFIO spent the last 3 years doing, except VFIO
also can do IOMMU groups.

> What "security threat" does it add
> that u don't already have today?

Yes, userspace can create this today if it tweaks PCI config space to
enable MSI-X, then corrupts the MSI-X table.  It's unfortunate that we
don't yet prevent this, but at least you need two things to go wrong for
this to trigger.

The reason, as I tried to point out, is simply that I didn't think
uio_pci_generic will be used for these configurations.
But there's nothing fundamental here that makes them secure
and that therefore makes your patches secure as well.

Fixing this to make uio_pci_generic write-protect MSI/MSI-X enable
registers sounds kind of reasonable, this shouldn't be too hard.

-- 
MST
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1240258 — Re: [PATCH v3 1/3] uio: add ioctl support

On 10/06/15 01:29, Michael S. Tsirkin wrote:
> On Tue, Oct 06, 2015 at 12:43:45AM +0300, Vladislav Zolotarov wrote:
>> So, like it has already been asked in a different thread I'm going to
>> ask a rhetorical question: what adding an MSI and MSI-X interrupts support to
>> uio_pci_generic has to do with security?
> memory protection is a better term than security.
>
> It's very simple: you enable bus mastering and you ask userspace to map
> all device BARs. One of these BARs holds the address to which device
> writes to trigger MSI-X interrupt.
>
> This is how MSI-X works, internally: from the point of view of
> PCI it's a memory write. It just so happens that the destination
> address is in the interrupt controller, that triggers an interrupt.
>
> But a bug in this userspace application can corrupt the MSI-X table,
> which in turn can easily corrupt kernel memory, or unrelated processes's
> memory.  This is in my opinion unacceptable.
>
> So you need to be very careful
> - probably need to reset device before you even enable bus master
> - prevent userspace from touching msi config
> - prevent userspace from moving BARs since msi-x config is within a BAR
> - detect reset and prevent linux from touching device while it's under
>    reset
>
> The list goes on and on.
>
> This is pretty much what VFIO spent the last 3 years doing, except VFIO
> also can do IOMMU groups.
>
>> What "security threat" does it add
>> that u don't already have today?
> Yes, userspace can create this today if it tweaks PCI config space to
> enable MSI-X, then corrupts the MSI-X table.  It's unfortunate that we
> don't yet prevent this, but at least you need two things to go wrong for
> this to trigger.
>
> The reason, as I tried to point out, is simply that I didn't think
> uio_pci_generic will be used for these configurations.
> But there's nothing fundamental here that makes them secure
> and that therefore makes your patches secure as well.
>
> Fixing this to make uio_pci_generic write-protect MSI/MSI-X enable
> registers sounds kind of reasonable, this shouldn't be too hard.

Sure. But like u've just pointed out yourself - this is a general issue 
and it has nothing to do with the ability to get notifications per 
MSI-X/MSI interrupts, which this series adds (bus mastering may and is 
easily enabled from the user space - look for pci_uio_set_bus_master() 
function in the DPDK).

So, while I absolutely agree with u in regard to the fact that we have a 
security/memory corruption threat in the current in-tree uio_pci_generic 
- the solution u propose should be a matter of a separate patch and is 
obviously orthogonal to this series.

thanks,
vlad

>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1240462 — Re: [PATCH v3 1/3] uio: add ioctl support

On Tue, Oct 06, 2015 at 11:33:56AM +0300, Vlad Zolotarov wrote:
> the solution u propose should be a matter of a separate patch and is
> obviously orthogonal to this series.

Doesn't work this way, sorry. You want a patch enabling MSI merged,
you need to secure the MSI configuration.

And it's going to be a lot of work, duplicating a bunch of code from
VFIO.

-- 
MST
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1240489 — Re: [PATCH v3 1/3] uio: add ioctl support

On Tue, Oct 06, 2015 at 05:19:22PM +0300, Michael S. Tsirkin wrote:
> On Tue, Oct 06, 2015 at 11:33:56AM +0300, Vlad Zolotarov wrote:
> > the solution u propose should be a matter of a separate patch and is
> > obviously orthogonal to this series.
> 
> Doesn't work this way, sorry. You want a patch enabling MSI merged,
> you need to secure the MSI configuration.
> 
MSI can be enabled right now without the patch by writing directly into
PCI bar. The only thing this patch adds is forwarding the interrupt to
an eventfd.
 
--
			Gleb.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1240560 — Re: [PATCH v3 1/3] uio: add ioctl support

On Tue, Oct 06, 2015 at 05:30:31PM +0300, Gleb Natapov wrote:
> On Tue, Oct 06, 2015 at 05:19:22PM +0300, Michael S. Tsirkin wrote:
> > On Tue, Oct 06, 2015 at 11:33:56AM +0300, Vlad Zolotarov wrote:
> > > the solution u propose should be a matter of a separate patch and is
> > > obviously orthogonal to this series.
> > 
> > Doesn't work this way, sorry. You want a patch enabling MSI merged,
> > you need to secure the MSI configuration.
> > 
> MSI can be enabled right now without the patch by writing directly into
> PCI bar.

By poking at config registers in sysfs? We can block this, or we
can log this, pretty easily. We don't ATM but it's not hard to do.

> The only thing this patch adds is forwarding the interrupt to
> an eventfd.

This one just adds a bunch of ioctls. The next ones do
more than you describe.

> --
> 			Gleb.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1240582 — Re: [PATCH v3 1/3] uio: add ioctl support

On 10/06/15 18:19, Michael S. Tsirkin wrote:
> On Tue, Oct 06, 2015 at 05:30:31PM +0300, Gleb Natapov wrote:
>> On Tue, Oct 06, 2015 at 05:19:22PM +0300, Michael S. Tsirkin wrote:
>>> On Tue, Oct 06, 2015 at 11:33:56AM +0300, Vlad Zolotarov wrote:
>>>> the solution u propose should be a matter of a separate patch and is
>>>> obviously orthogonal to this series.
>>> Doesn't work this way, sorry. You want a patch enabling MSI merged,
>>> you need to secure the MSI configuration.
>>>
>> MSI can be enabled right now without the patch by writing directly into
>> PCI bar.
> By poking at config registers in sysfs? We can block this, or we
> can log this, pretty easily. We don't ATM but it's not hard to do.
>
>> The only thing this patch adds is forwarding the interrupt to
>> an eventfd.
> This one just adds a bunch of ioctls. The next ones do
> more than you describe.

This one adds zero ioctls and the next one does exactly what Gleb 
describes.

>
>> --
>> 			Gleb.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

Page 4 of 5 — ← Prev page 1 2 3 [4] 5  Next page →

Back to top | Article view | linux.kernel

csiph-web