From: "Michael S. Tsirkin" <mst@redhat.com>
To: Nathan Chancellor <natechancellor@gmail.com>
Cc: Jason Wang <jasowang@redhat.com>,
kvm@vger.kernel.org, virtualization@lists.linux-foundation.org,
netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
Nick Desaulniers <ndesaulniers@google.com>,
clang-built-linux@googlegroups.com
Subject: Re: [PATCH] vhost: Don't use defined in VHOST_ARCH_CAN_ACCEL_UACCESS definition
Date: Thu, 6 Jun 2019 14:28:55 -0400 [thread overview]
Message-ID: <20190606142606-mutt-send-email-mst@kernel.org> (raw)
In-Reply-To: <20190606161223.67979-1-natechancellor@gmail.com>
On Thu, Jun 06, 2019 at 09:12:23AM -0700, Nathan Chancellor wrote:
> Clang warns:
>
> drivers/vhost/vhost.c:2085:5: warning: macro expansion producing
> 'defined' has undefined behavior [-Wexpansion-to-defined]
> #if VHOST_ARCH_CAN_ACCEL_UACCESS
> ^
> drivers/vhost/vhost.h:98:38: note: expanded from macro
> 'VHOST_ARCH_CAN_ACCEL_UACCESS'
> #define VHOST_ARCH_CAN_ACCEL_UACCESS defined(CONFIG_MMU_NOTIFIER) && \
> ^
>
> Rework VHOST_ARCH_CAN_ACCEL_UACCESS to be defined under those conditions
> so that the meaning of the code doesn't change and clang no longer
> warns.
>
> Fixes: 7f466032dc9e ("vhost: access vq metadata through kernel virtual address")
> Link: https://github.com/ClangBuiltLinux/linux/issues/508
> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> ---
> drivers/vhost/vhost.c | 44 +++++++++++++++++++++----------------------
> drivers/vhost/vhost.h | 7 ++++---
> 2 files changed, 26 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index dc9301d31f12..cc56d08b4275 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -299,7 +299,7 @@ static void vhost_vq_meta_reset(struct vhost_dev *d)
> __vhost_vq_meta_reset(d->vqs[i]);
> }
>
> -#if VHOST_ARCH_CAN_ACCEL_UACCESS
> +#ifdef VHOST_ARCH_CAN_ACCEL_UACCESS
> static void vhost_map_unprefetch(struct vhost_map *map)
> {
> kfree(map->pages);
> @@ -483,7 +483,7 @@ static void vhost_vq_reset(struct vhost_dev *dev,
> vq->iotlb = NULL;
> vq->invalidate_count = 0;
> __vhost_vq_meta_reset(vq);
> -#if VHOST_ARCH_CAN_ACCEL_UACCESS
> +#ifdef VHOST_ARCH_CAN_ACCEL_UACCESS
> vhost_reset_vq_maps(vq);
> #endif
> }
> @@ -635,7 +635,7 @@ void vhost_dev_init(struct vhost_dev *dev,
> INIT_LIST_HEAD(&dev->read_list);
> INIT_LIST_HEAD(&dev->pending_list);
> spin_lock_init(&dev->iotlb_lock);
> -#if VHOST_ARCH_CAN_ACCEL_UACCESS
> +#ifdef VHOST_ARCH_CAN_ACCEL_UACCESS
> vhost_init_maps(dev);
> #endif
>
> @@ -726,7 +726,7 @@ long vhost_dev_set_owner(struct vhost_dev *dev)
> if (err)
> goto err_cgroup;
>
> -#if VHOST_ARCH_CAN_ACCEL_UACCESS
> +#ifdef VHOST_ARCH_CAN_ACCEL_UACCESS
> err = mmu_notifier_register(&dev->mmu_notifier, dev->mm);
> if (err)
> goto err_mmu_notifier;
> @@ -734,7 +734,7 @@ long vhost_dev_set_owner(struct vhost_dev *dev)
>
> return 0;
>
> -#if VHOST_ARCH_CAN_ACCEL_UACCESS
> +#ifdef VHOST_ARCH_CAN_ACCEL_UACCESS
> err_mmu_notifier:
> vhost_dev_free_iovecs(dev);
> #endif
> @@ -828,7 +828,7 @@ static void vhost_clear_msg(struct vhost_dev *dev)
> spin_unlock(&dev->iotlb_lock);
> }
>
> -#if VHOST_ARCH_CAN_ACCEL_UACCESS
> +#ifdef VHOST_ARCH_CAN_ACCEL_UACCESS
> static void vhost_setup_uaddr(struct vhost_virtqueue *vq,
> int index, unsigned long uaddr,
> size_t size, bool write)
> @@ -959,12 +959,12 @@ void vhost_dev_cleanup(struct vhost_dev *dev)
> dev->worker = NULL;
> }
> if (dev->mm) {
> -#if VHOST_ARCH_CAN_ACCEL_UACCESS
> +#ifdef VHOST_ARCH_CAN_ACCEL_UACCESS
> mmu_notifier_unregister(&dev->mmu_notifier, dev->mm);
> #endif
> mmput(dev->mm);
> }
> -#if VHOST_ARCH_CAN_ACCEL_UACCESS
> +#ifdef VHOST_ARCH_CAN_ACCEL_UACCESS
> for (i = 0; i < dev->nvqs; i++)
> vhost_uninit_vq_maps(dev->vqs[i]);
> #endif
> @@ -1196,7 +1196,7 @@ static inline void __user *__vhost_get_user(struct vhost_virtqueue *vq,
>
> static inline int vhost_put_avail_event(struct vhost_virtqueue *vq)
> {
> -#if VHOST_ARCH_CAN_ACCEL_UACCESS
> +#ifdef VHOST_ARCH_CAN_ACCEL_UACCESS
> struct vhost_map *map;
> struct vring_used *used;
>
> @@ -1224,7 +1224,7 @@ static inline int vhost_put_used(struct vhost_virtqueue *vq,
> struct vring_used_elem *head, int idx,
> int count)
> {
> -#if VHOST_ARCH_CAN_ACCEL_UACCESS
> +#ifdef VHOST_ARCH_CAN_ACCEL_UACCESS
> struct vhost_map *map;
> struct vring_used *used;
> size_t size;
> @@ -1252,7 +1252,7 @@ static inline int vhost_put_used(struct vhost_virtqueue *vq,
> static inline int vhost_put_used_flags(struct vhost_virtqueue *vq)
>
> {
> -#if VHOST_ARCH_CAN_ACCEL_UACCESS
> +#ifdef VHOST_ARCH_CAN_ACCEL_UACCESS
> struct vhost_map *map;
> struct vring_used *used;
>
> @@ -1278,7 +1278,7 @@ static inline int vhost_put_used_flags(struct vhost_virtqueue *vq)
> static inline int vhost_put_used_idx(struct vhost_virtqueue *vq)
>
> {
> -#if VHOST_ARCH_CAN_ACCEL_UACCESS
> +#ifdef VHOST_ARCH_CAN_ACCEL_UACCESS
> struct vhost_map *map;
> struct vring_used *used;
>
> @@ -1342,7 +1342,7 @@ static void vhost_dev_unlock_vqs(struct vhost_dev *d)
> static inline int vhost_get_avail_idx(struct vhost_virtqueue *vq,
> __virtio16 *idx)
> {
> -#if VHOST_ARCH_CAN_ACCEL_UACCESS
> +#ifdef VHOST_ARCH_CAN_ACCEL_UACCESS
> struct vhost_map *map;
> struct vring_avail *avail;
>
> @@ -1367,7 +1367,7 @@ static inline int vhost_get_avail_idx(struct vhost_virtqueue *vq,
> static inline int vhost_get_avail_head(struct vhost_virtqueue *vq,
> __virtio16 *head, int idx)
> {
> -#if VHOST_ARCH_CAN_ACCEL_UACCESS
> +#ifdef VHOST_ARCH_CAN_ACCEL_UACCESS
> struct vhost_map *map;
> struct vring_avail *avail;
>
> @@ -1393,7 +1393,7 @@ static inline int vhost_get_avail_head(struct vhost_virtqueue *vq,
> static inline int vhost_get_avail_flags(struct vhost_virtqueue *vq,
> __virtio16 *flags)
> {
> -#if VHOST_ARCH_CAN_ACCEL_UACCESS
> +#ifdef VHOST_ARCH_CAN_ACCEL_UACCESS
> struct vhost_map *map;
> struct vring_avail *avail;
>
> @@ -1418,7 +1418,7 @@ static inline int vhost_get_avail_flags(struct vhost_virtqueue *vq,
> static inline int vhost_get_used_event(struct vhost_virtqueue *vq,
> __virtio16 *event)
> {
> -#if VHOST_ARCH_CAN_ACCEL_UACCESS
> +#ifdef VHOST_ARCH_CAN_ACCEL_UACCESS
> struct vhost_map *map;
> struct vring_avail *avail;
>
> @@ -1441,7 +1441,7 @@ static inline int vhost_get_used_event(struct vhost_virtqueue *vq,
> static inline int vhost_get_used_idx(struct vhost_virtqueue *vq,
> __virtio16 *idx)
> {
> -#if VHOST_ARCH_CAN_ACCEL_UACCESS
> +#ifdef VHOST_ARCH_CAN_ACCEL_UACCESS
> struct vhost_map *map;
> struct vring_used *used;
>
> @@ -1466,7 +1466,7 @@ static inline int vhost_get_used_idx(struct vhost_virtqueue *vq,
> static inline int vhost_get_desc(struct vhost_virtqueue *vq,
> struct vring_desc *desc, int idx)
> {
> -#if VHOST_ARCH_CAN_ACCEL_UACCESS
> +#ifdef VHOST_ARCH_CAN_ACCEL_UACCESS
> struct vhost_map *map;
> struct vring_desc *d;
>
> @@ -1825,7 +1825,7 @@ static bool iotlb_access_ok(struct vhost_virtqueue *vq,
> return true;
> }
>
> -#if VHOST_ARCH_CAN_ACCEL_UACCESS
> +#ifdef VHOST_ARCH_CAN_ACCEL_UACCESS
> static void vhost_vq_map_prefetch(struct vhost_virtqueue *vq)
> {
> struct vhost_map __rcu *map;
> @@ -1846,7 +1846,7 @@ int vq_meta_prefetch(struct vhost_virtqueue *vq)
> unsigned int num = vq->num;
>
> if (!vq->iotlb) {
> -#if VHOST_ARCH_CAN_ACCEL_UACCESS
> +#ifdef VHOST_ARCH_CAN_ACCEL_UACCESS
> vhost_vq_map_prefetch(vq);
> #endif
> return 1;
> @@ -2061,7 +2061,7 @@ static long vhost_vring_set_num_addr(struct vhost_dev *d,
>
> mutex_lock(&vq->mutex);
>
> -#if VHOST_ARCH_CAN_ACCEL_UACCESS
> +#ifdef VHOST_ARCH_CAN_ACCEL_UACCESS
> /* Unregister MMU notifer to allow invalidation callback
> * can access vq->uaddrs[] without holding a lock.
> */
> @@ -2082,7 +2082,7 @@ static long vhost_vring_set_num_addr(struct vhost_dev *d,
> BUG();
> }
>
> -#if VHOST_ARCH_CAN_ACCEL_UACCESS
> +#ifdef VHOST_ARCH_CAN_ACCEL_UACCESS
> vhost_setup_vq_uaddr(vq);
>
> if (d->mm)
> diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
> index c5d950cf7627..d9f36c479fa7 100644
> --- a/drivers/vhost/vhost.h
> +++ b/drivers/vhost/vhost.h
> @@ -95,8 +95,9 @@ struct vhost_uaddr {
> bool write;
> };
>
> -#define VHOST_ARCH_CAN_ACCEL_UACCESS defined(CONFIG_MMU_NOTIFIER) && \
> - ARCH_IMPLEMENTS_FLUSH_DCACHE_PAGE == 0
> +#if defined(CONFIG_MMU_NOTIFIER) && ARCH_IMPLEMENTS_FLUSH_DCACHE_PAGE == 0
> +#define VHOST_ARCH_CAN_ACCEL_UACCESS
> +#endif
>
> /* The virtqueue structure describes a queue attached to a device. */
> struct vhost_virtqueue {
> @@ -109,7 +110,7 @@ struct vhost_virtqueue {
> struct vring_avail __user *avail;
> struct vring_used __user *used;
>
> -#if VHOST_ARCH_CAN_ACCEL_UACCESS
> +#ifdef VHOST_ARCH_CAN_ACCEL_UACCESS
> /* Read by memory accessors, modified by meta data
> * prefetching, MMU notifier and vring ioctl().
> * Synchonrized through mmu_lock (writers) and RCU (writers
> --
> 2.22.0.rc3
I'd prefer just changing the definition.
ifdefs have a disadvantage that it's easy to get
wrong code if you forget to include a header.
I queued the below - pls confirm it works for you.
diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
index c5d950cf7627..819296332913 100644
--- a/drivers/vhost/vhost.h
+++ b/drivers/vhost/vhost.h
@@ -95,8 +95,11 @@ struct vhost_uaddr {
bool write;
};
-#define VHOST_ARCH_CAN_ACCEL_UACCESS defined(CONFIG_MMU_NOTIFIER) && \
- ARCH_IMPLEMENTS_FLUSH_DCACHE_PAGE == 0
+#if defined(CONFIG_MMU_NOTIFIER) && ARCH_IMPLEMENTS_FLUSH_DCACHE_PAGE == 0
+#define VHOST_ARCH_CAN_ACCEL_UACCESS 1
+#else
+#define VHOST_ARCH_CAN_ACCEL_UACCESS 0
+#endif
/* The virtqueue structure describes a queue attached to a device. */
struct vhost_virtqueue {
next prev parent reply other threads:[~2019-06-06 18:29 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-06-06 16:12 [PATCH] vhost: Don't use defined in VHOST_ARCH_CAN_ACCEL_UACCESS definition Nathan Chancellor
2019-06-06 18:28 ` Michael S. Tsirkin
2019-06-06 18:28 ` Michael S. Tsirkin [this message]
2019-06-06 20:06 ` Nathan Chancellor
2019-06-06 20:06 ` Nathan Chancellor
-- strict thread matches above, loose matches on Subject: below --
2019-06-06 16:12 Nathan Chancellor
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190606142606-mutt-send-email-mst@kernel.org \
--to=mst@redhat.com \
--cc=clang-built-linux@googlegroups.com \
--cc=jasowang@redhat.com \
--cc=kvm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=natechancellor@gmail.com \
--cc=ndesaulniers@google.com \
--cc=netdev@vger.kernel.org \
--cc=virtualization@lists.linux-foundation.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.