[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH] x86/HVM: relinquish resources also from hvm_domain_destroy()
On 28.01.2020 15:14, Durrant, Paul wrote: >> -----Original Message----- >> From: Xen-devel <xen-devel-bounces@xxxxxxxxxxxxxxxxxxxx> On Behalf Of Jan >> Beulich >> Sent: 28 January 2020 13:17 >> >> --- a/xen/arch/x86/hvm/hpet.c >> +++ b/xen/arch/x86/hvm/hpet.c >> @@ -751,7 +751,7 @@ void hpet_deinit(struct domain *d) >> int i; >> HPETState *h = domain_vhpet(d); >> >> - if ( !has_vhpet(d) ) >> + if ( !has_vhpet(d) || !d->arch.hvm.pl_time || !h->stime_freq ) > > Why the extra checks here? Just to avoid taking the write_lock() > before init? If so, then wouldn't a check of stime_freq alone suffice? This and the other functions may now be called with d->arch.hvm.pl_time being NULL. domain_vhpet() would yield a pointer slightly offset from NULL in this case. Hence we have to do this check before we may deref h. >> --- a/xen/arch/x86/hvm/hvm.c >> +++ b/xen/arch/x86/hvm/hvm.c >> @@ -696,24 +696,24 @@ int hvm_domain_initialise(struct domain >> return 0; >> >> fail2: >> - rtc_deinit(d); > > I understand that this removal is done because > hvm_domain_relinquish_resources() will now deal with it, > but I notice it is also called from hvm_domain_destroy(), > which seems superfluous. Oh, indeed, that one could go away as well now. >> stdvga_deinit(d); >> vioapic_deinit(d); >> fail1: >> if ( is_hardware_domain(d) ) >> xfree(d->arch.hvm.io_bitmap); >> - xfree(d->arch.hvm.io_handler); >> - xfree(d->arch.hvm.params); >> - xfree(d->arch.hvm.pl_time); >> - xfree(d->arch.hvm.irq); >> + XFREE(d->arch.hvm.io_handler); >> + XFREE(d->arch.hvm.params); >> + XFREE(d->arch.hvm.pl_time); >> + XFREE(d->arch.hvm.irq); > > Should these XFREEs not now be removed from hvm_domain_destroy() too? I'm afraid I don't understand: This is in hvm_domain_initialise(). arch_domain_destroy() (and hence hvm_domain_destroy()) won't get called if hvm_domain_initialise() (and hence arch_domain_destroy()) fails (doing so is, I think, a future plan of Andrew's). >> --- a/xen/arch/x86/hvm/pmtimer.c >> +++ b/xen/arch/x86/hvm/pmtimer.c >> @@ -373,7 +373,7 @@ void pmtimer_deinit(struct domain *d) >> { >> PMTState *s = &d->arch.hvm.pl_time->vpmt; >> >> - if ( !has_vpm(d) ) >> + if ( !has_vpm(d) || !d->arch.hvm.pl_time || !s->vcpu ) >> return; > > Isn't a test of s->vcpu sufficient? No, for the same reason a that explained for hpet.c. >> --- a/xen/arch/x86/hvm/rtc.c >> +++ b/xen/arch/x86/hvm/rtc.c >> @@ -844,7 +844,8 @@ void rtc_deinit(struct domain *d) >> { >> RTCState *s = domain_vrtc(d); >> >> - if ( !has_vrtc(d) ) >> + if ( !has_vrtc(d) || !d->arch.hvm.pl_time || >> + s->update_timer.status == TIMER_STATUS_invalid ) >> return; > > Testing s->pt.source for a zero value would be sufficient and cheaper, I > think. It's not obvious to me where in rtc_init() s->pt.source would get set to a non-zero value. I'd prefer the check here to be obviously connected to what rtc_init() does. I'm also unclear why you think it would be cheaper. Jan _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxxxxxxxxx https://lists.xenproject.org/mailman/listinfo/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |