[RFC PATCH kernel] vfio-pci: Allow mapping MSIX BAR

[RFC PATCH kernel] vfio-pci: Allow mapping MSIX BAR

[Alexey Kardashevskiy]

By default VFIO disables mapping of MSIX BAR to the userspace as
the userspace may program it in a way allowing spurious interrupts;
instead the userspace uses the VFIO_DEVICE_SET_IRQS ioctl.

This works fine as long as the system page size equals to the MSIX
alignment requirement which is 4KB. However with a bigger page size
the existing code prohibits mapping non-MSIX parts of a page with MSIX
structures so these parts have to be emulated via slow reads/writes on
a VFIO device fd. If these emulated bits are accessed often, this has
serious impact on performance.

This adds an ioctl to the vfio-pci device which hides the sparse
capability and allows the userspace to map a BAR with MSIX structures.

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
 drivers/vfio/pci/vfio_pci_private.h |  1 +
 include/uapi/linux/vfio.h           | 15 +++++++++++++++
 drivers/vfio/pci/vfio_pci.c         | 19 +++++++++++++++++--
 3 files changed, 33 insertions(+), 2 deletions(-)

diff --git a/drivers/vfio/pci/vfio_pci_private.h b/drivers/vfio/pci/vfio_pci_private.h
index f561ac1..058fa9b 100644
--- a/drivers/vfio/pci/vfio_pci_private.h
+++ b/drivers/vfio/pci/vfio_pci_private.h
@@ -90,6 +90,7 @@ struct vfio_pci_device {
 	bool			has_vga;
 	bool			needs_reset;
 	bool			nointx;
+	bool			msix_mmap;
 	struct pci_saved_state	*pci_saved_state;
 	int			refcnt;
 	struct eventfd_ctx	*err_trigger;
diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
index ae46105..60cd4fd 100644
--- a/include/uapi/linux/vfio.h
+++ b/include/uapi/linux/vfio.h
@@ -502,6 +502,21 @@ struct vfio_pci_hot_reset {
 
 #define VFIO_DEVICE_PCI_HOT_RESET	_IO(VFIO_TYPE, VFIO_BASE + 13)
 
+/**
+ * VFIO_DEVICE_PCI_ALLOW_MSIX_MMAP - _IOW(VFIO_TYPE, VFIO_BASE + 14,
+ *				    struct vfio_pci_allow_msix_mmap)
+ *
+ * Return: 0 on success, -errno on failure.
+ */
+#define VFIO_PCI_ALLOW_MSIX_MMAP	(1 << 0) /* mmap of entire MSIX BAR */
+
+struct vfio_pci_allow_msix_mmap {
+	__u32	argsz;
+	__u32	flags;
+};
+
+#define VFIO_DEVICE_PCI_ALLOW_MSIX_MMAP	_IO(VFIO_TYPE, VFIO_BASE + 14)
+
 /* -------- API for Type1 VFIO IOMMU -------- */
 
 /**
diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
index f041b1a..adba5ab 100644
--- a/drivers/vfio/pci/vfio_pci.c
+++ b/drivers/vfio/pci/vfio_pci.c
@@ -691,7 +691,8 @@ static long vfio_pci_ioctl(void *device_data,
 				     VFIO_REGION_INFO_FLAG_WRITE;
 			if (vdev->bar_mmap_supported[info.index]) {
 				info.flags |= VFIO_REGION_INFO_FLAG_MMAP;
-				if (info.index == vdev->msix_bar) {
+				if (info.index == vdev->msix_bar &&
+						!vdev->msix_mmap) {
 					ret = msix_sparse_mmap_cap(vdev, &caps);
 					if (ret)
 						return ret;
@@ -1039,6 +1040,20 @@ static long vfio_pci_ioctl(void *device_data,
 
 		kfree(groups);
 		return ret;
+	} else if (cmd == VFIO_DEVICE_PCI_ALLOW_MSIX_MMAP) {
+		struct vfio_pci_allow_msix_mmap hdr;
+
+		minsz = offsetofend(struct vfio_pci_allow_msix_mmap, flags);
+
+		if (copy_from_user(&hdr, (void __user *)arg, minsz))
+			return -EFAULT;
+
+		if (hdr.argsz < minsz || hdr.flags & ~VFIO_PCI_ALLOW_MSIX_MMAP)
+			return -EINVAL;
+
+		vdev->msix_mmap = !!(hdr.flags & VFIO_PCI_ALLOW_MSIX_MMAP);
+
+		return 0;
 	}
 
 	return -ENOTTY;
@@ -1122,7 +1137,7 @@ static int vfio_pci_mmap(void *device_data, struct vm_area_struct *vma)
 	if (req_start + req_len > phys_len)
 		return -EINVAL;
 
-	if (index == vdev->msix_bar) {
+	if (!vdev->msix_mmap && index == vdev->msix_bar) {
 		/*
 		 * Disallow mmaps overlapping the MSI-X table; users don't
 		 * get to touch this directly.  We could find somewhere
-- 
2.11.0

Re: [RFC PATCH kernel] vfio-pci: Allow mapping MSIX BAR

[David Gibson]

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

On Wed, Nov 22, 2017 at 03:09:32PM +1100, Alexey Kardashevskiy wrote:
> By default VFIO disables mapping of MSIX BAR to the userspace as
> the userspace may program it in a way allowing spurious interrupts;
> instead the userspace uses the VFIO_DEVICE_SET_IRQS ioctl.
> 
> This works fine as long as the system page size equals to the MSIX
> alignment requirement which is 4KB. However with a bigger page size
> the existing code prohibits mapping non-MSIX parts of a page with MSIX
> structures so these parts have to be emulated via slow reads/writes on
> a VFIO device fd. If these emulated bits are accessed often, this has
> serious impact on performance.
> 
> This adds an ioctl to the vfio-pci device which hides the sparse
> capability and allows the userspace to map a BAR with MSIX structures.
> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>

The code looks good to me, but I think we need a comment explaining
what the use case for this is.  On the comments for
VFIO_DEVICE_PCI_ALLOW_MSIX_MMAP seems the logical place to me.

As Alex has said, this only actually works if your intended use case
doesn't (directly) access the MSI-X information via the BAR.  pseries
guests have that because of the paravirtualization.

[Aside: as a later extension, would it be useful to provide an ioctl()
interface to do the MSI-X setup.  Userspace VFIO programs could elect
to use that and then also make this call, which could be good for
performance if they have highly-used registers in the same page as the
MSI-X table]


> ---
>  drivers/vfio/pci/vfio_pci_private.h |  1 +
>  include/uapi/linux/vfio.h           | 15 +++++++++++++++
>  drivers/vfio/pci/vfio_pci.c         | 19 +++++++++++++++++--
>  3 files changed, 33 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/vfio/pci/vfio_pci_private.h b/drivers/vfio/pci/vfio_pci_private.h
> index f561ac1..058fa9b 100644
> --- a/drivers/vfio/pci/vfio_pci_private.h
> +++ b/drivers/vfio/pci/vfio_pci_private.h
> @@ -90,6 +90,7 @@ struct vfio_pci_device {
>  	bool			has_vga;
>  	bool			needs_reset;
>  	bool			nointx;
> +	bool			msix_mmap;
>  	struct pci_saved_state	*pci_saved_state;
>  	int			refcnt;
>  	struct eventfd_ctx	*err_trigger;
> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> index ae46105..60cd4fd 100644
> --- a/include/uapi/linux/vfio.h
> +++ b/include/uapi/linux/vfio.h
> @@ -502,6 +502,21 @@ struct vfio_pci_hot_reset {
>  
>  #define VFIO_DEVICE_PCI_HOT_RESET	_IO(VFIO_TYPE, VFIO_BASE + 13)
>  
> +/**
> + * VFIO_DEVICE_PCI_ALLOW_MSIX_MMAP - _IOW(VFIO_TYPE, VFIO_BASE + 14,
> + *				    struct vfio_pci_allow_msix_mmap)
> + *
> + * Return: 0 on success, -errno on failure.
> + */
> +#define VFIO_PCI_ALLOW_MSIX_MMAP	(1 << 0) /* mmap of entire MSIX BAR */
> +
> +struct vfio_pci_allow_msix_mmap {
> +	__u32	argsz;
> +	__u32	flags;
> +};
> +
> +#define VFIO_DEVICE_PCI_ALLOW_MSIX_MMAP	_IO(VFIO_TYPE, VFIO_BASE + 14)
> +
>  /* -------- API for Type1 VFIO IOMMU -------- */
>  
>  /**
> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> index f041b1a..adba5ab 100644
> --- a/drivers/vfio/pci/vfio_pci.c
> +++ b/drivers/vfio/pci/vfio_pci.c
> @@ -691,7 +691,8 @@ static long vfio_pci_ioctl(void *device_data,
>  				     VFIO_REGION_INFO_FLAG_WRITE;
>  			if (vdev->bar_mmap_supported[info.index]) {
>  				info.flags |= VFIO_REGION_INFO_FLAG_MMAP;
> -				if (info.index == vdev->msix_bar) {
> +				if (info.index == vdev->msix_bar &&
> +						!vdev->msix_mmap) {
>  					ret = msix_sparse_mmap_cap(vdev, &caps);
>  					if (ret)
>  						return ret;
> @@ -1039,6 +1040,20 @@ static long vfio_pci_ioctl(void *device_data,
>  
>  		kfree(groups);
>  		return ret;
> +	} else if (cmd == VFIO_DEVICE_PCI_ALLOW_MSIX_MMAP) {
> +		struct vfio_pci_allow_msix_mmap hdr;
> +
> +		minsz = offsetofend(struct vfio_pci_allow_msix_mmap, flags);
> +
> +		if (copy_from_user(&hdr, (void __user *)arg, minsz))
> +			return -EFAULT;
> +
> +		if (hdr.argsz < minsz || hdr.flags & ~VFIO_PCI_ALLOW_MSIX_MMAP)
> +			return -EINVAL;
> +
> +		vdev->msix_mmap = !!(hdr.flags & VFIO_PCI_ALLOW_MSIX_MMAP);
> +
> +		return 0;
>  	}
>  
>  	return -ENOTTY;
> @@ -1122,7 +1137,7 @@ static int vfio_pci_mmap(void *device_data, struct vm_area_struct *vma)
>  	if (req_start + req_len > phys_len)
>  		return -EINVAL;
>  
> -	if (index == vdev->msix_bar) {
> +	if (!vdev->msix_mmap && index == vdev->msix_bar) {
>  		/*
>  		 * Disallow mmaps overlapping the MSI-X table; users don't
>  		 * get to touch this directly.  We could find somewhere

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC PATCH kernel] vfio-pci: Allow mapping MSIX BAR

[Alex Williamson]

On Wed, 22 Nov 2017 15:09:32 +1100
Alexey Kardashevskiy <aik@ozlabs.ru> wrote:

> By default VFIO disables mapping of MSIX BAR to the userspace as
> the userspace may program it in a way allowing spurious interrupts;
> instead the userspace uses the VFIO_DEVICE_SET_IRQS ioctl.
> 
> This works fine as long as the system page size equals to the MSIX
> alignment requirement which is 4KB. However with a bigger page size
> the existing code prohibits mapping non-MSIX parts of a page with MSIX
> structures so these parts have to be emulated via slow reads/writes on
> a VFIO device fd. If these emulated bits are accessed often, this has
> serious impact on performance.
> 
> This adds an ioctl to the vfio-pci device which hides the sparse
> capability and allows the userspace to map a BAR with MSIX structures.

So the user is in control of telling the kernel whether they're allowed
to mmap the msi-x vector table.  That makes absolutely no sense.  If
you're trying to figure out how userspace knows whether to implicitly
avoid mmap'ing the msix region, I think there are far better ways in
the existing region info ioctl.  We could use a flag, or maybe the
existence of a capability chain pointer, or a new capability.  But
absolutely not this.  The kernel needs to decide whether it's going to
let the user do this, not the user.  Thanks,

Alex

> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
>  drivers/vfio/pci/vfio_pci_private.h |  1 +
>  include/uapi/linux/vfio.h           | 15 +++++++++++++++
>  drivers/vfio/pci/vfio_pci.c         | 19 +++++++++++++++++--
>  3 files changed, 33 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/vfio/pci/vfio_pci_private.h b/drivers/vfio/pci/vfio_pci_private.h
> index f561ac1..058fa9b 100644
> --- a/drivers/vfio/pci/vfio_pci_private.h
> +++ b/drivers/vfio/pci/vfio_pci_private.h
> @@ -90,6 +90,7 @@ struct vfio_pci_device {
>  	bool			has_vga;
>  	bool			needs_reset;
>  	bool			nointx;
> +	bool			msix_mmap;
>  	struct pci_saved_state	*pci_saved_state;
>  	int			refcnt;
>  	struct eventfd_ctx	*err_trigger;
> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> index ae46105..60cd4fd 100644
> --- a/include/uapi/linux/vfio.h
> +++ b/include/uapi/linux/vfio.h
> @@ -502,6 +502,21 @@ struct vfio_pci_hot_reset {
>  
>  #define VFIO_DEVICE_PCI_HOT_RESET	_IO(VFIO_TYPE, VFIO_BASE + 13)
>  
> +/**
> + * VFIO_DEVICE_PCI_ALLOW_MSIX_MMAP - _IOW(VFIO_TYPE, VFIO_BASE + 14,
> + *				    struct vfio_pci_allow_msix_mmap)
> + *
> + * Return: 0 on success, -errno on failure.
> + */
> +#define VFIO_PCI_ALLOW_MSIX_MMAP	(1 << 0) /* mmap of entire MSIX BAR */
> +
> +struct vfio_pci_allow_msix_mmap {
> +	__u32	argsz;
> +	__u32	flags;
> +};
> +
> +#define VFIO_DEVICE_PCI_ALLOW_MSIX_MMAP	_IO(VFIO_TYPE, VFIO_BASE + 14)
> +
>  /* -------- API for Type1 VFIO IOMMU -------- */
>  
>  /**
> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> index f041b1a..adba5ab 100644
> --- a/drivers/vfio/pci/vfio_pci.c
> +++ b/drivers/vfio/pci/vfio_pci.c
> @@ -691,7 +691,8 @@ static long vfio_pci_ioctl(void *device_data,
>  				     VFIO_REGION_INFO_FLAG_WRITE;
>  			if (vdev->bar_mmap_supported[info.index]) {
>  				info.flags |= VFIO_REGION_INFO_FLAG_MMAP;
> -				if (info.index == vdev->msix_bar) {
> +				if (info.index == vdev->msix_bar &&
> +						!vdev->msix_mmap) {
>  					ret = msix_sparse_mmap_cap(vdev, &caps);
>  					if (ret)
>  						return ret;
> @@ -1039,6 +1040,20 @@ static long vfio_pci_ioctl(void *device_data,
>  
>  		kfree(groups);
>  		return ret;
> +	} else if (cmd == VFIO_DEVICE_PCI_ALLOW_MSIX_MMAP) {
> +		struct vfio_pci_allow_msix_mmap hdr;
> +
> +		minsz = offsetofend(struct vfio_pci_allow_msix_mmap, flags);
> +
> +		if (copy_from_user(&hdr, (void __user *)arg, minsz))
> +			return -EFAULT;
> +
> +		if (hdr.argsz < minsz || hdr.flags & ~VFIO_PCI_ALLOW_MSIX_MMAP)
> +			return -EINVAL;
> +
> +		vdev->msix_mmap = !!(hdr.flags & VFIO_PCI_ALLOW_MSIX_MMAP);
> +
> +		return 0;
>  	}
>  
>  	return -ENOTTY;
> @@ -1122,7 +1137,7 @@ static int vfio_pci_mmap(void *device_data, struct vm_area_struct *vma)
>  	if (req_start + req_len > phys_len)
>  		return -EINVAL;
>  
> -	if (index == vdev->msix_bar) {
> +	if (!vdev->msix_mmap && index == vdev->msix_bar) {
>  		/*
>  		 * Disallow mmaps overlapping the MSI-X table; users don't
>  		 * get to touch this directly.  We could find somewhere

Re: [RFC PATCH kernel] vfio-pci: Allow mapping MSIX BAR

[David Gibson]

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

On Tue, Nov 21, 2017 at 09:28:46PM -0700, Alex Williamson wrote:
> On Wed, 22 Nov 2017 15:09:32 +1100
> Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> 
> > By default VFIO disables mapping of MSIX BAR to the userspace as
> > the userspace may program it in a way allowing spurious interrupts;
> > instead the userspace uses the VFIO_DEVICE_SET_IRQS ioctl.
> > 
> > This works fine as long as the system page size equals to the MSIX
> > alignment requirement which is 4KB. However with a bigger page size
> > the existing code prohibits mapping non-MSIX parts of a page with MSIX
> > structures so these parts have to be emulated via slow reads/writes on
> > a VFIO device fd. If these emulated bits are accessed often, this has
> > serious impact on performance.
> > 
> > This adds an ioctl to the vfio-pci device which hides the sparse
> > capability and allows the userspace to map a BAR with MSIX structures.
> 
> So the user is in control of telling the kernel whether they're allowed
> to mmap the msi-x vector table.  That makes absolutely no sense.  If
> you're trying to figure out how userspace knows whether to implicitly
> avoid mmap'ing the msix region, I think there are far better ways in
> the existing region info ioctl.  We could use a flag, or maybe the
> existence of a capability chain pointer, or a new capability.  But
> absolutely not this.  The kernel needs to decide whether it's going to
> let the user do this, not the user.  Thanks,

No, it doesn't.  This is actually the approach we discussed in Prague.

Remember that intercepting access to the MSI-X table is not a host
safety / security issue.  It's just that without that we won't wire up
the device's MSI-X vectors properly so they won't work.

Basically the decision here is between

   A) Allow MSI-X configuration via standard PCI mechanisms, at the
      cost of making access slow for any registers sharing a page with
      the MSI-X table.

or

   B) Make access to BAR registers sharing a page with the MSI-X table
      fast, at the cost of requiring some alternative mechanism to
      configure MSI-X vectors.

And that is a tradeoff that it is reasonable for userspace to make.

In the case of KVM guests, the decision depends entirely on the
*guest* platform.  Usually we need (A) because the guest expects to be
able to poke the MSI-X table in the usual way.  However for PAPR
guests, there's an alternative mechanism via an RTAS call, which means
we can use (B).

The host kernel can't make this decision, because it doesn't know the
guest platform (well, KVM might, but VFIO doesn't).

A userspace VFIO program could also elect for (B) if it does care
about performance of access to registers in the same BAR as the MSI-X
table, but doesn't need MSI-X for example.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC PATCH kernel] vfio-pci: Allow mapping MSIX BAR

[Alex Williamson]

On Wed, 22 Nov 2017 15:44:55 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Tue, Nov 21, 2017 at 09:28:46PM -0700, Alex Williamson wrote:
> > On Wed, 22 Nov 2017 15:09:32 +1100
> > Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> >   
> > > By default VFIO disables mapping of MSIX BAR to the userspace as
> > > the userspace may program it in a way allowing spurious interrupts;
> > > instead the userspace uses the VFIO_DEVICE_SET_IRQS ioctl.
> > > 
> > > This works fine as long as the system page size equals to the MSIX
> > > alignment requirement which is 4KB. However with a bigger page size
> > > the existing code prohibits mapping non-MSIX parts of a page with MSIX
> > > structures so these parts have to be emulated via slow reads/writes on
> > > a VFIO device fd. If these emulated bits are accessed often, this has
> > > serious impact on performance.
> > > 
> > > This adds an ioctl to the vfio-pci device which hides the sparse
> > > capability and allows the userspace to map a BAR with MSIX structures.  
> > 
> > So the user is in control of telling the kernel whether they're allowed
> > to mmap the msi-x vector table.  That makes absolutely no sense.  If
> > you're trying to figure out how userspace knows whether to implicitly
> > avoid mmap'ing the msix region, I think there are far better ways in
> > the existing region info ioctl.  We could use a flag, or maybe the
> > existence of a capability chain pointer, or a new capability.  But
> > absolutely not this.  The kernel needs to decide whether it's going to
> > let the user do this, not the user.  Thanks,  
> 
> No, it doesn't.  This is actually the approach we discussed in Prague.
> 
> Remember that intercepting access to the MSI-X table is not a host
> safety / security issue.  It's just that without that we won't wire up
> the device's MSI-X vectors properly so they won't work.
> 
> Basically the decision here is between
> 
>    A) Allow MSI-X configuration via standard PCI mechanisms, at the
>       cost of making access slow for any registers sharing a page with
>       the MSI-X table.
> 
> or
> 
>    B) Make access to BAR registers sharing a page with the MSI-X table
>       fast, at the cost of requiring some alternative mechanism to
>       configure MSI-X vectors.
> 
> And that is a tradeoff that it is reasonable for userspace to make.
> 
> In the case of KVM guests, the decision depends entirely on the
> *guest* platform.  Usually we need (A) because the guest expects to be
> able to poke the MSI-X table in the usual way.  However for PAPR
> guests, there's an alternative mechanism via an RTAS call, which means
> we can use (B).
> 
> The host kernel can't make this decision, because it doesn't know the
> guest platform (well, KVM might, but VFIO doesn't).
> 
> A userspace VFIO program could also elect for (B) if it does care
> about performance of access to registers in the same BAR as the MSI-X
> table, but doesn't need MSI-X for example.

You're asking for an ioctl to allow the kernel to allow the user to
mmap the page, when instead we could just allow the user to mmap the
page and whether the user does that and how they make use of it is up
to them...

I understand that there are different virtualization techniques at play
here, it just doesn't seem relevant.  In the case of (A), the user can
choose not to mmap the page overlapping the vector table even if the
kernel allows it.  The user can also choose to mmap that page, but not
use the portion overlapping the vector table.  QEMU already does this
by overlaying a MemoryRegion for vector table emulation.  We might even
be able to get away with mmaping that page and emulating the vector
table elsewhere, which seems like the only option for a 64k page ARM
system.  For (B), clearly it's just a nuisance that we can't currently
mmap this page, but I still don't see how the user allowing the kernel
to allow the user to mmap that page makes any sense.  I can't even
describe it without it sounding ridiculous.  Thanks,

Alex

Re: [RFC PATCH kernel] vfio-pci: Allow mapping MSIX BAR

[Alexey Kardashevskiy]

On 22/11/17 16:14, Alex Williamson wrote:
> On Wed, 22 Nov 2017 15:44:55 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
>> On Tue, Nov 21, 2017 at 09:28:46PM -0700, Alex Williamson wrote:
>>> On Wed, 22 Nov 2017 15:09:32 +1100
>>> Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
>>>   
>>>> By default VFIO disables mapping of MSIX BAR to the userspace as
>>>> the userspace may program it in a way allowing spurious interrupts;
>>>> instead the userspace uses the VFIO_DEVICE_SET_IRQS ioctl.
>>>>
>>>> This works fine as long as the system page size equals to the MSIX
>>>> alignment requirement which is 4KB. However with a bigger page size
>>>> the existing code prohibits mapping non-MSIX parts of a page with MSIX
>>>> structures so these parts have to be emulated via slow reads/writes on
>>>> a VFIO device fd. If these emulated bits are accessed often, this has
>>>> serious impact on performance.
>>>>
>>>> This adds an ioctl to the vfio-pci device which hides the sparse
>>>> capability and allows the userspace to map a BAR with MSIX structures.  
>>>
>>> So the user is in control of telling the kernel whether they're allowed
>>> to mmap the msi-x vector table.  That makes absolutely no sense.  If
>>> you're trying to figure out how userspace knows whether to implicitly
>>> avoid mmap'ing the msix region, I think there are far better ways in
>>> the existing region info ioctl.  We could use a flag, or maybe the
>>> existence of a capability chain pointer, or a new capability.  But
>>> absolutely not this.  The kernel needs to decide whether it's going to
>>> let the user do this, not the user.  Thanks,  
>>
>> No, it doesn't.  This is actually the approach we discussed in Prague.
>>
>> Remember that intercepting access to the MSI-X table is not a host
>> safety / security issue.  It's just that without that we won't wire up
>> the device's MSI-X vectors properly so they won't work.
>>
>> Basically the decision here is between
>>
>>    A) Allow MSI-X configuration via standard PCI mechanisms, at the
>>       cost of making access slow for any registers sharing a page with
>>       the MSI-X table.
>>
>> or
>>
>>    B) Make access to BAR registers sharing a page with the MSI-X table
>>       fast, at the cost of requiring some alternative mechanism to
>>       configure MSI-X vectors.
>>
>> And that is a tradeoff that it is reasonable for userspace to make.
>>
>> In the case of KVM guests, the decision depends entirely on the
>> *guest* platform.  Usually we need (A) because the guest expects to be
>> able to poke the MSI-X table in the usual way.  However for PAPR
>> guests, there's an alternative mechanism via an RTAS call, which means
>> we can use (B).
>>
>> The host kernel can't make this decision, because it doesn't know the
>> guest platform (well, KVM might, but VFIO doesn't).
>>
>> A userspace VFIO program could also elect for (B) if it does care
>> about performance of access to registers in the same BAR as the MSI-X
>> table, but doesn't need MSI-X for example.
> 
> You're asking for an ioctl to allow the kernel to allow the user to
> mmap the page, when instead we could just allow the user to mmap the
> page and


How do we decide if we allow the user to mmap that? I tried several
approaches, with no clear reaction from the community...



-- 
Alexey

Re: [RFC PATCH kernel] vfio-pci: Allow mapping MSIX BAR

[David Gibson]

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

On Tue, Nov 21, 2017 at 10:14:45PM -0700, Alex Williamson wrote:
> On Wed, 22 Nov 2017 15:44:55 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Tue, Nov 21, 2017 at 09:28:46PM -0700, Alex Williamson wrote:
> > > On Wed, 22 Nov 2017 15:09:32 +1100
> > > Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> > >   
> > > > By default VFIO disables mapping of MSIX BAR to the userspace as
> > > > the userspace may program it in a way allowing spurious interrupts;
> > > > instead the userspace uses the VFIO_DEVICE_SET_IRQS ioctl.
> > > > 
> > > > This works fine as long as the system page size equals to the MSIX
> > > > alignment requirement which is 4KB. However with a bigger page size
> > > > the existing code prohibits mapping non-MSIX parts of a page with MSIX
> > > > structures so these parts have to be emulated via slow reads/writes on
> > > > a VFIO device fd. If these emulated bits are accessed often, this has
> > > > serious impact on performance.
> > > > 
> > > > This adds an ioctl to the vfio-pci device which hides the sparse
> > > > capability and allows the userspace to map a BAR with MSIX structures.  
> > > 
> > > So the user is in control of telling the kernel whether they're allowed
> > > to mmap the msi-x vector table.  That makes absolutely no sense.  If
> > > you're trying to figure out how userspace knows whether to implicitly
> > > avoid mmap'ing the msix region, I think there are far better ways in
> > > the existing region info ioctl.  We could use a flag, or maybe the
> > > existence of a capability chain pointer, or a new capability.  But
> > > absolutely not this.  The kernel needs to decide whether it's going to
> > > let the user do this, not the user.  Thanks,  
> > 
> > No, it doesn't.  This is actually the approach we discussed in Prague.
> > 
> > Remember that intercepting access to the MSI-X table is not a host
> > safety / security issue.  It's just that without that we won't wire up
> > the device's MSI-X vectors properly so they won't work.
> > 
> > Basically the decision here is between
> > 
> >    A) Allow MSI-X configuration via standard PCI mechanisms, at the
> >       cost of making access slow for any registers sharing a page with
> >       the MSI-X table.
> > 
> > or
> > 
> >    B) Make access to BAR registers sharing a page with the MSI-X table
> >       fast, at the cost of requiring some alternative mechanism to
> >       configure MSI-X vectors.
> > 
> > And that is a tradeoff that it is reasonable for userspace to make.
> > 
> > In the case of KVM guests, the decision depends entirely on the
> > *guest* platform.  Usually we need (A) because the guest expects to be
> > able to poke the MSI-X table in the usual way.  However for PAPR
> > guests, there's an alternative mechanism via an RTAS call, which means
> > we can use (B).
> > 
> > The host kernel can't make this decision, because it doesn't know the
> > guest platform (well, KVM might, but VFIO doesn't).
> > 
> > A userspace VFIO program could also elect for (B) if it does care
> > about performance of access to registers in the same BAR as the MSI-X
> > table, but doesn't need MSI-X for example.
> 
> You're asking for an ioctl to allow the kernel to allow the user to
> mmap the page, when instead we could just allow the user to mmap the
> page and whether the user does that and how they make use of it is up
> to them...

Duh.  Sorry.  For some reason I was thinking the magic MSI-X
interception was happening in the host kernel rather than in qemu.

> I understand that there are different virtualization techniques at play
> here, it just doesn't seem relevant.  In the case of (A), the user can
> choose not to mmap the page overlapping the vector table even if the
> kernel allows it.  The user can also choose to mmap that page, but not
> use the portion overlapping the vector table.  QEMU already does this
> by overlaying a MemoryRegion for vector table emulation.  We might even
> be able to get away with mmaping that page and emulating the vector
> table elsewhere, which seems like the only option for a 64k page ARM
> system.  For (B), clearly it's just a nuisance that we can't currently
> mmap this page, but I still don't see how the user allowing the kernel
> to allow the user to mmap that page makes any sense.  I can't even
> describe it without it sounding ridiculous.  Thanks,

Right.  Rethinking..  it seems to me we should just completely remove
the logic from the kernel banning mmap()s overlapping the MSI-X
table.  All it does is poorly attempt to stop the user shooting
themselves in the foot.

Then we just need logic in qemu to avoid doing the overlapping memory
region nonsense on a per-machine basis

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC PATCH kernel] vfio-pci: Allow mapping MSIX BAR

[Alexey Kardashevskiy]

[-- Attachment #1.1: Type: text/plain, Size: 4930 bytes --]

On 22/11/17 17:51, David Gibson wrote:
> On Tue, Nov 21, 2017 at 10:14:45PM -0700, Alex Williamson wrote:
>> On Wed, 22 Nov 2017 15:44:55 +1100
>> David Gibson <david@gibson.dropbear.id.au> wrote:
>>
>>> On Tue, Nov 21, 2017 at 09:28:46PM -0700, Alex Williamson wrote:
>>>> On Wed, 22 Nov 2017 15:09:32 +1100
>>>> Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
>>>>   
>>>>> By default VFIO disables mapping of MSIX BAR to the userspace as
>>>>> the userspace may program it in a way allowing spurious interrupts;
>>>>> instead the userspace uses the VFIO_DEVICE_SET_IRQS ioctl.
>>>>>
>>>>> This works fine as long as the system page size equals to the MSIX
>>>>> alignment requirement which is 4KB. However with a bigger page size
>>>>> the existing code prohibits mapping non-MSIX parts of a page with MSIX
>>>>> structures so these parts have to be emulated via slow reads/writes on
>>>>> a VFIO device fd. If these emulated bits are accessed often, this has
>>>>> serious impact on performance.
>>>>>
>>>>> This adds an ioctl to the vfio-pci device which hides the sparse
>>>>> capability and allows the userspace to map a BAR with MSIX structures.  
>>>>
>>>> So the user is in control of telling the kernel whether they're allowed
>>>> to mmap the msi-x vector table.  That makes absolutely no sense.  If
>>>> you're trying to figure out how userspace knows whether to implicitly
>>>> avoid mmap'ing the msix region, I think there are far better ways in
>>>> the existing region info ioctl.  We could use a flag, or maybe the
>>>> existence of a capability chain pointer, or a new capability.  But
>>>> absolutely not this.  The kernel needs to decide whether it's going to
>>>> let the user do this, not the user.  Thanks,  
>>>
>>> No, it doesn't.  This is actually the approach we discussed in Prague.
>>>
>>> Remember that intercepting access to the MSI-X table is not a host
>>> safety / security issue.  It's just that without that we won't wire up
>>> the device's MSI-X vectors properly so they won't work.
>>>
>>> Basically the decision here is between
>>>
>>>    A) Allow MSI-X configuration via standard PCI mechanisms, at the
>>>       cost of making access slow for any registers sharing a page with
>>>       the MSI-X table.
>>>
>>> or
>>>
>>>    B) Make access to BAR registers sharing a page with the MSI-X table
>>>       fast, at the cost of requiring some alternative mechanism to
>>>       configure MSI-X vectors.
>>>
>>> And that is a tradeoff that it is reasonable for userspace to make.
>>>
>>> In the case of KVM guests, the decision depends entirely on the
>>> *guest* platform.  Usually we need (A) because the guest expects to be
>>> able to poke the MSI-X table in the usual way.  However for PAPR
>>> guests, there's an alternative mechanism via an RTAS call, which means
>>> we can use (B).
>>>
>>> The host kernel can't make this decision, because it doesn't know the
>>> guest platform (well, KVM might, but VFIO doesn't).
>>>
>>> A userspace VFIO program could also elect for (B) if it does care
>>> about performance of access to registers in the same BAR as the MSI-X
>>> table, but doesn't need MSI-X for example.
>>
>> You're asking for an ioctl to allow the kernel to allow the user to
>> mmap the page, when instead we could just allow the user to mmap the
>> page and whether the user does that and how they make use of it is up
>> to them...
> 
> Duh.  Sorry.  For some reason I was thinking the magic MSI-X
> interception was happening in the host kernel rather than in qemu.
> 
>> I understand that there are different virtualization techniques at play
>> here, it just doesn't seem relevant.  In the case of (A), the user can
>> choose not to mmap the page overlapping the vector table even if the
>> kernel allows it.  The user can also choose to mmap that page, but not
>> use the portion overlapping the vector table.  QEMU already does this
>> by overlaying a MemoryRegion for vector table emulation.  We might even
>> be able to get away with mmaping that page and emulating the vector
>> table elsewhere, which seems like the only option for a 64k page ARM
>> system.  For (B), clearly it's just a nuisance that we can't currently
>> mmap this page, but I still don't see how the user allowing the kernel
>> to allow the user to mmap that page makes any sense.  I can't even
>> describe it without it sounding ridiculous.  Thanks,
> 
> Right.  Rethinking..  it seems to me we should just completely remove
> the logic from the kernel banning mmap()s overlapping the MSI-X
> table.  All it does is poorly attempt to stop the user shooting
> themselves in the foot.
> 
> Then we just need logic in qemu to avoid doing the overlapping memory
> region nonsense on a per-machine basis


So is there still any plan or we just ditch the feature? I am confused now.



-- 
Alexey


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

Re: [RFC PATCH kernel] vfio-pci: Allow mapping MSIX BAR

[David Gibson]

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

On Wed, Nov 29, 2017 at 02:25:43PM +1100, Alexey Kardashevskiy wrote:
> On 22/11/17 17:51, David Gibson wrote:
> > On Tue, Nov 21, 2017 at 10:14:45PM -0700, Alex Williamson wrote:
> >> On Wed, 22 Nov 2017 15:44:55 +1100
> >> David Gibson <david@gibson.dropbear.id.au> wrote:
> >>
> >>> On Tue, Nov 21, 2017 at 09:28:46PM -0700, Alex Williamson wrote:
> >>>> On Wed, 22 Nov 2017 15:09:32 +1100
> >>>> Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> >>>>   
> >>>>> By default VFIO disables mapping of MSIX BAR to the userspace as
> >>>>> the userspace may program it in a way allowing spurious interrupts;
> >>>>> instead the userspace uses the VFIO_DEVICE_SET_IRQS ioctl.
> >>>>>
> >>>>> This works fine as long as the system page size equals to the MSIX
> >>>>> alignment requirement which is 4KB. However with a bigger page size
> >>>>> the existing code prohibits mapping non-MSIX parts of a page with MSIX
> >>>>> structures so these parts have to be emulated via slow reads/writes on
> >>>>> a VFIO device fd. If these emulated bits are accessed often, this has
> >>>>> serious impact on performance.
> >>>>>
> >>>>> This adds an ioctl to the vfio-pci device which hides the sparse
> >>>>> capability and allows the userspace to map a BAR with MSIX structures.  
> >>>>
> >>>> So the user is in control of telling the kernel whether they're allowed
> >>>> to mmap the msi-x vector table.  That makes absolutely no sense.  If
> >>>> you're trying to figure out how userspace knows whether to implicitly
> >>>> avoid mmap'ing the msix region, I think there are far better ways in
> >>>> the existing region info ioctl.  We could use a flag, or maybe the
> >>>> existence of a capability chain pointer, or a new capability.  But
> >>>> absolutely not this.  The kernel needs to decide whether it's going to
> >>>> let the user do this, not the user.  Thanks,  
> >>>
> >>> No, it doesn't.  This is actually the approach we discussed in Prague.
> >>>
> >>> Remember that intercepting access to the MSI-X table is not a host
> >>> safety / security issue.  It's just that without that we won't wire up
> >>> the device's MSI-X vectors properly so they won't work.
> >>>
> >>> Basically the decision here is between
> >>>
> >>>    A) Allow MSI-X configuration via standard PCI mechanisms, at the
> >>>       cost of making access slow for any registers sharing a page with
> >>>       the MSI-X table.
> >>>
> >>> or
> >>>
> >>>    B) Make access to BAR registers sharing a page with the MSI-X table
> >>>       fast, at the cost of requiring some alternative mechanism to
> >>>       configure MSI-X vectors.
> >>>
> >>> And that is a tradeoff that it is reasonable for userspace to make.
> >>>
> >>> In the case of KVM guests, the decision depends entirely on the
> >>> *guest* platform.  Usually we need (A) because the guest expects to be
> >>> able to poke the MSI-X table in the usual way.  However for PAPR
> >>> guests, there's an alternative mechanism via an RTAS call, which means
> >>> we can use (B).
> >>>
> >>> The host kernel can't make this decision, because it doesn't know the
> >>> guest platform (well, KVM might, but VFIO doesn't).
> >>>
> >>> A userspace VFIO program could also elect for (B) if it does care
> >>> about performance of access to registers in the same BAR as the MSI-X
> >>> table, but doesn't need MSI-X for example.
> >>
> >> You're asking for an ioctl to allow the kernel to allow the user to
> >> mmap the page, when instead we could just allow the user to mmap the
> >> page and whether the user does that and how they make use of it is up
> >> to them...
> > 
> > Duh.  Sorry.  For some reason I was thinking the magic MSI-X
> > interception was happening in the host kernel rather than in qemu.
> > 
> >> I understand that there are different virtualization techniques at play
> >> here, it just doesn't seem relevant.  In the case of (A), the user can
> >> choose not to mmap the page overlapping the vector table even if the
> >> kernel allows it.  The user can also choose to mmap that page, but not
> >> use the portion overlapping the vector table.  QEMU already does this
> >> by overlaying a MemoryRegion for vector table emulation.  We might even
> >> be able to get away with mmaping that page and emulating the vector
> >> table elsewhere, which seems like the only option for a 64k page ARM
> >> system.  For (B), clearly it's just a nuisance that we can't currently
> >> mmap this page, but I still don't see how the user allowing the kernel
> >> to allow the user to mmap that page makes any sense.  I can't even
> >> describe it without it sounding ridiculous.  Thanks,
> > 
> > Right.  Rethinking..  it seems to me we should just completely remove
> > the logic from the kernel banning mmap()s overlapping the MSI-X
> > table.  All it does is poorly attempt to stop the user shooting
> > themselves in the foot.
> > 
> > Then we just need logic in qemu to avoid doing the overlapping memory
> > region nonsense on a per-machine basis
> 
> 
> So is there still any plan or we just ditch the feature? I am confused now.

The plan is what I said above.  Remove the bogus check logic from the
kernel, then solve within qemu, by not creating the MSI-X intercept
region for pseries guests.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC PATCH kernel] vfio-pci: Allow mapping MSIX BAR

[Alexey Kardashevskiy]

[-- Attachment #1.1: Type: text/plain, Size: 5819 bytes --]

On 29/11/17 14:57, David Gibson wrote:
> On Wed, Nov 29, 2017 at 02:25:43PM +1100, Alexey Kardashevskiy wrote:
>> On 22/11/17 17:51, David Gibson wrote:
>>> On Tue, Nov 21, 2017 at 10:14:45PM -0700, Alex Williamson wrote:
>>>> On Wed, 22 Nov 2017 15:44:55 +1100
>>>> David Gibson <david@gibson.dropbear.id.au> wrote:
>>>>
>>>>> On Tue, Nov 21, 2017 at 09:28:46PM -0700, Alex Williamson wrote:
>>>>>> On Wed, 22 Nov 2017 15:09:32 +1100
>>>>>> Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
>>>>>>   
>>>>>>> By default VFIO disables mapping of MSIX BAR to the userspace as
>>>>>>> the userspace may program it in a way allowing spurious interrupts;
>>>>>>> instead the userspace uses the VFIO_DEVICE_SET_IRQS ioctl.
>>>>>>>
>>>>>>> This works fine as long as the system page size equals to the MSIX
>>>>>>> alignment requirement which is 4KB. However with a bigger page size
>>>>>>> the existing code prohibits mapping non-MSIX parts of a page with MSIX
>>>>>>> structures so these parts have to be emulated via slow reads/writes on
>>>>>>> a VFIO device fd. If these emulated bits are accessed often, this has
>>>>>>> serious impact on performance.
>>>>>>>
>>>>>>> This adds an ioctl to the vfio-pci device which hides the sparse
>>>>>>> capability and allows the userspace to map a BAR with MSIX structures.  
>>>>>>
>>>>>> So the user is in control of telling the kernel whether they're allowed
>>>>>> to mmap the msi-x vector table.  That makes absolutely no sense.  If
>>>>>> you're trying to figure out how userspace knows whether to implicitly
>>>>>> avoid mmap'ing the msix region, I think there are far better ways in
>>>>>> the existing region info ioctl.  We could use a flag, or maybe the
>>>>>> existence of a capability chain pointer, or a new capability.  But
>>>>>> absolutely not this.  The kernel needs to decide whether it's going to
>>>>>> let the user do this, not the user.  Thanks,  
>>>>>
>>>>> No, it doesn't.  This is actually the approach we discussed in Prague.
>>>>>
>>>>> Remember that intercepting access to the MSI-X table is not a host
>>>>> safety / security issue.  It's just that without that we won't wire up
>>>>> the device's MSI-X vectors properly so they won't work.
>>>>>
>>>>> Basically the decision here is between
>>>>>
>>>>>    A) Allow MSI-X configuration via standard PCI mechanisms, at the
>>>>>       cost of making access slow for any registers sharing a page with
>>>>>       the MSI-X table.
>>>>>
>>>>> or
>>>>>
>>>>>    B) Make access to BAR registers sharing a page with the MSI-X table
>>>>>       fast, at the cost of requiring some alternative mechanism to
>>>>>       configure MSI-X vectors.
>>>>>
>>>>> And that is a tradeoff that it is reasonable for userspace to make.
>>>>>
>>>>> In the case of KVM guests, the decision depends entirely on the
>>>>> *guest* platform.  Usually we need (A) because the guest expects to be
>>>>> able to poke the MSI-X table in the usual way.  However for PAPR
>>>>> guests, there's an alternative mechanism via an RTAS call, which means
>>>>> we can use (B).
>>>>>
>>>>> The host kernel can't make this decision, because it doesn't know the
>>>>> guest platform (well, KVM might, but VFIO doesn't).
>>>>>
>>>>> A userspace VFIO program could also elect for (B) if it does care
>>>>> about performance of access to registers in the same BAR as the MSI-X
>>>>> table, but doesn't need MSI-X for example.
>>>>
>>>> You're asking for an ioctl to allow the kernel to allow the user to
>>>> mmap the page, when instead we could just allow the user to mmap the
>>>> page and whether the user does that and how they make use of it is up
>>>> to them...
>>>
>>> Duh.  Sorry.  For some reason I was thinking the magic MSI-X
>>> interception was happening in the host kernel rather than in qemu.
>>>
>>>> I understand that there are different virtualization techniques at play
>>>> here, it just doesn't seem relevant.  In the case of (A), the user can
>>>> choose not to mmap the page overlapping the vector table even if the
>>>> kernel allows it.  The user can also choose to mmap that page, but not
>>>> use the portion overlapping the vector table.  QEMU already does this
>>>> by overlaying a MemoryRegion for vector table emulation.  We might even
>>>> be able to get away with mmaping that page and emulating the vector
>>>> table elsewhere, which seems like the only option for a 64k page ARM
>>>> system.  For (B), clearly it's just a nuisance that we can't currently
>>>> mmap this page, but I still don't see how the user allowing the kernel
>>>> to allow the user to mmap that page makes any sense.  I can't even
>>>> describe it without it sounding ridiculous.  Thanks,
>>>
>>> Right.  Rethinking..  it seems to me we should just completely remove
>>> the logic from the kernel banning mmap()s overlapping the MSI-X
>>> table.  All it does is poorly attempt to stop the user shooting
>>> themselves in the foot.
>>>
>>> Then we just need logic in qemu to avoid doing the overlapping memory
>>> region nonsense on a per-machine basis
>>
>>
>> So is there still any plan or we just ditch the feature? I am confused now.
> 
> The plan is what I said above.  Remove the bogus check logic from the
> kernel, then solve within qemu, by not creating the MSI-X intercept
> region for pseries guests.


There were 2 proposals how to do that. Both included platform code to
decide whether to allow mapping or not  and  some transport to pass that
enablement flag from the plafform code to the VFIO-PCI driver, one was via
an IOMMU group flag, the other via a PCI bus flag. Neither was accepted so
reposting those won't make any progress, what do I miss here? Was there any
agreement on how to do this?



-- 
Alexey


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

Re: [RFC PATCH kernel] vfio-pci: Allow mapping MSIX BAR

[David Gibson]

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

On Wed, Nov 29, 2017 at 03:38:30PM +1100, Alexey Kardashevskiy wrote:
> On 29/11/17 14:57, David Gibson wrote:
> > On Wed, Nov 29, 2017 at 02:25:43PM +1100, Alexey Kardashevskiy wrote:
> >> On 22/11/17 17:51, David Gibson wrote:
> >>> On Tue, Nov 21, 2017 at 10:14:45PM -0700, Alex Williamson wrote:
> >>>> On Wed, 22 Nov 2017 15:44:55 +1100
> >>>> David Gibson <david@gibson.dropbear.id.au> wrote:
> >>>>
> >>>>> On Tue, Nov 21, 2017 at 09:28:46PM -0700, Alex Williamson wrote:
> >>>>>> On Wed, 22 Nov 2017 15:09:32 +1100
> >>>>>> Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> >>>>>>   
> >>>>>>> By default VFIO disables mapping of MSIX BAR to the userspace as
> >>>>>>> the userspace may program it in a way allowing spurious interrupts;
> >>>>>>> instead the userspace uses the VFIO_DEVICE_SET_IRQS ioctl.
> >>>>>>>
> >>>>>>> This works fine as long as the system page size equals to the MSIX
> >>>>>>> alignment requirement which is 4KB. However with a bigger page size
> >>>>>>> the existing code prohibits mapping non-MSIX parts of a page with MSIX
> >>>>>>> structures so these parts have to be emulated via slow reads/writes on
> >>>>>>> a VFIO device fd. If these emulated bits are accessed often, this has
> >>>>>>> serious impact on performance.
> >>>>>>>
> >>>>>>> This adds an ioctl to the vfio-pci device which hides the sparse
> >>>>>>> capability and allows the userspace to map a BAR with MSIX structures.  
> >>>>>>
> >>>>>> So the user is in control of telling the kernel whether they're allowed
> >>>>>> to mmap the msi-x vector table.  That makes absolutely no sense.  If
> >>>>>> you're trying to figure out how userspace knows whether to implicitly
> >>>>>> avoid mmap'ing the msix region, I think there are far better ways in
> >>>>>> the existing region info ioctl.  We could use a flag, or maybe the
> >>>>>> existence of a capability chain pointer, or a new capability.  But
> >>>>>> absolutely not this.  The kernel needs to decide whether it's going to
> >>>>>> let the user do this, not the user.  Thanks,  
> >>>>>
> >>>>> No, it doesn't.  This is actually the approach we discussed in Prague.
> >>>>>
> >>>>> Remember that intercepting access to the MSI-X table is not a host
> >>>>> safety / security issue.  It's just that without that we won't wire up
> >>>>> the device's MSI-X vectors properly so they won't work.
> >>>>>
> >>>>> Basically the decision here is between
> >>>>>
> >>>>>    A) Allow MSI-X configuration via standard PCI mechanisms, at the
> >>>>>       cost of making access slow for any registers sharing a page with
> >>>>>       the MSI-X table.
> >>>>>
> >>>>> or
> >>>>>
> >>>>>    B) Make access to BAR registers sharing a page with the MSI-X table
> >>>>>       fast, at the cost of requiring some alternative mechanism to
> >>>>>       configure MSI-X vectors.
> >>>>>
> >>>>> And that is a tradeoff that it is reasonable for userspace to make.
> >>>>>
> >>>>> In the case of KVM guests, the decision depends entirely on the
> >>>>> *guest* platform.  Usually we need (A) because the guest expects to be
> >>>>> able to poke the MSI-X table in the usual way.  However for PAPR
> >>>>> guests, there's an alternative mechanism via an RTAS call, which means
> >>>>> we can use (B).
> >>>>>
> >>>>> The host kernel can't make this decision, because it doesn't know the
> >>>>> guest platform (well, KVM might, but VFIO doesn't).
> >>>>>
> >>>>> A userspace VFIO program could also elect for (B) if it does care
> >>>>> about performance of access to registers in the same BAR as the MSI-X
> >>>>> table, but doesn't need MSI-X for example.
> >>>>
> >>>> You're asking for an ioctl to allow the kernel to allow the user to
> >>>> mmap the page, when instead we could just allow the user to mmap the
> >>>> page and whether the user does that and how they make use of it is up
> >>>> to them...
> >>>
> >>> Duh.  Sorry.  For some reason I was thinking the magic MSI-X
> >>> interception was happening in the host kernel rather than in qemu.
> >>>
> >>>> I understand that there are different virtualization techniques at play
> >>>> here, it just doesn't seem relevant.  In the case of (A), the user can
> >>>> choose not to mmap the page overlapping the vector table even if the
> >>>> kernel allows it.  The user can also choose to mmap that page, but not
> >>>> use the portion overlapping the vector table.  QEMU already does this
> >>>> by overlaying a MemoryRegion for vector table emulation.  We might even
> >>>> be able to get away with mmaping that page and emulating the vector
> >>>> table elsewhere, which seems like the only option for a 64k page ARM
> >>>> system.  For (B), clearly it's just a nuisance that we can't currently
> >>>> mmap this page, but I still don't see how the user allowing the kernel
> >>>> to allow the user to mmap that page makes any sense.  I can't even
> >>>> describe it without it sounding ridiculous.  Thanks,
> >>>
> >>> Right.  Rethinking..  it seems to me we should just completely remove
> >>> the logic from the kernel banning mmap()s overlapping the MSI-X
> >>> table.  All it does is poorly attempt to stop the user shooting
> >>> themselves in the foot.
> >>>
> >>> Then we just need logic in qemu to avoid doing the overlapping memory
> >>> region nonsense on a per-machine basis
> >>
> >>
> >> So is there still any plan or we just ditch the feature? I am confused now.
> > 
> > The plan is what I said above.  Remove the bogus check logic from the
> > kernel, then solve within qemu, by not creating the MSI-X intercept
> > region for pseries guests.
> 
> 
> There were 2 proposals how to do that. Both included platform code to
> decide whether to allow mapping or not  and  some transport to pass that
> enablement flag from the plafform code to the VFIO-PCI driver, one was via
> an IOMMU group flag, the other via a PCI bus flag. Neither was accepted so
> reposting those won't make any progress, what do I miss here? Was there any
> agreement on how to do this?

As we've discussed the filtering of mmap()able regions in the kernel
is not a security concern.  All it accomplishes is poorly trying to
stop userspace from shooting itself in the foot by directly mapping
and accessing the MSI-X table instead of using the VFIO interfaces to
set up MSI vectors.

We should simply remove - unconditionally - the checks in the kernel
which prevent parts of the BARs from being mapped.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC PATCH kernel] vfio-pci: Allow mapping MSIX BAR

[Alexey Kardashevskiy]

[-- Attachment #1.1: Type: text/plain, Size: 7811 bytes --]

On 29/11/17 16:17, David Gibson wrote:
> On Wed, Nov 29, 2017 at 03:38:30PM +1100, Alexey Kardashevskiy wrote:
>> On 29/11/17 14:57, David Gibson wrote:
>>> On Wed, Nov 29, 2017 at 02:25:43PM +1100, Alexey Kardashevskiy wrote:
>>>> On 22/11/17 17:51, David Gibson wrote:
>>>>> On Tue, Nov 21, 2017 at 10:14:45PM -0700, Alex Williamson wrote:
>>>>>> On Wed, 22 Nov 2017 15:44:55 +1100
>>>>>> David Gibson <david@gibson.dropbear.id.au> wrote:
>>>>>>
>>>>>>> On Tue, Nov 21, 2017 at 09:28:46PM -0700, Alex Williamson wrote:
>>>>>>>> On Wed, 22 Nov 2017 15:09:32 +1100
>>>>>>>> Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
>>>>>>>>   
>>>>>>>>> By default VFIO disables mapping of MSIX BAR to the userspace as
>>>>>>>>> the userspace may program it in a way allowing spurious interrupts;
>>>>>>>>> instead the userspace uses the VFIO_DEVICE_SET_IRQS ioctl.
>>>>>>>>>
>>>>>>>>> This works fine as long as the system page size equals to the MSIX
>>>>>>>>> alignment requirement which is 4KB. However with a bigger page size
>>>>>>>>> the existing code prohibits mapping non-MSIX parts of a page with MSIX
>>>>>>>>> structures so these parts have to be emulated via slow reads/writes on
>>>>>>>>> a VFIO device fd. If these emulated bits are accessed often, this has
>>>>>>>>> serious impact on performance.
>>>>>>>>>
>>>>>>>>> This adds an ioctl to the vfio-pci device which hides the sparse
>>>>>>>>> capability and allows the userspace to map a BAR with MSIX structures.  
>>>>>>>>
>>>>>>>> So the user is in control of telling the kernel whether they're allowed
>>>>>>>> to mmap the msi-x vector table.  That makes absolutely no sense.  If
>>>>>>>> you're trying to figure out how userspace knows whether to implicitly
>>>>>>>> avoid mmap'ing the msix region, I think there are far better ways in
>>>>>>>> the existing region info ioctl.  We could use a flag, or maybe the
>>>>>>>> existence of a capability chain pointer, or a new capability.  But
>>>>>>>> absolutely not this.  The kernel needs to decide whether it's going to
>>>>>>>> let the user do this, not the user.  Thanks,  
>>>>>>>
>>>>>>> No, it doesn't.  This is actually the approach we discussed in Prague.
>>>>>>>
>>>>>>> Remember that intercepting access to the MSI-X table is not a host
>>>>>>> safety / security issue.  It's just that without that we won't wire up
>>>>>>> the device's MSI-X vectors properly so they won't work.
>>>>>>>
>>>>>>> Basically the decision here is between
>>>>>>>
>>>>>>>    A) Allow MSI-X configuration via standard PCI mechanisms, at the
>>>>>>>       cost of making access slow for any registers sharing a page with
>>>>>>>       the MSI-X table.
>>>>>>>
>>>>>>> or
>>>>>>>
>>>>>>>    B) Make access to BAR registers sharing a page with the MSI-X table
>>>>>>>       fast, at the cost of requiring some alternative mechanism to
>>>>>>>       configure MSI-X vectors.
>>>>>>>
>>>>>>> And that is a tradeoff that it is reasonable for userspace to make.
>>>>>>>
>>>>>>> In the case of KVM guests, the decision depends entirely on the
>>>>>>> *guest* platform.  Usually we need (A) because the guest expects to be
>>>>>>> able to poke the MSI-X table in the usual way.  However for PAPR
>>>>>>> guests, there's an alternative mechanism via an RTAS call, which means
>>>>>>> we can use (B).
>>>>>>>
>>>>>>> The host kernel can't make this decision, because it doesn't know the
>>>>>>> guest platform (well, KVM might, but VFIO doesn't).
>>>>>>>
>>>>>>> A userspace VFIO program could also elect for (B) if it does care
>>>>>>> about performance of access to registers in the same BAR as the MSI-X
>>>>>>> table, but doesn't need MSI-X for example.
>>>>>>
>>>>>> You're asking for an ioctl to allow the kernel to allow the user to
>>>>>> mmap the page, when instead we could just allow the user to mmap the
>>>>>> page and whether the user does that and how they make use of it is up
>>>>>> to them...
>>>>>
>>>>> Duh.  Sorry.  For some reason I was thinking the magic MSI-X
>>>>> interception was happening in the host kernel rather than in qemu.
>>>>>
>>>>>> I understand that there are different virtualization techniques at play
>>>>>> here, it just doesn't seem relevant.  In the case of (A), the user can
>>>>>> choose not to mmap the page overlapping the vector table even if the
>>>>>> kernel allows it.  The user can also choose to mmap that page, but not
>>>>>> use the portion overlapping the vector table.  QEMU already does this
>>>>>> by overlaying a MemoryRegion for vector table emulation.  We might even
>>>>>> be able to get away with mmaping that page and emulating the vector
>>>>>> table elsewhere, which seems like the only option for a 64k page ARM
>>>>>> system.  For (B), clearly it's just a nuisance that we can't currently
>>>>>> mmap this page, but I still don't see how the user allowing the kernel
>>>>>> to allow the user to mmap that page makes any sense.  I can't even
>>>>>> describe it without it sounding ridiculous.  Thanks,
>>>>>
>>>>> Right.  Rethinking..  it seems to me we should just completely remove
>>>>> the logic from the kernel banning mmap()s overlapping the MSI-X
>>>>> table.  All it does is poorly attempt to stop the user shooting
>>>>> themselves in the foot.
>>>>>
>>>>> Then we just need logic in qemu to avoid doing the overlapping memory
>>>>> region nonsense on a per-machine basis
>>>>
>>>>
>>>> So is there still any plan or we just ditch the feature? I am confused now.
>>>
>>> The plan is what I said above.  Remove the bogus check logic from the
>>> kernel, then solve within qemu, by not creating the MSI-X intercept
>>> region for pseries guests.
>>
>>
>> There were 2 proposals how to do that. Both included platform code to
>> decide whether to allow mapping or not  and  some transport to pass that
>> enablement flag from the plafform code to the VFIO-PCI driver, one was via
>> an IOMMU group flag, the other via a PCI bus flag. Neither was accepted so
>> reposting those won't make any progress, what do I miss here? Was there any
>> agreement on how to do this?
> 
> As we've discussed the filtering of mmap()able regions in the kernel
> is not a security concern.  All it accomplishes is poorly trying to
> stop userspace from shooting itself in the foot by directly mapping
> and accessing the MSI-X table instead of using the VFIO interfaces to
> set up MSI vectors.
> 
> We should simply remove - unconditionally - the checks in the kernel
> which prevent parts of the BARs from being mapped.
> 


Just like this? And Alex is ok with that?



diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
index 9ed1ecf..106b825 100644
--- a/drivers/vfio/pci/vfio_pci.c
+++ b/drivers/vfio/pci/vfio_pci.c
@@ -1146,22 +1146,6 @@ static int vfio_pci_mmap(void *device_data, struct
vm_area_struct *vma)
        if (req_start + req_len > phys_len)
                return -EINVAL;

-       if (!vdev->msix_mmap && index == vdev->msix_bar) {
-               /*
-                * Disallow mmaps overlapping the MSI-X table; users don't
-                * get to touch this directly.  We could find somewhere
-                * else to map the overlap, but page granularity is only
-                * a recommendation, not a requirement, so the user needs
-                * to know which bits are real.  Requiring them to mmap
-                * around the table makes that clear.
-                */
-
-               /* If neither entirely above nor below, then it overlaps */
-               if (!(req_start >= vdev->msix_offset + vdev->msix_size ||
-                     req_start + req_len <= vdev->msix_offset))
-                       return -EINVAL;
-       }




-- 
Alexey


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

Re: [RFC PATCH kernel] vfio-pci: Allow mapping MSIX BAR

[David Gibson]

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

On Wed, Nov 29, 2017 at 06:58:11PM +1100, Alexey Kardashevskiy wrote:
> On 29/11/17 16:17, David Gibson wrote:
> > On Wed, Nov 29, 2017 at 03:38:30PM +1100, Alexey Kardashevskiy wrote:
> >> On 29/11/17 14:57, David Gibson wrote:
> >>> On Wed, Nov 29, 2017 at 02:25:43PM +1100, Alexey Kardashevskiy wrote:
> >>>> On 22/11/17 17:51, David Gibson wrote:
> >>>>> On Tue, Nov 21, 2017 at 10:14:45PM -0700, Alex Williamson wrote:
> >>>>>> On Wed, 22 Nov 2017 15:44:55 +1100
> >>>>>> David Gibson <david@gibson.dropbear.id.au> wrote:
> >>>>>>
> >>>>>>> On Tue, Nov 21, 2017 at 09:28:46PM -0700, Alex Williamson wrote:
> >>>>>>>> On Wed, 22 Nov 2017 15:09:32 +1100
> >>>>>>>> Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> >>>>>>>>   
> >>>>>>>>> By default VFIO disables mapping of MSIX BAR to the userspace as
> >>>>>>>>> the userspace may program it in a way allowing spurious interrupts;
> >>>>>>>>> instead the userspace uses the VFIO_DEVICE_SET_IRQS ioctl.
> >>>>>>>>>
> >>>>>>>>> This works fine as long as the system page size equals to the MSIX
> >>>>>>>>> alignment requirement which is 4KB. However with a bigger page size
> >>>>>>>>> the existing code prohibits mapping non-MSIX parts of a page with MSIX
> >>>>>>>>> structures so these parts have to be emulated via slow reads/writes on
> >>>>>>>>> a VFIO device fd. If these emulated bits are accessed often, this has
> >>>>>>>>> serious impact on performance.
> >>>>>>>>>
> >>>>>>>>> This adds an ioctl to the vfio-pci device which hides the sparse
> >>>>>>>>> capability and allows the userspace to map a BAR with MSIX structures.  
> >>>>>>>>
> >>>>>>>> So the user is in control of telling the kernel whether they're allowed
> >>>>>>>> to mmap the msi-x vector table.  That makes absolutely no sense.  If
> >>>>>>>> you're trying to figure out how userspace knows whether to implicitly
> >>>>>>>> avoid mmap'ing the msix region, I think there are far better ways in
> >>>>>>>> the existing region info ioctl.  We could use a flag, or maybe the
> >>>>>>>> existence of a capability chain pointer, or a new capability.  But
> >>>>>>>> absolutely not this.  The kernel needs to decide whether it's going to
> >>>>>>>> let the user do this, not the user.  Thanks,  
> >>>>>>>
> >>>>>>> No, it doesn't.  This is actually the approach we discussed in Prague.
> >>>>>>>
> >>>>>>> Remember that intercepting access to the MSI-X table is not a host
> >>>>>>> safety / security issue.  It's just that without that we won't wire up
> >>>>>>> the device's MSI-X vectors properly so they won't work.
> >>>>>>>
> >>>>>>> Basically the decision here is between
> >>>>>>>
> >>>>>>>    A) Allow MSI-X configuration via standard PCI mechanisms, at the
> >>>>>>>       cost of making access slow for any registers sharing a page with
> >>>>>>>       the MSI-X table.
> >>>>>>>
> >>>>>>> or
> >>>>>>>
> >>>>>>>    B) Make access to BAR registers sharing a page with the MSI-X table
> >>>>>>>       fast, at the cost of requiring some alternative mechanism to
> >>>>>>>       configure MSI-X vectors.
> >>>>>>>
> >>>>>>> And that is a tradeoff that it is reasonable for userspace to make.
> >>>>>>>
> >>>>>>> In the case of KVM guests, the decision depends entirely on the
> >>>>>>> *guest* platform.  Usually we need (A) because the guest expects to be
> >>>>>>> able to poke the MSI-X table in the usual way.  However for PAPR
> >>>>>>> guests, there's an alternative mechanism via an RTAS call, which means
> >>>>>>> we can use (B).
> >>>>>>>
> >>>>>>> The host kernel can't make this decision, because it doesn't know the
> >>>>>>> guest platform (well, KVM might, but VFIO doesn't).
> >>>>>>>
> >>>>>>> A userspace VFIO program could also elect for (B) if it does care
> >>>>>>> about performance of access to registers in the same BAR as the MSI-X
> >>>>>>> table, but doesn't need MSI-X for example.
> >>>>>>
> >>>>>> You're asking for an ioctl to allow the kernel to allow the user to
> >>>>>> mmap the page, when instead we could just allow the user to mmap the
> >>>>>> page and whether the user does that and how they make use of it is up
> >>>>>> to them...
> >>>>>
> >>>>> Duh.  Sorry.  For some reason I was thinking the magic MSI-X
> >>>>> interception was happening in the host kernel rather than in qemu.
> >>>>>
> >>>>>> I understand that there are different virtualization techniques at play
> >>>>>> here, it just doesn't seem relevant.  In the case of (A), the user can
> >>>>>> choose not to mmap the page overlapping the vector table even if the
> >>>>>> kernel allows it.  The user can also choose to mmap that page, but not
> >>>>>> use the portion overlapping the vector table.  QEMU already does this
> >>>>>> by overlaying a MemoryRegion for vector table emulation.  We might even
> >>>>>> be able to get away with mmaping that page and emulating the vector
> >>>>>> table elsewhere, which seems like the only option for a 64k page ARM
> >>>>>> system.  For (B), clearly it's just a nuisance that we can't currently
> >>>>>> mmap this page, but I still don't see how the user allowing the kernel
> >>>>>> to allow the user to mmap that page makes any sense.  I can't even
> >>>>>> describe it without it sounding ridiculous.  Thanks,
> >>>>>
> >>>>> Right.  Rethinking..  it seems to me we should just completely remove
> >>>>> the logic from the kernel banning mmap()s overlapping the MSI-X
> >>>>> table.  All it does is poorly attempt to stop the user shooting
> >>>>> themselves in the foot.
> >>>>>
> >>>>> Then we just need logic in qemu to avoid doing the overlapping memory
> >>>>> region nonsense on a per-machine basis
> >>>>
> >>>>
> >>>> So is there still any plan or we just ditch the feature? I am confused now.
> >>>
> >>> The plan is what I said above.  Remove the bogus check logic from the
> >>> kernel, then solve within qemu, by not creating the MSI-X intercept
> >>> region for pseries guests.
> >>
> >>
> >> There were 2 proposals how to do that. Both included platform code to
> >> decide whether to allow mapping or not  and  some transport to pass that
> >> enablement flag from the plafform code to the VFIO-PCI driver, one was via
> >> an IOMMU group flag, the other via a PCI bus flag. Neither was accepted so
> >> reposting those won't make any progress, what do I miss here? Was there any
> >> agreement on how to do this?
> > 
> > As we've discussed the filtering of mmap()able regions in the kernel
> > is not a security concern.  All it accomplishes is poorly trying to
> > stop userspace from shooting itself in the foot by directly mapping
> > and accessing the MSI-X table instead of using the VFIO interfaces to
> > set up MSI vectors.
> > 
> > We should simply remove - unconditionally - the checks in the kernel
> > which prevent parts of the BARs from being mapped.
> 
> 
> Just like this? And Alex is ok with that?

Yes, just like this.  I was kind of hoping Alex would express an
opinion when I mentioned it above.  But at any rate I will advocate
for this approach with him.

At least until someone points out another error in my thinking and I
try again.

> 
> 
> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> index 9ed1ecf..106b825 100644
> --- a/drivers/vfio/pci/vfio_pci.c
> +++ b/drivers/vfio/pci/vfio_pci.c
> @@ -1146,22 +1146,6 @@ static int vfio_pci_mmap(void *device_data, struct
> vm_area_struct *vma)
>         if (req_start + req_len > phys_len)
>                 return -EINVAL;
> 
> -       if (!vdev->msix_mmap && index == vdev->msix_bar) {
> -               /*
> -                * Disallow mmaps overlapping the MSI-X table; users don't
> -                * get to touch this directly.  We could find somewhere
> -                * else to map the overlap, but page granularity is only
> -                * a recommendation, not a requirement, so the user needs
> -                * to know which bits are real.  Requiring them to mmap
> -                * around the table makes that clear.
> -                */
> -
> -               /* If neither entirely above nor below, then it overlaps */
> -               if (!(req_start >= vdev->msix_offset + vdev->msix_size ||
> -                     req_start + req_len <= vdev->msix_offset))
> -                       return -EINVAL;
> -       }
> 
> 
> 
> 




-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC PATCH kernel] vfio-pci: Allow mapping MSIX BAR

