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

[Jan Beulich <jbeulich@xxxxxxxx>]

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:
>>> I guess we should also zap the runstate handlers, or else we might
>>> corrupt guest state.
>>
>> So you think the guest might re-register a different area post resume?
>> I can certainly add another patch to zap the handles; I don't think it
>> should be done right here, not the least because if we see room for
>> such behavior, that change may even want backporting.
> 
> For correctness it would be good to zap them, but there's no rush, as
> I do think guests will set to the same address on resume.

Well, I've already added a new patch coming ahead of this one.

>>>> @@ -1568,6 +1572,19 @@ void unmap_vcpu_info(struct vcpu *v)
>>>>      put_page_and_type(mfn_to_page(mfn));
>>>>  }
>>>>  
>>>> +/*
>>>> + * This is only intended to be used for domain cleanup (or more generally 
>>>> only
>>>> + * with at least the respective vCPU, if it's not the current one, 
>>>> reliably
>>>> + * paused).
>>>> + */
>>>> +void unmap_guest_area(struct vcpu *v, struct guest_area *area)
>>>
>>> vcpu param could be const given the current code, but I assume this
>>> will be changed by future patches.  Same could be said about
>>> guest_area but I'm confident that will change.
>>
>> True for both.
>>
>>>> +{
>>>> +    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.

Jan