x86/HVM: relinquish resources also from hvm_domain_destroy()

Commit Message

Jan Beulich Jan. 28, 2020, 1:16 p.m. UTC

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 <[e-mail]>

Comments

Durrant, Paul Jan. 28, 2020, 2:14 p.m. UTC | #1

> -----Original Message-----
> From: Xen-devel <[e-mail]> On Behalf Of Jan
> Beulich
> Sent: 28 January 2020 13:17
> To: [e-mail]
> Cc: Andrew Cooper <[e-mail]>; Paul Durrant
> <[e-mail]>; Wei Liu <[e-mail]>; Roger Pau Monné
> <[e-mail]>
> 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 <[e-mail]>
> 
> --- 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
> [e-mail]
> https://lists.xenproject.org/mailman/listinfo/xen-devel

Jan Beulich Jan. 28, 2020, 2:31 p.m. UTC | #2

On 28.01.2020 15:14, Durrant, Paul wrote:
>> -----Original Message-----
>> From: Xen-devel <[e-mail]> 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

Roger Pau Monne Jan. 28, 2020, 2:54 p.m. UTC | #3

On Tue, Jan 28, 2020 at 02:16:53PM +0100, Jan Beulich wrote:
> 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 <[e-mail]>
> 
> --- 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);

Would be nice to unify all those labels into a single fail label, but
I'm not sure if all functions are prepared to handle this.

>      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;

Could you add a note here that the 'relocation' is just used as a mean
to figure out if iooreq servers have been setup (ie: the lock has been
initialized)?

I find this kind of dirty, and would need rework if other arches gain
ioreq support.

> +
>      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 )

You could also check for the port registration AFAICT, which is maybe
more obvious?

I also wonder whether all those time-related emulations could be
grouped into a single helper, that could have a d->arch.hvm.pl_time
instead of having to sprinkle such checks for each device?

Thanks, Roger.

Durrant, Paul Jan. 28, 2020, 2:58 p.m. UTC | #4

> -----Original Message-----
> From: Jan Beulich <[e-mail]>
> Sent: 28 January 2020 14:31
> To: Durrant, Paul <[e-mail]>
> Cc: [e-mail]; Andrew Cooper
> <[e-mail]>; Paul Durrant <[e-mail]>; Wei Liu
> <[e-mail]>; Roger Pau Monné <[e-mail]>
> Subject: Re: [PATCH] x86/HVM: relinquish resources also from
> hvm_domain_destroy()
> 
> On 28.01.2020 15:14, Durrant, Paul wrote:
> >> -----Original Message-----
> >> From: Xen-devel <[e-mail]> 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.
> 

Ah, I'd missed that domain_vhpet() dereferenced pl_time.

> >> --- 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).
> 

Oh, sorry. For some reason I thought this hunk was in hvm_domain_relinquish_resources() so yes the XFREEs in destroy need to stay as is.

> >> --- 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.

Ok. I'd assume a non-zero rather than equality test would be cheaper but I notice that TIMER_STATUS_invalid is defined to 0 anyway, so it should be optimised at compile time.

  Paul

> 
> Jan

Jan Beulich Jan. 28, 2020, 3:49 p.m. UTC | #5

On 28.01.2020 15:54, Roger Pau Monné wrote:
> On Tue, Jan 28, 2020 at 02:16:53PM +0100, Jan Beulich wrote:
>> 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 <[e-mail]>
>>
>> --- 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);
> 
> Would be nice to unify all those labels into a single fail label, but
> I'm not sure if all functions are prepared to handle this.

That's the mid- to long-term plan, yes. But it has taken me quite a
bit more time than intended to at least sort out this part. I really
don't want to extend this right now (and even less so in this patch).

>> --- 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;
> 
> Could you add a note here that the 'relocation' is just used as a mean
> to figure out if iooreq servers have been setup (ie: the lock has been
> initialized)?

Hmm, I'm explaining this in the description, and twice the same
number I thought would make the purpose sufficiently clear
(anyone who still wonders could still go back to the commit).

> I find this kind of dirty, and would need rework if other arches gain
> ioreq support.

This is x86 code - how are other arches going to be affected?
Even if they gain ioreq server support, the notion of port I/O
in general will remain an x86 specific.

I agree it's a little dirty, but I consider it better than
putting in another bool (and probably 7 bytes of padding) into
struct hvm_domain.

