Re: [PATCH v8 3/3] xen/events: do some cleanups in evtchn_fifo_set_pending()

[Julien Grall <julien@xxxxxxx>]

On 25/11/2020 10:51, Juergen Gross wrote:
> evtchn_fifo_set_pending() can be simplified a little bit.
The commit message is quite light... For posterity, it would be good to 
explain why the simplication can be done. In particular, there is a 
chance in behavior after this patch.
> Suggested-by: Jan Beulich <jbeulich@xxxxxxxx>
> Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> ---
> V8:
> - new patch
> ---
>   xen/common/event_fifo.c | 34 +++++++++++++++-------------------
>   1 file changed, 15 insertions(+), 19 deletions(-)
>
> diff --git a/xen/common/event_fifo.c b/xen/common/event_fifo.c
> index 443593c3b3..77609539b1 100644
> --- a/xen/common/event_fifo.c
> +++ b/xen/common/event_fifo.c
> @@ -175,6 +175,18 @@ static void evtchn_fifo_set_pending(struct vcpu *v, struct 
> evtchn *evtchn)
>           return;
>       }
>   
> +    /*
> +     * Control block not mapped.  The guest must not unmask an
> +     * event until the control block is initialized, so we can
> +     * just drop the event.
> +     */
> +    if ( unlikely(!v->evtchn_fifo->control_block) )
Sort of unrelated, AFAICT, v->evtchn_fifo->control_block can be set 
concurrently to this access.
Thankfully, once the control block is mapped, it can't be unmapped. 
However, there is still a possibility that you may see half of the update.
Shouldn't the field access with ACCESS_ONCE()?

Cheers,

--
Julien Grall