Re: [PATCH v3 7/8] xen/arm: Add support for SMMUv3 driver

[Julien Grall <julien@xxxxxxx>]

On 14/12/2020 19:08, Rahul Singh wrote:
> Hello Julien,
Hi Rahul,

> > On 11 Dec 2020, at 2:25 pm, Julien Grall <julien@xxxxxxx> wrote:
> >
> > Hi Rahul,
> >
> > On 10/12/2020 16:57, Rahul Singh wrote:
> > >   struct arm_smmu_strtab_cfg {
> > > @@ -613,8 +847,13 @@ struct arm_smmu_device {
> > >                 u64                     padding;
> > >         };
> > >   -     /* IOMMU core code handle */
> > > -       struct iommu_device             iommu;
> > > +       /* Need to keep a list of SMMU devices */
> > > +       struct list_head                devices;
> > > +
> > > +       /* Tasklets for handling evts/faults and pci page request IRQs*/
> > > +       struct tasklet          evtq_irq_tasklet;
> > > +       struct tasklet          priq_irq_tasklet;
> > > +       struct tasklet          combined_irq_tasklet;
> > >   };
> > >     /* SMMU private data for each master */
> > > @@ -638,7 +877,6 @@ enum arm_smmu_domain_stage {
> > >     struct arm_smmu_domain {
> > >         struct arm_smmu_device          *smmu;
> > > -       struct mutex                    init_mutex; /* Protects smmu pointer */
> > Hmmm... Your commit message says the mutex would be replaced by a spinlock. 
> > However, you are dropping the lock. What I did miss?
> Linux code using the mutex in the function arm_smmu_attach_dev() but in XEN 
> this function is called from arm_smmu_assign_dev() which already has the 
> spin_lock when arm_smmu_attach_dev() function I called so I drop the mutex to 
> avoid nested spinlock.
> Timing analysis of using spin lock in place of mutex as compared to linux  when 
> attaching a  device to SMMU is still valid.
I think it would be better to keep the current locking until the 
investigation is done.
But if you still want to make this change, then you should explain in 
the commit message why the lock is dropped.
[...]

> WARN_ON(q->base_dma & (qsz - 1));
> if (!unlikely(q->base_dma & (qsz - 1))) {
>         dev_info(smmu->dev, "allocated %u entries for %s\n",
>                 1 << q->llq.max_n_shift, name);
> }
Right, but this doesn't address the second part of my comment.

This change would *not* be necessary if the implementation of WARN_ON() 
in Xen return whether the warn was triggered.
Before considering to change the SMMU code, you should first attempt to 
modify implementation of the WARN_ON(). We can discuss other approach if 
the discussion goes nowhere.
Cheers,

--
Julien Grall