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

[Jan Beulich]

>>> On 08.09.15 at 07:18,  wrote:

> 
>> -Original Message-
>> From: Jan Beulich [mailto:jbeul...@suse.com]
>> Sent: Monday, September 07, 2015 8:10 PM
>> To: Wu, Feng; Zhang, Yang Z
>> Cc: Andrew Cooper; Tian, Kevin; xen-devel@lists.xen.org; Keir Fraser
>> Subject: Re: [PATCH v6 15/18] vmx: Properly handle notification event when
>> vCPU is running
>> 
>> >>> On 25.08.15 at 03:57,  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(_intr_vector,
>> event_check_interrupt);
>> > +alloc_direct_apic_vector(_intr_vector,
>> pi_notification_interrupt);
>> >
>> >  if ( iommu_intpost )
>> >  alloc_direct_apic_vector(_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@lists.xen.org
http://lists.xen.org/xen-devel

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

[Jan Beulich]

>>> On 08.09.15 at 11:23,  wrote:

> 
>> -Original Message-
>> From: Jan Beulich [mailto:jbeul...@suse.com]
>> Sent: Tuesday, September 08, 2015 5:14 PM
>> To: Wu, Feng
>> Cc: Andrew Cooper; Tian, Kevin; Zhang, Yang Z; xen-devel@lists.xen.org; Keir
>> Fraser
>> Subject: RE: [PATCH v6 15/18] vmx: Properly handle notification event when
>> vCPU is running
>> 
>> >>> On 08.09.15 at 07:18,  wrote:
>> 
>> >
>> >> -Original Message-
>> >> From: Jan Beulich [mailto:jbeul...@suse.com]
>> >> Sent: Monday, September 07, 2015 8:10 PM
>> >> To: Wu, Feng; Zhang, Yang Z
>> >> Cc: Andrew Cooper; Tian, Kevin; xen-devel@lists.xen.org; Keir Fraser
>> >> Subject: Re: [PATCH v6 15/18] vmx: Properly handle notification event when
>> >> vCPU is running
>> >>
>> >> >>> On 25.08.15 at 03:57,  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(_intr_vector,
>> >> event_check_interrupt);
>> >> > +alloc_direct_apic_vector(_intr_vector,
>> >> pi_notification_interrupt);
>> >> >
>> >> >  if ( iommu_intpost )
>> >> >  alloc_direct_apic_vector(_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@lists.xen.org
http://lists.xen.org/xen-devel

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

[Wu, Feng]

> -Original Message-
> From: Jan Beulich [mailto:jbeul...@suse.com]
> Sent: Tuesday, September 08, 2015 5:14 PM
> To: Wu, Feng
> Cc: Andrew Cooper; Tian, Kevin; Zhang, Yang Z; xen-devel@lists.xen.org; Keir
> Fraser
> Subject: RE: [PATCH v6 15/18] vmx: Properly handle notification event when
> vCPU is running
> 
> >>> On 08.09.15 at 07:18,  wrote:
> 
> >
> >> -Original Message-
> >> From: Jan Beulich [mailto:jbeul...@suse.com]
> >> Sent: Monday, September 07, 2015 8:10 PM
> >> To: Wu, Feng; Zhang, Yang Z
> >> Cc: Andrew Cooper; Tian, Kevin; xen-devel@lists.xen.org; Keir Fraser
> >> Subject: Re: [PATCH v6 15/18] vmx: Properly handle notification event when
> >> vCPU is running
> >>
> >> >>> On 25.08.15 at 03:57,  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(_intr_vector,
> >> event_check_interrupt);
> >> > +alloc_direct_apic_vector(_intr_vector,
> >> pi_notification_interrupt);
> >> >
> >> >  if ( iommu_intpost )
> >> >  alloc_direct_apic_vector(_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.

Thanks,
Feng

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

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

[Wu, Feng]

> -Original Message-
> From: Jan Beulich [mailto:jbeul...@suse.com]
> Sent: Tuesday, September 08, 2015 5:31 PM
> To: Wu, Feng
> Cc: Andrew Cooper; Tian, Kevin; Zhang, Yang Z; xen-devel@lists.xen.org; Keir
> Fraser
> Subject: RE: [PATCH v6 15/18] vmx: Properly handle notification event when
> vCPU is running
> 
> >>> On 08.09.15 at 11:23,  wrote:
> 
> >>
> >> 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.)

No problem. Two solutions:
#1, Register pi_notification_interrupt when iommu_intpost and still
use event_check_interrupt in the !iommu_intpost case.
#1, Use pi_notification_interrupt() for both iommu_intpost and
!iommu_intpost, add the following check it:
if ( iommu_intpost )
raise_softirq(VCPU_KICK_SOFTIRQ);

Which one do you prefer?

Thanks,
Feng


> 
> Jan


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

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

[Jan Beulich]

>>> On 08.09.15 at 03:38,  wrote:
> Jan Beulich wrote on 2015-09-07:
> On 07.09.15 at 15:00,  wrote:
>>> Jan Beulich wrote on 2015-09-07:
 Yang, in this context: Why does __vmx_deliver_posted_interrupt()
 not use cpu_raise_softirq(), instead kind of open coding it (see your
 d7dafa375b ["VMX: Add posted interrupt supporting"])?
>>> 
>>> Sorry, I am not in the context. What do you mean of using
>>> cpu_raise_softirq() in __vmx_deliver_posted_interrupt()?
>> 
>> Why is the function not using that ready to use helper? Looking at
>> it ...
>> 
>>> static void __vmx_deliver_posted_interrupt(struct vcpu *v)
>>> {
>>>bool_t running = v->is_running;
>>>
>>>vcpu_unblock(v);
>>>if ( running && (in_irq() || (v != current)) )
>>>{
>>>unsigned int cpu = v->processor;
>>>
>>>if ( !test_and_set_bit(VCPU_KICK_SOFTIRQ, _pending(cpu))
>> 
>> ... this line as well as ...
>> 
>>> && (cpu != smp_processor_id()) )
>>>send_IPI_mask(cpumask_of(cpu), posted_intr_vector);
>> 
>> ... this one ...
>> 
>>>}
>>> }
>> 
>> ... pretty certainly don't belong into vmx.c, or the apparent open
>> coding of cpu_raise_softirq() would require a justifying comment.
> 
> I still don't see how to use cpu_raise_softirq() since the 
> posted_intr_vector doesn't belong to softirq.

I realize that because of the special in-processor treatment of that
vector it ma not be possible to eliminate this open coding. That's why
I said above "would require a justifying comment". After all, namely
with the handler for the vector being the same as for normal event
check interrupts, it is far from obvious why the open coding can't be
avoided.

Jan


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

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

[Jan Beulich]

>>> On 08.09.15 at 11:36,  wrote:

> 
>> -Original Message-
>> From: Jan Beulich [mailto:jbeul...@suse.com]
>> Sent: Tuesday, September 08, 2015 5:31 PM
>> To: Wu, Feng
>> Cc: Andrew Cooper; Tian, Kevin; Zhang, Yang Z; xen-devel@lists.xen.org; Keir
>> Fraser
>> Subject: RE: [PATCH v6 15/18] vmx: Properly handle notification event when
>> vCPU is running
>> 
>> >>> On 08.09.15 at 11:23,  wrote:
>> 
>> >>
>> >> 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.)
> 
> No problem. Two solutions:
> #1, Register pi_notification_interrupt when iommu_intpost and still
> use event_check_interrupt in the !iommu_intpost case.
> #1, Use pi_notification_interrupt() for both iommu_intpost and
> !iommu_intpost, add the following check it:
>   if ( iommu_intpost )
>   raise_softirq(VCPU_KICK_SOFTIRQ);
> 
> Which one do you prefer?

#1 ;-) (the earlier one, to avoid you guessing)

Jan


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

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

[Wu, Feng]

> -Original Message-
> From: Jan Beulich [mailto:jbeul...@suse.com]
> Sent: Tuesday, September 08, 2015 6:13 PM
> To: Wu, Feng
> Cc: Andrew Cooper; Tian, Kevin; Zhang, Yang Z; xen-devel@lists.xen.org; Keir
> Fraser
> Subject: RE: [PATCH v6 15/18] vmx: Properly handle notification event when
> vCPU is running
> 
> >>> On 08.09.15 at 11:36,  wrote:
> 
> >
> >> -Original Message-
> >> From: Jan Beulich [mailto:jbeul...@suse.com]
> >> Sent: Tuesday, September 08, 2015 5:31 PM
> >> To: Wu, Feng
> >> Cc: Andrew Cooper; Tian, Kevin; Zhang, Yang Z; xen-devel@lists.xen.org;
> Keir
> >> Fraser
> >> Subject: RE: [PATCH v6 15/18] vmx: Properly handle notification event when
> >> vCPU is running
> > No problem. Two solutions:
> > #1, Register pi_notification_interrupt when iommu_intpost and still
> > use event_check_interrupt in the !iommu_intpost case.
> > #1, Use pi_notification_interrupt() for both iommu_intpost and
> > !iommu_intpost, add the following check it:
> > if ( iommu_intpost )
> > raise_softirq(VCPU_KICK_SOFTIRQ);
> >
> > Which one do you prefer?
> 
> #1 ;-) (the earlier one, to avoid you guessing)

Hmm, sorry for the typo. :)

