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

[Jan Beulich <jbeulich@xxxxxxxx>]

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