>> --- 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 )
> 
> You could also check for the port registration AFAICT, which is maybe
> more obvious?

You called that approach dirty above - I'd like to restrict it
to just where no better alternative exists.

> I also wonder whether all those time-related emulations could be
> grouped into a single helper, that could have a d->arch.hvm.pl_time
> instead of having to sprinkle such checks for each device?

Quite possible, but not here and not now.

Jan

Roger Pau Monne Jan. 28, 2020, 5:25 p.m. UTC | #6

On Tue, Jan 28, 2020 at 04:49:09PM +0100, Jan Beulich wrote:
> On 28.01.2020 15:54, Roger Pau Monné wrote:
> > On Tue, Jan 28, 2020 at 02:16:53PM +0100, Jan Beulich wrote:
> >> 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 <[e-mail]>
> >>
> >> --- 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);
> > 
> > Would be nice to unify all those labels into a single fail label, but
> > I'm not sure if all functions are prepared to handle this.
> 
> That's the mid- to long-term plan, yes. But it has taken me quite a
> bit more time than intended to at least sort out this part. I really
> don't want to extend this right now (and even less so in this patch).
> 
> >> --- 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;
> > 
> > Could you add a note here that the 'relocation' is just used as a mean
> > to figure out if iooreq servers have been setup (ie: the lock has been
> > initialized)?
> 
> Hmm, I'm explaining this in the description, and twice the same
> number I thought would make the purpose sufficiently clear
> (anyone who still wonders could still go back to the commit).
> 
> > I find this kind of dirty, and would need rework if other arches gain
> > ioreq support.
> 
> This is x86 code - how are other arches going to be affected?
> Even if they gain ioreq server support, the notion of port I/O
> in general will remain an x86 specific.
> 
> I agree it's a little dirty, but I consider it better than
> putting in another bool (and probably 7 bytes of padding) into
> struct hvm_domain.

Ack, I guess it would become clear enough for readers when seeing the
init function.

> 
> >> --- 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 )
> > 
> > You could also check for the port registration AFAICT, which is maybe
> > more obvious?
> 
> You called that approach dirty above - I'd like to restrict it
> to just where no better alternative exists.

Ack, it didn't seem that bad here because this is a x86 emulated
device that relies on IO ports, while the ioreq code (albeit x86
specific ATM) could be used by other arches, and hence would likely
prefer to avoid using x86 specific details for generic functions, like
the init or deinit ones.

> > I also wonder whether all those time-related emulations could be
> > grouped into a single helper, that could have a d->arch.hvm.pl_time
> > instead of having to sprinkle such checks for each device?
> 
> Quite possible, but not here and not now.

Sure.

Reviewed-by: Roger Pau Monné <[e-mail]>

Thanks, Roger.

Jan Beulich Jan. 29, 2020, 8:01 a.m. UTC | #7

On 28.01.2020 18:25, Roger Pau Monné wrote:
> On Tue, Jan 28, 2020 at 04:49:09PM +0100, Jan Beulich wrote:
>> On 28.01.2020 15:54, Roger Pau Monné wrote:
>>> On Tue, Jan 28, 2020 at 02:16:53PM +0100, Jan Beulich wrote:
>>>> --- 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 )
>>>
>>> You could also check for the port registration AFAICT, which is maybe
>>> more obvious?
>>
>> You called that approach dirty above - I'd like to restrict it
>> to just where no better alternative exists.
> 
> Ack, it didn't seem that bad here because this is a x86 emulated
> device that relies on IO ports, while the ioreq code (albeit x86
> specific ATM) could be used by other arches, and hence would likely
> prefer to avoid using x86 specific details for generic functions, like
> the init or deinit ones.

Likely, but the port I/O handler registration is going to remain
x86-specific, and hence there would pretty certainly also be an
arch-specific init (and may a deinit) function.

>>> I also wonder whether all those time-related emulations could be
>>> grouped into a single helper, that could have a d->arch.hvm.pl_time
>>> instead of having to sprinkle such checks for each device?
>>
>> Quite possible, but not here and not now.
> 
> Sure.
> 
> Reviewed-by: Roger Pau Monné <[e-mail]>

Thanks. There are two small changes I intend to do, one directly
and one indirectly resulting from Paul's feedback: Also drop
rtc_deinit() from hvm_domain_destroy(). Also drop now pointless
if() from hvm_domain_relinquish_resources(). I'd therefore like
to ask you to confirm the R-b can be left in place, or whether
instead you'd rather wait for v2 to be posted.

