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

[Jan Beulich <jbeulich@xxxxxxxx>]

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