Re: [Xen-devel] [PATCH] xen: fix booting ballooned down hvm guest

[Juergen Gross <jgross@xxxxxxxx>]

On 24/10/17 16:33, Boris Ostrovsky wrote:
> On 10/24/2017 04:10 AM, Juergen Gross wrote:
>> Commit 96edd61dcf44362d3ef0bed1a5361e0ac7886a63 ("xen/balloon: don't
>> online new memory initially") introduced a regression when booting a
>> HVM domain with memory less than mem-max: instead of ballooning down
>> immediately the system would try to use the memory up to mem-max
>> resulting in Xen crashing the domain.
>>
>> For HVM domains the current size will be reflected in Xenstore node
>> memory/static-max instead of memory/target.
>>
>> Additionally we have to trigger the ballooning process at once.
>>
>> Cc: <stable@xxxxxxxxxxxxxxx> # 4.13
>> Fixes: 96edd61dcf44362d3ef0bed1a5361e0ac7886a63 ("xen/balloon: don't
>>        online new memory initially")
>>
>> Suggested-by: Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>
>> Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> 
> Reported-by: HW42 <hw42@xxxxxxxxx>

Hmm, is an anonymous Reported-by: tag okay?

> 
>> ---
>>  drivers/xen/xen-balloon.c | 18 ++++++++++++------
>>  1 file changed, 12 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/xen/xen-balloon.c b/drivers/xen/xen-balloon.c
>> index e89136ab851e..3745748d9644 100644
>> --- a/drivers/xen/xen-balloon.c
>> +++ b/drivers/xen/xen-balloon.c
>> @@ -57,7 +57,7 @@ static int register_balloon(struct device *dev);
>>  static void watch_target(struct xenbus_watch *watch,
>>                       const char *path, const char *token)
>>  {
>> -    unsigned long long new_target;
>> +    unsigned long long new_target, static_max;
>>      int err;
>>      static bool watch_fired;
>>      static long target_diff;
>> @@ -72,13 +72,19 @@ static void watch_target(struct xenbus_watch *watch,
>>       * pages. PAGE_SHIFT converts bytes to pages, hence PAGE_SHIFT - 10.
>>       */
>>      new_target >>= PAGE_SHIFT - 10;
>> -    if (watch_fired) {
>> -            balloon_set_new_target(new_target - target_diff);
>> -            return;
>> +
>> +    if (!watch_fired) {
>> +            watch_fired = true;
>> +            err = xenbus_scanf(XBT_NIL, "memory", "static-max", "%llu",
>> +                               &static_max);
>> +            if (err != 1)
>> +                    static_max = new_target;
>> +            static_max >>= PAGE_SHIFT - 10;
> 
> if you set static_max to new_target you've already done the shift,
> haven't you?

Aah, right. I moved reading static-max down into the if without
adjustment after having it right at the start of the function
initially. Thanks for catching this.

>> +            target_diff = xen_pv_domain() ? 0
> 
> Why do we special-case PV?

Because the initial value of balloon_stats.target_pages is special-cased
for PV, too.


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel