On 15/12/2020 09:42, Bertrand Marquis wrote:
Hi Julien,
Hi,
On 14 Dec 2020, at 19:35, Julien Grall <jul...@xen.org> wrote:
On 14/12/2020 19:08, Rahul Singh wrote:
Hello Julien,
Hi Rahul,
On 11 Dec 2020, at 2:25 pm, Julien Grall <jul...@xen.org> 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.
The code proposed by Rahul is providing the equivalent functionality to what
linux does.
Modifying WARN_ON implementation in Xen to fit how Linux version is working
would make sense but should be done in its own patch as it will imply
modification on more Xen code and
some of it will not be related to SMMU and will need some validation.
Let me start that this was a request I already made on v2 and Rahul
agreed. I saw no pushback on the request until now. So to me this meant
this would be addressed in v3.
Further, the validation seems to be a common argument everytime I ask to
make a change in this series... Yes validation is important, but this
often doesn't require a lot of effort when the changes are simple...
TBH, you are probably spending more time arguing against it.
So I do not think it would be fare to ask Rahul to also do this in the scope of
this serie
I would have agreed with this statement if the change is difficult. This
is not the case here.
The first step when working upstream should always to improve existing
helpers rather than working around it.
If it is not possible because it is either too complex or there are push
back from the maintainers. Then we can discuss about workaround.
Cheers,
--
Julien Grall