[Alex Williamson]

On Wed, 29 Nov 2017 19:52:07 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Wed, Nov 29, 2017 at 06:58:11PM +1100, Alexey Kardashevskiy wrote:
> > On 29/11/17 16:17, David Gibson wrote:  
> > > On Wed, Nov 29, 2017 at 03:38:30PM +1100, Alexey Kardashevskiy wrote:  
> > >> On 29/11/17 14:57, David Gibson wrote:  
> > >>> On Wed, Nov 29, 2017 at 02:25:43PM +1100, Alexey Kardashevskiy wrote:  
> > >>>> On 22/11/17 17:51, David Gibson wrote:  
> > >>>>> On Tue, Nov 21, 2017 at 10:14:45PM -0700, Alex Williamson wrote:  
> > >>>>>> On Wed, 22 Nov 2017 15:44:55 +1100
> > >>>>>> David Gibson <david@gibson.dropbear.id.au> wrote:
> > >>>>>>  
> > >>>>>>> On Tue, Nov 21, 2017 at 09:28:46PM -0700, Alex Williamson wrote:  
> > >>>>>>>> On Wed, 22 Nov 2017 15:09:32 +1100
> > >>>>>>>> Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> > >>>>>>>>     
> > >>>>>>>>> By default VFIO disables mapping of MSIX BAR to the userspace as
> > >>>>>>>>> the userspace may program it in a way allowing spurious interrupts;
> > >>>>>>>>> instead the userspace uses the VFIO_DEVICE_SET_IRQS ioctl.
> > >>>>>>>>>
> > >>>>>>>>> This works fine as long as the system page size equals to the MSIX
> > >>>>>>>>> alignment requirement which is 4KB. However with a bigger page size
> > >>>>>>>>> the existing code prohibits mapping non-MSIX parts of a page with MSIX
> > >>>>>>>>> structures so these parts have to be emulated via slow reads/writes on
> > >>>>>>>>> a VFIO device fd. If these emulated bits are accessed often, this has
> > >>>>>>>>> serious impact on performance.
> > >>>>>>>>>
> > >>>>>>>>> This adds an ioctl to the vfio-pci device which hides the sparse
> > >>>>>>>>> capability and allows the userspace to map a BAR with MSIX structures.    
> > >>>>>>>>
> > >>>>>>>> So the user is in control of telling the kernel whether they're allowed
> > >>>>>>>> to mmap the msi-x vector table.  That makes absolutely no sense.  If
> > >>>>>>>> you're trying to figure out how userspace knows whether to implicitly
> > >>>>>>>> avoid mmap'ing the msix region, I think there are far better ways in
> > >>>>>>>> the existing region info ioctl.  We could use a flag, or maybe the
> > >>>>>>>> existence of a capability chain pointer, or a new capability.  But
> > >>>>>>>> absolutely not this.  The kernel needs to decide whether it's going to
> > >>>>>>>> let the user do this, not the user.  Thanks,    
> > >>>>>>>
> > >>>>>>> No, it doesn't.  This is actually the approach we discussed in Prague.
> > >>>>>>>
> > >>>>>>> Remember that intercepting access to the MSI-X table is not a host
> > >>>>>>> safety / security issue.  It's just that without that we won't wire up
> > >>>>>>> the device's MSI-X vectors properly so they won't work.
> > >>>>>>>
> > >>>>>>> Basically the decision here is between
> > >>>>>>>
> > >>>>>>>    A) Allow MSI-X configuration via standard PCI mechanisms, at the
> > >>>>>>>       cost of making access slow for any registers sharing a page with
> > >>>>>>>       the MSI-X table.
> > >>>>>>>
> > >>>>>>> or
> > >>>>>>>
> > >>>>>>>    B) Make access to BAR registers sharing a page with the MSI-X table
> > >>>>>>>       fast, at the cost of requiring some alternative mechanism to
> > >>>>>>>       configure MSI-X vectors.
> > >>>>>>>
> > >>>>>>> And that is a tradeoff that it is reasonable for userspace to make.
> > >>>>>>>
> > >>>>>>> In the case of KVM guests, the decision depends entirely on the
> > >>>>>>> *guest* platform.  Usually we need (A) because the guest expects to be
> > >>>>>>> able to poke the MSI-X table in the usual way.  However for PAPR
> > >>>>>>> guests, there's an alternative mechanism via an RTAS call, which means
> > >>>>>>> we can use (B).
> > >>>>>>>
> > >>>>>>> The host kernel can't make this decision, because it doesn't know the
> > >>>>>>> guest platform (well, KVM might, but VFIO doesn't).
> > >>>>>>>
> > >>>>>>> A userspace VFIO program could also elect for (B) if it does care
> > >>>>>>> about performance of access to registers in the same BAR as the MSI-X
> > >>>>>>> table, but doesn't need MSI-X for example.  
> > >>>>>>
> > >>>>>> You're asking for an ioctl to allow the kernel to allow the user to
> > >>>>>> mmap the page, when instead we could just allow the user to mmap the
> > >>>>>> page and whether the user does that and how they make use of it is up
> > >>>>>> to them...  
> > >>>>>
> > >>>>> Duh.  Sorry.  For some reason I was thinking the magic MSI-X
> > >>>>> interception was happening in the host kernel rather than in qemu.
> > >>>>>  
> > >>>>>> I understand that there are different virtualization techniques at play
> > >>>>>> here, it just doesn't seem relevant.  In the case of (A), the user can
> > >>>>>> choose not to mmap the page overlapping the vector table even if the
> > >>>>>> kernel allows it.  The user can also choose to mmap that page, but not
> > >>>>>> use the portion overlapping the vector table.  QEMU already does this
> > >>>>>> by overlaying a MemoryRegion for vector table emulation.  We might even
> > >>>>>> be able to get away with mmaping that page and emulating the vector
> > >>>>>> table elsewhere, which seems like the only option for a 64k page ARM
> > >>>>>> system.  For (B), clearly it's just a nuisance that we can't currently
> > >>>>>> mmap this page, but I still don't see how the user allowing the kernel
> > >>>>>> to allow the user to mmap that page makes any sense.  I can't even
> > >>>>>> describe it without it sounding ridiculous.  Thanks,  
> > >>>>>
> > >>>>> Right.  Rethinking..  it seems to me we should just completely remove
> > >>>>> the logic from the kernel banning mmap()s overlapping the MSI-X
> > >>>>> table.  All it does is poorly attempt to stop the user shooting
> > >>>>> themselves in the foot.
> > >>>>>
> > >>>>> Then we just need logic in qemu to avoid doing the overlapping memory
> > >>>>> region nonsense on a per-machine basis  
> > >>>>
> > >>>>
> > >>>> So is there still any plan or we just ditch the feature? I am confused now.  
> > >>>
> > >>> The plan is what I said above.  Remove the bogus check logic from the
> > >>> kernel, then solve within qemu, by not creating the MSI-X intercept
> > >>> region for pseries guests.  
> > >>
> > >>
> > >> There were 2 proposals how to do that. Both included platform code to
> > >> decide whether to allow mapping or not  and  some transport to pass that
> > >> enablement flag from the plafform code to the VFIO-PCI driver, one was via
> > >> an IOMMU group flag, the other via a PCI bus flag. Neither was accepted so
> > >> reposting those won't make any progress, what do I miss here? Was there any
> > >> agreement on how to do this?  
> > > 
> > > As we've discussed the filtering of mmap()able regions in the kernel
> > > is not a security concern.  All it accomplishes is poorly trying to
> > > stop userspace from shooting itself in the foot by directly mapping
> > > and accessing the MSI-X table instead of using the VFIO interfaces to
> > > set up MSI vectors.
> > > 
> > > We should simply remove - unconditionally - the checks in the kernel
> > > which prevent parts of the BARs from being mapped.  
> > 
> > 
> > Just like this? And Alex is ok with that?  
> 
> Yes, just like this.  I was kind of hoping Alex would express an
> opinion when I mentioned it above.  But at any rate I will advocate
> for this approach with him.

Sorry, I was on PTO even before this thread started.
 
> At least until someone points out another error in my thinking and I
> try again.
> 
> > 
> > 
> > diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> > index 9ed1ecf..106b825 100644
> > --- a/drivers/vfio/pci/vfio_pci.c
> > +++ b/drivers/vfio/pci/vfio_pci.c
> > @@ -1146,22 +1146,6 @@ static int vfio_pci_mmap(void *device_data, struct
> > vm_area_struct *vma)
> >         if (req_start + req_len > phys_len)
> >                 return -EINVAL;
> > 
> > -       if (!vdev->msix_mmap && index == vdev->msix_bar) {
> > -               /*
> > -                * Disallow mmaps overlapping the MSI-X table; users don't
> > -                * get to touch this directly.  We could find somewhere
> > -                * else to map the overlap, but page granularity is only
> > -                * a recommendation, not a requirement, so the user needs
> > -                * to know which bits are real.  Requiring them to mmap
> > -                * around the table makes that clear.
> > -                */
> > -
> > -               /* If neither entirely above nor below, then it overlaps */
> > -               if (!(req_start >= vdev->msix_offset + vdev->msix_size ||
> > -                     req_start + req_len <= vdev->msix_offset))
> > -                       return -EINVAL;
> > -       }

Well, there's a bit more to it than that.  The sparse mmap capability
needs to be updated (eliminated for msi-x usage - remains for mdev
devices) and then we need to figure out how userspaces like QEMU take
advantage of this new feature.  QEMU will implicitly assume that it
needs to mask the vector table area if either no sparse mmap capability
exists or the resulting mmap covers the entire BAR hosting the vector
table.  Do we simply update QEMU to try the mmap and fall back to
excluding the vector table area if it fails?  We already screwed up
once with the implicit non-mmap'able vector area, so I think I'd rather
see an explicit interface so that userspace knows what should work and
knows there's something wrong if it doesn't.  I see two ways to do
that, one would be a bit in vfio_region_info.flags indicating the user
doesn't need to account for implicitly avoiding the vector table, the
other would be a new capability to do the same.  I tend to prefer the
latter because we have a 16bit address space to work with for
capability ids but only 32 flag bits on the region.  So I'd probably
define a VFIO_REGION_INFO_CAP_MSIX_MAPPABLE capability that gets added
in place of the sparse capability.  Unfortunately this means that
current userspace will fall back to the implicit avoidance, but I don't
think we can avoid that, or can we?

vfio-pci also mangles read(3P) and write(3P) to the msi-x vector table,
dropping writes and returning -1 for reads.  I don't see any reason to
change this behavior.  Our ONLY reason for allowing mmap is to allow
direct access to NON-MSI-X registers, especially when system page sizes
exceed the minimum alignment guidelines noted in the PCI spec
implementation notes.  We are not providing an alternative method of
programming device interrupts and the comments around the definition of
the new VFIO_REGION_INFO_CAP_MSIX_MAPPABLE capability should make this
abundantly clear, ie. while we allow mmap and therefore direct access
to the device msi-x vector table, there is no legitimate use case for
the user to manipulate the vector table via this mmap.  Thanks,

Alex

Re: [RFC PATCH kernel] vfio-pci: Allow mapping MSIX BAR

[Konrad Rzeszutek Wilk]

On Wed, Nov 22, 2017 at 03:44:55PM +1100, David Gibson wrote:
> On Tue, Nov 21, 2017 at 09:28:46PM -0700, Alex Williamson wrote:
> > On Wed, 22 Nov 2017 15:09:32 +1100
> > Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> > 
> > > By default VFIO disables mapping of MSIX BAR to the userspace as
> > > the userspace may program it in a way allowing spurious interrupts;
> > > instead the userspace uses the VFIO_DEVICE_SET_IRQS ioctl.
> > > 
> > > This works fine as long as the system page size equals to the MSIX
> > > alignment requirement which is 4KB. However with a bigger page size
> > > the existing code prohibits mapping non-MSIX parts of a page with MSIX
> > > structures so these parts have to be emulated via slow reads/writes on
> > > a VFIO device fd. If these emulated bits are accessed often, this has
> > > serious impact on performance.
> > > 
> > > This adds an ioctl to the vfio-pci device which hides the sparse
> > > capability and allows the userspace to map a BAR with MSIX structures.
> > 
> > So the user is in control of telling the kernel whether they're allowed
> > to mmap the msi-x vector table.  That makes absolutely no sense.  If
> > you're trying to figure out how userspace knows whether to implicitly
> > avoid mmap'ing the msix region, I think there are far better ways in
> > the existing region info ioctl.  We could use a flag, or maybe the
> > existence of a capability chain pointer, or a new capability.  But
> > absolutely not this.  The kernel needs to decide whether it's going to
> > let the user do this, not the user.  Thanks,
> 
> No, it doesn't.  This is actually the approach we discussed in Prague.
> 
> Remember that intercepting access to the MSI-X table is not a host
> safety / security issue.  It's just that without that we won't wire up

How is that not a security issue? Having an guest or an user-space
access to muck with the MSI-X vectors allows them to do some form
of memory writes using the IOAPIC.


> the device's MSI-X vectors properly so they won't work.
> 
> Basically the decision here is between
> 
>    A) Allow MSI-X configuration via standard PCI mechanisms, at the
>       cost of making access slow for any registers sharing a page with
>       the MSI-X table.
> 
> or
> 
>    B) Make access to BAR registers sharing a page with the MSI-X table
>       fast, at the cost of requiring some alternative mechanism to
>       configure MSI-X vectors.
> 
> And that is a tradeoff that it is reasonable for userspace to make.
> 
> In the case of KVM guests, the decision depends entirely on the
> *guest* platform.  Usually we need (A) because the guest expects to be
> able to poke the MSI-X table in the usual way.  However for PAPR
> guests, there's an alternative mechanism via an RTAS call, which means
> we can use (B).
> 
> The host kernel can't make this decision, because it doesn't know the
> guest platform (well, KVM might, but VFIO doesn't).
> 
> A userspace VFIO program could also elect for (B) if it does care
> about performance of access to registers in the same BAR as the MSI-X
> table, but doesn't need MSI-X for example.
> 
> -- 
> David Gibson			| I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
> 				| _way_ _around_!
> http://www.ozlabs.org/~dgibson

Re: [RFC PATCH kernel] vfio-pci: Allow mapping MSIX BAR

[Alex Williamson]

On Wed, 29 Nov 2017 17:03:31 -0500
Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:

> On Wed, Nov 22, 2017 at 03:44:55PM +1100, David Gibson wrote:
> > On Tue, Nov 21, 2017 at 09:28:46PM -0700, Alex Williamson wrote:  
> > > On Wed, 22 Nov 2017 15:09:32 +1100
> > > Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> > >   
> > > > By default VFIO disables mapping of MSIX BAR to the userspace as
> > > > the userspace may program it in a way allowing spurious interrupts;
> > > > instead the userspace uses the VFIO_DEVICE_SET_IRQS ioctl.
> > > > 
> > > > This works fine as long as the system page size equals to the MSIX
> > > > alignment requirement which is 4KB. However with a bigger page size
> > > > the existing code prohibits mapping non-MSIX parts of a page with MSIX
> > > > structures so these parts have to be emulated via slow reads/writes on
> > > > a VFIO device fd. If these emulated bits are accessed often, this has
> > > > serious impact on performance.
> > > > 
> > > > This adds an ioctl to the vfio-pci device which hides the sparse
> > > > capability and allows the userspace to map a BAR with MSIX structures.  
> > > 
> > > So the user is in control of telling the kernel whether they're allowed
> > > to mmap the msi-x vector table.  That makes absolutely no sense.  If
> > > you're trying to figure out how userspace knows whether to implicitly
> > > avoid mmap'ing the msix region, I think there are far better ways in
> > > the existing region info ioctl.  We could use a flag, or maybe the
> > > existence of a capability chain pointer, or a new capability.  But
> > > absolutely not this.  The kernel needs to decide whether it's going to
> > > let the user do this, not the user.  Thanks,  
> > 
> > No, it doesn't.  This is actually the approach we discussed in Prague.
> > 
> > Remember that intercepting access to the MSI-X table is not a host
> > safety / security issue.  It's just that without that we won't wire up  
> 
> How is that not a security issue? Having an guest or an user-space
> access to muck with the MSI-X vectors allows them to do some form
> of memory writes using the IOAPIC.

The MSI-X vector table only specifies the address and data used for a
DMA write to trigger an interrupt.  Any device capable of DMA is
already able to generate those same DMA writes regardless of what's in
the MSI-X vector table.  Therefore preventing mmap of the vector table
is at best an obfuscation, which isn't worth the potentially significant
handicap it imposes for architectures with system page sizes larger
than the PCI spec recommended alignments.  All DMA, including DMA
writes to interrupt controllers, is expected to be handled by the IOMMU
or the user should have had to opt-in to insecure interrupts already.
Thanks,

Alex