Re: [RFC 1/2] misc: Add vboxguest driver for Virtual Box Guest integration
Hi, On 21-08-17 13:43, Hans de Goede wrote: I'm also thinking that under Linux since we never use userspace to alloc memory for the balloon, we don't need to involve userspace at all, the drivers interrupt handler could detect the event that the host wants to change the balloonsize itself and start a workqueue item which then asks for what the new size should be and do the adjustment all inside the kernel. I think that will even lead to simpler code as we no longer need to track the balloon userspace owner and do reclaim when it dies. Typing this out I really see no reason to not do this inside the kernel, so I think I'm going to implement that right away / for the [RFC v2] I plan to post later today or tomorrow. Done: https://github.com/jwrdegoede/vboxguest/commit/2920882cb0ce9af11a843affc58d4a08ef47e16b Regards, Hans
Re: [RFC 1/2] misc: Add vboxguest driver for Virtual Box Guest integration
Hi,
On 14-08-17 14:15, Hans de Goede wrote:
Hi,
On 14-08-17 11:30, Arnd Bergmann wrote:
On Sat, Aug 12, 2017 at 11:56 PM, Hans de Goede wrote:
On 11-08-17 23:23, Arnd Bergmann wrote:
On Fri, Aug 11, 2017 at 3:23 PM, Hans de Goede
wrote:
Most of the issues I would normally comment on are already moot based
on the assumption that we won't be able to make substantial changes to
either the user space portion or the hypervisor side.
+/**
+ * Inflate the balloon by one chunk.
+ *
+ * The caller owns the balloon mutex.
+ *
+ * @returns VBox status code
+ * @param gdevThe Guest extension device.
+ * @param chunk_idx Index of the chunk.
+ */
+static int vbg_balloon_inflate(PVBOXGUESTDEVEXT gdev, u32 chunk_idx)
+{
+ VMMDevChangeMemBalloon *req = gdev->mem_balloon.change_req;
+ struct page **pages;
+ int i, rc;
+
+ pages = kmalloc(sizeof(*pages) *
VMMDEV_MEMORY_BALLOON_CHUNK_PAGES,
+ GFP_KERNEL | __GFP_NOWARN);
+ if (!pages)
+ return VERR_NO_MEMORY;
+
+ req->header.size = sizeof(*req);
+ req->inflate = true;
+ req->pages = VMMDEV_MEMORY_BALLOON_CHUNK_PAGES;
The balloon section seems to be rather simplistic with its ioctl
interface.
Ideally this should use CONFIG_BALLOON_COMPACTION and an
oom notifier like the virtio_balloon driver does. Yes, I realize that only
one of the six (or more) balloon drivers we have in the kernel does it ;-)
The way the balloon code works is that the baloon-size is fully controlled
by the host, all the guest-additions do is wait for an event indicating that
the host wants to change it and then call this ioctl which will get the
desired balloon size from the host and tries to adjust the size accordingly.
Hooking into the oom killer is not really useful because there is no way we
can indicate to the host we are running low on mem and I don't think that
trying to shrink the balloon to be smaller then the host requested size
is a good idea.
Are you sure the guest can't just claim the memory back by using it?
I don't think so the original code certainly does not do anything of
the sorts, the balloon code uses alloc_page(GFP_KERNEL | __GFP_NOWARN)
and only calls __free_page() again on the pages after a successful
deflate request to the host.
So first we tell the host we want a chunk (1MiB worth of pages) back and
only if the host says ok do we call __free_page which means it can
get used for something else again.
Ok, so while working on the final cleanups of the code I realized that
the kernel driver will actually re-claim the memory without the host
indicating that this is ok when the vboxservice which monitors for
host events to change the balloon size dies. When it dies the kernel
tries to reclaim all the memory from the host and this seems to work fine.
So we could hook up to an OOM signal and try to reclaim some memory here
then, but the out-of-tree version of the driver does not do this.
Michael, Knut, what is the intended behavior of the kernel here?
I'm also thinking that under Linux since we never use userspace to
alloc memory for the balloon, we don't need to involve userspace at
all, the drivers interrupt handler could detect the event that the
host wants to change the balloonsize itself and start a workqueue
item which then asks for what the new size should be and do the
adjustment all inside the kernel. I think that will even lead to
simpler code as we no longer need to track the balloon userspace
owner and do reclaim when it dies.
Typing this out I really see no reason to not do this inside the
kernel, so I think I'm going to implement that right away / for
the [RFC v2] I plan to post later today or tomorrow.
Regards,
Hans
Michael, Knut can you shed some light on this ?
Usually that is how the balloon drivers work: the host asks the guest
to free up some memory, and the guest frees up memory that it
tells to the host, but if the guest runs out of memory, it just starts using
it again and the host faults in empty pages.
For CONFIG_BALLOON_COMPACTION, you don't even need that,
you just put some memory into the balloon before you take some
other page out that is required for compaction. This might be less
of an issue here when the balloon always operates on 1MB chunks.
+*/
+ if (cbData <= sizeof(au64Buf) &&
+ VBOXGUEST_IOCTL_STRIP_SIZE(uCmd) !=
+ VBOXGUEST_IOCTL_STRIP_SIZE(VBOXGUEST_IOCTL_VMMREQUEST(0))) {
+ pvBufFree = NULL;
+ pvBuf = &au64Buf[0];
+ } else {
+ /* __GFP_DMA32 for VBOXGUEST_IOCTL_VMMREQUEST */
+ pvBufFree = pvBuf = kmalloc(cbData, GFP_KERNEL |
__GFP_DMA32);
+ if (!pvBuf)
+ return -ENOMEM;
+ }
+ if (copy_from_user(pvBuf, (void *)ulArg, cbData) == 0) {
I'd also change the commands
to not always do both read and write, but only whichever applies. This
function
would then do the copy_fro
Re: [RFC 1/2] misc: Add vboxguest driver for Virtual Box Guest integration
Hi,
On 14-08-17 11:30, Arnd Bergmann wrote:
On Sat, Aug 12, 2017 at 11:56 PM, Hans de Goede wrote:
On 11-08-17 23:23, Arnd Bergmann wrote:
On Fri, Aug 11, 2017 at 3:23 PM, Hans de Goede
wrote:
Most of the issues I would normally comment on are already moot based
on the assumption that we won't be able to make substantial changes to
either the user space portion or the hypervisor side.
+/**
+ * Inflate the balloon by one chunk.
+ *
+ * The caller owns the balloon mutex.
+ *
+ * @returns VBox status code
+ * @param gdevThe Guest extension device.
+ * @param chunk_idx Index of the chunk.
+ */
+static int vbg_balloon_inflate(PVBOXGUESTDEVEXT gdev, u32 chunk_idx)
+{
+ VMMDevChangeMemBalloon *req = gdev->mem_balloon.change_req;
+ struct page **pages;
+ int i, rc;
+
+ pages = kmalloc(sizeof(*pages) *
VMMDEV_MEMORY_BALLOON_CHUNK_PAGES,
+ GFP_KERNEL | __GFP_NOWARN);
+ if (!pages)
+ return VERR_NO_MEMORY;
+
+ req->header.size = sizeof(*req);
+ req->inflate = true;
+ req->pages = VMMDEV_MEMORY_BALLOON_CHUNK_PAGES;
The balloon section seems to be rather simplistic with its ioctl
interface.
Ideally this should use CONFIG_BALLOON_COMPACTION and an
oom notifier like the virtio_balloon driver does. Yes, I realize that only
one of the six (or more) balloon drivers we have in the kernel does it ;-)
The way the balloon code works is that the baloon-size is fully controlled
by the host, all the guest-additions do is wait for an event indicating that
the host wants to change it and then call this ioctl which will get the
desired balloon size from the host and tries to adjust the size accordingly.
Hooking into the oom killer is not really useful because there is no way we
can indicate to the host we are running low on mem and I don't think that
trying to shrink the balloon to be smaller then the host requested size
is a good idea.
Are you sure the guest can't just claim the memory back by using it?
I don't think so the original code certainly does not do anything of
the sorts, the balloon code uses alloc_page(GFP_KERNEL | __GFP_NOWARN)
and only calls __free_page() again on the pages after a successful
deflate request to the host.
So first we tell the host we want a chunk (1MiB worth of pages) back and
only if the host says ok do we call __free_page which means it can
get used for something else again.
Michael, Knut can you shed some light on this ?
Usually that is how the balloon drivers work: the host asks the guest
to free up some memory, and the guest frees up memory that it
tells to the host, but if the guest runs out of memory, it just starts using
it again and the host faults in empty pages.
For CONFIG_BALLOON_COMPACTION, you don't even need that,
you just put some memory into the balloon before you take some
other page out that is required for compaction. This might be less
of an issue here when the balloon always operates on 1MB chunks.
+*/
+ if (cbData <= sizeof(au64Buf) &&
+ VBOXGUEST_IOCTL_STRIP_SIZE(uCmd) !=
+ VBOXGUEST_IOCTL_STRIP_SIZE(VBOXGUEST_IOCTL_VMMREQUEST(0))) {
+ pvBufFree = NULL;
+ pvBuf = &au64Buf[0];
+ } else {
+ /* __GFP_DMA32 for VBOXGUEST_IOCTL_VMMREQUEST */
+ pvBufFree = pvBuf = kmalloc(cbData, GFP_KERNEL |
__GFP_DMA32);
+ if (!pvBuf)
+ return -ENOMEM;
+ }
+ if (copy_from_user(pvBuf, (void *)ulArg, cbData) == 0) {
I'd also change the commands
to not always do both read and write, but only whichever applies. This
function
would then do the copy_from_user/copy_to_user conditionally.
This is actually an upstream TODO item I believe. One which would
probably be best fixed by using _IOR/_IOW/_IORW IOCTL macros, but
that needs to be coordinated with upstream.
It would be good to clean this up and always use the same structure here.
The HGCMFunctionParameter struct describes function-parameters
in a Host-Guest-Control-Method function call, so this is part of the
host ABI interface and we cannot change this.
Any 32 bit apps running on a 64 bit kernel will be using the 32 bit
version of this struct and we need to translate. >
I don't see the difference between changing the command codes and
changing the structure layout here. In both cases, that is an incompatible
ABI change but shouldn't much impact the source-level ABI if you do
it right.
This struct is defined in vmmdev.h because it is part of the hardware
interface, a 32 bit host expects the 32 bit version of the struct
when calling into the host, where as a 64 bit host expects the 64 bit
version.
The hgcm-call ioctl uses this struct _directly_, so a 64 bit app
will pass in the 64 bit version and a 32 bit app the 32 bit version,
and on 64 bit we need to translate the 32 bit version coming from
a 32 bit app.
The only way around this would be to make userspace always see
Re: [RFC 1/2] misc: Add vboxguest driver for Virtual Box Guest integration
On Sat, Aug 12, 2017 at 11:56 PM, Hans de Goede wrote:
> On 11-08-17 23:23, Arnd Bergmann wrote:
>> On Fri, Aug 11, 2017 at 3:23 PM, Hans de Goede
>> wrote:
>> Most of the issues I would normally comment on are already moot based
>> on the assumption that we won't be able to make substantial changes to
>> either the user space portion or the hypervisor side.
>>
>>> +/**
>>> + * Inflate the balloon by one chunk.
>>> + *
>>> + * The caller owns the balloon mutex.
>>> + *
>>> + * @returns VBox status code
>>> + * @param gdevThe Guest extension device.
>>> + * @param chunk_idx Index of the chunk.
>>> + */
>>> +static int vbg_balloon_inflate(PVBOXGUESTDEVEXT gdev, u32 chunk_idx)
>>> +{
>>> + VMMDevChangeMemBalloon *req = gdev->mem_balloon.change_req;
>>> + struct page **pages;
>>> + int i, rc;
>>> +
>>> + pages = kmalloc(sizeof(*pages) *
>>> VMMDEV_MEMORY_BALLOON_CHUNK_PAGES,
>>> + GFP_KERNEL | __GFP_NOWARN);
>>> + if (!pages)
>>> + return VERR_NO_MEMORY;
>>> +
>>> + req->header.size = sizeof(*req);
>>> + req->inflate = true;
>>> + req->pages = VMMDEV_MEMORY_BALLOON_CHUNK_PAGES;
>>
>>
>> The balloon section seems to be rather simplistic with its ioctl
>> interface.
>> Ideally this should use CONFIG_BALLOON_COMPACTION and an
>> oom notifier like the virtio_balloon driver does. Yes, I realize that only
>> one of the six (or more) balloon drivers we have in the kernel does it ;-)
>
>
> The way the balloon code works is that the baloon-size is fully controlled
> by the host, all the guest-additions do is wait for an event indicating that
> the host wants to change it and then call this ioctl which will get the
> desired balloon size from the host and tries to adjust the size accordingly.
>
> Hooking into the oom killer is not really useful because there is no way we
> can indicate to the host we are running low on mem and I don't think that
> trying to shrink the balloon to be smaller then the host requested size
> is a good idea.
Are you sure the guest can't just claim the memory back by using it?
Usually that is how the balloon drivers work: the host asks the guest
to free up some memory, and the guest frees up memory that it
tells to the host, but if the guest runs out of memory, it just starts using
it again and the host faults in empty pages.
For CONFIG_BALLOON_COMPACTION, you don't even need that,
you just put some memory into the balloon before you take some
other page out that is required for compaction. This might be less
of an issue here when the balloon always operates on 1MB chunks.
>>> +/**
>>> + * Callback for heartbeat timer.
>>> + */
>>> +static void vbg_heartbeat_timer(unsigned long data)
>>> +{
>>> + PVBOXGUESTDEVEXT gdev = (PVBOXGUESTDEVEXT)data;
>>> +
>>> + vbg_req_perform(gdev, gdev->guest_heartbeat_req);
>>> + mod_timer(&gdev->heartbeat_timer,
>>> + msecs_to_jiffies(gdev->heartbeat_interval_ms));
>>> +}
>>
>>
>> This looks like a watchdog and should use the drivers/watchdog
>> subsystem interfaces.
>
>
> This is not really a watchdog, this also is fully under host control,
> the host controls if the guest should send a heartbeat and if so at
> which interval, where normally with a watchdog starting the timer
> and deciding the interval is under control of the watchdog user.
>
> Also if we miss to generate the heartbeat then the host will just
> log a message, not reset the system as one would expect with a
> watchdog. TL;DR: I understand why suggest this but the watchdog
> interface seems a poor match for this.
Ok.
>>> diff --git a/drivers/misc/vboxguest/vboxguest_linux.c
>>> b/drivers/misc/vboxguest/vboxguest_linux.c
>>> new file mode 100644
>>> index ..8468c7139b98
>>> --- /dev/null
>>> +++ b/drivers/misc/vboxguest/vboxguest_linux.c
>>
>>
>> This looks like a fairly short file, and could be merged into the core
>> file.
>
>
> I would prefer to keep it separate to keep more or less 1 : 1 mapping
> between mainline kernel and vbox upstream svn files.
ok.
>>> +*/
>>> + if (cbData <= sizeof(au64Buf) &&
>>> + VBOXGUEST_IOCTL_STRIP_SIZE(uCmd) !=
>>> + VBOXGUEST_IOCTL_STRIP_SIZE(VBOXGUEST_IOCTL_VMMREQUEST(0))) {
>>> + pvBufFree = NULL;
>>> + pvBuf = &au64Buf[0];
>>> + } else {
>>> + /* __GFP_DMA32 for VBOXGUEST_IOCTL_VMMREQUEST */
>>> + pvBufFree = pvBuf = kmalloc(cbData, GFP_KERNEL |
>>> __GFP_DMA32);
>>> + if (!pvBuf)
>>> + return -ENOMEM;
>>> + }
>>> + if (copy_from_user(pvBuf, (void *)ulArg, cbData) == 0) {
>
>> I'd also change the commands
>> to not always do both read and write, but only whichever applies. This
>> function
>> would then do the copy_from_user/copy_to_user conditionally.
>
>
> This is actually an upstream TODO item I believe. One which would
> probably be best fixed by using _IOR/_IOW/_
Re: [RFC 1/2] misc: Add vboxguest driver for Virtual Box Guest integration
Hi,
On 12-08-17 23:56, Hans de Goede wrote:
On 11-08-17 23:23, Arnd Bergmann wrote:
+/**
+ * @name VBoxGuest IOCTL codes and structures.
+ *
+ * The range 0..15 is for basic driver communication.
+ * The range 16..31 is for HGCM communication.
+ * The range 32..47 is reserved for future use.
+ * The range 48..63 is for OS specific communication.
+ * The 7th bit is reserved for future hacks.
+ * The 8th bit is reserved for distinguishing between 32-bit and 64-bit
+ * processes in future 64-bit guest additions.
+ * @{
+ */
+#if __BITS_PER_LONG == 64
+#define VBOXGUEST_IOCTL_FLAG 128
+#else
+#define VBOXGUEST_IOCTL_FLAG 0
+#endif
+/** @} */
This seems misguided, we already know the difference between
32-bit and 64-bit tasks based on the entry point, and if the
interface is properly designed then we don't care about this
difference at all.
I guess (and really guess at that) that this is there because on
some guest OS-es supported by vbox the distinction between a
32 bit or 64 bit app calling the ioctl cannot be made in another
way.
But I would not mind dropping this flag for linux.
Michael / Knut would something like this be acceptable ?
--- a/include/VBox/VBoxGuest.h
+++ b/include/VBox/VBoxGuest.h
@@ -123,9 +123,9 @@
* used.
* @{
*/
-#if defined(RT_ARCH_AMD64) || defined(RT_ARCH_SPARC64)
+#if (defined(RT_ARCH_AMD64) || defined(RT_ARCH_SPARC64)) &&
!defined(RT_OS_LINUX)
# define VBOXGUEST_IOCTL_FLAG 128
-#elif defined(RT_ARCH_X86) || defined(RT_ARCH_SPARC)
+#elif defined(RT_ARCH_X86) || defined(RT_ARCH_SPARC) || defined(RT_OS_LINUX)
# define VBOXGUEST_IOCTL_FLAG 0
#else
# error "dunno which arch this is!"
(extend with some other changes such as properly adding a .compat_ioctl
file-op ?
So I've extended the above into a proper patch to upstream
vbox svn (attached for reference). One thing which I've
learned while doing that is that Linux seems to be unique
with its compat_ioctl callback. Neither the BSDs nor Windows
has this, they all use different ioctl numbers for 32 / 64bit,
see e.g. :
https://tsyrklevich.net/clang_analyzer/freebsd_030917/report-de8f10.html
Which deals with 32 bit ioctl compat on freebsd, note
all the ioctls checked for are post_fixed with _32.
So given the cross-platform nature of the vboxguest ioctl
API I think we may end up having to stick with the (small)
VBOXGUEST_IOCTL_FLAG wart in the API. Because I certainly
don't want to break ABI compat between the mainline version
and the out of tree version.
But first lets see what vbox upstream has to say about my
patch to always make VBOXGUEST_IOCTL_FLAG 0 under Linux.
Regards,
Hans
>From 17237897ed2b654ba9de90a7dad8e0dffdefe65b Mon Sep 17 00:00:00 2001
From: Hans de Goede
Date: Mon, 14 Aug 2017 09:01:29 +0200
Subject: [PATCH] VGDrvCommonIoCtl: Add f32bit flag argument
On some operating-systems the driver can tell whether an ioctl is a
32bit compat callback or not without looking at the ioctl-nr, at a flag
argument to pass this info along and use it for Linux to not need
separate ioctl codes for 32 and 64 bit.
Signed-off-by: Hans de Goede
---
include/VBox/VBoxGuest.h | 4 +--
.../Additions/common/VBoxGuest/VBoxDev-haiku.c | 2 +-
.../common/VBoxGuest/VBoxGuest-darwin.cpp | 2 +-
.../Additions/common/VBoxGuest/VBoxGuest-freebsd.c | 2 +-
.../common/VBoxGuest/VBoxGuest-haiku-stubs.c | 4 +--
.../Additions/common/VBoxGuest/VBoxGuest-haiku.h | 2 +-
.../Additions/common/VBoxGuest/VBoxGuest-linux.c | 35 ++
.../Additions/common/VBoxGuest/VBoxGuest-netbsd.c | 2 +-
.../Additions/common/VBoxGuest/VBoxGuest-os2.cpp | 4 +--
.../Additions/common/VBoxGuest/VBoxGuest-solaris.c | 2 +-
.../Additions/common/VBoxGuest/VBoxGuest-win.cpp | 2 +-
src/VBox/Additions/common/VBoxGuest/VBoxGuest.cpp | 16 +-
.../common/VBoxGuest/VBoxGuestIDC-unix.c.h | 2 +-
.../Additions/common/VBoxGuest/VBoxGuestInternal.h | 2 +-
14 files changed, 47 insertions(+), 34 deletions(-)
diff --git a/include/VBox/VBoxGuest.h b/include/VBox/VBoxGuest.h
index 743fbb3c..5327bb1c 100644
--- a/include/VBox/VBoxGuest.h
+++ b/include/VBox/VBoxGuest.h
@@ -123,9 +123,9 @@
* used.
* @{
*/
-#if defined(RT_ARCH_AMD64) || defined(RT_ARCH_SPARC64)
+#if (defined(RT_ARCH_AMD64) || defined(RT_ARCH_SPARC64)) && !defined(RT_OS_LINUX)
# define VBOXGUEST_IOCTL_FLAG 128
-#elif defined(RT_ARCH_X86) || defined(RT_ARCH_SPARC)
+#elif defined(RT_ARCH_X86) || defined(RT_ARCH_SPARC) || defined(RT_OS_LINUX)
# define VBOXGUEST_IOCTL_FLAG 0
#else
# error "dunno which arch this is!"
diff --git a/src/VBox/Additions/common/VBoxGuest/VBoxDev-haiku.c b/src/VBox/Additions/common/VBoxGuest/VBoxDev-haiku.c
index 8a664f12..6a240eb6 100644
--- a/src/VBox/Additions/common/VBoxGuest/VBoxDev-haiku.c
+++ b/src/VBox/Additions/common/VBoxGuest/VBoxDev-haiku.c
@@ -236,7 +236,7 @@ static status_t vgdrvHaikuIOCtl(void *cookie
Re: [RFC 1/2] misc: Add vboxguest driver for Virtual Box Guest integration
On Sat, Aug 12, 2017 at 11:56:22PM +0200, Hans de Goede wrote: > > 'u32' is not an approprioate type for a kernel header, use '__u32' > > instead. > > Huh I thought that u32 was preferred, but I guess that it is not allowed > in uapi headers due to potential conflicts and it should be __u32 in uapi > headers ? The __ version should always be used in structures/variables that cross the user/kernel boundry as they are guaranteed to be correct that way. That is a requirement. Use the "normal" non __ versions for in-kernel only variables. hope this helps, greg k-h
Re: [RFC 1/2] misc: Add vboxguest driver for Virtual Box Guest integration
Hi,
On 11-08-17 23:23, Arnd Bergmann wrote:
On Fri, Aug 11, 2017 at 3:23 PM, Hans de Goede wrote:
This commit adds a driver for the Virtual Box Guest PCI device used in
Virtual Box virtual machines. Enabling this driver will add support for
Virtual Box Guest integration features such as copy-and-paste, seamless
mode and OpenGL pass-through.
This driver also offers vboxguest IPC functionality which is needed
for the vboxfs driver which offers folder sharing support.
Signed-off-by: Hans de Goede
Since you are still working on coding style cleanups, I'll comment mainly on
the ioctl interface for now.
Thank you for the taking the time to review the ioctl interface.
Overall it already looks much better than
I expected
from previous experience with the vbox source.
I'm glad you like it, cleaning it up has been quite a bit of work,
so I'm happy to see this being appreciated.
Most of the issues I would normally comment on are already moot based
on the assumption that we won't be able to make substantial changes to
either the user space portion or the hypervisor side.
+/**
+ * Inflate the balloon by one chunk.
+ *
+ * The caller owns the balloon mutex.
+ *
+ * @returns VBox status code
+ * @param gdevThe Guest extension device.
+ * @param chunk_idx Index of the chunk.
+ */
+static int vbg_balloon_inflate(PVBOXGUESTDEVEXT gdev, u32 chunk_idx)
+{
+ VMMDevChangeMemBalloon *req = gdev->mem_balloon.change_req;
+ struct page **pages;
+ int i, rc;
+
+ pages = kmalloc(sizeof(*pages) * VMMDEV_MEMORY_BALLOON_CHUNK_PAGES,
+ GFP_KERNEL | __GFP_NOWARN);
+ if (!pages)
+ return VERR_NO_MEMORY;
+
+ req->header.size = sizeof(*req);
+ req->inflate = true;
+ req->pages = VMMDEV_MEMORY_BALLOON_CHUNK_PAGES;
The balloon section seems to be rather simplistic with its ioctl interface.
Ideally this should use CONFIG_BALLOON_COMPACTION and an
oom notifier like the virtio_balloon driver does. Yes, I realize that only
one of the six (or more) balloon drivers we have in the kernel does it ;-)
The way the balloon code works is that the baloon-size is fully controlled
by the host, all the guest-additions do is wait for an event indicating that
the host wants to change it and then call this ioctl which will get the
desired balloon size from the host and tries to adjust the size accordingly.
Hooking into the oom killer is not really useful because there is no way we
can indicate to the host we are running low on mem and I don't think that
trying to shrink the balloon to be smaller then the host requested size
is a good idea.
+/**
+ * Callback for heartbeat timer.
+ */
+static void vbg_heartbeat_timer(unsigned long data)
+{
+ PVBOXGUESTDEVEXT gdev = (PVBOXGUESTDEVEXT)data;
+
+ vbg_req_perform(gdev, gdev->guest_heartbeat_req);
+ mod_timer(&gdev->heartbeat_timer,
+ msecs_to_jiffies(gdev->heartbeat_interval_ms));
+}
This looks like a watchdog and should use the drivers/watchdog
subsystem interfaces.
This is not really a watchdog, this also is fully under host control,
the host controls if the guest should send a heartbeat and if so at
which interval, where normally with a watchdog starting the timer
and deciding the interval is under control of the watchdog user.
Also if we miss to generate the heartbeat then the host will just
log a message, not reset the system as one would expect with a
watchdog. TL;DR: I understand why suggest this but the watchdog
interface seems a poor match for this.
+/**
+ * vbg_query_host_version try get the host feature mask and version information
+ * (vbg_host_version).
+ *
+ * @returns 0 or negative errno value (ignored).
+ * @param gdev The Guest extension device.
+ */
+static int vbg_query_host_version(PVBOXGUESTDEVEXT gdev)
+{
+ VMMDevReqHostVersion *req;
+ int rc, ret;
+
+ req = vbg_req_alloc(sizeof(*req), VMMDevReq_GetHostVersion);
+ if (!req)
+ return -ENOMEM;
While I understand you want to keep the ioctl interface, the version information
looks like something that we should also have in sysfs in an appropriate
location.
Yes I can add a sysfs attribute for the version and flags :)
diff --git a/drivers/misc/vboxguest/vboxguest_linux.c
b/drivers/misc/vboxguest/vboxguest_linux.c
new file mode 100644
index ..8468c7139b98
--- /dev/null
+++ b/drivers/misc/vboxguest/vboxguest_linux.c
This looks like a fairly short file, and could be merged into the core file.
I would prefer to keep it separate to keep more or less 1 : 1 mapping
between mainline kernel and vbox upstream svn files.
+/** The file_operations structures. */
+static const struct file_operations vbg_misc_device_fops = {
+ .owner = THIS_MODULE,
+ .open = vbg_misc_device_open,
+ .release= vbg_misc_device_close,
+ .unlocked_ioctl = vgdrvLinuxIOCtl
Re: [RFC 1/2] misc: Add vboxguest driver for Virtual Box Guest integration
On Fri, Aug 11, 2017 at 3:23 PM, Hans de Goede wrote:
> This commit adds a driver for the Virtual Box Guest PCI device used in
> Virtual Box virtual machines. Enabling this driver will add support for
> Virtual Box Guest integration features such as copy-and-paste, seamless
> mode and OpenGL pass-through.
>
> This driver also offers vboxguest IPC functionality which is needed
> for the vboxfs driver which offers folder sharing support.
>
> Signed-off-by: Hans de Goede
Since you are still working on coding style cleanups, I'll comment mainly on
the ioctl interface for now. Overall it already looks much better than
I expected
from previous experience with the vbox source.
Most of the issues I would normally comment on are already moot based
on the assumption that we won't be able to make substantial changes to
either the user space portion or the hypervisor side.
> +/**
> + * Inflate the balloon by one chunk.
> + *
> + * The caller owns the balloon mutex.
> + *
> + * @returns VBox status code
> + * @param gdevThe Guest extension device.
> + * @param chunk_idx Index of the chunk.
> + */
> +static int vbg_balloon_inflate(PVBOXGUESTDEVEXT gdev, u32 chunk_idx)
> +{
> + VMMDevChangeMemBalloon *req = gdev->mem_balloon.change_req;
> + struct page **pages;
> + int i, rc;
> +
> + pages = kmalloc(sizeof(*pages) * VMMDEV_MEMORY_BALLOON_CHUNK_PAGES,
> + GFP_KERNEL | __GFP_NOWARN);
> + if (!pages)
> + return VERR_NO_MEMORY;
> +
> + req->header.size = sizeof(*req);
> + req->inflate = true;
> + req->pages = VMMDEV_MEMORY_BALLOON_CHUNK_PAGES;
The balloon section seems to be rather simplistic with its ioctl interface.
Ideally this should use CONFIG_BALLOON_COMPACTION and an
oom notifier like the virtio_balloon driver does. Yes, I realize that only
one of the six (or more) balloon drivers we have in the kernel does it ;-)
> +/**
> + * Callback for heartbeat timer.
> + */
> +static void vbg_heartbeat_timer(unsigned long data)
> +{
> + PVBOXGUESTDEVEXT gdev = (PVBOXGUESTDEVEXT)data;
> +
> + vbg_req_perform(gdev, gdev->guest_heartbeat_req);
> + mod_timer(&gdev->heartbeat_timer,
> + msecs_to_jiffies(gdev->heartbeat_interval_ms));
> +}
This looks like a watchdog and should use the drivers/watchdog
subsystem interfaces.
> +/**
> + * vbg_query_host_version try get the host feature mask and version
> information
> + * (vbg_host_version).
> + *
> + * @returns 0 or negative errno value (ignored).
> + * @param gdev The Guest extension device.
> + */
> +static int vbg_query_host_version(PVBOXGUESTDEVEXT gdev)
> +{
> + VMMDevReqHostVersion *req;
> + int rc, ret;
> +
> + req = vbg_req_alloc(sizeof(*req), VMMDevReq_GetHostVersion);
> + if (!req)
> + return -ENOMEM;
While I understand you want to keep the ioctl interface, the version information
looks like something that we should also have in sysfs in an appropriate
location.
> diff --git a/drivers/misc/vboxguest/vboxguest_linux.c
> b/drivers/misc/vboxguest/vboxguest_linux.c
> new file mode 100644
> index ..8468c7139b98
> --- /dev/null
> +++ b/drivers/misc/vboxguest/vboxguest_linux.c
This looks like a fairly short file, and could be merged into the core file.
> +/** The file_operations structures. */
> +static const struct file_operations vbg_misc_device_fops = {
> + .owner = THIS_MODULE,
> + .open = vbg_misc_device_open,
> + .release= vbg_misc_device_close,
> + .unlocked_ioctl = vgdrvLinuxIOCtl,
> +};
> +static const struct file_operations vbg_misc_device_user_fops = {
> + .owner = THIS_MODULE,
> + .open = vbg_misc_device_user_open,
> + .release= vbg_misc_device_close,
> + .unlocked_ioctl = vgdrvLinuxIOCtl,
> +};
These lack a .compat_ioctl callback.
> +/**
> + * Device I/O Control entry point.
> + *
> + * @returns -ENOMEM or -EFAULT for errors inside the ioctl callback; 0
> + * on success, or a positive VBox status code on vbox guest-device errors.
> + *
> + * @param pInode Associated inode pointer.
> + * @param pFilp Associated file pointer.
> + * @param uCmdThe function specified to ioctl().
> + * @param ulArg The argument specified to ioctl().
> + */
> +static long vgdrvLinuxIOCtl(struct file *pFilp, unsigned int uCmd,
> + unsigned long ulArg)
> +{
> + PVBOXGUESTSESSION session = (PVBOXGUESTSESSION) pFilp->private_data;
> + u32 cbData = _IOC_SIZE(uCmd);
> + void *pvBufFree;
> + void *pvBuf;
> + int rc, ret = 0;
> + u64 au64Buf[32 / sizeof(u64)];
> +
> + /*
> +* For small amounts of data being passed we use a stack based buffer
> +* except for VMMREQUESTs where the data must not be on the stack.
> +
