Re: [RFC PATCH] vfio/type1: Do not support IOMMUs that allow bypass

[Robin Murphy]

On 29/10/15 18:28, Will Deacon wrote:

On Tue, Oct 27, 2015 at 10:00:11AM -0600, Alex Williamson wrote:

On Tue, 2015-10-27 at 15:40 +, Will Deacon wrote:

On Fri, Oct 16, 2015 at 09:51:22AM -0600, Alex Williamson wrote:

Would it be possible to add iommu_domain_geometry support to arm-smmu.c?
In addition to this test to verify that DMA cannot bypass the IOMMU, I'd
eventually like to pass the aperture information out through the VFIO
API.  Thanks,


The slight snag here is that we don't know which SMMU in the system the
domain is attached to at the point when the geometry is queried, so I
can't give you an upper bound on the aperture. For example, if there is
an SMMU with a 32-bit input address and another with a 48-bit input
address.

We could play the same horrible games that we do with the pgsize bitmap,
and truncate some global aperture everytime we probe an SMMU device, but
I'd really like to have fewer hacks like that if possible. The root of
the problem is still that domains are allocated for a bus, rather than
an IOMMU instance.


Yes, Intel VT-d has this issue as well.  In theory we can have
heterogeneous IOMMU hardware units (DRHDs) in a system and the upper
bound of the geometry could be diminished if we add a less capable DRHD
into the domain.  I suspect this is more a theoretical problem than a
practical one though as we're typically mixing similar DRHDs and I think
we're still capable of 39-bit addressing in the least capable version
per the spec.

In any case, I really want to start testing geometry.force_aperture,
even if we're not yet comfortable to expose the IOMMU limits to the
user.  The vfio type1 shouldn't be enabled at all for underlying
hardware that allows DMA bypass.  Thanks,


Ok, I'll put it on my list of things to look at under the assumption that
the actual aperture limits don't need to be accurate as long as DMA to
an arbitrary unmapped address always faults.


