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

[Baolu Lu]

On 2023/6/27 16:15, Tian, Kevin wrote:
> > From: Baolu Lu<baolu.lu@xxxxxxxxxxxxxxx>
> > Sent: Tuesday, June 27, 2023 4:01 PM
> >
> > On 2023/6/27 15:54, Tian, Kevin wrote:
> > > > From: Baolu Lu<baolu.lu@xxxxxxxxxxxxxxx>
> > > > Sent: Tuesday, June 13, 2023 11:15 AM
> > > >
> > > > On 6/12/23 4:28 PM, Liu, Jingqi wrote:
> > > > > On 6/7/2023 11:51 AM, Lu Baolu wrote:
> > > > > > -
> > > > > > -    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.
> > > You could also add the dma domain check here. pg_size is static
> > > then it makes more sense to verify it once instead of in a loop.
> > Agreed. Does below additional change make sense?
> >
> > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> > index e59de7852067..3be88b5f36bb 100644
> > --- a/drivers/iommu/iommu.c
> > +++ b/drivers/iommu/iommu.c
> > @@ -962,6 +962,9 @@ static int
> > iommu_create_device_direct_mappings(struct iommu_domain *domain,
> >           pg_size = domain->pgsize_bitmap ? 1UL <<
> > __ffs(domain->pgsize_bitmap) : 0;
> >           INIT_LIST_HEAD(&mappings);
> >
> > +       if (WARN_ON_ONCE((domain->type & __IOMMU_DOMAIN_PAGING)
> > &&
> > !pg_size))
> > +               return -EINVAL;
> what's the reason of not using iommu_is_dma_domain()? this is called
> in the probe path only for the default domain. Otherwise if you change
> like this then you also want to change the check in the loop later to be
> consistent.

Yes. iommu_is_dma_domain() is better.

Best regards,
baolu