Re: [PATCH RFC 03/10] domain: GADDR based shared guest area registration alternative - teardown

[Jan Beulich <jbeulich@xxxxxxxx>]

On 13.12.2022 22:44, Julien Grall wrote:
> On 19/10/2022 08:40, Jan Beulich wrote:
>> In preparation of the introduction of new vCPU operations allowing to
>> register the respective areas (one of the two is x86-specific) by
>> guest-physical address, add the necessary domain cleanup hooks.
>>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>> ---
>> RFC: Zapping the areas in pv_shim_shutdown() may not be strictly
>>       necessary: Aiui unmap_vcpu_info() is called only because the vCPU
>>       info area cannot be re-registered. Beyond that I guess the
>>       assumption is that the areas would only be re-registered as they
>>       were before. If that's not the case I wonder whether the guest
>>       handles for both areas shouldn't also be zapped.
> 
> I don't know the code enough to be able to answer it.

Right; I hope the original shim authors to be able to shed some light
on this.

> The code itself looks good to me. With one remark below:
> 
> Reviewed-by: Julien Grall <jgrall@xxxxxxxxxx>

Thanks.

>> @@ -1555,6 +1559,15 @@ 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)
>> +{
> 
> IIUC, you will add the ASSERT() we discussed in patch #7 in this patch. 
> I would be fine if you keep my reviewed-by.

And thanks again. Indeed this is what I have pending for v2:

/*
 * 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)
{
    if ( v != current )
        ASSERT(atomic_read(&v->pause_count) | atomic_read(&d->pause_count));
}

Jan