Re: [Xen-devel] [PATCH v6 15/18] vmx: Properly handle notification event when vCPU is running

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 08.09.15 at 07:18, <feng.wu@xxxxxxxxx> wrote:

> 
>> -----Original Message-----
>> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
>> Sent: Monday, September 07, 2015 8:10 PM
>> To: Wu, Feng; Zhang, Yang Z
>> Cc: Andrew Cooper; Tian, Kevin; xen-devel@xxxxxxxxxxxxx; Keir Fraser
>> Subject: Re: [PATCH v6 15/18] vmx: Properly handle notification event when
>> vCPU is running
>> 
>> >>> On 25.08.15 at 03:57, <feng.wu@xxxxxxxxx> wrote:
>> > --- a/xen/arch/x86/hvm/vmx/vmx.c
>> > +++ b/xen/arch/x86/hvm/vmx/vmx.c
>> >  const struct hvm_function_table * __init start_vmx(void)
>> >  {
>> >      set_in_cr4(X86_CR4_VMXE);
>> > @@ -2073,7 +2119,7 @@ const struct hvm_function_table * __init
>> start_vmx(void)
>> >
>> >      if ( cpu_has_vmx_posted_intr_processing )
>> >      {
>> > -        alloc_direct_apic_vector(&posted_intr_vector,
>> event_check_interrupt);
>> > +        alloc_direct_apic_vector(&posted_intr_vector,
>> pi_notification_interrupt);
>> >
>> >          if ( iommu_intpost )
>> >              alloc_direct_apic_vector(&pi_wakeup_vector,
>> pi_wakeup_interrupt);
>> 
>> Considering that you do this setup independent of iommu_intpost,
>> won't this (namely, but not only) for the !iommu_inpost case result
>> in a whole lot of useless softirqs to be raised? 
> 
> I don't think lots of useless softirqs will be raised in !iommu_intpost
> case, since we can get pi_notification_interrupt() only when someone
> called __vmx_deliver_posted_interrupt() in which, the VCPU_KICK_SOFTIRQ
> bit was set.

Now that you say it, this looks even more odd: Why would you need
to raise that softirq if the only way to come here is via the triggering
in __vmx_deliver_posted_interrupt() (which already raised that
softirq)? I suppose I must be missing something...

>> IOW - shouldn't this setup be conditional, 
> 
> We cannot setup pi_notification_interrupt() conditional, since it is
> for all cases, non-vtd-pi and vt-d pi.

But you could continue to use event_check_interrupt in the
!iommu_intpost case.

>> and shouldn't the handler also only conditionally raise the softirq
>> (to as much as possible limit their amount)?
> 
> As mentioned above, there are not extra softirq raised. If you think
> It is necessary, I can add a condition here as below, which I really think
> is useless, but I am fine to add it if you really think it is a must.
> 
>       if ( iommu_intpost )
>               raise_softirq(VCPU_KICK_SOFTIRQ);

See above - if this doesn't raise any extra softirqs, then why do
you call raise_softirq() in the first place?

Jan


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