[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v3 1/8] domain: GADDR based shared guest area registration alternative - teardown
On 27.09.2023 10:51, Roger Pau Monné wrote: > On Wed, May 03, 2023 at 05:54:47PM +0200, 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> >> Reviewed-by: Julien Grall <jgrall@xxxxxxxxxx> >> --- >> 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. > > IIRC the reason was to unset v->vcpu_info_mfn so that it could be set > again on resume from suspension, or else the hypercall will return an > error. Right, that's the re-registration aspect mentioned. > 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. >> @@ -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. Jan
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |