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 <[email protected]> >> --- >> 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 <[email protected]> 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
