Xen devel - Oct 2007 - [VTD][RESEND]add a timer for the shared interrupt issue for vt-d

Keir,

It''s a resending patch for the timeout mechanism to deal with the
shared
interrupt issue for vt-d enabled hvm guest. 

We modify the patch following your comments last time and make some
other small fix: 

1)       We don''t touch the locking around the hvm_dpci_eoi().

2)       Remove the HZ from the TIME_OUT_PERIOD macro which may confuse
others.

3)       Add some explanation to the return value for the
hvm_pci_intx_assert(), and the has_timer parameter for the hvm_dpci_eoi.

We have tested it with shared interrupt between dom0/HVM(pcie/disk) and
HVM/HVM(pcie/pcie).

 

Signed-off-by: Xin, Xiaohui<xiaohui.xin@intel.com

Signed-off-by: Kevin Tian <kevin.tian@intel.com>

 

 




_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Getting better, some more points:
 1. The timeout values are bizarrely non-round (8192000). Do you really need
4 significant figures of precision? Do you really mean for an 8ms timeout?
It would then be better to define as MILLISECS(8). Much clearer as it shows
the units.
 2. You can¹t stick timer_hvm[] in the generic common domain.c. It belongs
in the hvm_irq_dpci structure, making it clear it is HVM-specific, and also
meaning that memory for the array is only allocated for those domains that
actually have devices passed through to them.
 3. The extra argument to hvm_dpci_eoi() is used only to decide whether to
stop_timer() or not. You¹re aware you can call stop_timer() quite safely
from within the timeout callback handler? I think that means this extra
argument is unnecessary.
 4. The comment added to hvm_pci_intx_assert() is delightfully vague. Also I
think it may be wrong ‹ do you really mean ³before the driver loading, the
vioapic redir entry may be unmasked²?

So you need another spin. I¹m still unconvinced that the
hvm_pci_intx_assert() changes are the right way to go, but I need to
consider the patch some more to see if there is a cleaner way. If there is I
will modify the patch myself before I check it in. The other points listed
above you need to address yourself.

 -- Keir

On 17/10/07 14:44, "Xin, Xiaohui" <xiaohui.xin@intel.com> wrote:
> Keir,
> It¹s a resending patch for the timeout mechanism to deal with the shared
> interrupt issue for vt-d enabled hvm guest.
> We modify the patch following your comments last time and make some other
> small fix: 
> 1)       We don¹t touch the locking around the hvm_dpci_eoi().
> 2)       Remove the HZ from the TIME_OUT_PERIOD macro which may confuse
> others.
> 3)       Add some explanation to the return value for the
> hvm_pci_intx_assert(), and the has_timer parameter for the hvm_dpci_eoi.
> We have tested it with shared interrupt between dom0/HVM(pcie/disk) and
> HVM/HVM(pcie/pcie).
>  
> Signed-off-by: Xin, Xiaohui<xiaohui.xin@intel.com
> Signed-off-by: Kevin Tian <kevin.tian@intel.com>
>  
>  
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Keir,

Thanks for your points. This is the updated patch. I have fixed 1,2,3,4 of your
comments.

 

Signed-off-by: Xin, Xiaohui<xiaohui.xin@intel.com
Signed-off-by: Kevin Tian <kevin.tian@intel.com>



 

________________________________

From: Keir Fraser [mailto:Keir.Fraser@cl.cam.ac.uk] 
Sent: 2007年10月18日 17:36
To: Xin, Xiaohui; xen-devel@lists.xensource.com
Subject: Re: [Xen-devel][VTD][RESEND]add a timer for the shared interrupt issue
for vt-d

 

Getting better, some more points:
 1. The timeout values are bizarrely non-round (8192000). Do you really need 4
significant figures of precision? Do you really mean for an 8ms timeout? It
would then be better to define as MILLISECS(8). Much clearer as it shows the
units.
 2. You can’t stick timer_hvm[] in the generic common domain.c. It belongs in
the hvm_irq_dpci structure, making it clear it is HVM-specific, and also meaning
that memory for the array is only allocated for those domains that actually have
devices passed through to them.
 3. The extra argument to hvm_dpci_eoi() is used only to decide whether to
stop_timer() or not. You’re aware you can call stop_timer() quite safely from
within the timeout callback handler? I think that means this extra argument is
unnecessary.
 4. The comment added to hvm_pci_intx_assert() is delightfully vague. Also I
think it may be wrong — do you really mean “before the driver loading, the
vioapic redir entry may be unmasked”?

