x86/shim: fix ballooning up the guest

[Xen-devel] [PATCH] x86/shim: fix ballooning up the guest

Posted by Igor Druzhinin 4 years, 1 month ago

args.preempted as meaningless here and doesn't show if the hypercall
was preempted before. Use start_extent instead which is correct.

Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
---
This fixes ballooning for 64-bit PV guests inside shim. 32-bit PV guests
require a little bit more work due to compat layer being involved.
---
 xen/common/memory.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/common/memory.c b/xen/common/memory.c
index 444c081..5fdd2a2 100644
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -1191,7 +1191,7 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
         }
 
 #ifdef CONFIG_X86
-        if ( pv_shim && op != XENMEM_decrease_reservation && !args.preempted )
+        if ( pv_shim && op != XENMEM_decrease_reservation && !start_extent )
             /* Avoid calling pv_shim_online_memory when preempted. */
             pv_shim_online_memory(args.nr_extents, args.extent_order);
 #endif
-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] x86/shim: fix ballooning up the guest

Posted by Jan Beulich 4 years, 1 month ago

On 16.03.2020 22:47, Igor Druzhinin wrote:
> args.preempted as meaningless here and doesn't show if the hypercall
> was preempted before. Use start_extent instead which is correct.

... as long as the hypercall was invoked in a "normal" way.

> Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
> ---
> This fixes ballooning for 64-bit PV guests inside shim. 32-bit PV guests
> require a little bit more work due to compat layer being involved.
> ---
>  xen/common/memory.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/common/memory.c b/xen/common/memory.c
> index 444c081..5fdd2a2 100644
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -1191,7 +1191,7 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>          }
>  
>  #ifdef CONFIG_X86
> -        if ( pv_shim && op != XENMEM_decrease_reservation && !args.preempted )
> +        if ( pv_shim && op != XENMEM_decrease_reservation && !start_extent )
>              /* Avoid calling pv_shim_online_memory when preempted. */
>              pv_shim_online_memory(args.nr_extents, args.extent_order);

The code change can have my R-b, but I'd like the comment to also
be changed then - it shouldn't talk about preemption in the way
it does. Perhaps "... when this is a continuation"? I'd be okay
making this change while committing.

As an aside, it would probably have been a good idea to also Cc
Roger as the original author of this code.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] x86/shim: fix ballooning up the guest

Posted by Igor Druzhinin 4 years, 1 month ago

On 17/03/2020 10:24, Jan Beulich wrote:
> On 16.03.2020 22:47, Igor Druzhinin wrote:
>> args.preempted as meaningless here and doesn't show if the hypercall
>> was preempted before. Use start_extent instead which is correct.
> 
> ... as long as the hypercall was invoked in a "normal" way.
> 
>> Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
>> ---
>> This fixes ballooning for 64-bit PV guests inside shim. 32-bit PV guests
>> require a little bit more work due to compat layer being involved.
>> ---
>>  xen/common/memory.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/xen/common/memory.c b/xen/common/memory.c
>> index 444c081..5fdd2a2 100644
>> --- a/xen/common/memory.c
>> +++ b/xen/common/memory.c
>> @@ -1191,7 +1191,7 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>>          }
>>  
>>  #ifdef CONFIG_X86
>> -        if ( pv_shim && op != XENMEM_decrease_reservation && !args.preempted )
>> +        if ( pv_shim && op != XENMEM_decrease_reservation && !start_extent )
>>              /* Avoid calling pv_shim_online_memory when preempted. */
>>              pv_shim_online_memory(args.nr_extents, args.extent_order);
> 
> The code change can have my R-b, but I'd like the comment to also
> be changed then - it shouldn't talk about preemption in the way
> it does. Perhaps "... when this is a continuation"? I'd be okay
> making this change while committing.

Sure, if you like.

> As an aside, it would probably have been a good idea to also Cc
> Roger as the original author of this code.

I didn't check "git blame" and he wasn't on a maintainers list.

Igor

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] x86/shim: fix ballooning up the guest

Posted by Roger Pau Monné 4 years, 1 month ago

On Mon, Mar 16, 2020 at 09:47:44PM +0000, Igor Druzhinin wrote:
> args.preempted as meaningless here and doesn't show if the hypercall
                 ^ is                ^ as it doesn't signal whether the hypercall ...
> was preempted before. Use start_extent instead which is correct.
> 
> Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

FWIW, I agree with the change to the comment requested by Jan, I think
this can all be done while committing.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel