support FreeBSD14 in hvt

[hannesm]

The API around VM_RUN changed. With this change, all tests pass. The first commit is to avoid the clang warning:

# cc -MT hvt/hvt_main.o -MMD -MP -MF hvt/hvt_main.Td -fstack-protector-strong -Wall -Werror -std=c11 -O2 -g -I/.opam/4.13.1/.opam-switch/build/solo5.0.8.0/include -c hvt/hvt_main.c -o hvt/hvt_main.o
# hvt/hvt_main.c:156:9: error: variable 'argc1' set but not used [-Werror,-Wunused-but-set-variable]
#     int argc1 = argc;
#         ^
# 1 error generated.

I'm still dubious whether shipping releases of solo5 with -Werror is a good idea. IMHO it is nice for development, but since C compilers add new warnings in every release, it breaks the software.

FWIW, considering releases -- I'm still using the 0.7 series of solo5, and would appreciate a release in that series if possible. I have a branch (hannesm fork, FreeBSD14-0.7) that includes exactly these two patches, and it would be great to have them merged after review. If you could create a 0.7 branch (off of the v0.7.5 tag), I can open a separate PR.

[hannesm]

The above patches make the tests pass on my FreeBSD 14. Maybe @sg2342 is up for a review of the latter commit (I'm not a seasoned C programmer, and may have forgot stuff that should be done) -- and he has FreeBSD knowledge?

[palainp]

Thanks @hannesm . I'm also in favor of removing -Werror (and dealing with every warning during the dev process).

[palainp]

I have no knowledge with FreeBSD, and fail to understand the how the code works (I just guess that the vmrun.vm_exit field was a struct vm_exit and now is a struct vm_exit *), so maybe I'm off-topic :)

I'd wrote both #if pragma at the same place, like:

#if __FreeBSD_version >= 1400000
    struct vm_exit vmexit;
    hvb->vmrun.vm_exit = &vmexit;
    struct vm_exit *vme = &vmexit;
#else
    struct vm_exit *vme = &hvb->vmrun.vm_exit;
#endif

What I miss is if vme should be re-set for each loop iteration (it was before 1400000, but not after 1400000).

[hannesm]

My reference is bhyverun.c -- in 13.2 https://github.com/freebsd/freebsd-src/blob/releng/13.2/usr.sbin/bhyve/bhyverun.c#L984-L988

and in 14.0 https://github.com/freebsd/freebsd-src/blob/releng/14.0/usr.sbin/bhyve/bhyverun.c#L961-L965

So, vmexit is stack-allocated - it is upshed into the struct vm_run (hvb->vmrun.vm_exit = &vmexit). Previously it was extracted after the ioctl VM_RUN was called. I guess for symmetry we could put the struct vm_exit *vme at the same place, but I would avoid to change the 13.x code path (that has been tested for years in production).

I'm fine with putting:

#if __FreeBSD_version >= 1400000
    struct vm_exit vmexit;
    hvb->vmrun.vm_exit = &vmexit;
#endif

outside the loop, and:

#if __FreeBSD_version >= 1400000
    struct vm_exit *vme = &vmexit;
#else
    struct vm_exit *vme = &hvb->vmrun.vm_exit;
#endif

inside the loop - if you prefer that code. :)

--

NB: Yes, in 13.x (and 12.x), the struct vm_run was:

struct vm_run {
        int             cpuid;
        struct vm_exit  vm_exit;
};

while in a 14.x it has been revised to:

struct vm_run {
        int             cpuid;
        cpuset_t        *cpuset;        /* CPU set storage */
        size_t          cpusetsize;
        struct vm_exit  *vm_exit;
};

[hannesm]

Oh, and thanks for your review :)

[palainp]

That makes sense, thanks for the answer :)

Also regarding the new vm_run definition, shouldn't cpuset also be set to a non-NULL pointer (it probably shouldn't be, as cpusetsize is inited to 0). I was wondering if it could be better to enforce that with memset(&hvb->vmrun, 0, sizeof(struct vm_run)); but hvt is memset to 0 in hvt_init so I think this is good :)

[hannesm]

Thanks @hannesm . I'm also in favor of removing -Werror (and dealing with every warning during the dev process).

I'd be in favour to add this to the release script, i.e. before/when building the tarball temporarily remove the -Werror.

[dinosaure]

Thanks, I planned to integrate that into the next release.

[dinosaure]

We probably should add a new CI for FreeBSD 14. At least, this PR looks correct for FreeBSD 12.

[dinosaure]

Ok, the FreeBSD 14 was added into the CI, thanks for your patch, we are ready to cut a release. However, it seems that we have an issue with grub2-bhyve on FreeBSD14, the CI does an infinite loop. I will make an issue to track that but I decided to not test Solo5 virtio on FreeBSD 14.