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

[Jan Beulich <jbeulich@xxxxxxxx>]

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é <roger.pau@xxxxxxxxxx>
>>
>> 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

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