Re: [PATCH 1/3] iommu: Add Visconti5 IOMMU driver

[Jason Gunthorpe]

On Mon, Jun 20, 2022 at 05:49:13AM +0000, nobuhiro1.iwamatsu@xxxxxxxxxxxxx wrote:
> Hi,
> 
> Thanks for your review.
> 
> > -----Original Message-----
> > From: Jason Gunthorpe <jgg@xxxxxxxxxx>
> > Sent: Thursday, May 26, 2022 3:27 AM
> > To: Baolu Lu <baolu.lu@xxxxxxxxxxxxxxx>
> > Cc: iwamatsu nobuhiro(岩松 信洋 □ＳＷＣ◯ＡＣＴ)
> > <nobuhiro1.iwamatsu@xxxxxxxxxxxxx>; Joerg Roedel <joro@xxxxxxxxxx>; Will
> > Deacon <will@xxxxxxxxxx>; Rob Herring <robh+dt@xxxxxxxxxx>;
> > devicetree@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> > iommu@xxxxxxxxxxxxxxxxxxxxxxxxxx; ishikawa yuji(石川 悠司 ○ＲＤＣ□ＡＩＴ
> > Ｃ○ＥＡ開) <yuji2.ishikawa@xxxxxxxxxxxxx>;
> > linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> > Subject: Re: [PATCH 1/3] iommu: Add Visconti5 IOMMU driver
> > 
> > On Wed, May 25, 2022 at 02:26:37PM +0800, Baolu Lu wrote:
> > > On 2022/5/25 09:31, Nobuhiro Iwamatsu wrote:
> > > > +static const struct iommu_ops visconti_atu_ops = {
> > > > +	.domain_alloc = visconti_atu_domain_alloc,
> > > > +	.probe_device = visconti_atu_probe_device,
> > > > +	.release_device = visconti_atu_release_device,
> > > > +	.device_group = generic_device_group,
> > > > +	.of_xlate = visconti_atu_of_xlate,
> > > > +	.pgsize_bitmap = ATU_IOMMU_PGSIZE_BITMAP,
> > > > +	.default_domain_ops = &(const struct iommu_domain_ops) {
> > > > +		.attach_dev = visconti_atu_attach_device,
> > > > +		.detach_dev = visconti_atu_detach_device,
> > >
> > > The detach_dev callback is about to be deprecated. The new drivers
> > > should implement the default domain and blocking domain instead.
> > 
> > Yes please, new drivers need to use default_domains.
> > 
> > It is very strange that visconti_atu_detach_device() does nothing.  It is not
> > required that a domain is fully unmapped before being destructed, I think
> > detach should set ATU_AT_EN to 0.
> 
> I see, I rethink implementation.
> 
> > 
> > What behavior does the HW have when ATU_AT_ENTRY_EN == 0? If DMA is
> > rejected then this driver should have a IOMMU_DOMAIN_BLOCKING and
> > return that from ops->def_domain_type().
> 
> If ATU_AT_ENTRY_EN is 0, nothing happens. It does not work with IOMMU,
> it works with the memory space set in device tree.

So that would be an assignment to DOMAIN_IDENTITY

> > I'm feeling like these "special" drivers need some kind of handshake with their
> > only users because they don't work with things like VFIO..
> 
> Since the devices that utilize this IOMMU function are fixed, I do
> not think that a special handshake is required.  Could you you tell
> me where you thought you needed a handshake?

In this case the iommu driver is so limited that it will not work with
VFIO - it is only ment to be used with the fixed drivers that are
paired with it.

Ideally we'd prevent VFIO from connecting and only allow drivers that
know the limitations of the IOMMU to use the unmanaged domain.

Jason