So you need another spin. I’m still unconvinced that the hvm_pci_intx_assert()
changes are the right way to go, but I need to consider the patch some more to
see if there is a cleaner way. If there is I will modify the patch myself before
I check it in. The other points listed above you need to address yourself.

 -- Keir

On 17/10/07 14:44, "Xin, Xiaohui" <xiaohui.xin@intel.com> wrote:

Keir,
It’s a resending patch for the timeout mechanism to deal with the shared
interrupt issue for vt-d enabled hvm guest.
We modify the patch following your comments last time and make some other small
fix:
1)       We don’t touch the locking around the hvm_dpci_eoi().
2)       Remove the HZ from the TIME_OUT_PERIOD macro which may confuse others.
3)       Add some explanation to the return value for the hvm_pci_intx_assert(),
and the has_timer parameter for the hvm_dpci_eoi.
We have tested it with shared interrupt between dom0/HVM(pcie/disk) and
HVM/HVM(pcie/pcie).
 
Signed-off-by: Xin, Xiaohui<xiaohui.xin@intel.com
Signed-off-by: Kevin Tian <kevin.tian@intel.com>

 

________________________________

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

 




_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Applied, but I have the following questions:

 1. Your changes to hvm_pci_intx_assert() have completely disappeared. Did
you decide you didn’t need them after all, or have you simply resolved to do
without them to get the patch in more quickly?
 2. You set_timer() in hvm_do_IRQ_dpci(). Why is this necessary, given that
vmx_dirq_assist() does the same, and should run shortly after since you
vcpu_kick()? Is this to solve the delay between assigning a device to a new
HVM guest and when that HVM guest gets unpaused after it is fully
constructed? In that case we would be better to bail from hvm_do_IRQ_dpci()
if the domain ‘is_paused_by_controller’, and return 0.

 Thanks,
 Keir

On 19/10/07 04:46, "Xin, Xiaohui" <xiaohui.xin@intel.com> wrote:
> Keir,
> Thanks for your points. This is the updated patch. I have fixed 1,2,3,4 of
> your comments. 
>  
> Signed-off-by: Xin, Xiaohui<xiaohui.xin@intel.com
> Signed-off-by: Kevin Tian <kevin.tian@intel.com>
> 
> 
>  
> 
> 
> From: Keir Fraser [mailto:Keir.Fraser@cl.cam.ac.uk]
> Sent: 2007年10月18日 17:36
> To: Xin, Xiaohui; xen-devel@lists.xensource.com
> Subject: Re: [Xen-devel][VTD][RESEND]add a timer for the shared interrupt
> issue for vt-d
>  
> Getting better, some more points:
>  1. The timeout values are bizarrely non-round (8192000). Do you really
need 4
> significant figures of precision? Do you really mean for an 8ms timeout? It
> would then be better to define as MILLISECS(8). Much clearer as it shows
the
> units.
>  2. You can’t stick timer_hvm[] in the generic common domain.c. It belongs
in
> the hvm_irq_dpci structure, making it clear it is HVM-specific, and also
> meaning that memory for the array is only allocated for those domains that
> actually have devices passed through to them.
>  3. The extra argument to hvm_dpci_eoi() is used only to decide whether to
> stop_timer() or not. You’re aware you can call stop_timer() quite safely
from
> within the timeout callback handler? I think that means this extra argument
is
> unnecessary.
>  4. The comment added to hvm_pci_intx_assert() is delightfully vague. Also
I
> think it may be wrong — do you really mean “before the driver loading, the
> vioapic redir entry may be unmasked”?
> 
> So you need another spin. I’m still unconvinced that the
hvm_pci_intx_assert()
> changes are the right way to go, but I need to consider the patch some more
to
> see if there is a cleaner way. If there is I will modify the patch myself
> before I check it in. The other points listed above you need to address
> yourself.
> 
>  -- Keir
> 
> On 17/10/07 14:44, "Xin, Xiaohui" <xiaohui.xin@intel.com>
wrote:
> Keir,
> It’s a resending patch for the timeout mechanism to deal with the shared
> interrupt issue for vt-d enabled hvm guest.
> We modify the patch following your comments last time and make some other
> small fix: 
> 1)       We don’t touch the locking around the hvm_dpci_eoi().
> 2)       Remove the HZ from the TIME_OUT_PERIOD macro which may confuse
> others.
> 3)       Add some explanation to the return value for the
> hvm_pci_intx_assert(), and the has_timer parameter for the hvm_dpci_eoi.
> We have tested it with shared interrupt between dom0/HVM(pcie/disk) and
> HVM/HVM(pcie/pcie).
>  
> Signed-off-by: Xin, Xiaohui<xiaohui.xin@intel.com
> Signed-off-by: Kevin Tian <kevin.tian@intel.com>
> 
>  
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
>  
> 



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Keir,