I'm pretty sure we'd only ever set the aperture to the full input 
address range anyway (since we're not a GART-type thing), in which case 
we should only need to worry about unmatched streams that don't hit in a 
domain at all. Doesn't the disable_bypass option already cover that? 
(FWIW I hacked it up for v2 a while back, too[0]).


Robin.

[0]:http://www.linux-arm.org/git?p=linux-rm.git;a=commitdiff;h=23a251189fa3330b799a837bd8eb1023aa2dcea4

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [RFC PATCH] vfio/type1: Do not support IOMMUs that allow bypass

[Will Deacon]

On Tue, Oct 27, 2015 at 10:00:11AM -0600, Alex Williamson wrote:
> On Tue, 2015-10-27 at 15:40 +, Will Deacon wrote:
> > On Fri, Oct 16, 2015 at 09:51:22AM -0600, Alex Williamson wrote:
> > > Would it be possible to add iommu_domain_geometry support to arm-smmu.c?
> > > In addition to this test to verify that DMA cannot bypass the IOMMU, I'd
> > > eventually like to pass the aperture information out through the VFIO
> > > API.  Thanks,
> > 
> > The slight snag here is that we don't know which SMMU in the system the
> > domain is attached to at the point when the geometry is queried, so I
> > can't give you an upper bound on the aperture. For example, if there is
> > an SMMU with a 32-bit input address and another with a 48-bit input
> > address.
> > 
> > We could play the same horrible games that we do with the pgsize bitmap,
> > and truncate some global aperture everytime we probe an SMMU device, but
> > I'd really like to have fewer hacks like that if possible. The root of
> > the problem is still that domains are allocated for a bus, rather than
> > an IOMMU instance.
> 
> Yes, Intel VT-d has this issue as well.  In theory we can have
> heterogeneous IOMMU hardware units (DRHDs) in a system and the upper
> bound of the geometry could be diminished if we add a less capable DRHD
> into the domain.  I suspect this is more a theoretical problem than a
> practical one though as we're typically mixing similar DRHDs and I think
> we're still capable of 39-bit addressing in the least capable version
> per the spec.
> 
> In any case, I really want to start testing geometry.force_aperture,
> even if we're not yet comfortable to expose the IOMMU limits to the
> user.  The vfio type1 shouldn't be enabled at all for underlying
> hardware that allows DMA bypass.  Thanks,

Ok, I'll put it on my list of things to look at under the assumption that
the actual aperture limits don't need to be accurate as long as DMA to
an arbitrary unmapped address always faults.

Will
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [RFC PATCH] vfio/type1: Do not support IOMMUs that allow bypass

[Will Deacon]

On Thu, Oct 29, 2015 at 06:42:10PM +, Robin Murphy wrote:
> On 29/10/15 18:28, Will Deacon wrote:
> >On Tue, Oct 27, 2015 at 10:00:11AM -0600, Alex Williamson wrote:
> >>On Tue, 2015-10-27 at 15:40 +, Will Deacon wrote:
> >>>On Fri, Oct 16, 2015 at 09:51:22AM -0600, Alex Williamson wrote:
> Would it be possible to add iommu_domain_geometry support to arm-smmu.c?
> In addition to this test to verify that DMA cannot bypass the IOMMU, I'd
> eventually like to pass the aperture information out through the VFIO
> API.  Thanks,
> >>>
> >>>The slight snag here is that we don't know which SMMU in the system the
> >>>domain is attached to at the point when the geometry is queried, so I
> >>>can't give you an upper bound on the aperture. For example, if there is
> >>>an SMMU with a 32-bit input address and another with a 48-bit input
> >>>address.
> >>>
> >>>We could play the same horrible games that we do with the pgsize bitmap,
> >>>and truncate some global aperture everytime we probe an SMMU device, but
> >>>I'd really like to have fewer hacks like that if possible. The root of
> >>>the problem is still that domains are allocated for a bus, rather than
> >>>an IOMMU instance.
> >>
> >>Yes, Intel VT-d has this issue as well.  In theory we can have
> >>heterogeneous IOMMU hardware units (DRHDs) in a system and the upper
> >>bound of the geometry could be diminished if we add a less capable DRHD
> >>into the domain.  I suspect this is more a theoretical problem than a
> >>practical one though as we're typically mixing similar DRHDs and I think
> >>we're still capable of 39-bit addressing in the least capable version
> >>per the spec.
> >>
> >>In any case, I really want to start testing geometry.force_aperture,
> >>even if we're not yet comfortable to expose the IOMMU limits to the
> >>user.  The vfio type1 shouldn't be enabled at all for underlying
> >>hardware that allows DMA bypass.  Thanks,
> >
> >Ok, I'll put it on my list of things to look at under the assumption that
> >the actual aperture limits don't need to be accurate as long as DMA to
> >an arbitrary unmapped address always faults.
> 
> I'm pretty sure we'd only ever set the aperture to the full input address
> range anyway (since we're not a GART-type thing), in which case we should
> only need to worry about unmatched streams that don't hit in a domain at
> all. Doesn't the disable_bypass option already cover that? (FWIW I hacked it
> up for v2 a while back, too[0]).

Well, the "full input address range" is tricky when you have multiple SMMU
instances with different input address sizes. I can do something similar
to the pgsize_bitmap.

I also don't think the disable_bypass option is what we're after -- this
is about devices attached to a VFIO domain that can still mysteriously
bypass the SMMU for some ranges AFAIU (and shouldn't be an issue for ARM).

Will
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 8/8] iommu: Move default domain allocation to iommu_group_get_for_dev()

[Will Deacon]

Hi Joerg,

Looking at this from an SMMU perspective, I think there's an issue
with attaching to the default domain like this.

On Wed, Oct 21, 2015 at 11:51:43PM +0200, Joerg Roedel wrote:
> From: Joerg Roedel 
> 
> Now that the iommu core support for iommu groups is not
> pci-centric anymore, we can move default domain allocation
> to the bus independent iommu_group_get_for_dev() function.
> 
> Signed-off-by: Joerg Roedel 
> ---
>  drivers/iommu/iommu.c | 18 ++
>  1 file changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index e2b5526..abae363 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -810,14 +810,6 @@ struct iommu_group *pci_device_group(struct device *dev)
>   if (IS_ERR(group))
>   return NULL;
>  
> - /*
> -  * Try to allocate a default domain - needs support from the
> -  * IOMMU driver.
> -  */
> - group->default_domain = __iommu_domain_alloc(pdev->dev.bus,
> -  IOMMU_DOMAIN_DMA);
> - group->domain = group->default_domain;
> -
>   return group;
>  }
>  
> @@ -849,6 +841,16 @@ struct iommu_group *iommu_group_get_for_dev(struct 
> device *dev)
>   if (IS_ERR(group))
>   return group;
>  
> + /*
> +  * Try to allocate a default domain - needs support from the
> +  * IOMMU driver.
> +  */
> + if (!group->default_domain) {
> + group->default_domain = __iommu_domain_alloc(dev->bus,
> +  IOMMU_DOMAIN_DMA);
> + group->domain = group->default_domain;
> + }
> +
>   ret = iommu_group_add_device(group, dev);
>   if (ret) {
>   iommu_group_put(group);

The call to iommu_group_get_for_dev in arm_smmu_add_device will end up
calling __iommu_attach_device, since group->domain will now be initialised
by the code above. This means the SMMU driver will see an ->attach_dev
call for a device that is part-way through an ->add_device callback and
will be missing the initialisation necessary for us to idenfity the SMMU
instance to which is corresponds. In fact, the iommudata for the group
won't be initialised at all, so the whole thing will fail afaict.

Note that I haven't actually taken this for a spin, so I could be missing
something.

Will
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v6 1/3] iommu: Implement common IOMMU ops for DMA mapping

[Daniel Kurtz]

