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

["Wu, Feng" <feng.wu@xxxxxxxxx>]

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

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.

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

It is a little annoying.

Thanks,
Feng


>  1. is hard to tell without actually seeing how the code will end up
>     being
>  2. might be my opinion only, so let's see what others think.
> 
> Regards,
> Dario
> --
> <<This happens because I choose it to happen!>> (Raistlin Majere)
> -----------------------------------------------------------------
> Dario Faggioli, Ph.D, http://about.me/dario.faggioli
> Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

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