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 <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 )
>> 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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
