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

[Julien Grall <julien@xxxxxxx>]

On 07/12/2020 18:42, Rahul Singh wrote:
> Hello Julien,
Hi,

> > On 7 Dec 2020, at 5:39 pm, Julien Grall <julien@xxxxxxx> 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?

Cheers,

--
Julien Grall