Jan

Roger Pau Monne Jan. 29, 2020, 9:38 a.m. UTC | #8

On Wed, Jan 29, 2020 at 09:01:34AM +0100, Jan Beulich wrote:
> On 28.01.2020 18:25, Roger Pau Monné wrote:
> > On Tue, Jan 28, 2020 at 04:49:09PM +0100, Jan Beulich wrote:
> >> On 28.01.2020 15:54, Roger Pau Monné wrote:
> >>> On Tue, Jan 28, 2020 at 02:16:53PM +0100, Jan Beulich wrote:
> >>>> --- 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 )
> >>>
> >>> You could also check for the port registration AFAICT, which is maybe
> >>> more obvious?
> >>
> >> You called that approach dirty above - I'd like to restrict it
> >> to just where no better alternative exists.
> > 
> > Ack, it didn't seem that bad here because this is a x86 emulated
> > device that relies on IO ports, while the ioreq code (albeit x86
> > specific ATM) could be used by other arches, and hence would likely
> > prefer to avoid using x86 specific details for generic functions, like
> > the init or deinit ones.
> 
> Likely, but the port I/O handler registration is going to remain
> x86-specific, and hence there would pretty certainly also be an
> arch-specific init (and may a deinit) function.
> 
> >>> I also wonder whether all those time-related emulations could be
> >>> grouped into a single helper, that could have a d->arch.hvm.pl_time
> >>> instead of having to sprinkle such checks for each device?
> >>
> >> Quite possible, but not here and not now.
> > 
> > Sure.
> > 
> > Reviewed-by: Roger Pau Monné <[e-mail]>
> 
> Thanks. There are two small changes I intend to do, one directly
> and one indirectly resulting from Paul's feedback: Also drop
> rtc_deinit() from hvm_domain_destroy(). Also drop now pointless
> if() from hvm_domain_relinquish_resources().

I assume this is the if condition around the {pmtimer/hpet}_deinit
calls?

> I'd therefore like
> to ask you to confirm the R-b can be left in place, or whether
> instead you'd rather wait for v2 to be posted.

Yes, I think you can keep the R-b.

Roger.

Jan Beulich Jan. 29, 2020, 9:45 a.m. UTC | #9

On 29.01.2020 10:38, Roger Pau Monné wrote:
> On Wed, Jan 29, 2020 at 09:01:34AM +0100, Jan Beulich wrote:
>> On 28.01.2020 18:25, Roger Pau Monné wrote:
>>> On Tue, Jan 28, 2020 at 04:49:09PM +0100, Jan Beulich wrote:
>>>> On 28.01.2020 15:54, Roger Pau Monné wrote:
>>>>> On Tue, Jan 28, 2020 at 02:16:53PM +0100, Jan Beulich wrote:
>>>>>> --- 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 )
>>>>>
>>>>> You could also check for the port registration AFAICT, which is maybe
>>>>> more obvious?
>>>>
>>>> You called that approach dirty above - I'd like to restrict it
>>>> to just where no better alternative exists.
>>>
>>> Ack, it didn't seem that bad here because this is a x86 emulated
>>> device that relies on IO ports, while the ioreq code (albeit x86
>>> specific ATM) could be used by other arches, and hence would likely
>>> prefer to avoid using x86 specific details for generic functions, like
>>> the init or deinit ones.
>>
>> Likely, but the port I/O handler registration is going to remain
>> x86-specific, and hence there would pretty certainly also be an
>> arch-specific init (and may a deinit) function.
>>
>>>>> I also wonder whether all those time-related emulations could be
>>>>> grouped into a single helper, that could have a d->arch.hvm.pl_time
>>>>> instead of having to sprinkle such checks for each device?
>>>>
>>>> Quite possible, but not here and not now.
>>>
>>> Sure.
>>>
>>> Reviewed-by: Roger Pau Monné <[e-mail]>
>>
>> Thanks. There are two small changes I intend to do, one directly
>> and one indirectly resulting from Paul's feedback: Also drop
>> rtc_deinit() from hvm_domain_destroy(). Also drop now pointless
>> if() from hvm_domain_relinquish_resources().
> 
> I assume this is the if condition around the {pmtimer/hpet}_deinit
> calls?

Yes. The other two if()-s obviously can't go away.

Jan

Patch

--- 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 )
@@ -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;
+
     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);