For 1) I have changes the hvm_pci_intx_assert() to be untouched. The reason is
that, I want to get the result of the hvm_pci_intx_assert() since I think I
could get some hints of the failure of the assertion quickly, and I do get the
hints, so in the patches sent out before, I just do pirq_guest_eoi() quickly.
But from your response, seems you don’t like any change to the
hvm_pci_intx_assert(). So I think another way, I can still use timer to deal
with that though that it should be a little slower to call pirq_guest_eoi() at
last, you know, in that situation, if the assert failure, the timer will be
functional still.

For 2) Yes, firstly I think to use the flag of the domain as
“is_pasued_by_controller”, but from the experience and from the log file, at the
situation which IDE shares interrupt with NIC card, and when the NIC is assigned
to HVM guest, “is pasued_by_controller” can deal with most cases, but after
unpaused, there are still one or more interrupts can be scheduled. The
“is_paused_by_controller” can not deal with all the cases.  So I still use timer
to deal with that because timer is simple.

 

Any other comments?

 

Thanks

Xiaohui

 

________________________________

From: xen-devel-bounces@lists.xensource.com
[mailto:xen-devel-bounces@lists.xensource.com] On Behalf Of Keir Fraser
Sent: 2007年10月19日 18:25
To: Xin, Xiaohui; xen-devel@lists.xensource.com
Subject: Re: [Xen-devel][VTD][PATCH][RESEND]add a timer for the sharedinterrupt
issue for vt-d

 

Applied, but I have the following questions:

 1. Your changes to hvm_pci_intx_assert() have completely disappeared. Did you
decide you didn’t need them after all, or have you simply resolved to do without
them to get the patch in more quickly?
 2. You set_timer() in hvm_do_IRQ_dpci(). Why is this necessary, given that
vmx_dirq_assist() does the same, and should run shortly after since you
vcpu_kick()? Is this to solve the delay between assigning a device to a new HVM
guest and when that HVM guest gets unpaused after it is fully constructed? In
that case we would be better to bail from hvm_do_IRQ_dpci() if the domain
‘is_paused_by_controller’, and return 0.

 Thanks,
 Keir

On 19/10/07 04:46, "Xin, Xiaohui" <xiaohui.xin@intel.com> wrote:

Keir,
Thanks for your points. This is the updated patch. I have fixed 1,2,3,4 of your
comments.
 
Signed-off-by: Xin, Xiaohui<xiaohui.xin@intel.com
Signed-off-by: Kevin Tian <kevin.tian@intel.com>




________________________________

From: Keir Fraser [mailto:Keir.Fraser@cl.cam.ac.uk]
<mailto:Keir.Fraser@cl.cam.ac.uk%5d>
Sent: 2007年10月18日 17:36
To: Xin, Xiaohui; xen-devel@lists.xensource.com
Subject: Re: [Xen-devel][VTD][RESEND]add a timer for the shared interrupt issue
for vt-d

Getting better, some more points:
 1. The timeout values are bizarrely non-round (8192000). Do you really need 4
significant figures of precision? Do you really mean for an 8ms timeout? It
would then be better to define as MILLISECS(8). Much clearer as it shows the
units.
 2. You can’t stick timer_hvm[] in the generic common domain.c. It belongs in
the hvm_irq_dpci structure, making it clear it is HVM-specific, and also meaning
that memory for the array is only allocated for those domains that actually have
devices passed through to them.
 3. The extra argument to hvm_dpci_eoi() is used only to decide whether to
stop_timer() or not. You’re aware you can call stop_timer() quite safely from
within the timeout callback handler? I think that means this extra argument is
unnecessary.
 4. The comment added to hvm_pci_intx_assert() is delightfully vague. Also I
think it may be wrong — do you really mean “before the driver loading, the
vioapic redir entry may be unmasked”?

So you need another spin. I’m still unconvinced that the hvm_pci_intx_assert()
changes are the right way to go, but I need to consider the patch some more to
see if there is a cleaner way. If there is I will modify the patch myself before
I check it in. The other points listed above you need to address yourself.

 -- Keir

