Re: [PATCH v3 5/8] iommu/vt-d: Implement def_domain_type iommu ops entry

[Lu Baolu]

Hi,

On 5/6/19 11:25 PM, Tom Murphy wrote:
> It looks like there is a bug in this code.
>
> The behavior before this patch in __intel_map_single was that
> iommu_no_mapping would call remove the attached si_domain for 32 bit
> devices  (in the  dmar_remove_one_dev_info(dev) call in
> iommu_no_mapping) and then allocate a new domain in
> get_valid_domain_for_dev
> old:
> if (iommu_no_mapping(dev))
>     return paddr;
> domain = get_valid_domain_for_dev(dev);
> if (!domain)
>     return DMA_MAPPING_ERROR;
>
> but in the new code we remove the attached si_domain but we WON'T
> allocate a new domain and instead just return an error when we call
> find_domain
> new:
>          if (iommu_no_mapping(dev))
>                  return paddr;
>
>          domain = find_domain(dev);
>          if (!domain)
>                  return DMA_MAPPING_ERROR;
>
> This is a bug, right?

When we use the old lazy creation of iommu domain, we can change the
domain for a 32bit device from identity to dma by pulling it out of the
si_domain and allocating a new one for it.

When we switch to default domain in iommu generic layer, we can't do
this anymore. The logic in above code is if we find this case (32bit
device using an identity domain), we simple return error for dma api
and warn the user "hey, this is a 32bit device, don't use the default
pass-through mode".

I believe there should be better solutions, for example, how about
letting pci core to call iommu_request_dma_map_for_dev() when it
finds a 32bit device.

Best regards,
Lu Baolu

> On Tue, Apr 30, 2019 at 3:18 AM Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx> wrote:
> > Hi Christoph,
> >
> > On 4/30/19 4:03 AM, Christoph Hellwig wrote:
> > > > @@ -3631,35 +3607,30 @@ static int iommu_no_mapping(struct device *dev)
> > > >       if (iommu_dummy(dev))
> > > >               return 1;
> > > >
> > > > -    if (!iommu_identity_mapping)
> > > > -            return 0;
> > > > -
> > >
> > > FYI, iommu_no_mapping has been refactored in for-next:
> > >
> > > https://git.kernel.org/pub/scm/linux/kernel/git/joro/iommu.git/commit/?h=x86/vt-d&id=48b2c937ea37a3bece0094b46450ed5267525289
> >
> > Oh, yes! Thanks for letting me know this. Will rebase the code.
> >
> > > >       found = identity_mapping(dev);
> > > >       if (found) {
> > > > +            /*
> > > > +             * If the device's dma_mask is less than the system's memory
> > > > +             * size then this is not a candidate for identity mapping.
> > > > +             */
> > > > +            u64 dma_mask = *dev->dma_mask;
> > > > +
> > > > +            if (dev->coherent_dma_mask &&
> > > > +                dev->coherent_dma_mask < dma_mask)
> > > > +                    dma_mask = dev->coherent_dma_mask;
> > > > +
> > > > +            if (dma_mask < dma_get_required_mask(dev)) {
> > >
> > > I know this is mostly existing code moved around, but it really needs
> > > some fixing.  For one dma_get_required_mask is supposed to return the
> > > required to not bounce mask for the given device.  E.g. for a device
> > > behind an iommu it should always just return 32-bit.  If you really
> > > want to check vs system memory please call dma_direct_get_required_mask
> > > without the dma_ops indirection.
> > >
> > > Second I don't even think we need to check the coherent_dma_mask,
> > > dma_direct is pretty good at always finding memory even without
> > > an iommu.
> > >
> > > Third this doesn't take take the bus_dma_mask into account.
> > >
> > > This probably should just be:
> > >
> > >                if (min(*dev->dma_mask, dev->bus_dma_mask) <
> > >                    dma_direct_get_required_mask(dev)) {
> >
> > Agreed and will add this in the next version.
> >
> > Best regards,
> > Lu Baolu