Thanks,
Feng

> 
> Jan


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

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

[Zhang, Yang Z]

Jan Beulich wrote on 2015-09-07:
>> + *  jnz  .Lvmx_process_softirqs
>> + *
>> + *  ..
>> + *
>> + *  je   .Lvmx_launch
>> + *
>> + *  ..
>> + *
>> + * .Lvmx_process_softirqs:
>> + *  sti
>> + *  call do_softirq
>> + *  jmp  .Lvmx_do_vmentry
>> + *
>> + * If VT-d engine issues a notification event at point 1 above, it 
>> cannot
>> + * be delivered to the guest during this VM-entry without raising the
>> + * softirq in this notification handler.
>> + */
>> +raise_softirq(VCPU_KICK_SOFTIRQ);
>> +
>> +this_cpu(irq_count)++;
>> +}
>> +
>>  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(_intr_vector,
>> event_check_interrupt); +   
>> alloc_direct_apic_vector(_intr_vector,
>> pi_notification_interrupt);
>> 
>>  if ( iommu_intpost )
>>  alloc_direct_apic_vector(_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? IOW - shouldn't this setup be
> conditional, and shouldn't the handler also only conditionally raise the
> softirq (to as much as possible limit their amount)?
> 
> Yang, in this context: Why does __vmx_deliver_posted_interrupt()
> not use cpu_raise_softirq(), instead kind of open coding it (see your
> d7dafa375b ["VMX: Add posted interrupt supporting"])?

Sorry, I am not in the context. What do you mean of using cpu_raise_softirq() 
in __vmx_deliver_posted_interrupt()?

Best regards,
Yang


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

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

[Jan Beulich]

>>> On 25.08.15 at 03:57,  wrote:
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -2035,6 +2035,52 @@ static void pi_wakeup_interrupt(struct cpu_user_regs 
> *regs)
>  this_cpu(irq_count)++;
>  }
>  
> +/* Handle VT-d posted-interrupt when VCPU is running. */
> +static void pi_notification_interrupt(struct cpu_user_regs *regs)
> +{
> +ack_APIC_irq();
> +
> +/*
> + * We get here when a vCPU is running in root-mode (such as via 
> hypercall,
> + * or any other reasons which can result in VM-Exit), and before vCPU is
> + * back to non-root, external interrupts from an assigned device happen
> + * and a notification event is delivered to this logical CPU.
> + *
> + * we need to set VCPU_KICK_SOFTIRQ for the current cpu, just like
> + * __vmx_deliver_posted_interrupt(). So the pending interrupt in PIRR 
> will
> + * be synced to vIRR before VM-Exit in time.
> + *
> + * Please refer to the following code fragments from
> + * xen/arch/x86/hvm/vmx/entry.S:
> + *
> + * .Lvmx_do_vmentry
> + *
> + *  ..
> + *  point 1
> + *
> + *  cmp  %ecx,(%rdx,%rax,1)

I think retaining the "cli" right above this is quite critical for
understanding why code past this check isn't susceptible to the
issue anymore.

> + *  jnz  .Lvmx_process_softirqs
> + *
> + *  ..
> + *
> + *  je   .Lvmx_launch
> + *
> + *  ..
> + *
> + * .Lvmx_process_softirqs:
> + *  sti
> + *  call do_softirq
> + *  jmp  .Lvmx_do_vmentry
> + *
> + * If VT-d engine issues a notification event at point 1 above, it cannot
> + * be delivered to the guest during this VM-entry without raising the
> + * softirq in this notification handler.
> + */
> +raise_softirq(VCPU_KICK_SOFTIRQ);
> +
> +this_cpu(irq_count)++;
> +}
> +
>  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(_intr_vector, event_check_interrupt);
> +alloc_direct_apic_vector(_intr_vector, 
> pi_notification_interrupt);
>  
>  if ( iommu_intpost )
>  alloc_direct_apic_vector(_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? IOW - shouldn't this
setup be conditional, and shouldn't the handler also only
conditionally raise the softirq (to as much as possible limit their
amount)?

Yang, in this context: Why does __vmx_deliver_posted_interrupt()
not use cpu_raise_softirq(), instead kind of open coding it (see your
d7dafa375b ["VMX: Add posted interrupt supporting"])?

Jan


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

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

[Jan Beulich]

>>> On 07.09.15 at 15:00,  wrote:
> Jan Beulich wrote on 2015-09-07:
>> Yang, in this context: Why does __vmx_deliver_posted_interrupt()
>> not use cpu_raise_softirq(), instead kind of open coding it (see your
>> d7dafa375b ["VMX: Add posted interrupt supporting"])?
> 
> Sorry, I am not in the context. What do you mean of using 
> cpu_raise_softirq() in __vmx_deliver_posted_interrupt()?

Why is the function not using that ready to use helper? Looking at
it ...

>static void __vmx_deliver_posted_interrupt(struct vcpu *v)
>{
>bool_t running = v->is_running;
>
>vcpu_unblock(v);
>if ( running && (in_irq() || (v != current)) )
>{
>unsigned int cpu = v->processor;
>
>if ( !test_and_set_bit(VCPU_KICK_SOFTIRQ, _pending(cpu))

... this line as well as ...

> && (cpu != smp_processor_id()) )
>send_IPI_mask(cpumask_of(cpu), posted_intr_vector);

... this one ...

>}
>}

... pretty certainly don't belong into vmx.c, or the apparent open
coding of cpu_raise_softirq() would require a justifying comment.

Jan


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

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

[Zhang, Yang Z]

Jan Beulich wrote on 2015-09-07:
 On 07.09.15 at 15:00,  wrote:
>> Jan Beulich wrote on 2015-09-07:
>>> Yang, in this context: Why does __vmx_deliver_posted_interrupt()
>>> not use cpu_raise_softirq(), instead kind of open coding it (see your
>>> d7dafa375b ["VMX: Add posted interrupt supporting"])?
>> 
>> Sorry, I am not in the context. What do you mean of using
>> cpu_raise_softirq() in __vmx_deliver_posted_interrupt()?
> 
> Why is the function not using that ready to use helper? Looking at
> it ...
> 
>> static void __vmx_deliver_posted_interrupt(struct vcpu *v)
>> {
>>bool_t running = v->is_running;
>>
>>vcpu_unblock(v);
>>if ( running && (in_irq() || (v != current)) )
>>{
>>unsigned int cpu = v->processor;
>>
>>if ( !test_and_set_bit(VCPU_KICK_SOFTIRQ, _pending(cpu))
> 
> ... this line as well as ...
> 
>> && (cpu != smp_processor_id()) )
>>send_IPI_mask(cpumask_of(cpu), posted_intr_vector);
> 
> ... this one ...
> 
>>}
>> }
> 
> ... pretty certainly don't belong into vmx.c, or the apparent open
> coding of cpu_raise_softirq() would require a justifying comment.

I still don't see how to use cpu_raise_softirq() since the posted_intr_vector 
doesn't belong to softirq.

Best regards,
Yang



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

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

[Wu, Feng]

> -Original Message-
> From: Jan Beulich [mailto:jbeul...@suse.com]
> Sent: Monday, September 07, 2015 8:10 PM
> To: Wu, Feng; Zhang, Yang Z
> Cc: Andrew Cooper; Tian, Kevin; xen-devel@lists.xen.org; Keir Fraser
> Subject: Re: [PATCH v6 15/18] vmx: Properly handle notification event when
> vCPU is running
> 
> >>> On 25.08.15 at 03:57,  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(_intr_vector,
> event_check_interrupt);
> > +alloc_direct_apic_vector(_intr_vector,
> pi_notification_interrupt);
> >
> >  if ( iommu_intpost )
> >  alloc_direct_apic_vector(_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.

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

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

Thanks,
Feng


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

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

[Feng Wu]

When a vCPU is running in Root mode and a notification event
has been injected to it. we need to set VCPU_KICK_SOFTIRQ for
the current cpu, so the pending interrupt in PIRR will be
synced to vIRR before VM-Exit in time.

CC: Kevin Tian kevin.t...@intel.com
CC: Keir Fraser k...@xen.org
CC: Jan Beulich jbeul...@suse.com
CC: Andrew Cooper andrew.coop...@citrix.com
Signed-off-by: Feng Wu feng...@intel.com
Acked-by: Kevin Tian kevin.t...@intel.com
---
v6:
- Ack the interrupt in the beginning of pi_notification_interrupt()

v4:
- Coding style.

v3:
- Make pi_notification_interrupt() static

 xen/arch/x86/hvm/vmx/vmx.c | 48 +-
 1 file changed, 47 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 9cde9a4..5167fae 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -2035,6 +2035,52 @@ static void pi_wakeup_interrupt(struct cpu_user_regs 
*regs)
 this_cpu(irq_count)++;
 }
 
+/* Handle VT-d posted-interrupt when VCPU is running. */
+static void pi_notification_interrupt(struct cpu_user_regs *regs)
+{
+ack_APIC_irq();
+
+/*
+ * We get here when a vCPU is running in root-mode (such as via hypercall,
+ * or any other reasons which can result in VM-Exit), and before vCPU is
+ * back to non-root, external interrupts from an assigned device happen
+ * and a notification event is delivered to this logical CPU.
+ *
+ * we need to set VCPU_KICK_SOFTIRQ for the current cpu, just like
+ * __vmx_deliver_posted_interrupt(). So the pending interrupt in PIRR will
+ * be synced to vIRR before VM-Exit in time.
+ *
+ * Please refer to the following code fragments from
+ * xen/arch/x86/hvm/vmx/entry.S:
+ *
+ * .Lvmx_do_vmentry
+ *
+ *  ..
+ *  point 1
+ *
+ *  cmp  %ecx,(%rdx,%rax,1)
+ *  jnz  .Lvmx_process_softirqs
+ *
+ *  ..
+ *
+ *  je   .Lvmx_launch
+ *
+ *  ..
+ *
+ * .Lvmx_process_softirqs:
+ *  sti
+ *  call do_softirq
+ *  jmp  .Lvmx_do_vmentry
+ *
+ * If VT-d engine issues a notification event at point 1 above, it cannot
+ * be delivered to the guest during this VM-entry without raising the
+ * softirq in this notification handler.
+ */
+raise_softirq(VCPU_KICK_SOFTIRQ);
+
+this_cpu(irq_count)++;
+}
+
 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);
-- 
2.1.0


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