On 17/10/07 14:44, "Xin, Xiaohui" <xiaohui.xin@intel.com> wrote:
Keir,
It’s a resending patch for the timeout mechanism to deal with the shared
interrupt issue for vt-d enabled hvm guest.
We modify the patch following your comments last time and make some other small
fix:
1)       We don’t touch the locking around the hvm_dpci_eoi().
2)       Remove the HZ from the TIME_OUT_PERIOD macro which may confuse others.
3)       Add some explanation to the return value for the hvm_pci_intx_assert(),
and the has_timer parameter for the hvm_dpci_eoi.
We have tested it with shared interrupt between dom0/HVM(pcie/disk) and
HVM/HVM(pcie/pcie).
 
Signed-off-by: Xin, Xiaohui<xiaohui.xin@intel.com
Signed-off-by: Kevin Tian <kevin.tian@intel.com>

  

________________________________


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel



 



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Sorry, last mail, For 2) there still one or more interrupt CAN NOT be scheduled
for HVM after “is_paused_by_controller” changed

 

Thanks

Xiaohui. 

 

________________________________

From: Xin, Xiaohui 
Sent: 2007年10月20日 19:05
To: ''Keir Fraser''; xen-devel@lists.xensource.com
Subject: RE: [Xen-devel][VTD][PATCH][RESEND]add a timer for the sharedinterrupt
issue for vt-d

 

Keir,

For 1) I have changes the hvm_pci_intx_assert() to be untouched. The reason is
that, I want to get the result of the hvm_pci_intx_assert() since I think I
could get some hints of the failure of the assertion quickly, and I do get the
hints, so in the patches sent out before, I just do pirq_guest_eoi() quickly.
But from your response, seems you don’t like any change to the
hvm_pci_intx_assert(). So I think another way, I can still use timer to deal
with that though that it should be a little slower to call pirq_guest_eoi() at
last, you know, in that situation, if the assert failure, the timer will be
functional still.

For 2) Yes, firstly I think to use the flag of the domain as
“is_pasued_by_controller”, but from the experience and from the log file, at the
situation which IDE shares interrupt with NIC card, and when the NIC is assigned
to HVM guest, “is pasued_by_controller” can deal with most cases, but after
unpaused, there are still one or more interrupts can be scheduled. The
“is_paused_by_controller” can not deal with all the cases.  So I still use timer
to deal with that because timer is simple.

 

Any other comments?

 

Thanks

Xiaohui

 

________________________________

From: xen-devel-bounces@lists.xensource.com
[mailto:xen-devel-bounces@lists.xensource.com] On Behalf Of Keir Fraser
Sent: 2007年10月19日 18:25
To: Xin, Xiaohui; xen-devel@lists.xensource.com
Subject: Re: [Xen-devel][VTD][PATCH][RESEND]add a timer for the sharedinterrupt
issue for vt-d

 

Applied, but I have the following questions:

 1. Your changes to hvm_pci_intx_assert() have completely disappeared. Did you
decide you didn’t need them after all, or have you simply resolved to do without
them to get the patch in more quickly?
 2. You set_timer() in hvm_do_IRQ_dpci(). Why is this necessary, given that
vmx_dirq_assist() does the same, and should run shortly after since you
vcpu_kick()? Is this to solve the delay between assigning a device to a new HVM
guest and when that HVM guest gets unpaused after it is fully constructed? In
that case we would be better to bail from hvm_do_IRQ_dpci() if the domain
‘is_paused_by_controller’, and return 0.

 Thanks,
 Keir

On 19/10/07 04:46, "Xin, Xiaohui" <xiaohui.xin@intel.com> wrote:

Keir,
Thanks for your points. This is the updated patch. I have fixed 1,2,3,4 of your
comments.
 
Signed-off-by: Xin, Xiaohui<xiaohui.xin@intel.com
Signed-off-by: Kevin Tian <kevin.tian@intel.com>



________________________________

From: Keir Fraser [mailto:Keir.Fraser@cl.cam.ac.uk]
<mailto:Keir.Fraser@cl.cam.ac.uk%5d>
Sent: 2007年10月18日 17:36
To: Xin, Xiaohui; xen-devel@lists.xensource.com
Subject: Re: [Xen-devel][VTD][RESEND]add a timer for the shared interrupt issue
for vt-d

Getting better, some more points:
 1. The timeout values are bizarrely non-round (8192000). Do you really need 4
