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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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?

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

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