[PATCH v2 1/7] vmw_balloon: fix inflation of 64-bit GFNs

[PATCH v2 1/7] vmw_balloon: fix inflation of 64-bit GFNs

[Nadav Amit]

When balloon batching is not supported by the hypervisor, the guest
frame number (GFN) must fit in 32-bit. However, due to a bug, this check
was mistakenly ignored. In practice, when total RAM is greater than
16TB, the balloon does not work currently, making this bug unlikely to
happen.

Fixes: ef0f8f112984 ("VMware balloon: partially inline vmballoon_reserve_page.")

Cc: stable@vger.kernel.org
Reviewed-by: Xavier Deguillard <xdeguillard@vmware.com>
Signed-off-by: Nadav Amit <namit@vmware.com>
---
 drivers/misc/vmw_balloon.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/misc/vmw_balloon.c b/drivers/misc/vmw_balloon.c
index efd733472a35..28e77ab1e136 100644
--- a/drivers/misc/vmw_balloon.c
+++ b/drivers/misc/vmw_balloon.c
@@ -450,7 +450,7 @@ static int vmballoon_send_lock_page(struct vmballoon *b, unsigned long pfn,
 
 	pfn32 = (u32)pfn;
 	if (pfn32 != pfn)
-		return -1;
+		return -EINVAL;
 
 	STATS_INC(b->stats.lock[false]);
 
@@ -460,7 +460,7 @@ static int vmballoon_send_lock_page(struct vmballoon *b, unsigned long pfn,
 
 	pr_debug("%s - ppn %lx, hv returns %ld\n", __func__, pfn, status);
 	STATS_INC(b->stats.lock_fail[false]);
-	return 1;
+	return -EIO;
 }
 
 static int vmballoon_send_batched_lock(struct vmballoon *b,
@@ -597,11 +597,12 @@ static int vmballoon_lock_page(struct vmballoon *b, unsigned int num_pages,
 
 	locked = vmballoon_send_lock_page(b, page_to_pfn(page), &hv_status,
 								target);
-	if (locked > 0) {
+	if (locked) {
 		STATS_INC(b->stats.refused_alloc[false]);
 
-		if (hv_status == VMW_BALLOON_ERROR_RESET ||
-				hv_status == VMW_BALLOON_ERROR_PPN_NOTNEEDED) {
+		if (locked == -EIO &&
+		    (hv_status == VMW_BALLOON_ERROR_RESET ||
+		     hv_status == VMW_BALLOON_ERROR_PPN_NOTNEEDED)) {
 			vmballoon_free_page(page, false);
 			return -EIO;
 		}
@@ -617,7 +618,7 @@ static int vmballoon_lock_page(struct vmballoon *b, unsigned int num_pages,
 		} else {
 			vmballoon_free_page(page, false);
 		}
-		return -EIO;
+		return locked;
 	}
 
 	/* track allocated page */
-- 
2.17.0

[PATCH v2 2/7] vmw_balloon: do not use 2MB without batching

[Nadav Amit]

If the hypervisor sets 2MB batching is on, while batching is cleared,
the balloon code breaks. In this case the legacy mechanism is used with
2MB page. The VM would report a 2MB page is ballooned, and the
hypervisor would only take the first 4KB.

While the hypervisor should not report such settings, make the code more
robust by not enabling 2MB support without batching.

Fixes: 365bd7ef7ec8e ("VMware balloon: Support 2m page ballooning.")

Cc: stable@vger.kernel.org
Reviewed-by: Xavier Deguillard <xdeguillard@vmware.com>
Signed-off-by: Nadav Amit <nadav.amit@gmail.com>
---
 drivers/misc/vmw_balloon.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/misc/vmw_balloon.c b/drivers/misc/vmw_balloon.c
index 28e77ab1e136..60ab83d3d0ef 100644
--- a/drivers/misc/vmw_balloon.c
+++ b/drivers/misc/vmw_balloon.c
@@ -341,7 +341,13 @@ static bool vmballoon_send_start(struct vmballoon *b, unsigned long req_caps)
 		success = false;
 	}
 
-	if (b->capabilities & VMW_BALLOON_BATCHED_2M_CMDS)
+	/*
+	 * 2MB pages are only supported with batching. If batching is for some
+	 * reason disabled, do not use 2MB pages, since otherwise the legacy
+	 * mechanism is used with 2MB pages, causing a failure.
+	 */
+	if ((b->capabilities & VMW_BALLOON_BATCHED_2M_CMDS) &&
+	    (b->capabilities & VMW_BALLOON_BATCHED_CMDS))
 		b->supported_page_sizes = 2;
 	else
 		b->supported_page_sizes = 1;
-- 
2.17.0

[PATCH v2 3/7] vmw_balloon: VMCI_DOORBELL_SET does not check status

[Nadav Amit]

When vmballoon_vmci_init() sets a doorbell using VMCI_DOORBELL_SET, for
some reason it does not consider the status and looks at the result.
However, the hypervisor does not update the result - it updates the
status. This might cause VMCI doorbell not to be enabled, resulting in
degraded performance.

Fixes: 48e3d668b790 ("VMware balloon: Enable notification via VMCI")

Cc: stable@vger.kernel.org
Reviewed-by: Xavier Deguillard <xdeguillard@vmware.com>
Signed-off-by: Nadav Amit <namit@vmware.com>
---
 drivers/misc/vmw_balloon.c | 37 +++++++++++++++++++------------------
 1 file changed, 19 insertions(+), 18 deletions(-)

diff --git a/drivers/misc/vmw_balloon.c b/drivers/misc/vmw_balloon.c
index 60ab83d3d0ef..a7df4c24a28d 100644
--- a/drivers/misc/vmw_balloon.c
+++ b/drivers/misc/vmw_balloon.c
@@ -1036,29 +1036,30 @@ static void vmballoon_vmci_cleanup(struct vmballoon *b)
  */
 static int vmballoon_vmci_init(struct vmballoon *b)
 {
-	int error = 0;
+	unsigned long error, dummy;
 
-	if ((b->capabilities & VMW_BALLOON_SIGNALLED_WAKEUP_CMD) != 0) {
-		error = vmci_doorbell_create(&b->vmci_doorbell,
-				VMCI_FLAG_DELAYED_CB,
-				VMCI_PRIVILEGE_FLAG_RESTRICTED,
-				vmballoon_doorbell, b);
-
-		if (error == VMCI_SUCCESS) {
-			VMWARE_BALLOON_CMD(VMCI_DOORBELL_SET,
-					b->vmci_doorbell.context,
-					b->vmci_doorbell.resource, error);
-			STATS_INC(b->stats.doorbell_set);
-		}
-	}
+	if ((b->capabilities & VMW_BALLOON_SIGNALLED_WAKEUP_CMD) == 0)
+		return 0;
 
-	if (error != 0) {
-		vmballoon_vmci_cleanup(b);
+	error = vmci_doorbell_create(&b->vmci_doorbell, VMCI_FLAG_DELAYED_CB,
+				     VMCI_PRIVILEGE_FLAG_RESTRICTED,
+				     vmballoon_doorbell, b);
 
-		return -EIO;
-	}
+	if (error != VMCI_SUCCESS)
+		goto fail;
+
+	error = VMWARE_BALLOON_CMD(VMCI_DOORBELL_SET, b->vmci_doorbell.context,
+				   b->vmci_doorbell.resource, dummy);
+
+	STATS_INC(b->stats.doorbell_set);
+
+	if (error != VMW_BALLOON_SUCCESS)
+		goto fail;
 
 	return 0;
+fail:
+	vmballoon_vmci_cleanup(b);
+	return -EIO;
 }
 
 /*
-- 
2.17.0

[PATCH v2 4/7] vmw_balloon: fix VMCI use when balloon built into kernel

[Nadav Amit]

Currently, when all modules, including VMCI and VMware balloon are built
into the kernel, the initialization of the balloon happens before the
VMCI is probed. As a result, the balloon fails to initialize the VMCI
doorbell, which it uses to get asynchronous requests for balloon size
changes.

The problem can be seen in the logs, in the form of the following
message:
	"vmw_balloon: failed to initialize vmci doorbell"

The driver would work correctly but slightly less efficiently, probing
for requests periodically. This patch changes the balloon to be
initialized using late_initcall() instead of module_init() to address
this issue. It does not address a situation in which VMCI is built as a
module and the balloon is built into the kernel.

Fixes: 48e3d668b790 ("VMware balloon: Enable notification via VMCI")

Cc: stable@vger.kernel.org
Reviewed-by: Xavier Deguillard <xdeguillard@vmware.com>
Signed-off-by: Nadav Amit <namit@vmware.com>
---
 drivers/misc/vmw_balloon.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/misc/vmw_balloon.c b/drivers/misc/vmw_balloon.c
index a7df4c24a28d..e7cfc85f6961 100644
--- a/drivers/misc/vmw_balloon.c
+++ b/drivers/misc/vmw_balloon.c
@@ -1297,7 +1297,14 @@ static int __init vmballoon_init(void)
 
 	return 0;
 }
-module_init(vmballoon_init);
+
+/*
+ * Using late_initcall() instead of module_init() allows the balloon to use the
+ * VMCI doorbell even when the balloon is built into the kernel. Otherwise the
+ * VMCI is probed only after the balloon is initialized. If the balloon is used
+ * as a module, late_initcall() is equivalent to module_init().
+ */
+late_initcall(vmballoon_init);
 
 static void __exit vmballoon_exit(void)
 {
-- 
2.17.0

Re: [PATCH v2 1/7] vmw_balloon: fix inflation of 64-bit GFNs

[Greg Kroah-Hartman]

On Wed, Jun 13, 2018 at 06:54:06AM -0700, Nadav Amit wrote:
> When balloon batching is not supported by the hypervisor, the guest
> frame number (GFN) must fit in 32-bit. However, due to a bug, this check
> was mistakenly ignored. In practice, when total RAM is greater than
> 16TB, the balloon does not work currently, making this bug unlikely to
> happen.
> 
> Fixes: ef0f8f112984 ("VMware balloon: partially inline vmballoon_reserve_page.")
> 
> Cc: stable@vger.kernel.org

Again, no blank line between these things.

Also, please break this up into two different patch series.  One for
4.18-final and one for 4.19-rc1.  You should always split up bugfixes
from new features/cleanups as that is what I have to do when sending
them to Linus.

thanks,

greg k-h

Re: [PATCH v2 1/7] vmw_balloon: fix inflation of 64-bit GFNs

[Nadav Amit]

at 10:23 PM, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:

> On Wed, Jun 13, 2018 at 06:54:06AM -0700, Nadav Amit wrote:
>> When balloon batching is not supported by the hypervisor, the guest
>> frame number (GFN) must fit in 32-bit. However, due to a bug, this check
>> was mistakenly ignored. In practice, when total RAM is greater than
>> 16TB, the balloon does not work currently, making this bug unlikely to
>> happen.
>> 
>> Fixes: ef0f8f112984 ("VMware balloon: partially inline vmballoon_reserve_page.")
>> 
>> Cc: stable@vger.kernel.org
> 
> Again, no blank line between these things.
> 
> Also, please break this up into two different patch series.  One for
> 4.18-final and one for 4.19-rc1.  You should always split up bugfixes
> from new features/cleanups as that is what I have to do when sending
> them to Linus.

I have only sent what I thought is appropriate for 4.18 (cleanup/features 
will be sent separately):

* 5 bug fixes (1-5/7)
* 1 update the maintainer list (7/7)
* 1 update to the comment to prevent checkpatch from complaining (6/7)

If you think any patch is unsuitable to 4.18 - please say which.

Thanks,
Nadav

Re: [PATCH v2 1/7] vmw_balloon: fix inflation of 64-bit GFNs

[Greg Kroah-Hartman]

On Thu, Jun 14, 2018 at 05:41:01AM +0000, Nadav Amit wrote:
> at 10:23 PM, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> 
> > On Wed, Jun 13, 2018 at 06:54:06AM -0700, Nadav Amit wrote:
> >> When balloon batching is not supported by the hypervisor, the guest
> >> frame number (GFN) must fit in 32-bit. However, due to a bug, this check
> >> was mistakenly ignored. In practice, when total RAM is greater than
> >> 16TB, the balloon does not work currently, making this bug unlikely to
> >> happen.
> >> 
> >> Fixes: ef0f8f112984 ("VMware balloon: partially inline vmballoon_reserve_page.")
> >> 
> >> Cc: stable@vger.kernel.org
> > 
> > Again, no blank line between these things.
> > 
> > Also, please break this up into two different patch series.  One for
> > 4.18-final and one for 4.19-rc1.  You should always split up bugfixes
> > from new features/cleanups as that is what I have to do when sending
> > them to Linus.
> 
> I have only sent what I thought is appropriate for 4.18 (cleanup/features 
> will be sent separately):
> 
> * 5 bug fixes (1-5/7)
> * 1 update the maintainer list (7/7)
> * 1 update to the comment to prevent checkpatch from complaining (6/7)
> 
> If you think any patch is unsuitable to 4.18 - please say which.

How do I know which patch goes to which branch?  Please make it so
obvious I can not get it wrong.  Remember, I get 1000 emails a day, you
don't want me to have to make a judgement call about anything, as I will
mess it up  :)

thanks,

greg k-h

Re: [PATCH v2 1/7] vmw_balloon: fix inflation of 64-bit GFNs

[Nadav Amit]

at 10:50 PM, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:

> On Thu, Jun 14, 2018 at 05:41:01AM +0000, Nadav Amit wrote:
>> at 10:23 PM, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
>> 
>>> On Wed, Jun 13, 2018 at 06:54:06AM -0700, Nadav Amit wrote:
>>>> When balloon batching is not supported by the hypervisor, the guest
>>>> frame number (GFN) must fit in 32-bit. However, due to a bug, this check
>>>> was mistakenly ignored. In practice, when total RAM is greater than
>>>> 16TB, the balloon does not work currently, making this bug unlikely to
>>>> happen.
>>>> 
>>>> Fixes: ef0f8f112984 ("VMware balloon: partially inline vmballoon_reserve_page.")
>>>> 
>>>> Cc: stable@vger.kernel.org
>>> 
>>> Again, no blank line between these things.
>>> 
>>> Also, please break this up into two different patch series.  One for
>>> 4.18-final and one for 4.19-rc1.  You should always split up bugfixes
>>> from new features/cleanups as that is what I have to do when sending
>>> them to Linus.
>> 
>> I have only sent what I thought is appropriate for 4.18 (cleanup/features 
>> will be sent separately):
>> 
>> * 5 bug fixes (1-5/7)
>> * 1 update the maintainer list (7/7)
>> * 1 update to the comment to prevent checkpatch from complaining (6/7)
>> 
>> If you think any patch is unsuitable to 4.18 - please say which.
> 
> How do I know which patch goes to which branch?  Please make it so
> obvious I can not get it wrong.  Remember, I get 1000 emails a day, you
> don't want me to have to make a judgement call about anything, as I will
> mess it up  :)

I appreciate your time, and understand you even need to read students’
dissertations. ;-)

I will write it clearly in the cover-letter in the next version, and move
the license changes to 4.19.

Thanks,
Nadav