Re: [PATCH v3 5/7] vpci: fix execution of long running operations

["Jan Beulich" <JBeulich@suse.com>]

>>> On 07.11.18 at 12:11, <roger.pau@citrix.com> wrote:
> On Mon, Nov 05, 2018 at 09:56:13AM -0700, Jan Beulich wrote:
>> >>> On 30.10.18 at 16:41, <roger.pau@citrix.com> wrote:
>> > BAR map/unmap is a long running operation that needs to be preempted
>> > in order to avoid overrunning the assigned vCPU time (or even
>> > triggering the watchdog).
>> > 
>> > Current logic for this preemption is wrong, and won't work at all for
>> > AMD since only Intel makes use of hvm_io_pending (and even in that
>> > case the current code is wrong).
>> 
>> I'm having trouble understanding this, both for the AMD aspect
>> (it is only vvmx.c which has a function call not mirrored on the
>> AMD side) and for the supposed general brokenness. Without
>> some clarification I can't judge whether re-implementing via
>> tasklet is actually the best approach.
> 
> hvm_io_pending itself cannot block the vCPU from executing, it's used
> by nvmx_switch_guest in order to prevent changing the nested VMCS if
> there's pending IO emulation work AFAICT.
> 
> The only way I could find to actually prevent a vCPU from running
> while doing some work on it's behalf in a preemptive way is by
> blocking it and using a tasklet. What's done with IOREQs is not
> suitable here since Xen needs to do some work instead of just wait on
> an external event (an event channel from the IOREQ).

No, there is a second means, I've just confused the functions. The
question is whether your vpci_process_pending() invocation
perhaps sits in the wrong function. handle_hvm_io_completion() is
what hvm_do_resume() calls, and what can prevent a guest from
resuming execution. The hvm_io_pending() invocation just sits on
a special case path down from there (through handle_pio()).

>> > +void vpci_init_vcpu(struct vcpu *v)
>> > +{
>> > +    tasklet_init(&v->vpci.task, vpci_process_pending, (unsigned long)v);
>> >  }
>> 
>> Since there's no respective cleanup code added afaics - what
>> if the domain gets cleaned up behind the back of the (long
>> running) tasklet? Don't you want to acquire (and then release)
>> an extra domain reference somewhere?
> 
> Yes, that's correct. Isn't just doing a tasklet_kill at domain
> destruction enough?

Yes, that should do it.

Jan



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