LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Alex Williamson <alex.williamson@redhat.com>
To: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
Cc: <eric.auger@redhat.com>, <pmorel@linux.vnet.ibm.com>,
	<kvm@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
	<iommu@lists.linux-foundation.org>, <linuxarm@huawei.com>,
	<john.garry@huawei.com>, <xuwei5@hisilicon.com>,
	Joerg Roedel <joro@8bytes.org>
Subject: Re: [PATCH v6 0/7] vfio/type1: Add support for valid iova list management
Date: Thu, 24 May 2018 12:20:32 -0600	[thread overview]
Message-ID: <20180524122032.724676b9@w520.home> (raw)
In-Reply-To: <20180418114045.7968-1-shameerali.kolothum.thodi@huawei.com>

[Cc +Joerg: AMD-Vi observation towards the end]

On Wed, 18 Apr 2018 12:40:38 +0100
Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:

> This series introduces an iova list associated with a vfio 
> iommu. The list is kept updated taking care of iommu apertures,
> and reserved regions. Also this series adds checks for any conflict
> with existing dma mappings whenever a new device group is attached to
> the domain.
> 
> User-space can retrieve valid iova ranges using VFIO_IOMMU_GET_INFO
> ioctl capability chains. Any dma map request outside the valid iova
> range will be rejected.

Hi Shameer,

I ran into two minor issues in testing this series, both related to
mdev usage of type1.  First, in patch 5/7 when we try to validate a dma
map request:

> +static bool vfio_iommu_iova_dma_valid(struct vfio_iommu *iommu,
> +				dma_addr_t start, dma_addr_t end)
> +{
> +	struct list_head *iova = &iommu->iova_list;
> +	struct vfio_iova *node;
> +
> +	list_for_each_entry(node, iova, list) {
> +		if ((start >= node->start) && (end <= node->end))
> +			return true;
> +	}
> +
> +	return false;
> +}

A container with only an mdev device will have an empty list because it
has not backing iommu to set ranges or reserved regions, so any dma map
will fail.  I think this is resolved as follows:

--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -1100,7 +1100,7 @@ static bool vfio_iommu_iova_dma_valid(struct vfio_iommu *iommu,
                        return true;
        }
 
-       return false;
+       return list_empty(&iommu->iova_list);
 }
 
ie. return false only if there was anything to test against.

The second issue is similar, patch 6/7 adds to VFIO_IOMMU_GET_INFO:

+		ret = vfio_iommu_iova_build_caps(iommu, &caps);
+		if (ret)
+			return ret;

And build_caps has:

