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

[Bertrand Marquis <Bertrand.Marquis@arm.com>]

Hi,

> On 9 Dec 2020, at 01:19, Stefano Stabellini <sstabellini@kernel.org> wrote:
> 
> On Tue, 8 Dec 2020, Julien Grall wrote:
>> On 07/12/2020 18:42, Rahul Singh wrote:
>>>> On 7 Dec 2020, at 5:39 pm, Julien Grall <julien@xen.org> wrote:
>>>> On 07/12/2020 12:12, Rahul Singh wrote:
>>>>>>> +typedef paddr_t dma_addr_t;
>>>>>>> +typedef unsigned int gfp_t;
>>>>>>> +
>>>>>>> +#define platform_device device
>>>>>>> +
>>>>>>> +#define GFP_KERNEL 0
>>>>>>> +
>>>>>>> +/* Alias to Xen device tree helpers */
>>>>>>> +#define device_node dt_device_node
>>>>>>> +#define of_phandle_args dt_phandle_args
>>>>>>> +#define of_device_id dt_device_match
>>>>>>> +#define of_match_node dt_match_node
>>>>>>> +#define of_property_read_u32(np, pname, out)
>>>>>>> (!dt_property_read_u32(np, pname, out))
>>>>>>> +#define of_property_read_bool dt_property_read_bool
>>>>>>> +#define of_parse_phandle_with_args dt_parse_phandle_with_args
>>>>>>> +
>>>>>>> +/* Alias to Xen lock functions */
>>>>>>> +#define mutex spinlock
>>>>>>> +#define mutex_init spin_lock_init
>>>>>>> +#define mutex_lock spin_lock
>>>>>>> +#define mutex_unlock spin_unlock
>>>>>> 
>>>>>> Hmm... mutex are not spinlock. Can you explain why this is fine to
>>>>>> switch to spinlock?
>>>>> Yes mutex are not spinlock. As mutex is not implemented in XEN I thought
>>>>> of using spinlock in place of mutex as this is the only locking
>>>>> mechanism available in XEN.
>>>>> Let me know if there is another blocking lock available in XEN. I will
>>>>> check if we can use that.
>>>> 
>>>> There are no blocking lock available in Xen so far. However, if Linux were
>>>> using mutex instead of spinlock, then it likely means they operations in
>>>> the critical section can be long running.
>>> 
>>> Yes you are right Linux is using mutex when attaching a device to the SMMU
>>> as this operation might take longer time.
>>>> 
>>>> How did you came to the conclusion that using spinlock in the SMMU driver
>>>> would be fine?
>>> 
>>> Mutex is replaced by spinlock  in the SMMU driver when there is a request to
>>> assign a device to the guest. As we are in user context at that time its ok
>>> to use spinlock.
>> 
>> I am not sure to understand what you mean by "user context" here. Can you
>> clarify it?
>> 
>>> As per my understanding there is one scenario when CPU will spin when there
>>> is a request from the user at the same time to assign another device to the
>>> SMMU and I think that is very rare.
>> 
>> What "user" are you referring to?
>> 
>>> 
>>> Please suggest how we can proceed on this.
>> 
>> I am guessing that what you are saying is the request to assign/de-assign
>> device will be issued by the toolstack and therefore they should be trusted.
>> 
>> My concern here is not about someone waiting on the lock to be released. It is
>> more the fact that using a mutex() is an insight that the operation protected
>> can be long. Depending on the length, this may result to unwanted side effect
>> (e.g. other vCPU not scheduled, RCU stall in dom0, watchdog hit...).
>> 
>> I recall a discussion from a couple of years ago mentioning that STE
>> programming operations can take quite a long time. So I would like to
>> understand how long the operation is meant to last.
>> 
>> For a tech preview, this is probably OK to replace the mutex with an spinlock.
>> But I would not want this to go past the tech preview stage without a proper
>> analysis.
>> 
>> Stefano, what do you think?
> 
> In short, I agree.
> 
> 
> We need to be very careful replacing mutexes with spinlocks. We need to
> look closely at the ways the spinlocks could introduce unwanted
> latencies. Concurrent assign_device operations are possible but rare
> and, more importantly, they are user-driven so they could be mitigated.
> I am more worried about other possible scenarios, e.g. STE or other
> operations.
> 
> Rahul clearly put a lot of work into this series already and I think it
> is better to take this incrementally, which will allow us to do better
> testing and also move faster overall. So I am fine to take the series as
> is now, pending an investigation on the spinlocks later.

I also agree with the issue on the spinlock but we have no equivalent of something
looking like a mutex for now in Xen so this would require some major redesign and
will take us far from the linux driver.

I would suggest to add a comment before this part of the code with a “TODO” so that
it is clear inside the code.
We could also add some comment in Kconfig to mention this possible “faulty” behaviour.

Would you agree on going forward like this ?

Regards
Bertrand