Xen devel - Nov 2008 - [PATCH] Dom0 Kernel - Fixes for saving/restoring MSI/MSI-X across Dom0 S3

Hi, Keir,

This patch is a bugfix for saving and restoring MSI/MSI-X across S3. Currently,
Dom0''s PCI layer unmaps MSI when S3 and maps them back when resuming.
However, this triggers unexpected behaviors. For example, if the drivers still
holds that irq at the point of unmapping MSI, Xen will force unbind that pirq.
But after resume, we have no mechanism to rebind that pirq. The device can not
receive interrupt then.
With this patch, MSI/MSI-X capabilities and tables are saved in Dom0 when S3 and
restored when resume. Actually, this is also the approach that kernel takes. The
only concern is that Dom0 should not touch MSI/MSI-X, they are owned by VMM
itself. Maybe adding a hypercall to instruct Xen to do the saving/restoring is
good. I wonder whether the reason is strong enough for adding a hypercall for
such purpose.

So Keir, maybe you can tell what is your prefer?

Best Regards
Haitao Shan


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

>>> "Shan, Haitao" <haitao.shan@intel.com> 24.11.08
12:03 >>>
>With this patch, MSI/MSI-X capabilities and tables are saved in Dom0 when
>S3 and restored when resume. Actually, this is also the approach that
>kernel takes. The only concern is that Dom0 should not touch MSI/MSI-X,
>they are owned by VMM itself. Maybe adding a hypercall to instruct Xen to
>do the saving/restoring is good. I wonder whether the reason is strong
>enough for adding a hypercall for such purpose.
Is it at all necessary to use a hypercall here? Shouldn''t Xen itself be
able to
do the necessary saving/restoring (just like it does for IO-APIC)?

Jan


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

On 24/11/08 11:41, "Jan Beulich" <jbeulich@novell.com> wrote:
>>>> "Shan, Haitao" <haitao.shan@intel.com> 24.11.08
12:03 >>>
>> With this patch, MSI/MSI-X capabilities and tables are saved in Dom0
when
>> S3 and restored when resume. Actually, this is also the approach that
>> kernel takes. The only concern is that Dom0 should not touch MSI/MSI-X,
>> they are owned by VMM itself. Maybe adding a hypercall to instruct Xen
to
>> do the saving/restoring is good. I wonder whether the reason is strong
>> enough for adding a hypercall for such purpose.
> 
> Is it at all necessary to use a hypercall here? Shouldn''t Xen
itself be able
> to
> do the necessary saving/restoring (just like it does for IO-APIC)?
I was thinking the same thing. Haitao: shall I hold off on your original
patch while we think about this?

 -- Keir



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

>From: Keir Fraser
>Sent: Monday, November 24, 2008 8:04 PM
>
>On 24/11/08 11:41, "Jan Beulich" <jbeulich@novell.com>
wrote:
>
>>>>> "Shan, Haitao" <haitao.shan@intel.com>
24.11.08 12:03 >>>
>>> With this patch, MSI/MSI-X capabilities and tables are 
>saved in Dom0 when
>>> S3 and restored when resume. Actually, this is also the 
>approach that
>>> kernel takes. The only concern is that Dom0 should not 
>touch MSI/MSI-X,
>>> they are owned by VMM itself. Maybe adding a hypercall to 
>instruct Xen to
>>> do the saving/restoring is good. I wonder whether the 
>reason is strong
>>> enough for adding a hypercall for such purpose.
>> 
>> Is it at all necessary to use a hypercall here? Shouldn''t 
>Xen itself be able
>> to
>> do the necessary saving/restoring (just like it does for IO-APIC)?
>
>I was thinking the same thing. Haitao: shall I hold off on 
>your original
>patch while we think about this?
>
It''s possible that given device has been placed in D3cold state,
and then no change left for Xen to save.

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

>>> "Tian, Kevin" <kevin.tian@intel.com> 24.11.08 13:53
>>>
>It''s possible that given device has been placed in D3cold state,
>and then no change left for Xen to save.
But isn''t it the driver''s resume handler that would have to
restore (i.e.
re-initialize) MSI in that case?

