Re: [PATCH v3 1/8] domain: GADDR based shared guest area registration alternative - teardown

[Jan Beulich <jbeulich@xxxxxxxx>]

On 27.09.2023 12:50, Roger Pau Monné wrote:
> On Wed, Sep 27, 2023 at 12:46:07PM +0200, Jan Beulich wrote:
>> On 27.09.2023 12:42, Roger Pau Monné wrote:
>>> On Wed, Sep 27, 2023 at 11:55:19AM +0200, Jan Beulich wrote:
>>>> On 27.09.2023 10:51, Roger Pau Monné wrote:
>>>>> On Wed, May 03, 2023 at 05:54:47PM +0200, Jan Beulich wrote:
>>>>>> +{
>>>>>> +    struct domain *d = v->domain;
>>>>>> +
>>>>>> +    if ( v != current )
>>>>>> +        ASSERT(atomic_read(&v->pause_count) | 
>>>>>> atomic_read(&d->pause_count));
>>>>>
>>>>> Isn't this racy?
>>>>
>>>> It is, yes.
>>>>
>>>>>  What guarantees that the vcpu won't be kicked just
>>>>> after the check has been performed?
>>>>
>>>> Nothing. This check isn't any better than assertions towards an ordinary
>>>> spinlock being held. I assume you realize that we've got a number of such
>>>> assertions elsewhere already.
>>>
>>> Right, but different from spinlock assertions, the code here could be
>>> made safe just by pausing the vCPU?
>>
>> That's what the assertion is checking (see also the comment ahead of the
>> function). It's just that the assertions cannot be made more strict, at
>> least from all I can tell.
> 
> But the assertion might no longer be true by the time the code
> afterwards is executed.  Why not wrap the code in a pair of
> vcpu_{,un}pause() calls?

Because it's not quite as simple (if I was to do so, I'd want to do it
correctly, and not just give the impression of universal usability). See
how map_guest_area() involves hypercall_deadlock_mutex. Hence I continue
to think it is okay the way I have it, with all callers satisfying the
requirement (afaict).

Jan