[PATCH] xen/grant-table: fix suspend for non-PV guests

[PATCH] xen/grant-table: fix suspend for non-PV guests

[David Vrabel]

Commit aa8532c32216ae07c3813b9aeb774517878a7573 (xen: refactor suspend
pre/post hooks) broke resuming PVHVM (auto-translated physmap) guests.

The gnttab_suspend() would clear the mapping for the grant table
frames, but the ->unmap_frames() call is only applicable to PV guests.

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
---
 drivers/xen/grant-table.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/xen/grant-table.c b/drivers/xen/grant-table.c
index 6d325bd..5d4de88 100644
--- a/drivers/xen/grant-table.c
+++ b/drivers/xen/grant-table.c
@@ -1168,7 +1168,8 @@ int gnttab_resume(void)
 
 int gnttab_suspend(void)
 {
-	gnttab_interface->unmap_frames();
+	if (!xen_feature(XENFEAT_auto_translated_physmap))
+		gnttab_interface->unmap_frames();
 	return 0;
 }
 
-- 
1.7.10.4

Re: [PATCH] xen/grant-table: fix suspend for non-PV guests

[Konrad Rzeszutek Wilk]

On Mon, Jun 16, 2014 at 12:49:18PM +0100, David Vrabel wrote:
> Commit aa8532c32216ae07c3813b9aeb774517878a7573 (xen: refactor suspend
> pre/post hooks) broke resuming PVHVM (auto-translated physmap) guests.
> 
> The gnttab_suspend() would clear the mapping for the grant table
> frames, but the ->unmap_frames() call is only applicable to PV guests.
> 
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>

You forgot:

Reported-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

However, if the code is structured like this, should we just
make the 'unmap_frames' be a NOP for HVM and instead have:


if (gnttab_interface->unmap_frames)
	gnttab_interface->unmap_frames();

And the HVM path will make sure that 'unmap_frames' is
set to NULL?

Maybe even change the name to 'unmap_frames_pv' ?

> ---
>  drivers/xen/grant-table.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/xen/grant-table.c b/drivers/xen/grant-table.c
> index 6d325bd..5d4de88 100644
> --- a/drivers/xen/grant-table.c
> +++ b/drivers/xen/grant-table.c
> @@ -1168,7 +1168,8 @@ int gnttab_resume(void)
>  
>  int gnttab_suspend(void)
>  {
> -	gnttab_interface->unmap_frames();
> +	if (!xen_feature(XENFEAT_auto_translated_physmap))
> +		gnttab_interface->unmap_frames();
>  	return 0;
>  }
>  
> -- 
> 1.7.10.4
> 

Re: [PATCH] xen/grant-table: fix suspend for non-PV guests

[David Vrabel]

On 16/06/14 15:20, Konrad Rzeszutek Wilk wrote:
> On Mon, Jun 16, 2014 at 12:49:18PM +0100, David Vrabel wrote:
>> Commit aa8532c32216ae07c3813b9aeb774517878a7573 (xen: refactor suspend
>> pre/post hooks) broke resuming PVHVM (auto-translated physmap) guests.
>>
>> The gnttab_suspend() would clear the mapping for the grant table
>> frames, but the ->unmap_frames() call is only applicable to PV guests.
>>
>> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
> 
> You forgot:
> 
> Reported-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

I'm more interested in a Tested-by, any chance of one?

> However, if the code is structured like this, should we just
> make the 'unmap_frames' be a NOP for HVM and instead have:
> 
> if (gnttab_interface->unmap_frames)
> 	gnttab_interface->unmap_frames();
> 
> And the HVM path will make sure that 'unmap_frames' is
> set to NULL?
> 
> Maybe even change the name to 'unmap_frames_pv' ?

That may be useful as part of a wider refactor.  But the use of the
feature test mirrors the use in gnttab_setup() (as called by
gnttab_resume()) so I think it is clearer.

David

Re: [PATCH] xen/grant-table: fix suspend for non-PV guests

[Konrad Rzeszutek Wilk]

On Mon, Jun 16, 2014 at 12:49:18PM +0100, David Vrabel wrote:
> Commit aa8532c32216ae07c3813b9aeb774517878a7573 (xen: refactor suspend
> pre/post hooks) broke resuming PVHVM (auto-translated physmap) guests.
> 
> The gnttab_suspend() would clear the mapping for the grant table
> frames, but the ->unmap_frames() call is only applicable to PV guests.
> 
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>

When I do 'v3.15' + 'stable/for-linus-3.16' merge it fixes the
issue I had seen. You can pluck:

Reported-and-Tested-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Thank you.
> ---
>  drivers/xen/grant-table.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/xen/grant-table.c b/drivers/xen/grant-table.c
> index 6d325bd..5d4de88 100644
> --- a/drivers/xen/grant-table.c
> +++ b/drivers/xen/grant-table.c
> @@ -1168,7 +1168,8 @@ int gnttab_resume(void)
>  
>  int gnttab_suspend(void)
>  {
> -	gnttab_interface->unmap_frames();
> +	if (!xen_feature(XENFEAT_auto_translated_physmap))
> +		gnttab_interface->unmap_frames();
>  	return 0;
>  }
>  
> -- 
> 1.7.10.4
>