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....@citrix.com>
Thanks, Roger.