significant figures of precision? Do you really mean for an 8ms timeout? It
would then be better to define as MILLISECS(8). Much clearer as it shows the
units.
 2. You can’t stick timer_hvm[] in the generic common domain.c. It belongs in
the hvm_irq_dpci structure, making it clear it is HVM-specific, and also meaning
that memory for the array is only allocated for those domains that actually have
devices passed through to them.
 3. The extra argument to hvm_dpci_eoi() is used only to decide whether to
stop_timer() or not. You’re aware you can call stop_timer() quite safely from
within the timeout callback handler? I think that means this extra argument is
unnecessary.
 4. The comment added to hvm_pci_intx_assert() is delightfully vague. Also I
think it may be wrong — do you really mean “before the driver loading, the
vioapic redir entry may be unmasked”?

So you need another spin. I’m still unconvinced that the hvm_pci_intx_assert()
changes are the right way to go, but I need to consider the patch some more to
see if there is a cleaner way. If there is I will modify the patch myself before
I check it in. The other points listed above you need to address yourself.

 -- Keir

On 17/10/07 14:44, "Xin, Xiaohui" <xiaohui.xin@intel.com> wrote:
Keir,
It’s a resending patch for the timeout mechanism to deal with the shared
interrupt issue for vt-d enabled hvm guest.
We modify the patch following your comments last time and make some other small
fix:
1)       We don’t touch the locking around the hvm_dpci_eoi().
2)       Remove the HZ from the TIME_OUT_PERIOD macro which may confuse others.
3)       Add some explanation to the return value for the hvm_pci_intx_assert(),
and the has_timer parameter for the hvm_dpci_eoi.
We have tested it with shared interrupt between dom0/HVM(pcie/disk) and
HVM/HVM(pcie/pcie).
 
Signed-off-by: Xin, Xiaohui<xiaohui.xin@intel.com
Signed-off-by: Kevin Tian <kevin.tian@intel.com>

  

________________________________


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

 



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On 20/10/07 12:04, "Xin, Xiaohui" <xiaohui.xin@intel.com> wrote:
> For 2) Yes, firstly I think to use the flag of the domain as
> ³is_pasued_by_controller², but from the experience and from the log file,
at
> the situation which IDE shares interrupt with NIC card, and when the NIC is
> assigned to HVM guest, ³is pasued_by_controller² can deal with most cases,
but
> after unpaused, there are still one or more interrupts can be scheduled.
The
> ³is_paused_by_controller² can not deal with all the cases.  So I still use
> timer to deal with that because timer is simple.
How about setting the timer unconditionally in hvm_do_IRQ_dpci() (i.e., as
you do now), but then not touching it at all in vmx_dirq_assist()? Is there
much point in resetting the timer in vmx_dirq_assist()? It seems to me you
should have an assert-to-eoi latency that you are prepared to tolerate. Does
separating out assert-to-inject and inject-to-eoi latencies actually make
sense?

 -- Keir



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

The timer value is an experimental value to tolerate. My thought is that is
there some possibility that assert-to-inject is too large and then the
inject-to-eoi is too small that real interrupt for the HVM cannot be finished
normally?

 

Thanks

Xiaohui  

 

________________________________

From: Keir Fraser [mailto:Keir.Fraser@cl.cam.ac.uk] 
Sent: 2007年10月20日 19:24
To: Xin, Xiaohui; xen-devel@lists.xensource.com
Subject: Re: [Xen-devel][VTD][PATCH][RESEND]add a timer for the sharedinterrupt
issue for vt-d

 

On 20/10/07 12:04, "Xin, Xiaohui" <xiaohui.xin@intel.com> wrote:

For 2) Yes, firstly I think to use the flag of the domain as
“is_pasued_by_controller”, but from the experience and from the log file, at the
situation which IDE shares interrupt with NIC card, and when the NIC is assigned
to HVM guest, “is pasued_by_controller” can deal with most cases, but after
unpaused, there are still one or more interrupts can be scheduled. The
“is_paused_by_controller” can not deal with all the cases.  So I still use timer
to deal with that because timer is simple.


How about setting the timer unconditionally in hvm_do_IRQ_dpci() (i.e., as you
do now), but then not touching it at all in vmx_dirq_assist()? Is there much
point in resetting the timer in vmx_dirq_assist()? It seems to me you should
have an assert-to-eoi latency that you are prepared to tolerate. Does separating
out assert-to-inject and inject-to-eoi latencies actually make sense?

 -- Keir



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel