On Wed, Mar 18, 2020 at 03:47:44PM +0530, Bharat Bhushan wrote:
> Hi Jean,
> 
> On Tue, Mar 17, 2020 at 9:29 PM Jean-Philippe Brucker
> <jean-phili...@linaro.org> wrote:
> >
> > On Tue, Mar 17, 2020 at 02:46:55PM +0530, Bharat Bhushan wrote:
> > > Hi Jean,
> > >
> > > On Tue, Mar 17, 2020 at 2:23 PM Jean-Philippe Brucker
> > > <jean-phili...@linaro.org> wrote:
> > > >
> > > > On Tue, Mar 17, 2020 at 12:40:39PM +0530, Bharat Bhushan wrote:
> > > > > Hi Jean,
> > > > >
> > > > > On Mon, Mar 16, 2020 at 3:41 PM Jean-Philippe Brucker
> > > > > <jean-phili...@linaro.org> wrote:
> > > > > >
> > > > > > Hi Bharat,
> > > > > >
> > > > > > Could you Cc me on your next posting?  Unfortunately I don't have 
> > > > > > much
> > > > > > hardware for testing this at the moment, but I might be able to 
> > > > > > help a
> > > > > > little on the review.
> > > > > >
> > > > > > On Mon, Mar 16, 2020 at 02:40:00PM +0530, Bharat Bhushan wrote:
> > > > > > > > >>> First issue is: your guest can use 4K page and your host 
> > > > > > > > >>> can use 64KB
> > > > > > > > >>> pages. In that case VFIO_DMA_MAP will fail with -EINVAL. We 
> > > > > > > > >>> must devise
> > > > > > > > >>> a way to pass the host settings to the VIRTIO-IOMMU device.
> > > > > > > > >>>
> > > > > > > > >>> Even with 64KB pages, it did not work for me. I have 
> > > > > > > > >>> obviously not the
> > > > > > > > >>> storm of VFIO_DMA_MAP failures but I have some, most 
> > > > > > > > >>> probably due to
> > > > > > > > >>> some wrong notifications somewhere. I will try to 
> > > > > > > > >>> investigate on my side.
> > > > > > > > >>>
> > > > > > > > >>> Did you test with VFIO on your side?
> > > > > > > > >>
> > > > > > > > >> I did not tried with different page sizes, only tested with 
> > > > > > > > >> 4K page size.
> > > > > > > > >>
> > > > > > > > >> Yes it works, I tested with two n/w device assigned to VM, 
> > > > > > > > >> both interfaces works
> > > > > > > > >>
> > > > > > > > >> First I will try with 64k page size.
> > > > > > > > >
> > > > > > > > > 64K page size does not work for me as well,
> > > > > > > > >
> > > > > > > > > I think we are not passing correct page_size_mask here
> > > > > > > > > (config.page_size_mask is set to TARGET_PAGE_MASK ( which is
> > > > > > > > > 0xfffffffffffff000))
> > > > > > > > I guess you mean with guest using 4K and host using 64K.
> > > > > > > > >
> > > > > > > > > We need to set this correctly as per host page size, correct?
> > > > > > > > Yes that's correct. We need to put in place a control path to 
> > > > > > > > retrieve
> > > > > > > > the page settings on host through VFIO to inform the 
> > > > > > > > virtio-iommu device.
> > > > > > > >
> > > > > > > > Besides this issue, did you try with 64kB on host and guest?
> > > > > > >
> > > > > > > I tried Followings
> > > > > > >   - 4k host and 4k guest  - it works with v7 version
> > > > > > >   - 64k host and 64k guest - it does not work with v7
> > > > > > >     hard-coded config.page_size_mask to 0xffffffffffff0000 and it 
> > > > > > > works
> > > > > >
> > > > > > You might get this from the iova_pgsize bitmap returned by
> > > > > > VFIO_IOMMU_GET_INFO. The virtio config.page_size_mask is global so 
> > > > > > there
> > > > > > is the usual problem of aggregating consistent properties, but I'm
> > > > > > guessing using the host page size as a granule here is safe enough.
> > > > > >
> > > > > > If it is a problem, we can add a PROBE property for page size mask,
> > > > > > allowing to define per-endpoint page masks. I have kernel patches
> > > > > > somewhere to do just that.
> > > > >
> > > > > I do not see we need page size mask per endpoint.
> > > > >
> > > > > While I am trying to understand what "page-size-mask" guest will work 
> > > > > with
> > > > >
> > > > > - 4K page size host and 4k page size guest
> > > > >   config.page_size_mask = 0xffffffffffff000 will work
> > > > >
> > > > > - 64K page size host and 64k page size guest
> > > > >   config.page_size_mask = 0xfffffffffff0000 will work
> > > > >
> > > > > - 64K page size host and 4k page size guest
> > > > >    1) config.page_size_mask = 0xffffffffffff000 will also not work as
> > > > > VFIO in host expect iova and size to be aligned to 64k (PAGE_SIZE in
> > > > > host)
> > > > >    2) config.page_size_mask = 0xfffffffffff0000 will not work, iova
> > > > > initialization (in guest) expect minimum page-size supported by h/w to
> > > > > be equal to 4k (PAGE_SIZE in guest)
> > > > >        Should we look to relax this in iova allocation code?
> > > >
> > > > Oh right, that's not great. Maybe the BUG_ON() can be removed, I'll ask 
> > > > on
> > > > the list.
> > >
> > > yes, the BUG_ON in iova_init.
> > > I tried with removing same and it worked, but not analyzed side effects.
> >
> > It might break the assumption from device drivers that mapping a page is
> > safe. For example they call alloc_page() followed by dma_map_page(). In
> > our situation dma-iommu.c will oblige and create one 64k mapping to one 4k
> > physical page. As a result the endpoint can access the neighbouring 60k of
> > memory.
> >
> > This isn't too terrible. After all, even when the page sizes match, device
> > drivers can call dma_map_single() on sub-page buffers, which will also let
> > the endpoint access a whole page. The solution, if you don't trust the
> > endpoint, is to use bounce buffers.
> >
> > But I suspect it's not as simple as removing the BUG_ON(), we'll need to
> > go over dma-iommu.c first. And it seems like assigning endpoints to guest
> > userspace won't work either in this config. In vfio_dma_do_map():
> >
> >         mask = ((uint64_t)1 << __ffs(vfio_pgsize_bitmap(iommu))) - 1;
> >
> >         WARN_ON(mask & PAGE_MASK);
> 
> Yes, Agree
> 
> >
> > If I read this correctly the WARN will trigger in a 4k guest under 64k
> > host, right?  So maybe we can just say that this config isn't supported,
> > unless it's an important use-case for virtio-iommu?
> 
> I sent v8 version of patch and with that guest and host with same page
> size should work.
> While i have not yet added analyzed how to mark 4k guest and 64k host
> as un-supported configuration, will analyze and send patch.

