Re: [PATCH 04/12] evtchn: evtchn_set_priority() needs to acquire the per-channel lock

[Julien Grall <julien@xxxxxxx>]

Hi Jan,

On 28/09/2020 11:57, Jan Beulich wrote:
> evtchn_fifo_set_pending() (invoked with the per-channel lock held) has
> two uses of the channel's priority field. The field gets updated by
> evtchn_fifo_set_priority() with only the per-domain event_lock held,
> i.e. the two reads may observe two different values. While the 2nd use
> could - afaict - in principle be replaced by q->priority, I think
> evtchn_set_priority() should acquire the per-channel lock in any event.
>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>
> --- a/xen/common/event_channel.c
> +++ b/xen/common/event_channel.c
> @@ -1132,7 +1132,9 @@ static long evtchn_set_priority(const st
>   {
>       struct domain *d = current->domain;
>       unsigned int port = set_priority->port;
> +    struct evtchn *chn;
>       long ret;
> +    unsigned long flags;
>   
>       spin_lock(&d->event_lock);
Is it still necessary to hold d->event_lock?

>   
> @@ -1142,8 +1144,10 @@ static long evtchn_set_priority(const st
>           return -EINVAL;
>       }
>   
> -    ret = evtchn_port_set_priority(d, evtchn_from_port(d, port),
> -                                   set_priority->priority);
> +    chn = evtchn_from_port(d, port);
> +    spin_lock_irqsave(&chn->lock, flags);
> +    ret = evtchn_port_set_priority(d, chn, set_priority->priority);
> +    spin_unlock_irqrestore(&chn->lock, flags);
>   
>       spin_unlock(&d->event_lock);
>   
Cheers,

--
Julien Grall