+	list_for_each_entry(iova, &iommu->iova_list, list)
+		iovas++;
+
+	if (!iovas) {
+		ret = -EINVAL;

Therefore if the iova list is empty, as for mdevs, the use can no
longer even call VFIO_IOMMU_GET_INFO on the container, which is a
regression.  Again, I think the fix is simple:

@@ -2090,7 +2090,7 @@ static int vfio_iommu_iova_build_caps(struct vfio_iommu *iommu,
                iovas++;
 
        if (!iovas) {
-               ret = -EINVAL;
+               ret = 0;
                goto out_unlock;
        }
 
ie. build_caps needs to handle lack of an iova_list as a non-error.

Also, I wrote a small unit test to validate the iova list for my
systems[1].  With the above changes, my Intel test system gives expected
results:

# ./vfio-type1-iova-list /sys/bus/mdev/devices/c08db5ed-05d3-4b39-b150-438a18bc698f /sys/bus/pci/devices/0000:00:1b.0
---- Adding device: c08db5ed-05d3-4b39-b150-438a18bc698f ----
Initial info struct size: 0x18
No caps
---- Adding device: 0000:00:1b.0 ----
Initial info struct size: 0x18
Requested info struct size: 0x48
New info struct size: 0x48
argsz: 0x48, flags: 0x3, cap_offset: 0x18
	00: 4800 0000 0300 0000 00f0 ffff ffff ffff
	10: 1800 0000 0000 0000 0100 0100 0000 0000
	20: 0200 0000 0000 0000 0000 0000 0000 0000
	30: ffff dffe 0000 0000 0000 f0fe 0000 0000
	40: ffff ffff ffff ff01 
[cap id: 1, version: 1, next: 0x0]
Found type1 iova range version: 1
	00: 0000000000000000 - 00000000fedfffff
	01: 00000000fef00000 - 01ffffffffffffff

Adding an mdev device to the container results in no iova list, adding
the physical device updates to the expected set with the MSI range
excluded.

I was a little surprised by an AMD system:

# ./vfio-type1-iova-list /sys/bus/pci/devices/0000:01:00.0
---- Adding device: 0000:01:00.0 ----
Initial info struct size: 0x18
Requested info struct size: 0x58
New info struct size: 0x58
argsz: 0x58, flags: 0x3, cap_offset: 0x18
	00: 5800 0000 0300 0000 00f0 ffff 7fff ffff
	10: 1800 0000 0000 0000 0100 0100 0000 0000
	20: 0300 0000 0000 0000 0000 0000 0000 0000
	30: ffff dffe 0000 0000 0000 f0fe 0000 0000
	40: ffff ffff fc00 0000 0000 0000 0001 0000
	50: ffff ffff ffff ffff 
[cap id: 1, version: 1, next: 0x0]
Found type1 iova range version: 1
	00: 0000000000000000 - 00000000fedfffff
	01: 00000000fef00000 - 000000fcffffffff
	02: 0000010000000000 - ffffffffffffffff

The additional excluded range is the HyperTransport area (which for
99.9+% of use cases isn't going to be a problem) is a rather surprising
limit for the size of a VM we can use on AMD, just under 1TB.  I fully
expect we'll see a bug report from that and we should be thinking about
how to address it.  Otherwise, if the changes above look good to you
I'll incorporate them into their respective patches and push to my next
branch.  Thanks,

Alex

[1] https://gist.github.com/awilliam/25e39252ab28101a2d89ace1ff765fdd

  parent reply	other threads:[~2018-05-24 18:20 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-04-18 11:40 Shameer Kolothum
2018-04-18 11:40 ` [PATCH v6 1/7] vfio/type1: Introduce iova list and add iommu aperture validity check Shameer Kolothum
2018-04-18 11:40 ` [PATCH v6 2/7] vfio/type1: Check reserve region conflict and update iova list Shameer Kolothum
2018-04-18 11:40 ` [PATCH v6 3/7] vfio/type1: Update iova list on detach Shameer Kolothum
2018-04-18 11:40 ` [PATCH v6 4/7] iommu/dma: Move PCI window region reservation back into dma specific path Shameer Kolothum
2018-04-24 13:16   ` Shameerali Kolothum Thodi
2018-04-30 16:10     ` Alex Williamson
2018-05-03 13:16   ` Joerg Roedel
2018-04-18 11:40 ` [PATCH v6 5/7] vfio/type1: check dma map request is within a valid iova range Shameer Kolothum
2018-04-18 11:40 ` [PATCH v6 6/7] vfio/type1: Add IOVA range capability support Shameer Kolothum
2018-04-18 11:40 ` [PATCH v6 7/7] vfio/type1: remove duplicate retrieval of reserved regions Shameer Kolothum
2018-05-24 18:20 ` Alex Williamson [this message]
2018-05-25  8:45   ` [PATCH v6 0/7] vfio/type1: Add support for valid iova list management Shameerali Kolothum Thodi
2018-05-25 20:54     ` Alex Williamson
2018-06-05 17:03       ` Alex Williamson
2018-06-06  6:52         ` Shameerali Kolothum Thodi
2018-06-07 15:05           ` Alex Williamson

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180524122032.724676b9@w520.home \
    --to=alex.williamson@redhat.com \
    --cc=eric.auger@redhat.com \
    --cc=iommu@lists.linux-foundation.org \
    --cc=john.garry@huawei.com \
    --cc=joro@8bytes.org \
    --cc=kvm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linuxarm@huawei.com \
    --cc=pmorel@linux.vnet.ibm.com \
    --cc=shameerali.kolothum.thodi@huawei.com \
    --cc=xuwei5@hisilicon.com \
    --subject='Re: [PATCH v6 0/7] vfio/type1: Add support for valid iova list management' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).