I don't think there is anything to do for QEMU, it's Linux that doesn't
support the configuration. We could add something like the attached patch,
in the virtio-iommu driver, to abort more gracefully than with a BUG_ON().

Thanks,
Jean
>From 1fa08800c94f7ad6720b7e6fe26a65ed3d6ce715 Mon Sep 17 00:00:00 2001
From: Jean-Philippe Brucker <jean-phili...@linaro.org>
Date: Wed, 18 Mar 2020 11:59:19 +0100
Subject: [PATCH] iommu/virtio: Reject IOMMU page granule larger than PAGE_SIZE

We don't currently support IOMMUs with a page granule larger than the
system page size. Currently the IOVA allocator has a BUG_ON() in this
case, and VFIO has a WARN_ON().

It might be possible to remove these obstacles if necessary. If the host
uses 64kB pages and the guest uses 4kB, then a device driver calling
alloc_page() followed by dma_map_page() will create a 64kB mapping for a
4kB physical page, allowing the endpoint to access the neighbouring 60kB
of memory. This problem could be worked around with bounce buffers.

For the moment, rather than triggering the IOVA BUG_ON() on mismatched
page sizes, abort the virtio-iommu probe with an error message.

Reported-by: Bharat Bhushan <bbhush...@marvell.com>
Signed-off-by: Jean-Philippe Brucker <jean-phili...@linaro.org>
---
 drivers/iommu/virtio-iommu.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c
index 6d4e3c2a2ddb..80d5d8f621ab 100644
--- a/drivers/iommu/virtio-iommu.c
+++ b/drivers/iommu/virtio-iommu.c
@@ -998,6 +998,7 @@ static int viommu_probe(struct virtio_device *vdev)
        struct device *parent_dev = vdev->dev.parent;
        struct viommu_dev *viommu = NULL;
        struct device *dev = &vdev->dev;
+       unsigned long viommu_page_size;
        u64 input_start = 0;
        u64 input_end = -1UL;
        int ret;
@@ -1028,6 +1029,14 @@ static int viommu_probe(struct virtio_device *vdev)
                goto err_free_vqs;
        }
 
+       viommu_page_size = 1UL << __ffs(viommu->pgsize_bitmap);
+       if (viommu_page_size > PAGE_SIZE) {
+               dev_err(dev, "granule 0x%lx larger than system page size 
0x%lx\n",
+                       viommu_page_size, PAGE_SIZE);
+               ret = -EINVAL;
+               goto err_free_vqs;
+       }
+
        viommu->map_flags = VIRTIO_IOMMU_MAP_F_READ | VIRTIO_IOMMU_MAP_F_WRITE;
        viommu->last_domain = ~0U;
 
-- 
2.25.1

Reply via email to