Re: [PATCH 1/2] iommu: Prevent RESV_DIRECT devices from blocking domains

[Baolu Lu]

On 6/12/23 4:28 PM, Liu, Jingqi wrote:
> On 6/7/2023 11:51 AM, Lu Baolu wrote:
> > The IOMMU_RESV_DIRECT flag indicates that a memory region must be mapped
> > 1:1 at all times. This means that the region must always be accessible to
> > the device, even if the device is attached to a blocking domain. This is
> > equal to saying that IOMMU_RESV_DIRECT flag prevents devices from being
> > attached to blocking domains.
> >
> > This also implies that devices that implement RESV_DIRECT regions will be
> > prevented from being assigned to user space since taking the DMA 
> > ownership
> > immediately switches to a blocking domain.
> >
> > The rule of preventing devices with the IOMMU_RESV_DIRECT regions from
> > being assigned to user space has existed in the Intel IOMMU driver for
> > a long time. Now, this rule is being lifted up to a general core rule,
> > as other architectures like AMD and ARM also have RMRR-like reserved
> > regions. This has been discussed in the community mailing list and refer
> > to below link for more details.
> >
> > Other places using unmanaged domains for kernel DMA must follow the
> > iommu_get_resv_regions() and setup IOMMU_RESV_DIRECT - we do not restrict
> > them in the core code.
> >
> > Cc: Robin Murphy <robin.murphy@xxxxxxx>
> > Cc: Alex Williamson <alex.williamson@xxxxxxxxxx>
> > Cc: Kevin Tian <kevin.tian@xxxxxxxxx>
> > Signed-off-by: Jason Gunthorpe <jgg@xxxxxxxxxx>
> > Link: 
> > https://lore.kernel.org/linux-iommu/BN9PR11MB5276E84229B5BD952D78E9598C639@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
> > Signed-off-by: Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx>
> > ---
> >   include/linux/iommu.h |  2 ++
> >   drivers/iommu/iommu.c | 39 +++++++++++++++++++++++++++++----------
> >   2 files changed, 31 insertions(+), 10 deletions(-)
> >
> > diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> > index d31642596675..fd18019ac951 100644
> > --- a/include/linux/iommu.h
> > +++ b/include/linux/iommu.h
> > @@ -409,6 +409,7 @@ struct iommu_fault_param {
> >    * @priv:     IOMMU Driver private data
> >    * @max_pasids:  number of PASIDs this device can consume
> >    * @attach_deferred: the dma domain attachment is deferred
> > + * @requires_direct: The driver requested IOMMU_RESV_DIRECT
> >    *
> >    * TODO: migrate other per device data pointers under 
> > iommu_dev_data, e.g.
> >    *    struct iommu_group    *iommu_group;
> > @@ -422,6 +423,7 @@ struct dev_iommu {
> >       void                *priv;
> >       u32                max_pasids;
> >       u32                attach_deferred:1;
> > +    u32                requires_direct:1;
> >   };
> >   int iommu_device_register(struct iommu_device *iommu,
> > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> > index 9e0228ef612b..e59de7852067 100644
> > --- a/drivers/iommu/iommu.c
> > +++ b/drivers/iommu/iommu.c
> > @@ -959,12 +959,7 @@ static int 
> > iommu_create_device_direct_mappings(struct iommu_domain *domain,
> >       unsigned long pg_size;
> >       int ret = 0;
> > -    if (!iommu_is_dma_domain(domain))
> > -        return 0;
> > -
> > -    BUG_ON(!domain->pgsize_bitmap);
> > -
> > -    pg_size = 1UL << __ffs(domain->pgsize_bitmap);
> > +    pg_size = domain->pgsize_bitmap ? 1UL << 
> > __ffs(domain->pgsize_bitmap) : 0;
> Would it be better to add the following check here?
>      if (WARN_ON(!pg_size))
>              return -EINVAL;
>
> Instead of checking latter in the loop as follows.
>      if (WARN_ON_ONCE(!pg_size)) {
>              ret = -EINVAL;
>              goto out;
>      }

I am afraid no. Only the paging domains need a valid pg_size. That's the
reason why I put it after the iommu_is_dma_domain() check. The previous
code has the same behavior too.

Best regards,
baolu