Hi, > On 9 Dec 2020, at 01:19, Stefano Stabellini <sstabell...@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 <jul...@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
