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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Julien,

> On 15 Dec 2020, at 11:31, Julien Grall <julien@xxxxxxx> wrote:
> 
> 
> 
> On 15/12/2020 10:51, Bertrand Marquis wrote:
>> Hi Julien,
> 
> Hi Bertrand,
> 
>>> On 15 Dec 2020, at 10:13, Julien Grall <julien@xxxxxxx> wrote:
>>> 
>>> 
>>> 
>>> On 15/12/2020 09:42, Bertrand Marquis wrote:
>>>> Hi Julien,
>>> 
>>> Hi,
>>> 
>>>>> On 14 Dec 2020, at 19:35, Julien Grall <julien@xxxxxxx> wrote:
>>>>> 
>>>>> 
>>>>> 
>>>>> 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.
>>>> 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.
>> I think he agreed on the analysis but he did not say he was going to do it.
>>> 
>>> 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.
>> Testing is important and effort evaluation also depends on other priorities 
>> we have.
>> There are 20 WARN_ON in Xen and most of them are in x86 code.
>> If we do this change, the serie will impact a lot more code then it 
>> originally did.
> 
> What's the problem?
> 
>> I am not saying it should not be done, I am saying it should not be done in 
>> this serie.
>> Such a change would need a serie upfront and then rebasing this serie on top 
>> of it to prevent mixing stuff to much.
> 
> It is trivial enough to be part of this series. But if you prefer to create a 
> separate series then so be 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.
>> I agree with that statement but we should be carefull not to ask to much to 
>> people who try to contribute so that they
>> do not feel like all changes asked are not too much to handle.
> 
> I am well aware of that and I don't think this request is asking a lot.
> 
>> I am open to create new tasks on our side for the future when things to be 
>> improved like this ones are revealed by a
>> serie.
>> If this is a blocker from your point of view, we will evaluate the effort to 
>> do this extra work and the serie will wait until
>> january to be pushed again.
> This sounds like it would require more effort than it is actually necessary. 
> In fact...
> 
> ... it took me one minute to check the existing use of WARN_ON() (all of them 
> don't care about the return so far), another 2 minutes to write it,  an extra 
> 5 minutes to test it and 2 minutes to write the commit message.
> 
> So a grand total of 10 minutes.

Sadly what takes 10 minutes on your side is not taking the same effort on our 
side for now (due to internal review, validation and required tests).

> 
> Anyway, please see the patch [1].

Thanks for that.
We will review it and rebase the SMMU serie on top of it.

Cheers
Bertrand

> 
> Cheers,
> 
> [1] 
> https://lore.kernel.org/xen-devel/20201215112610.1986-1-julien@xxxxxxx/T/#u
> 
> -- 
> Julien Grall