bhyve: Use the default compiler warnings

[markj]

Summary

Disable -Wcast-align for now since we have many instances of that
warning (I fixed some but not most of them) and platforms on which bhyve
runs don't particularly care about unaligned accesses.

[rew]

Not sure if I missed a step while applying this stack of patches but, I got the following error while build testing. I've provided a diff for context that allowed the build to complete, https://reviews.freebsd.org/differential/diff/112707/

--- pci_virtio_9p.o ---
In file included from /usr/src/usr.sbin/bhyve/pci_virtio_9p.c:57:
In file included from /usr/src/usr.sbin/bhyve/virtio.h:36:
In file included from /usr/src/sys/dev/virtio/virtio.h:34:
/usr/src/sys/dev/virtio/virtio_endian.h:46:25: error: unused parameter 'modern' [-Werror,-Wunused-parameter]
virtio_swap_endian(bool modern)
                        ^
1 error generated.
*** [pci_virtio_9p.o] Error code 1

make: stopped in /usr/src/usr.sbin/bhyve
--- pci_virtio_block.o ---
In file included from /usr/src/usr.sbin/bhyve/pci_virtio_block.c:60:
In file included from /usr/src/usr.sbin/bhyve/virtio.h:36:
In file included from /usr/src/sys/dev/virtio/virtio.h:34:
/usr/src/sys/dev/virtio/virtio_endian.h:46:25: error: unused parameter 'modern' [-Werror,-Wunused-parameter]
virtio_swap_endian(bool modern)
                        ^
1 error generated.
*** [pci_virtio_block.o] Error code 1

make: stopped in /usr/src/usr.sbin/bhyve
2 errors

make: stopped in /usr/src/usr.sbin/bhyve

[markj]

In D37296#847196, @rew wrote:

Not sure if I missed a step while applying this stack of patches but, I got the following error while build testing. I've provided a diff for context that allowed the build to complete, https://reviews.freebsd.org/differential/diff/112707/

I had a local patch for that: https://reviews.freebsd.org/D37298

The patches are also here now: https://github.com/markjdb/freebsd/tree/bhyve-warns

[corvink]

Disabling this warning will lead to even more unaligned accesses. If you're working on removing all unaligned accesses and this commit is inteded to increase the warn level early, it's fine but please mention it in the commit message.

usr.sbin/bhyve/Makefile

135–143

This should work too. Not sure which style is preferred by FreeBSD.

[rew]

In D37296#847263, @corvink wrote:

Disabling this warning will lead to even more unaligned accesses. If you're working on removing all unaligned accesses and this commit is inteded to increase the warn level early, it's fine but please mention it in the commit message.

it's already disabled.

I consider it a step forward to bump WARNS level to the default even if it means singling out a couple options - it clearly defines the warnings that have yet to be addressed.

[markj]

Use NO_WCAST_ALIGN instead of adding to CWARNFLAGS.

[markj]

In D37296#847263, @corvink wrote:

Disabling this warning will lead to even more unaligned accesses. If you're working on removing all unaligned accesses and this commit is inteded to increase the warn level early, it's fine but please mention it in the commit message.

I started working on removing unaligned accesses, but there are quite a few to go through.

[corvink]

In D37296#848809, @markj wrote:

I started working on removing unaligned accesses, but there are quite a few to go through.

Great. Thank you very much for your work.

[andy_omniosce.org]

Thanks for doing this work, I really appreciate the clean ups and being able to turn off the compiler and static analysis overrides in the illumos downstream.

As part of porting this over, I found a few remaining warnings that I have addressed there, which should probably be fixed in FreeBSD too. I'm posting them here for information, and if anyone wants to pick them up please feel free. Otherwise I should have time over Christmas to look at upstreaming them:

