[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v7 15/17] vmx: VT-d posted-interrupt core logic handling




> -----Original Message-----
> From: George Dunlap [mailto:george.dunlap@xxxxxxxxxx]
> Sent: Tuesday, September 22, 2015 6:27 PM
> To: Wu, Feng; Dario Faggioli; George Dunlap
> Cc: Jan Beulich; Tian, Kevin; Keir Fraser; Andrew Cooper;
> xen-devel@xxxxxxxxxxxxx
> Subject: Re: [Xen-devel] [PATCH v7 15/17] vmx: VT-d posted-interrupt core 
> logic
> handling
> 
> On 09/22/2015 08:19 AM, Wu, Feng wrote:
> >
> >
> >> -----Original Message-----
> >> From: Dario Faggioli [mailto:dario.faggioli@xxxxxxxxxx]
> >> Sent: Monday, September 21, 2015 10:25 PM
> >> To: Wu, Feng; George Dunlap; George Dunlap
> >> Cc: Jan Beulich; Tian, Kevin; Keir Fraser; Andrew Cooper;
> >> xen-devel@xxxxxxxxxxxxx
> >> Subject: Re: [Xen-devel] [PATCH v7 15/17] vmx: VT-d posted-interrupt core
> logic
> >> handling
> >>
> >> On Mon, 2015-09-21 at 12:22 +0000, Wu, Feng wrote:
> >>>
> >>>> -----Original Message-----
> >>>> From: George Dunlap [mailto:george.dunlap@xxxxxxxxxx]
> >>
> >>>> You also need to check that local_events_need_delivery() will
> >>>> return
> >>>> "true" if you get an interrupt between that time and entering the
> >>>> hypervisor.  Will that happen automatically from
> >>>> hvm_local_events_need_delivery() -> hvm_vcpu_has_pending_irq() ->
> >>>> vlapic_has_pending_irq()?  Or will you need to add a hook in
> >>>> hvm_vcpu_has_pending_irq()?
> >>>
> >>> I think I don't need to add hook in hvm_vcpu_has_pending_irq(), what
> >>> I need
> >>> to do in vcpu_block() and do_poll() is as below:
> >>>
> >>> 1. set_bit(_VPF_blocked, &v->pause_flags);
> >>>
> >>> 2. ret = v->arch.arch_block(), in this hook, we can re-use the same
> >>> logic in
> >>> vmx_pre_ctx_switch_pi(), and check whether ON bit is set during
> >>> updating
> >>> posted-interrupt descriptor, can return 1 when ON is set
> >>>
> >> It think it would be ok for an hook to return a value (maybe, if doing
> >> that, let's pick variable names and use comments to explain what goes
> >> on as good as we can).
> >>
> >> I think I also see why you seem to prefer doing it that way, rather
> >> than hacking local_events_need_delivery(), but can you please elaborate
> >> on that? (My feeling is that you want to avoid having to update the
> >> data structures in between _VPF_blocked and the check
> >> local_events_need_delivery(), and then having to roll such update back
> >> if local_events_need_delivery() ends up being false, is that the
> >> case?).
> >
> > In the arch_block() hook, we actually need to
> >     - Put vCPU to the blocking list
> >     - Set the NV to wakeup vector
> >     - Set NDST to the right pCPU
> >     - Clear SN
> 
> Nit: We shouldn't need to actually clear SN here; SN should already be
> clear because the vcpu should be currently running.  (I don't think
> there's a way for a vcpu to go from runnable->blocked, is there?)  And
> if it's just been running, then NDST should also already be the correct
> pcpu.

Yes, we can go to blocked only from running state. let me clarify a
question first: Xen doesn't support kernel preemption, right?( i.e. we
can only perform context switch before returning to guest.) If that is
case, we can make sure the pcpu is not changed for the running
vCPU before we set the NDST filed in PI descriptor, so we don't need
to update NDST.

> 
> > During we are updating the posted-interrupt descriptor, the VT-d
> > hardware can issue notification event and hence ON is set. If that is the
> > case, it is straightforward to return directly, and it doesn't make sense
> > we continue to do the above operations since we don't need to actually.
> 
> But checking to see if any interrupts have come in in the middle of your
> code just adds unnecessary complication.  We need to have the code in
> place to un-do all the blocking steps in the case that
> local_events_need_delivery() returns true anyway.
> 
> Additionally, while local_events_need_delivery() is only called from
> do_block and do_poll, hvm_local_events_need_delivery() is called from a
> number of other places, as is hvm_vcpu_has_pending_irq().  All those
> places presumably also need to know whether there's a PI pending to work
> properly.

local_events_need_delivery() can be called in other places, since it is wrapped
in hypercall_preempt_check(), which can be called in bunch of places. But
that shouldn't be a question here. In fact, ON bit is checked in
local_events_need_delivery() call path: (this is added when the CPU side PI
patches is merged years ago)

local_events_need_delivery() --> hvm_local_events_need_delivery()
--> hvm_vcpu_has_pending_irq() --> vlapic_has_pending_irq()
--> vlapic_find_highest_irr() --> hvm_funcs.sync_pir_to_irr()
--> pi_test_and_clear_on()

What we need to do is to find a good way to recover the PI state in vcpu_block()
and do_poll() if local event delivery is needed. Need to think this more.

Thanks,
Feng

> 
> >> Code would look better, IMO, if we manage to fold that somehow inside
> >> local_events_need_delivery(), but that:
> >
> > As mentioned above, during updating the PI descriptor for blocking, we
> > need to check whether ON is set, and return if it is set. This logic cannot
> > be included in local_events_need_delivery(), since the update itself
> > has not much relationship with local_events_need_delivery().
> >
> > However, I do find some issues with my proposal above, see below:
> >
> > 1. Set _VPF_blocked
> > 2. ret = arch_block()
> > 3. if ( ret || local_events_need_delivery() )
> >     clear_bit(_VPF_blocked, &v->pause_flags);
> >
> > After step #2, if ret == false, that means, we successfully changed the PI
> > descriptor in arch_block(), if local_events_need_delivery() returns true,
> > _VPF_blocked is cleared. After that, external interrupt come in, hence
> > pi_wakeup_interrupt() --> vcpu_unblock(), but _VPF_blocked is cleared,
> > so vcpu_unblock() does nothing, so the vCPU's PI state is incorrect.
> >
> > One possible solution for it is:
> > - Disable the interrupts before the check in step #3 above
> > - if local_events_need_delivery() returns true, undo all the operations
> >  done in arch_block()
> > - Enable interrupts after _VPF_blocked gets cleared.
> 
> Well, yes, if as a result of checking to see that there are interrupts
> you unblock a processor, then you need to do *all* the things related to
> unblocking, including switching the interrupt vector and removing it
> from the blocked list.
> 
> If we're going to put hooks here in do_block(), then the best thing to
> do is as much as possible to make PI interrupts act exactly the same as
> other interrupts; i.e.,
> 
> * Do a full transition to blocked (set _VPF_blocked, add vcpu to PI
> list, switch vector to wake)
> * Check to see if there are any pending interrupts (either event
> channels, virtual interrupts, or PIs)
> * If so, do a full transition to unblocked (clear _VPF_blocked, switch
> vector to PI, remove vcpu from list).
> 
> We should be able to order the operations such that if interrupts come
> in the middle nothing bad happens, without needing to actually disable
> interrupts.
> 
> OTOH -- I think if we grab a lock during an interrupt, we're not allowed
> to grab it with interrupts disabled, correct?  So we may end up having
> to disable interrupts anyway.
> 
>  -George

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.