Jan


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

No problem, given that MSI is disabled by default now.
Actually, this patch is a fix for xen3.3 and some distributions based on xen3.3,
where MSI is still available via options.
The bug shows: S3 failed when SATA mode is set to be AHCI. I think Jan might
know this bug already.
At least, it can be viewed as a workaround if someone is really in need of that.

Shan Haitao

-----Original Message-----
From: Keir Fraser [mailto:keir.fraser@eu.citrix.com] 
Sent: 2008年11月24日 20:04
To: Jan Beulich; Shan, Haitao
Cc: ''xen-devel@lists.xensource.com''
Subject: Re: [Xen-devel] [PATCH] Dom0 Kernel - Fixes for
saving/restoringMSI/MSI-X across Dom0 S3

On 24/11/08 11:41, "Jan Beulich" <jbeulich@novell.com> wrote:
>>>> "Shan, Haitao" <haitao.shan@intel.com> 24.11.08
12:03 >>>
>> With this patch, MSI/MSI-X capabilities and tables are saved in Dom0
when
>> S3 and restored when resume. Actually, this is also the approach that
>> kernel takes. The only concern is that Dom0 should not touch MSI/MSI-X,
>> they are owned by VMM itself. Maybe adding a hypercall to instruct Xen
to
>> do the saving/restoring is good. I wonder whether the reason is strong
>> enough for adding a hypercall for such purpose.
> 
> Is it at all necessary to use a hypercall here? Shouldn''t Xen
itself be able
> to
> do the necessary saving/restoring (just like it does for IO-APIC)?
I was thinking the same thing. Haitao: shall I hold off on your original
patch while we think about this?

 -- Keir



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

>From: Jan Beulich [mailto:jbeulich@novell.com] 
>Sent: Monday, November 24, 2008 9:35 PM
>
>>>> "Tian, Kevin" <kevin.tian@intel.com> 24.11.08
13:53 >>>
>>It''s possible that given device has been placed in D3cold
state,
>>and then no change left for Xen to save.
>
>But isn''t it the driver''s resume handler that would have
to
>restore (i.e.
>re-initialize) MSI in that case?
>
Yes, that''s my original assumption. I was told by Haitao that initial
msi support is designed in such way that dom0 is delibrately 
prevented from touch msi state, and now it looks like that Haitao
is adding back those supposed-to-be lines from native msi.c. But
I may be wrong about background and let Haitao to clarify later. :-)

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

Yes, clear enough as Kevin said. That is what I saw why S3 failed when AHCI is
enabled (AHCI uses MSI). I do not know whether it is also the reason that Jan
sees the need to add force unbind support of MSI.
I have another question for saving/restoring in Xen if we do not use a new
hypercall. Devices are controller by dom0. At the point Xen wants to save MSI
during S3, dom0 may already places that device in D3hot state, or it may also
cease the device''s function via pci_disable_device. I doubt whether Xen
can read device MMIO at that time.

Shan Haitao

-----Original Message-----
From: Tian, Kevin 
Sent: 2008年11月24日 21:46
To: ''Jan Beulich''
Cc: ''Keir Fraser''; Shan, Haitao;
''xen-devel@lists.xensource.com''
Subject: RE: [Xen-devel] [PATCH] Dom0 Kernel - Fixes
forsaving/restoringMSI/MSI-X across Dom0 S3
>From: Jan Beulich [mailto:jbeulich@novell.com] 
>Sent: Monday, November 24, 2008 9:35 PM
>
>>>> "Tian, Kevin" <kevin.tian@intel.com> 24.11.08
13:53 >>>
>>It''s possible that given device has been placed in D3cold
state,
>>and then no change left for Xen to save.
>
>But isn''t it the driver''s resume handler that would have
to
>restore (i.e.
>re-initialize) MSI in that case?
>
Yes, that''s my original assumption. I was told by Haitao that initial
msi support is designed in such way that dom0 is delibrately 
prevented from touch msi state, and now it looks like that Haitao
is adding back those supposed-to-be lines from native msi.c. But
I may be wrong about background and let Haitao to clarify later. :-)