diff --git a/usr/src/cmd/bhyve/mem.c b/usr/src/cmd/bhyve/mem.c
index 08756161a4..c7ab17f401 100644
--- a/usr/src/cmd/bhyve/mem.c
+++ b/usr/src/cmd/bhyve/mem.c
@@ -320,6 +320,10 @@ register_mem_int(struct mmio_rb_tree *rbt, struct mem_range *memp)
                pthread_rwlock_wrlock(&mmio_rwlock);
                if (mmio_rb_lookup(rbt, memp->base, &entry) != 0)
                        err = mmio_rb_add(rbt, mrp);
+#ifndef        __FreeBSD__
+               else /* smatch warn: possible memory leak of 'mrp' */
+                       free(mrp);
+               /*
+                * When it comes to upstreaming, do the mmio_rb_lookup() check
+                * earlier and avoid allocating mrp entirely.
+                */
+#endif
                perror = pthread_rwlock_unlock(&mmio_rwlock);
                assert(perror == 0);
                if (err)
diff --git a/usr/src/cmd/bhyve/pci_emul.h b/usr/src/cmd/bhyve/pci_emul.h
index c19b6d2fac..eebea3b5aa 100644
--- a/usr/src/cmd/bhyve/pci_emul.h
+++ b/usr/src/cmd/bhyve/pci_emul.h
@@ -191,7 +191,15 @@ struct msixcap {
        uint16_t        msgctrl;
        uint32_t        table_info;     /* bar index and offset within it */
        uint32_t        pba_info;       /* bar index and offset within it */
-} __packed;
+} __packed __aligned(4);
+#ifndef        __FreeBSD__
+/*
+ * Upstream divergence note: the __aligned(4) was added for illumos.
+ * __packed forces the alignment to 1 and pci_passthru.c casts a uint32_t * to
+ * the address of an instance of this struct on the stack. Ensure the struct
+ * itself is aligned to support this. This should be upstreamed.
+ */
+#endif
 static_assert(sizeof(struct msixcap) == 12, "compile-time assertion failed");

 struct pciecap {
diff --git a/usr/src/cmd/bhyve/pci_nvme.c b/usr/src/cmd/bhyve/pci_nvme.c
index 270173a06a..6f19e9c950 100644
--- a/usr/src/cmd/bhyve/pci_nvme.c
+++ b/usr/src/cmd/bhyve/pci_nvme.c
@@ -2666,7 +2666,13 @@ nvme_opc_dataset_mgmt(struct pci_nvme_softc *sc,
        nr = cmd->cdw10 & 0xff;

        /* copy locally because a range entry could straddle PRPs */
+#ifdef __FreeBSD__
        range = calloc(1, NVME_MAX_DSM_TRIM);
+#else
+       /* smatch: warn: double check that we're allocating correct size: 16 vs 4096 */
+       _Static_assert(NVME_MAX_DSM_TRIM % sizeof(struct nvme_dsm_range) == 0,
+           "NVME_MAX_DSM_TRIM is not a multiple of struct size");
+       range = calloc(NVME_MAX_DSM_TRIM / sizeof (*range), sizeof (*range));
+#endif
        if (range == NULL) {
                pci_nvme_status_genc(status, NVME_SC_INTERNAL_DEVICE_ERROR);
                goto out;
@@ -3186,8 +3192,13 @@ pci_nvme_parse_config(struct pci_nvme_softc *sc, nvlist_t *nvl)
        sc->num_cqueues = sc->max_queues;
        sc->dataset_management = NVME_DATASET_MANAGEMENT_AUTO;
        sectsz = 0;
+#ifdef __FreeBSD__
        snprintf(sc->ctrldata.sn, sizeof(sc->ctrldata.sn),
                 "NVME-%d-%d", sc->nsc_pi->pi_slot, sc->nsc_pi->pi_func);
+#else
+       snprintf((char *)sc->ctrldata.sn, sizeof(sc->ctrldata.sn),
+                "NVME-%d-%d", sc->nsc_pi->pi_slot, sc->nsc_pi->pi_func);
+#endif

        value = get_config_value_node(nvl, "maxq");
        if (value != NULL)
diff --git a/usr/src/cmd/bhyve/pci_virtio_9p.c b/usr/src/cmd/bhyve/pci_virtio_9p.c
index 86ec29301d..386eba31b1 100644
--- a/usr/src/cmd/bhyve/pci_virtio_9p.c
+++ b/usr/src/cmd/bhyve/pci_virtio_9p.c
@@ -98,7 +98,11 @@ struct pci_vt9p_request {

 struct pci_vt9p_config {
        uint16_t tag_len;
+#ifdef __FreeBSD__
        char tag[0];
+#else
+       char tag[VT9P_MAXTAGSZ];
+#endif
 } __attribute__((packed));

 static int pci_vt9p_send(struct l9p_request *, const struct iovec *,
@@ -312,16 +316,16 @@ pci_vt9p_init(struct vmctx *ctx __unused, struct pci_devinst *pi, nvlist_t *nvl)
        }

        sc = calloc(1, sizeof(struct pci_vt9p_softc));
+#ifdef __FreeBSD__
+       sc->vsc_config = calloc(1, sizeof(struct pci_vt9p_config) +
+           VT9P_MAXTAGSZ);
+#else
+       /* smatch warn: double check that we're allocating correct size: 2 vs 258 */
+       sc->vsc_config = calloc(1, sizeof(struct pci_vt9p_config));
+#endif
        if (sc == NULL) {
                EPRINTLN("virtio-9p: vsc_config allocation failure: %s",
                    strerror(errno));

[markj]

In D37296#851377, @andy_omniosce.org wrote:

Thanks for doing this work, I really appreciate the clean ups and being able to turn off the compiler and static analysis overrides in the illumos downstream.

As part of porting this over, I found a few remaining warnings that I have addressed there, which should probably be fixed in FreeBSD too. I'm posting them here for information, and if anyone wants to pick them up please feel free. Otherwise I should have time over Christmas to look at upstreaming them:

Thanks for these patches. They all seem reasonable to me. If it's just these patches I can spend some time getting them committed.

diff --git a/usr/src/cmd/bhyve/mem.c b/usr/src/cmd/bhyve/mem.c
index 08756161a4..c7ab17f401 100644
--- a/usr/src/cmd/bhyve/mem.c
+++ b/usr/src/cmd/bhyve/mem.c
@@ -320,6 +320,10 @@ register_mem_int(struct mmio_rb_tree *rbt, struct mem_range *memp)
                pthread_rwlock_wrlock(&mmio_rwlock);
                if (mmio_rb_lookup(rbt, memp->base, &entry) != 0)
                        err = mmio_rb_add(rbt, mrp);
+#ifndef        __FreeBSD__
+               else /* smatch warn: possible memory leak of 'mrp' */
+                       free(mrp);

I suspect we should return an error (EEXIST?) to the caller in this case as well, since callers probably don't expect the range to already be reserved?

+ /*
+ * When it comes to upstreaming, do the mmio_rb_lookup() check
+ * earlier and avoid allocating mrp entirely.
+ */
+#endif

perror = pthread_rwlock_unlock(&mmio_rwlock);
assert(perror == 0);
if (err)

diff --git a/usr/src/cmd/bhyve/pci_emul.h b/usr/src/cmd/bhyve/pci_emul.h
index c19b6d2fac..eebea3b5aa 100644

• a/usr/src/cmd/bhyve/pci_emul.h

+++ b/usr/src/cmd/bhyve/pci_emul.h
@@ -191,7 +191,15 @@ struct msixcap {

uint16_t        msgctrl;
uint32_t        table_info;     /* bar index and offset within it */
uint32_t        pba_info;       /* bar index and offset within it */

-} packed;
+} packed aligned(4);
+#ifndef FreeBSD__
+/*
+ * Upstream divergence note: the aligned(4) was added for illumos.
+ * packed forces the alignment to 1 and pci_passthru.c casts a uint32_t * to
+ * the address of an instance of this struct on the stack. Ensure the struct
+ * itself is aligned to support this. This should be upstreamed.
+ */
+#endif

Isn't it enough to add the __aligned directive to the stack variable?

static_assert(sizeof(struct msixcap) == 12, "compile-time assertion failed");

struct pciecap {
diff --git a/usr/src/cmd/bhyve/pci_nvme.c b/usr/src/cmd/bhyve/pci_nvme.c
index 270173a06a..6f19e9c950 100644

• a/usr/src/cmd/bhyve/pci_nvme.c

+++ b/usr/src/cmd/bhyve/pci_nvme.c
@@ -2666,7 +2666,13 @@ nvme_opc_dataset_mgmt(struct pci_nvme_softc *sc,

nr = cmd->cdw10 & 0xff;

/* copy locally because a range entry could straddle PRPs */

+#ifdef FreeBSD

range = calloc(1, NVME_MAX_DSM_TRIM);

+#else
+ /* smatch: warn: double check that we're allocating correct size: 16 vs 4096 */
+ _Static_assert(NVME_MAX_DSM_TRIM % sizeof(struct nvme_dsm_range) == 0,
+ "NVME_MAX_DSM_TRIM is not a multiple of struct size");
+ range = calloc(NVME_MAX_DSM_TRIM / sizeof (*range), sizeof (*range));
+#endif

This seems fine.

if (range == NULL) {
        pci_nvme_status_genc(status, NVME_SC_INTERNAL_DEVICE_ERROR);
        goto out;

@@ -3186,8 +3192,13 @@ pci_nvme_parse_config(struct pci_nvme_softc *sc, nvlist_t *nvl)

sc->num_cqueues = sc->max_queues;
sc->dataset_management = NVME_DATASET_MANAGEMENT_AUTO;
sectsz = 0;

+#ifdef FreeBSD

snprintf(sc->ctrldata.sn, sizeof(sc->ctrldata.sn),
         "NVME-%d-%d", sc->nsc_pi->pi_slot, sc->nsc_pi->pi_func);

+#else
+ snprintf((char *)sc->ctrldata.sn, sizeof(sc->ctrldata.sn),
+ "NVME-%d-%d", sc->nsc_pi->pi_slot, sc->nsc_pi->pi_func);
+#endif

Seems ok.

value = get_config_value_node(nvl, "maxq");
if (value != NULL)

diff --git a/usr/src/cmd/bhyve/pci_virtio_9p.c b/usr/src/cmd/bhyve/pci_virtio_9p.c
index 86ec29301d..386eba31b1 100644

• a/usr/src/cmd/bhyve/pci_virtio_9p.c

+++ b/usr/src/cmd/bhyve/pci_virtio_9p.c
@@ -98,7 +98,11 @@ struct pci_vt9p_request {

struct pci_vt9p_config {

uint16_t tag_len;

+#ifdef FreeBSD

char tag[0];

+#else
+ char tag[VT9P_MAXTAGSZ];
+#endif
} attribute((packed));

static int pci_vt9p_send(struct l9p_request *, const struct iovec *,
@@ -312,16 +316,16 @@ pci_vt9p_init(struct vmctx *ctx __unused, struct pci_devinst *pi, nvlist_t *nvl)

}

sc = calloc(1, sizeof(struct pci_vt9p_softc));

+#ifdef FreeBSD
+ sc->vsc_config = calloc(1, sizeof(struct pci_vt9p_config) +
+ VT9P_MAXTAGSZ);
+#else
+ /* smatch warn: double check that we're allocating correct size: 2 vs 258 */
+ sc->vsc_config = calloc(1, sizeof(struct pci_vt9p_config));
+#endif

This seems right. We should always allocate the maximum size, otherwise pci_vt9p_cfgread() can potentially trigger an out-of-bounds read...

if (sc == NULL) {
        EPRINTLN("virtio-9p: vsc_config allocation failure: %s",
            strerror(errno));