Re: [Xen-devel] [PATCH] x86/HVM: relinquish resources also from hvm_domain_destroy()

["Durrant, Paul" <pdurrant@xxxxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: 28 January 2020 14:31
> To: Durrant, Paul <pdurrant@xxxxxxxxxxxx>
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; Andrew Cooper
> <andrew.cooper3@xxxxxxxxxx>; Paul Durrant <paul@xxxxxxx>; Wei Liu
> <wl@xxxxxxx>; Roger Pau Monné <roger.pau@xxxxxxxxxx>
> Subject: Re: [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.
> 

Ah, I'd missed that domain_vhpet() dereferenced pl_time.

> >> --- 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).
> 

Oh, sorry. For some reason I thought this hunk was in 
hvm_domain_relinquish_resources() so yes the XFREEs in destroy need to stay as 
is.

> >> --- 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.

Ok. I'd assume a non-zero rather than equality test would be cheaper but I 
notice that TIMER_STATUS_invalid is defined to 0 anyway, so it should be 
optimised at compile time.

  Paul

> 
> Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel