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

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

> -----Original Message-----
> From: Xen-devel <xen-devel-bounces@xxxxxxxxxxxxxxxxxxxx> On Behalf Of Jan
> Beulich
> Sent: 28 January 2020 13:17
> To: xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>; Paul Durrant
> <paul@xxxxxxx>; Wei Liu <wl@xxxxxxx>; Roger Pau Monné
> <roger.pau@xxxxxxxxxx>
> 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 <jbeulich@xxxxxxxx>
> 
> --- 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@xxxxxxxxxxxxxxxxxxxx
> https://lists.xenproject.org/mailman/listinfo/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel