[PATCH] Dom0 Kernel - Fixes for saving/restoring MSI/MSI-X across Dom0 S3

[PATCH] Dom0 Kernel - Fixes for saving/restoring MSI/MSI-X across Dom0 S3

[Shan, Haitao]

[-- Attachment #1: Type: text/plain, Size: 916 bytes --]

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

[-- Attachment #2: msi.patch --]
[-- Type: application/octet-stream, Size: 8249 bytes --]

diff -r 832aac894efd drivers/pci/msi-xen.c
--- a/drivers/pci/msi-xen.c	Wed Nov 19 13:15:46 2008 +0000
+++ b/drivers/pci/msi-xen.c	Mon Nov 24 15:51:26 2008 +0800
@@ -48,6 +48,13 @@
 	struct list_head list;
 	int pirq;
 	int entry_nr;
+#ifdef CONFIG_PM
+	/* PM save area for MSIX address/data */
+	void __iomem *mask_base;
+	u32	address_hi_save;
+	u32	address_lo_save;
+	u32	data_save;
+#endif
 };
 
 static struct msi_dev_list *get_msi_dev_pirq_list(struct pci_dev *dev)
@@ -83,7 +90,7 @@
 	return ret;
 }
 
-static int attach_pirq_entry(int pirq, int entry_nr,
+static int attach_pirq_entry(int pirq, int entry_nr, u64 table_base,
                              struct msi_dev_list *msi_dev_entry)
 {
 	struct msi_pirq_entry *entry = kmalloc(sizeof(*entry), GFP_ATOMIC);
@@ -93,6 +100,9 @@
 		return -ENOMEM;
 	entry->pirq = pirq;
 	entry->entry_nr = entry_nr;
+#ifdef COMFIG_PM
+	entry->mask_base = table_base;
+#endif
 	spin_lock_irqsave(&msi_dev_entry->pirq_list_lock, flags);
 	list_add_tail(&entry->list, &msi_dev_entry->pirq_list_head);
 	spin_unlock_irqrestore(&msi_dev_entry->pirq_list_lock, flags);
@@ -299,104 +309,173 @@
 #ifdef CONFIG_PM
 int pci_save_msi_state(struct pci_dev *dev)
 {
-	int pos;
+	int pos, i = 0;
+	u16 control;
+	struct pci_cap_saved_state *save_state;
+	u32 *cap;
 
 	pos = pci_find_capability(dev, PCI_CAP_ID_MSI);
 	if (pos <= 0 || dev->no_msi)
 		return 0;
 
-	if (!dev->msi_enabled)
+	pci_read_config_word(dev, msi_control_reg(pos), &control);
+	if (!(control & PCI_MSI_FLAGS_ENABLE))
 		return 0;
 
-	/* Restore dev->irq to its default pin-assertion vector */
-	msi_unmap_pirq(dev, dev->irq);
-	/* Disable MSI mode */
-	disable_msi_mode(dev, pos, PCI_CAP_ID_MSI);
-	/* Set the flags for use of restore */
-	dev->msi_enabled = 1;
+	save_state = kzalloc(sizeof(struct pci_cap_saved_state) + sizeof(u32) * 5,
+		GFP_KERNEL);
+	if (!save_state) {
+		printk(KERN_ERR "Out of memory in pci_save_msi_state\n");
+		return -ENOMEM;
+	}
+	cap = &save_state->data[0];
+
+	pci_read_config_dword(dev, pos, &cap[i++]);
+	control = cap[0] >> 16;
+	pci_read_config_dword(dev, pos + PCI_MSI_ADDRESS_LO, &cap[i++]);
+	if (control & PCI_MSI_FLAGS_64BIT) {
+		pci_read_config_dword(dev, pos + PCI_MSI_ADDRESS_HI, &cap[i++]);
+		pci_read_config_dword(dev, pos + PCI_MSI_DATA_64, &cap[i++]);
+	} else
+		pci_read_config_dword(dev, pos + PCI_MSI_DATA_32, &cap[i++]);
+	if (control & PCI_MSI_FLAGS_MASKBIT)
+		pci_read_config_dword(dev, pos + PCI_MSI_MASK_BIT, &cap[i++]);
+	save_state->cap_nr = PCI_CAP_ID_MSI;
+	pci_add_saved_cap(dev, save_state);
 	return 0;
 }
 
 void pci_restore_msi_state(struct pci_dev *dev)
 {
-	int pos, pirq;
+	int i = 0, pos;
+	u16 control;
+	struct pci_cap_saved_state *save_state;
+	u32 *cap;
 
+	save_state = pci_find_saved_cap(dev, PCI_CAP_ID_MSI);
 	pos = pci_find_capability(dev, PCI_CAP_ID_MSI);
-	if (pos <= 0)
+	if (!save_state || pos <= 0)
 		return;
+	cap = &save_state->data[0];
 
-	if (!dev->msi_enabled)
-		return;
-
-	pirq = msi_map_pirq_to_vector(dev, dev->irq, 0, 0);
-	if (pirq < 0)
-		return;
+	control = cap[i++] >> 16;
+	pci_write_config_dword(dev, pos + PCI_MSI_ADDRESS_LO, cap[i++]);
+	if (control & PCI_MSI_FLAGS_64BIT) {
+		pci_write_config_dword(dev, pos + PCI_MSI_ADDRESS_HI, cap[i++]);
+		pci_write_config_dword(dev, pos + PCI_MSI_DATA_64, cap[i++]);
+	} else
+		pci_write_config_dword(dev, pos + PCI_MSI_DATA_32, cap[i++]);
+	if (control & PCI_MSI_FLAGS_MASKBIT)
+		pci_write_config_dword(dev, pos + PCI_MSI_MASK_BIT, cap[i++]);
+	pci_write_config_word(dev, pos + PCI_MSI_FLAGS, control);
 	enable_msi_mode(dev, pos, PCI_CAP_ID_MSI);
+	pci_remove_saved_cap(save_state);
+	kfree(save_state);
 }
 
 int pci_save_msix_state(struct pci_dev *dev)
 {
 	int pos;
+	u16 control;
+	struct pci_cap_saved_state *save_state;
 	unsigned long flags;
 	struct msi_dev_list *msi_dev_entry;
-	struct msi_pirq_entry *pirq_entry, *tmp;
+	struct msi_pirq_entry *pirq_entry;
 
 	pos = pci_find_capability(dev, PCI_CAP_ID_MSIX);
 	if (pos <= 0 || dev->no_msi)
 		return 0;
 
+	printk(KERN_CRIT "Saving MSIX cap\n");
+
 	/* save the capability */
-	if (!dev->msix_enabled)
+	pci_read_config_word(dev, msi_control_reg(pos), &control);
+	if (!(control & PCI_MSIX_FLAGS_ENABLE))
 		return 0;
+	save_state = kzalloc(sizeof(struct pci_cap_saved_state) + sizeof(u16),
+		GFP_KERNEL);
+	if (!save_state) {
+		printk(KERN_ERR "Out of memory in pci_save_msix_state\n");
+		return -ENOMEM;
+	}
+	*((u16 *)&save_state->data[0]) = control;
 
 	msi_dev_entry = get_msi_dev_pirq_list(dev);
 
 	spin_lock_irqsave(&msi_dev_entry->pirq_list_lock, flags);
-        list_for_each_entry_safe(pirq_entry, tmp,
-                                 &msi_dev_entry->pirq_list_head, list)
-		msi_unmap_pirq(dev, pirq_entry->pirq);
+	list_for_each_entry(pirq_entry, &msi_dev_entry->pirq_list_head, list) {
+		int j;
+		void __iomem *base;
+
+		/* save the table */
+		base = pirq_entry->mask_base;
+		j = pirq_entry->entry_nr;
+		printk(KERN_CRIT "Save msix table entry %d pirq %x base %p\n",
+		       j, pirq_entry->pirq, base);
+
+		pirq_entry->address_lo_save =
+			readl(base + j * PCI_MSIX_ENTRY_SIZE +
+			      PCI_MSIX_ENTRY_LOWER_ADDR_OFFSET);
+		pirq_entry->address_hi_save =
+			readl(base + j * PCI_MSIX_ENTRY_SIZE +
+			      PCI_MSIX_ENTRY_UPPER_ADDR_OFFSET);
+		pirq_entry->data_save =
+			readl(base + j * PCI_MSIX_ENTRY_SIZE +
+			      PCI_MSIX_ENTRY_DATA_OFFSET);
+	}
 	spin_unlock_irqrestore(&msi_dev_entry->pirq_list_lock, flags);
 
-	disable_msi_mode(dev, pos, PCI_CAP_ID_MSIX);
-	/* Set the flags for use of restore */
-	dev->msix_enabled = 1;
-
+	save_state->cap_nr = PCI_CAP_ID_MSIX;
+	pci_add_saved_cap(dev, save_state);
 	return 0;
 }
 
 void pci_restore_msix_state(struct pci_dev *dev)
 {
-	int pos;
+	u16 save;
+	int pos, j;
+	void __iomem *base;
+	struct pci_cap_saved_state *save_state;
 	unsigned long flags;
-	u64 table_base;
 	struct msi_dev_list *msi_dev_entry;
-	struct msi_pirq_entry *pirq_entry, *tmp;
+	struct msi_pirq_entry *pirq_entry;
+
+	save_state = pci_find_saved_cap(dev, PCI_CAP_ID_MSIX);
+	if (!save_state)
+		return;
+	printk(KERN_CRIT "Restoring MSIX cap\n");
+
+	save = *((u16 *)&save_state->data[0]);
+	pci_remove_saved_cap(save_state);
+	kfree(save_state);
 
 	pos = pci_find_capability(dev, PCI_CAP_ID_MSIX);
 	if (pos <= 0)
 		return;
 
-	if (!dev->msix_enabled)
-		return;
-
 	msi_dev_entry = get_msi_dev_pirq_list(dev);
-	table_base = find_table_base(dev, pos);
-	if (!table_base)
-		return;
 
 	spin_lock_irqsave(&msi_dev_entry->pirq_list_lock, flags);
-	list_for_each_entry_safe(pirq_entry, tmp,
-				 &msi_dev_entry->pirq_list_head, list) {
-		int rc = msi_map_pirq_to_vector(dev, pirq_entry->pirq,
-						pirq_entry->entry_nr, table_base);
-		if (rc < 0)
-			printk(KERN_WARNING
-			       "%s: re-mapping irq #%d (pirq%d) failed: %d\n",
-			       pci_name(dev), pirq_entry->entry_nr,
-			       pirq_entry->pirq, rc);
+	list_for_each_entry(pirq_entry, &msi_dev_entry->pirq_list_head, list) {
+		/* route the table */
+		base = pirq_entry->mask_base;
+		j = pirq_entry->entry_nr;
+
+		printk(KERN_CRIT "Restore msix table entry %d pirq %x base %p\n",
+		       j, pirq_entry->pirq, base);
+		writel(pirq_entry->address_lo_save,
+			base + j * PCI_MSIX_ENTRY_SIZE +
+			PCI_MSIX_ENTRY_LOWER_ADDR_OFFSET);
+		writel(pirq_entry->address_hi_save,
+			base + j * PCI_MSIX_ENTRY_SIZE +
+			PCI_MSIX_ENTRY_UPPER_ADDR_OFFSET);
+		writel(pirq_entry->data_save,
+			base + j * PCI_MSIX_ENTRY_SIZE +
+			PCI_MSIX_ENTRY_DATA_OFFSET);
 	}
 	spin_unlock_irqrestore(&msi_dev_entry->pirq_list_lock, flags);
 
+	pci_write_config_word(dev, msi_control_reg(pos), save);
 	enable_msi_mode(dev, pos, PCI_CAP_ID_MSIX);
 }
 #endif
@@ -475,7 +554,7 @@
 		pirq = msi_map_vector(dev, entries[i].entry, table_base);
 		if (pirq < 0)
 			break;
-		attach_pirq_entry(pirq, entries[i].entry, msi_dev_entry);
+		attach_pirq_entry(pirq, entries[i].entry, table_base, msi_dev_entry);
 		(entries + i)->vector = pirq;
 	}
 
@@ -660,7 +739,7 @@
 			if (mapped)
 				continue;
 			irq = evtchn_map_pirq(-1, entries[i].vector);
-			attach_pirq_entry(irq, entries[i].entry, msi_dev_entry);
+			attach_pirq_entry(irq, entries[i].entry, 0, msi_dev_entry);
 			entries[i].vector = irq;
 		}
         return 0;

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

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

Re: [PATCH] Dom0 Kernel - Fixes for saving/restoringMSI/MSI-X across Dom0 S3

[Jan Beulich]

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

Re: [PATCH] Dom0 Kernel - Fixes for saving/restoringMSI/MSI-X across Dom0 S3

[Keir Fraser]

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

RE: [PATCH] Dom0 Kernel - Fixes for saving/restoringMSI/MSI-X across Dom0 S3

[Tian, Kevin]

>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

RE: [PATCH] Dom0 Kernel - Fixes forsaving/restoringMSI/MSI-X across Dom0 S3

[Jan Beulich]

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

RE: [PATCH] Dom0 Kernel - Fixes forsaving/restoringMSI/MSI-X across Dom0 S3

[Tian, Kevin]

>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

RE: [PATCH] Dom0 Kernel - Fixes forsaving/restoringMSI/MSI-X across Dom0 S3

[Shan, Haitao]

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

RE: [PATCH] Dom0 Kernel - Fixes forsaving/restoringMSI/MSI-X across Dom0 S3

[Jan Beulich]

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

RE: [PATCH] Dom0 Kernel - Fixes forsaving/restoringMSI/MSI-X across Dom0 S3

[Shan, Haitao]

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

RE: [PATCH] Dom0 Kernel - Fixes for saving/restoringMSI/MSI-X across Dom0 S3

[Shan, Haitao]

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

Re: [PATCH] Dom0 Kernel - Fixes for saving/restoring MSI/MSI-X across Dom0 S3

[Keir Fraser]

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