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
>>>>
>>>> domain_lock(d);
>>>>
>>>> - if ( map )
>>>> - populate(map, v);
>>>> + /* No re-registration of the vCPU info area. */
>>>> + if ( area != &v->vcpu_info_area || !area->pg )
>>>
>>> It would be nice if this check could be done earlier, as to avoid
>>> having to fetch and map the page just to discard it. That would
>>> however require taking the domain lock earlier.
>>
>> In order to kind of balance the conflicting goals, there is an unlocked
>> early check in the caller. See also the related RFC remark; I might
>> interpret your remark as "yes, keep the early check".
>
> Oh, yes, do keep the check.
>
>>>> + {
>>>> + 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.
Jan
