vmm: fix ABI for vmexit

[corvink]

Summary

The size of cpuset_t is not fixed. It will change if we change the
number of maximum allowed cpus.

Sadly, this change already made it into 13.2. So, we have to keep
compatibility for 13.2 binaries. As we are supporting 256 cpus by
default on 13.2, we're now setting a fixed bitset size of 256 for the
ipi vmexit.

PR: 271330

[markj]

This is a partial solution, but to fix it properly we need a new version of the VM_RUN ioctl.

One way to do it is to modify struct vm_exit:

struct {
    uint32_t mode;
    uint8_t vector;
    size_t cpusetsize;
    cpuset_t *dmaskp;
} ipi;

(Or perhaps the pointer/cpusetsize should be outside of the union.)

Then, userspace sets dmaskp to point to a locally allocated cpuset before invoking VM_RUN, and vmm has to copyout the IPI mask before returning. In the kernel, you could do something like this:

case VM_RUN: {
    cpuset_t ipimask, *uipimask;
    uipimask = vmrun->ipi.dmaskp;
    vmrun->ipi.dmaskp = &ipimask;
    error = vm_run();
    if (error == 0)
        copyout(&vmrun->ipi.dmaskp, uipimask, min(vmrun->ipi.cpusetsize, sizeof(cpuset_t));

[corvink]

• add new VM_RUN ioctl

[corvink]

sys/amd64/vmm/vmm_dev.c

614

We can check if a copyout is required. E.g. vmrun->vm_exit.exitcode == VM_EXITCODE_IPI. However, I'm not sure if it's worth.

[corvink]

• put COMPAT_FREEBSD13 around declaration of vmrun_old in vmmdev_ioctl

[corvink]

I can split this patch into multiple commits (e.g. one commit which introduces the dmask member to the vcpu struct) if you like to.

[corvink]

In D40056#912304, @markj wrote:

This is a partial solution, but to fix it properly we need a new version of the VM_RUN ioctl.

One way to do it is to modify struct vm_exit:

struct {
    uint32_t mode;
    uint8_t vector;
    size_t cpusetsize;
    cpuset_t *dmaskp;
} ipi;

The union is large enough to hold the additional size_t and cpuset_t. So, we could avoid a new VM_RUN ioctl by extending the struct to:

struct {
  uint32_t mode;
  uint8_t vector;
  __BITSET_DEFINE(, 256) dmask;
  size_t cpusetsize;
  cpuset_t *dmaskp;
} ipi;

I don't like it but it could be worth to avoid the new ioctl. What do you think?

[markj]

In D40056#912799, @corvink wrote:

The union is large enough to hold the additional size_t and cpuset_t. So, we could avoid a new VM_RUN ioctl by extending the struct to:

struct {
  uint32_t mode;
  uint8_t vector;
  __BITSET_DEFINE(, 256) dmask;
  size_t cpusetsize;
  cpuset_t *dmaskp;
} ipi;

I don't like it but it could be worth to avoid the new ioctl. What do you think?

I don't really like it either. :) I think it is best to hide the compat bits as much as possible, and keep all extra complexity in the ioctl handler. In particular, things like struct vm_run and struct vm_exitinfo should not contain fields that are only used for compat.

Looking at VM_RUN a bit more, it is also a bit messy in general: libvmmapi zeros the exitinfo structure; the exitinfo structure is copied into the kernel but never read; at the end of vm_run() the kernel overwrites the copy with the one in vcpu->exitinfo; the kernel copies the copy back into userspace; libvmmapi copies the exitinfo into a caller-provided buffer. That's a lot of unnecessary copying.

I tried to write a patch to address both issues, since I think a new VM_RUN ioctl is needed anyway. The idea:

• change ipi.dmask to a pointer
• let userspace provide the cpuset buffer and buffer size in the vm_run structure
• add a cpuset to struct vcpu in the kernel, and use that when populating IPI exitinfo
• split struct vm_run so that struct vm_exit is not embedded directly
• avoid unnecessary copying of exit info: vcpu->exitinfo is copied directly out to userspace, and libvmmapi does no copying or zeroing

The diff is bigger but I think it's a bit cleaner. Any thoughts? https://reviews.freebsd.org/D40113

sys/amd64/vmm/vmm_dev.c

614

I think we should avoid a copyout when possible.

[corvink]

Solved by D40113