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

[Jan Beulich <jbeulich@xxxxxxxx>]

Domain creation failure paths don't call domain_relinquish_resources(),
yet allocations and alike done from hvm_domain_initialize() need to be
undone nevertheless. Call the function also from hvm_domain_destroy(),
after making sure all descendants are idempotent.

Note that while viridian_{domain,vcpu}_deinit() were already used in
ways suggesting they're idempotent, viridian_time_vcpu_deinit() actually
wasn't: One can't kill a timer that was never initialized.

For hvm_destroy_all_ioreq_servers()'s purposes make
relocate_portio_handler() return whether the to be relocated port range
was actually found. This seems cheaper than introducing a flag into
struct hvm_domain's ioreq_server sub-structure.

In hvm_domain_initialise() additionally
- use XFREE() also to replace adjacent xfree(),
- use hvm_domain_relinquish_resources() as being idempotent now.
There as well as in hvm_domain_destroy() the explicit call to
rtc_deinit() isn't needed anymore.

In hvm_domain_relinquish_resources() additionally drop a no longer
relevant if().

Fixes: e7a9b5e72f26 ("viridian: separately allocate domain and vcpu structures")
Fixes: 26fba3c85571 ("viridian: add implementation of synthetic timers")
Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
---
v2: Also drop rtc_deinit() from hvm_domain_destroy(). Also drop now
    pointless if() from hvm_domain_relinquish_resources().

--- 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 )
         return;
 
     write_lock(&h->lock);
@@ -763,6 +763,8 @@ void hpet_deinit(struct domain *d)
         for ( i = 0; i < HPET_TIMER_NUM; i++ )
             if ( timer_enabled(h, i) )
                 hpet_stop_timer(h, i, guest_time);
+
+        h->hpet.config = 0;
     }
 
     write_unlock(&h->lock);
--- 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);
     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);
  fail0:
     hvm_destroy_cacheattr_region_list(d);
     destroy_perdomain_mapping(d, PERDOMAIN_VIRT_START, 0);
  fail:
-    viridian_domain_deinit(d);
+    hvm_domain_relinquish_resources(d);
     return rc;
 }
 
+/* This function and all its descendants need to be to be idempotent. */
 void hvm_domain_relinquish_resources(struct domain *d)
 {
     if ( hvm_funcs.domain_relinquish_resources )
@@ -730,11 +730,8 @@ void hvm_domain_relinquish_resources(str
 
     /* Stop all asynchronous timer actions. */
     rtc_deinit(d);
-    if ( d->vcpu != NULL && d->vcpu[0] != NULL )
-    {
-        pmtimer_deinit(d);
-        hpet_deinit(d);
-    }
+    pmtimer_deinit(d);
+    hpet_deinit(d);
 }
 
 void hvm_domain_destroy(struct domain *d)
@@ -742,6 +739,13 @@ void hvm_domain_destroy(struct domain *d
     struct list_head *ioport_list, *tmp;
     struct g2m_ioport *ioport;
 
+    /*
+     * This function would not be called when domain initialization fails
+     * (late enough), so do so here. This requires the function and all its
+     * descendants to be idempotent.
+     */
+    hvm_domain_relinquish_resources(d);
+
     XFREE(d->arch.hvm.io_handler);
     XFREE(d->arch.hvm.params);
 
@@ -750,7 +754,6 @@ void hvm_domain_destroy(struct domain *d
     if ( hvm_funcs.domain_destroy )
         alternative_vcall(hvm_funcs.domain_destroy, d);
 
-    rtc_deinit(d);
     stdvga_deinit(d);
     vioapic_deinit(d);
 
--- a/xen/arch/x86/hvm/ioreq.c
+++ b/xen/arch/x86/hvm/ioreq.c
@@ -1228,6 +1228,9 @@ void hvm_destroy_all_ioreq_servers(struc
     struct hvm_ioreq_server *s;
     unsigned int id;
 
+    if ( !relocate_portio_handler(d, 0xcf8, 0xcf8, 4) )
+        return;
+
     spin_lock_recursive(&d->arch.hvm.ioreq_server.lock);
 
     /* No need to domain_pause() as the domain is being torn down */
--- a/xen/arch/x86/hvm/intercept.c
+++ b/xen/arch/x86/hvm/intercept.c
@@ -300,7 +300,7 @@ void register_portio_handler(struct doma
     handler->portio.action = action;
 }
 
-void relocate_portio_handler(struct domain *d, unsigned int old_port,
+bool relocate_portio_handler(struct domain *d, unsigned int old_port,
                              unsigned int new_port, unsigned int size)
 {
     unsigned int i;
@@ -317,9 +317,11 @@ void relocate_portio_handler(struct doma
              (handler->portio.size = size) )
         {
             handler->portio.port = new_port;
-            break;
+            return true;
         }
     }
+
+    return false;
 }
 
 bool_t hvm_mmio_internal(paddr_t gpa)
--- 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;
 
     kill_timer(&s->timer);
--- 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;
 
     spin_barrier(&s->lock);
--- a/xen/arch/x86/hvm/viridian/time.c
+++ b/xen/arch/x86/hvm/viridian/time.c
@@ -524,6 +524,8 @@ void viridian_time_vcpu_deinit(const str
     {
         struct viridian_stimer *vs = &vv->stimer[i];
 
+        if ( !vs->v )
+            continue;
         kill_timer(&vs->timer);
         vs->v = NULL;
     }
--- a/xen/include/asm-x86/hvm/io.h
+++ b/xen/include/asm-x86/hvm/io.h
@@ -112,7 +112,7 @@ void register_portio_handler(
     struct domain *d, unsigned int port, unsigned int size,
     portio_action_t action);
 
-void relocate_portio_handler(
+bool relocate_portio_handler(
     struct domain *d, unsigned int old_port, unsigned int new_port,
     unsigned int size);
 

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