+linux-media & VIDEOBUF2 FRAMEWORK maintainers since this is about the
v4l2-contig's usage of the DMA API.

Hi Robin,

On Tue, Oct 27, 2015 at 12:55 AM, Robin Murphy  wrote:
> On 26/10/15 13:44, Yong Wu wrote:
>>
>> On Thu, 2015-10-01 at 20:13 +0100, Robin Murphy wrote:
>> [...]
>>>
>>> +/*
>>> + * The DMA API client is passing in a scatterlist which could describe
>>> + * any old buffer layout, but the IOMMU API requires everything to be
>>> + * aligned to IOMMU pages. Hence the need for this complicated bit of
>>> + * impedance-matching, to be able to hand off a suitably-aligned list,
>>> + * but still preserve the original offsets and sizes for the caller.
>>> + */
>>> +int iommu_dma_map_sg(struct device *dev, struct scatterlist *sg,
>>> +   int nents, int prot)
>>> +{
>>> +   struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
>>> +   struct iova_domain *iovad = domain->iova_cookie;
>>> +   struct iova *iova;
>>> +   struct scatterlist *s, *prev = NULL;
>>> +   dma_addr_t dma_addr;
>>> +   size_t iova_len = 0;
>>> +   int i;
>>> +
>>> +   /*
>>> +* Work out how much IOVA space we need, and align the segments
>>> to
>>> +* IOVA granules for the IOMMU driver to handle. With some clever
>>> +* trickery we can modify the list in-place, but reversibly, by
>>> +* hiding the original data in the as-yet-unused DMA fields.
>>> +*/
>>> +   for_each_sg(sg, s, nents, i) {
>>> +   size_t s_offset = iova_offset(iovad, s->offset);
>>> +   size_t s_length = s->length;
>>> +
>>> +   sg_dma_address(s) = s->offset;
>>> +   sg_dma_len(s) = s_length;
>>> +   s->offset -= s_offset;
>>> +   s_length = iova_align(iovad, s_length + s_offset);
>>> +   s->length = s_length;
>>> +
>>> +   /*
>>> +* The simple way to avoid the rare case of a segment
>>> +* crossing the boundary mask is to pad the previous one
>>> +* to end at a naturally-aligned IOVA for this one's
>>> size,
>>> +* at the cost of potentially over-allocating a little.
>>> +*/
>>> +   if (prev) {
>>> +   size_t pad_len = roundup_pow_of_two(s_length);
>>> +
>>> +   pad_len = (pad_len - iova_len) & (pad_len - 1);
>>> +   prev->length += pad_len;
>>
>>
>> Hi Robin,
>>While our v4l2 testing, It seems that we met a problem here.
>>Here we update prev->length again, Do we need update
>> sg_dma_len(prev) again too?
>>
>>Some function like vb2_dc_get_contiguous_size[1] always get
>> sg_dma_len(s) to compare instead of s->length. so it may break
>> unexpectedly while sg_dma_len(s) is not same with s->length.
>
>
> This is just tweaking the faked-up length that we hand off to iommu_map_sg()
> (see also the iova_align() above), to trick it into bumping this segment up
> to a suitable starting IOVA. The real length at this point is stashed in
> sg_dma_len(s), and will be copied back into s->length in __finalise_sg(), so
> both will hold the same true length once we return to the caller.
>
> Yes, it does mean that if you have a list where the segment lengths are page
> aligned but not monotonically decreasing, e.g. {64k, 16k, 64k}, then you'll
> still end up with a gap between the second and third segments, but that's
> fine because the DMA API offers no guarantees about what the resulting DMA
> addresses will be (consider the no-IOMMU case where they would each just be
> "mapped" to their physical address). If that breaks v4l, then it's probably
> v4l's DMA API use that needs looking at (again).

Hmm, I thought the DMA API maps a (possibly) non-contiguous set of
memory pages into a contiguous block in device memory address space.
This would allow passing a dma mapped buffer to device dma using just
a device address and length.
IIUC, the change above breaks this model by inserting gaps in how the
buffer is mapped to device memory, such that the buffer is no longer
contiguous in dma address space.

Here is the code in question from
drivers/media/v4l2-core/videobuf2-dma-contig.c :

static unsigned long vb2_dc_get_contiguous_size(struct sg_table *sgt)
{
struct scatterlist *s;
dma_addr_t expected = sg_dma_address(sgt->sgl);
unsigned int i;
unsigned long size = 0;

for_each_sg(sgt->sgl, s, sgt->nents, i) {
if (sg_dma_address(s) != expected)
break;
expected = sg_dma_address(s) + sg_dma_len(s);
size += sg_dma_len(s);
}
return size;
}


static void *vb2_dc_get_userptr(void *alloc_ctx, unsigned long vaddr,
unsigned long size, enum dma_data_direction dma_dir)
{
struct vb2_dc_conf *conf = alloc_ctx;
struct vb2_dc_buf *buf;
struct frame_vector *vec;