Message-ID: <dbe4bc35-d7ed-d7ba-6f87-ad3b64fea779@redhat.com>
Date:   Mon, 14 Aug 2017 14:15:52 +0200
From:   Hans de Goede <hdegoede@...hat.com>
To:     Arnd Bergmann <arnd@...db.de>
Cc:     Michael Thayer <michael.thayer@...cle.com>,
        "Knut St . Osmundsen" <knut.osmundsen@...cle.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Alexander Viro <viro@...iv.linux.org.uk>,
        Larry Finger <Larry.Finger@...inger.net>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Linux FS-devel Mailing List <linux-fsdevel@...r.kernel.org>
Subject: 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 <hdegoede@...hat.com> wrote:
>> On 11-08-17 23:23, Arnd Bergmann wrote:
>>> On Fri, Aug 11, 2017 at 3:23 PM, Hans de Goede <hdegoede@...hat.com>
>>> 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   gdev        The 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.

<snip>

>>>> +        */
>>>> +       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 /
use the 64 bit version but then we would need to always translate
the structs back to their 32 bit version on 32 bit hosts, so we
would still end up with a translation function and now we are
calling it a lot more often.


>>>> + * The layout of VMMDEV RAM region that contains information for guest.
>>>> + */
>>>> +typedef struct VMMDevMemory {
>>>> +       /** The size of this structure. */
>>>> +       u32 u32Size;
>>>> +       /** The structure version. (VMMDEV_MEMORY_VERSION) */
>>>> +       u32 u32Version;
>>>> +
>>>> +       union {
>>>> +               struct {
>>>> +                       /** Flag telling that VMMDev has events pending.
>>>> */
>>>> +                       bool fHaveEvents;
>>>> +               } V1_04;
>>>> +
>>>
>>>
>>> As this is a hardware interface, maybe use u32 instead of 'bool'.
>>
>>
>> I guess that should work here, since we only every do "if (fHaveEvents)"
>> but in other cases where we also set bool variables in the hardware
>> interface structs we also need to know which bit(s) get set on true
>> to move this over to non bool use.
>>
>> I agree that the choice of bool in a (virtual) hardware interface
>> is unfortunate, but we should not be trying to change the (virtual)
>> hardware definition IMHO.
> 
> According to the x86 psABI (https://www.uclibc.org/docs/psABI-x86_64.pdf),
> a _Bool should be represented in memory like an 'unsigned char' but only
> contain the values 0 or 1 (the upper 7 bits are zero).
> 
> Before linux-2.6.18, we defined 'bool' as 'typedef enum { false, true}
> __packed bool;', which would make it a 32-bit integer that can take the
> values 0 and 1.
> 
> Inside of the union, the two have the same binary layout
> on little-endian architectures, as the information is still stored in
> the low bit of the first of four bytes.
> 
> I think if you do
> 
>         union {
>                 struct {
>                         /** Flag telling that VMMDev has events pending. */
>                         u8 fHaveEvents;
>                         u8 pad[3];
>                 } V1_04;
> 
> that will be a portable representation if the existing ABI.

Ok, looking at the various VMMDEV_ASSERT_SIZE calls in there bool indeed
seems to be u8, so I should be able to fix this.

Michael, Knut any input on this ?  Do you want me to submit a similar
change upstream ?

<snip>

>>>> +#define VBOXGUEST_IOCTL_CODE(function, size) \
>>>> +       VBOXGUEST_IOCTL_CODE_((function) | VBOXGUEST_IOCTL_FLAG, size)
>>>> +/* Define 32 bit codes to support 32 bit applications in 64 bit guest
>>>> driver. */
>>>> +#define VBOXGUEST_IOCTL_CODE_32(function, size) \
>>>> +VBOXGUEST_IOCTL_CODE_(function, size)
>>>
>>>
>>> If the command numbers can be changed wihtout causing too many
>>> problems, I'd just do away with these wrappers and use _IOR/_IOW/_IORW
>>> as needed.
>>
>>
>> The upstream vboxguest.h header defines VBOXGUEST_IOCTL_CODE for many
>> different guest platforms. I guess not all of them have the
>> _IOR/_IOW/_IORW flags embedded in the ioctl-code concept and I believe
>> some of them only have a small amount of bits which can actually be
>> used for the code, so we cannot just cram these flags in there.
> 
> Do you have a specific example? I think the BSDs and derived operating
> systems are actually much stricter with those flags than Linux is, and the
> macros I suggested originate in BSD.

So the vbox-upstream header has this:

/** @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.
  *
  * @remarks When creating new IOCtl interfaces keep in mind that not all OSes supports
  *          reporting back the output size. (This got messed up a little bit in VBoxDrv.)
  *
  *          The request size is also a little bit tricky as it's passed as part of the
  *          request code on unix. The size field is 14 bits on Linux, 12 bits on *BSD,
  *          13 bits Darwin, and 8-bits on Solaris. All the BSDs and Darwin kernels
  *          will make use of the size field, while Linux and Solaris will not. We're of
  *          course using the size to validate and/or map/lock the request, so it has
  *          to be valid.
  *
  *          For Solaris we will have to do something special though, 255 isn't
  *          sufficient for all we need. A 4KB restriction (BSD) is probably not
  *          too problematic (yet) as a general one.
  *
  *          More info can be found in SUPDRVIOC.h and related sources.
  *
  * @remarks If adding interfaces that only has input or only has output, some new macros
  *          needs to be created so the most efficient IOCtl data buffering method can be
  *          used.
  * @{
  */

Reading this a second time I think that the reasons why there are no
separate R / W / RW versions of the macros is because all current ioctls
are RW and from the top of my mind (I've not done a full audit) that seems
correct.

So yes we should be able to just use _IOWR instead of _IOC(_IOC_READ|_IOC_WRITE
but depending on how discussion on the 64 bit flag goes we may still
need a wrapper to add in the 64 bit flag, in which case not much will change.

Hmm, rereading the header I see that the 64 bit flag is not used everywhere, a bunch
of ioctls use VBOXGUEST_IOCTL_CODE_ instead of VBOXGUEST_IOCTL_CODE, so they never
get the flag (really 2 different macros with only a _ difference in name is a bad
idea).

So the flag is only used on the following calls:

VBOXGUEST_IOCTL_GETVMMDEVPORT

VBOXGUEST_IOCTL_WRITE_CORE_DUMP
VBOXGUEST_IOCTL_HGCM_CONNECT
VBOXGUEST_IOCTL_HGCM_DISCONNECT
VBOXGUEST_IOCTL_HGCM_CALL
VBOXGUEST_IOCTL_HGCM_CALL_TIMED

VBOXGUEST_IOCTL_HGCM_CALL_USERDATA
VBOXGUEST_IOCTL_SET_MOUSE_NOTIFY_CALLBACK
VBOXGUEST_IOCTL_GUEST_CAPS_ACQUIRE

Of which the first one and the last 3 are not used under Linux.

So would could use _IOWR('V', ... directly for most of the ioctls
and then keep the 64 bit flag and have:

VBOXGUEST_IOCTL_CODE
VBOXGUEST_IOCTL_CODE32

Just for the HGCM calls.

Also I believe it would be good to move the following over to
not using the 64 bit flag:

VBOXGUEST_IOCTL_WRITE_CORE_DUMP
VBOXGUEST_IOCTL_GETVMMDEVPORT
VBOXGUEST_IOCTL_HGCM_CALL_USERDATA
VBOXGUEST_IOCTL_SET_MOUSE_NOTIFY_CALLBACK
VBOXGUEST_IOCTL_GUEST_CAPS_ACQUIRE

Since they have no 32 bit equivalent. Michael, Knut would you
be willing to take a patch moving those to using
VBOXGUEST_IOCTL_CODE_ and thus not using the 64 bit flag?

>> I think that instead we need an ioctl_direction_flags table which can
>> be used to determine if we need todo the copy_from_user and friends,
>> without changing the codes.
> 
> I would not bother with that. Either we fix the command codes to correctly
> reflect the direction, or we should actually copy the data both ways as the
> current implementation does.

Ok.

Regards,

Hans

Powered by blists - more mailing lists