> -----Original Message-----
> From: Xen-devel <xen-devel-boun...@lists.xenproject.org> On Behalf Of Jan
> Beulich
> Sent: 28 January 2020 13:17
> To: xen-devel@lists.xenproject.org
> Cc: Andrew Cooper <andrew.coop...@citrix.com>; Paul Durrant
> <p...@xen.org>; Wei Liu <w...@xen.org>; Roger Pau Monné
> <roger....@citrix.com>
> Subject: [Xen-devel] [PATCH] x86/HVM: relinquish resources also from
> hvm_domain_destroy()
>
> 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.
>
> Fixes: e7a9b5e72f26 ("viridian: separately allocate domain and vcpu
> structures")
> Fixes: 26fba3c85571 ("viridian: add implementation of synthetic timers")
> Signed-off-by: Jan Beulich <jbeul...@suse.com>
>
> --- 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?
> 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);
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.
> 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?
> 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 )
> @@ -742,6 +742,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);
>
> --- 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;
> +
Seems a little bit hacky but agreed that it works and avoids the need for
another flag.
> 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;
Isn't a test of s->vcpu sufficient?
>
> 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;
Testing s->pt.source for a zero value would be sufficient and cheaper, I think.
Paul
>
> 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@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