Thanks,
Kevin

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

On 24/11/08 11:03, "Shan, Haitao" <haitao.shan@intel.com> wrote:
> With this patch, MSI/MSI-X capabilities and tables are saved in Dom0 when
S3
> and restored when resume. Actually, this is also the approach that kernel
> takes. The only concern is that Dom0 should not touch MSI/MSI-X, they are
> owned by VMM itself. Maybe adding a hypercall to instruct Xen to do the
> saving/restoring is good. I wonder whether the reason is strong enough for
> adding a hypercall for such purpose.
> 
> So Keir, maybe you can tell what is your prefer?
I think the approach in your patch is okay for now.

 -- Keir



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

>>> "Shan, Haitao" <haitao.shan@intel.com> 24.11.08
14:55 >>>
>Yes, clear enough as Kevin said. That is what I saw why S3 failed when AHCI
is enabled (AHCI uses MSI). I do not know whether it
>is also the reason that Jan sees the need to add force unbind support of
MSI.
>I have another question for saving/restoring in Xen if we do not use a new
hypercall. Devices are controller by dom0. At the point
>Xen wants to save MSI during S3, dom0 may already places that device in
D3hot state, or it may also cease the device''s function
>via pci_disable_device. I doubt whether Xen can read device MMIO at that
time.
But - as asked before - shouldn''t the device undergo full
re-initialization after coming out of D3? Iirc, the AHCI/S3 bug results in a
cannot-rebind-because-already-bound error, which would indicate to me that the
driver tries to create a new binding, and hence doesn''t really need the
device''s state saved (admitted, I didn''t look closely at the
driver''s code so far). It instead may just need Xen to clean up its
internal state properly. But really, I''m in no way a suspend/resume
expert...

Jan


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

Jan,
I have checked the code again. I still can not find where AHCI driver finalize
and reinitialize the MSI. The driver simply writes its internal registers so
that the device does not *generate* interrupts and invoke pci_save_state (which
will invoke our MSI code). Similar is the resume process.
For cannot-rebind-because-already-bound error, I do not mean it is the error
cause by device rebinding MSI. It is us not the device driver that force unbinds
MSI. After resume, no one does the rebinding work now.

Best Regards
Shan Haitao 

-----Original Message-----
From: Jan Beulich [mailto:jbeulich@novell.com] 
Sent: 2008年11月24日 23:33
To: Shan, Haitao; Tian, Kevin
Cc: ''Keir Fraser'';
''xen-devel@lists.xensource.com''
Subject: RE: [Xen-devel] [PATCH] Dom0 Kernel - Fixes
forsaving/restoringMSI/MSI-X across Dom0 S3
>>> "Shan, Haitao" <haitao.shan@intel.com> 24.11.08
14:55 >>>
>Yes, clear enough as Kevin said. That is what I saw why S3 failed when AHCI
is enabled (AHCI uses MSI). I do not know whether it
>is also the reason that Jan sees the need to add force unbind support of
MSI.
>I have another question for saving/restoring in Xen if we do not use a new
hypercall. Devices are controller by dom0. At the point
>Xen wants to save MSI during S3, dom0 may already places that device in
D3hot state, or it may also cease the device''s function
>via pci_disable_device. I doubt whether Xen can read device MMIO at that
time.
But - as asked before - shouldn''t the device undergo full
re-initialization after coming out of D3? Iirc, the AHCI/S3 bug results in a
cannot-rebind-because-already-bound error, which would indicate to me that the
driver tries to create a new binding, and hence doesn''t really need the
device''s state saved (admitted, I didn''t look closely at the
driver''s code so far). It instead may just need Xen to clean up its
internal state properly. But really, I''m in no way a suspend/resume
expert...

Jan


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