Re: [PATCH v3 8/8] common: convert vCPU info area registration

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Sep 28, 2023 at 12:35:23PM +0200, Jan Beulich wrote:
> On 28.09.2023 12:15, Roger Pau Monné wrote:
> > On Thu, Sep 28, 2023 at 11:53:36AM +0200, Jan Beulich wrote:
> >> On 28.09.2023 10:24, Roger Pau Monné wrote:
> >>> On Wed, May 03, 2023 at 05:58:30PM +0200, Jan Beulich wrote:
> >>>> @@ -1633,14 +1528,44 @@ int map_guest_area(struct vcpu *v, paddr
> >>>> +    {
> >>>> +        if ( map )
> >>>> +            populate(map, v);
> >>>>  
> >>>> -    SWAP(area->pg, pg);
> >>>> -    SWAP(area->map, map);
> >>>> +        SWAP(area->pg, pg);
> >>>> +        SWAP(area->map, map);
> >>>> +    }
> >>>> +    else
> >>>> +        rc = -EBUSY;
> >>>>  
> >>>>      domain_unlock(d);
> >>>>  
> >>>> +    /* Set pending flags /after/ new vcpu_info pointer was set. */
> >>>> +    if ( area == &v->vcpu_info_area && !rc )
> >>>> +    {
> >>>> +        /*
> >>>> +         * Mark everything as being pending just to make sure nothing 
> >>>> gets
> >>>> +         * lost.  The domain will get a spurious event, but it can cope.
> >>>> +         */
> >>>> +#ifdef CONFIG_COMPAT
> >>>> +        if ( !has_32bit_shinfo(d) )
> >>>> +        {
> >>>> +            vcpu_info_t *info = area->map;
> >>>> +
> >>>> +            /* For VCPUOP_register_vcpu_info handling in 
> >>>> common_vcpu_op(). */
> >>>> +            BUILD_BUG_ON(sizeof(*info) != sizeof(info->compat));
> >>>> +            write_atomic(&info->native.evtchn_pending_sel, ~0);
> >>>> +        }
> >>>> +        else
> >>>> +#endif
> >>>> +            write_atomic(&vcpu_info(v, evtchn_pending_sel), ~0);
> >>>
> >>> Can't the setting of evtchn_pending_sel be done in
> >>> vcpu_info_populate()?
> >>
> >> No, see the comment ahead of the outer if(). populate() needs calling
> >> ahead of updating the pointer.
> > 
> > I'm afraid I don't obviously see why evtchn_pending_sel can't be set
> > before v->vcpu_info is updated.  It will end up being ~0 anyway,
> > regardless of the order of operations, and we do force an event
> > channel injection.  There's something I'm clearly missing.
> 
> Considering the target vCPU is paused (or is current), doing so may be
> possible (albeit I fear I'm overlooking something). But re-ordering of
> operations shouldn't be done as a side effect of this change; if the
> original ordering constraints were too strict, then imo relaxing should
> either be a separate earlier change, or a separate follow-on one.

Let's do a followup then.  This will also need an RB instead of an
Ack:

Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Thanks, Roger.