hvm_destroy_all_ioreq_servers(), called from
hvm_domain_relinquish_resources(), invokes relocate_portio_handler(),
which uses d->arch.hvm.io_handler. Defer freeing of this array
accordingly on the error path of hvm_domain_initialise().

Similarly rtc_deinit() requires d->arch.hvm.pl_time to still be around,
or else an armed timer structure would get freed, and that timer never
get killed.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
We may want to consider moving the other two XFREE()s later as well,
if only to be on the safe side.

For vRTC I question the calling of check_update_timer() from rtc_init():
I would consider it more reasonable to do so immediately before the
guest gets first launched, especially if a guest remains paused for a
while after creation.

--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -714,15 +714,15 @@ int hvm_domain_initialise(struct domain
  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);
  fail0:
     hvm_destroy_cacheattr_region_list(d);
     destroy_perdomain_mapping(d, PERDOMAIN_VIRT_START, 0);
  fail:
     hvm_domain_relinquish_resources(d);
+    XFREE(d->arch.hvm.io_handler);
+    XFREE(d->arch.hvm.pl_time);
     return rc;
 }
 

On 28/01/2021 14:40, Jan Beulich wrote:
> hvm_destroy_all_ioreq_servers(), called from
> hvm_domain_relinquish_resources(), invokes relocate_portio_handler(),
> which uses d->arch.hvm.io_handler. Defer freeing of this array
> accordingly on the error path of hvm_domain_initialise().
>
> Similarly rtc_deinit() requires d->arch.hvm.pl_time to still be around,
> or else an armed timer structure would get freed, and that timer never
> get killed.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

> ---
> We may want to consider moving the other two XFREE()s later as well,
> if only to be on the safe side.

Wherever possible, I want to move stuff like this into the idempotent
domain_teardown()/_domain_destroy() logic, although I suspect you want
this suitable for backport as well?

This won't be the last ordering bug we find.

Also, I suspect this one is mixed up in the complexity of
arch_domain_destroy() which needs aligning carefully between x86 and ARM
before it can be switched.

> For vRTC I question the calling of check_update_timer() from rtc_init():
> I would consider it more reasonable to do so immediately before the
> guest gets first launched, especially if a guest remains paused for a
> while after creation.

That does look suspect.  (And yes - it can take minutes of wallclock
time to construct large guests, given the unintentional behaviour
introduced by the idle scrubbing logic.)

Honestly, its the kind of thing which shouldn't be firing until turned
on by HVMLoader.  Maybe we're too late to fix that...

~Andrew

On 28.01.2021 17:51, Andrew Cooper wrote:
> On 28/01/2021 14:40, Jan Beulich wrote:
>> hvm_destroy_all_ioreq_servers(), called from
>> hvm_domain_relinquish_resources(), invokes relocate_portio_handler(),
>> which uses d->arch.hvm.io_handler. Defer freeing of this array
>> accordingly on the error path of hvm_domain_initialise().
>>
>> Similarly rtc_deinit() requires d->arch.hvm.pl_time to still be around,
>> or else an armed timer structure would get freed, and that timer never
>> get killed.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

Thanks.

>> ---
>> We may want to consider moving the other two XFREE()s later as well,
>> if only to be on the safe side.
> 
> Wherever possible, I want to move stuff like this into the idempotent
> domain_teardown()/_domain_destroy() logic, although I suspect you want
> this suitable for backport as well?

This and I didn't want it more involved than necessary at this point.

Jan