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 11:23, <feng.wu@xxxxxxxxx> wrote:

> 
>> -----Original Message-----
>> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
>> Sent: Tuesday, September 08, 2015 5:14 PM
>> To: Wu, Feng
>> Cc: Andrew Cooper; Tian, Kevin; Zhang, Yang Z; xen-devel@xxxxxxxxxxxxx; Keir
>> Fraser
>> Subject: RE: [PATCH v6 15/18] vmx: Properly handle notification event when
>> vCPU is running
>> 
>> >>> 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...
> 
> Before VT-d PI, __vmx_deliver_posted_interrupt() is the only way
> to trigger interrupt with vector ' posted_intr_vector ', but after
> introducing VT-d PI, VT-d hardware can issue the interrupt with
> that vector as well. In __vmx_deliver_posted_interrupt(), it set
> the softirqs, the reason of which is described in the comments
> of pi_notification_interrupt(), however, I need do the same thing
> when VT-d hardware issue the interrupt, so pi_notification_interrupt()
> is the proper place to do it.

But again - my main concern is about the !iommu_intpost case: What
good does the raising of the softirq there? (As a general remark -
please, when you add code to support a new feature, don't just
think about the case where the new feature is available in hardware,
but also about the case where it's not. While over time the set of
systems lacking the feature will likely decrease, initially it may be the
vast majority of systems Xen gets run on which would get penalized.)

Jan


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