* [Qemu-devel] [PATCH] vhost-user: avoid misaligned access
@ 2018-03-16 18:20 Michael S. Tsirkin
2018-03-20 2:01 ` Zhoujian (jay)
0 siblings, 1 reply; 5+ messages in thread
From: Michael S. Tsirkin @ 2018-03-16 18:20 UTC (permalink / raw)
To: qemu-devel; +Cc: Jay Zhou
We can't pass a pointer to memory field directly since
it's within a packed structure, so isn't aligned.
Pass a pointer on stack and copy.
Fixes: 30c4cc7 ("vhost: used_memslots refactoring")
Cc: Jay Zhou <jianjay.zhou@huawei.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
I had to apply this to fix make check errors with clang.
Pls review, test and ack.
Thanks!
hw/virtio/vhost-user.c | 14 +++++++++++---
1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index c12fdd9..a44ee7f 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -396,6 +396,7 @@ static int vhost_user_set_mem_table_postcopy(struct vhost_dev *dev,
bool reply_supported = virtio_has_feature(dev->protocol_features,
VHOST_USER_PROTOCOL_F_REPLY_ACK);
VhostUserMsg msg_reply;
+ VhostUserMemory memory = {};
int region_i, msg_i;
VhostUserMsg msg = {
@@ -407,10 +408,11 @@ static int vhost_user_set_mem_table_postcopy(struct vhost_dev *dev,
msg.hdr.flags |= VHOST_USER_NEED_REPLY_MASK;
}
- if (vhost_user_prepare_msg(dev, &msg.payload.memory, fds) < 0) {
+ if (vhost_user_prepare_msg(dev, &memory, fds) < 0) {
error_report("Failed preparing vhost-user memory table msg");
return -1;
}
+ msg.payload.memory = memory;
fd_num = msg.payload.memory.nregions;
@@ -549,16 +551,19 @@ static int vhost_user_set_mem_table(struct vhost_dev *dev,
.hdr.request = VHOST_USER_SET_MEM_TABLE,
.hdr.flags = VHOST_USER_VERSION,
};
+ VhostUserMemory memory = {};
if (reply_supported) {
msg.hdr.flags |= VHOST_USER_NEED_REPLY_MASK;
}
- if (vhost_user_prepare_msg(dev, &msg.payload.memory, fds) < 0) {
+ if (vhost_user_prepare_msg(dev, &memory, fds) < 0) {
error_report("Failed preparing vhost-user memory table msg");
return -1;
}
+ msg.payload.memory = memory;
+
fd_num = msg.payload.memory.nregions;
if (!fd_num) {
@@ -1575,8 +1580,11 @@ static void vhost_user_set_used_memslots(struct vhost_dev *dev)
{
int fds[VHOST_MEMORY_MAX_NREGIONS];
VhostUserMsg msg;
+ VhostUserMemory memory = {};
+
+ vhost_user_prepare_msg(dev, &memory, fds);
- vhost_user_prepare_msg(dev, &msg.payload.memory, fds);
+ msg.payload.memory = memory;
}
const VhostOps user_ops = {
--
MST
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [Qemu-devel] [PATCH] vhost-user: avoid misaligned access 2018-03-16 18:20 [Qemu-devel] [PATCH] vhost-user: avoid misaligned access Michael S. Tsirkin @ 2018-03-20 2:01 ` Zhoujian (jay) 2018-03-20 2:36 ` Michael S. Tsirkin 0 siblings, 1 reply; 5+ messages in thread From: Zhoujian (jay) @ 2018-03-20 2:01 UTC (permalink / raw) To: Michael S. Tsirkin, qemu-devel@nongnu.org > -----Original Message----- > From: Michael S. Tsirkin [mailto:mst@redhat.com] > Sent: Saturday, March 17, 2018 2:20 AM > To: qemu-devel@nongnu.org > Cc: Zhoujian (jay) <jianjay.zhou@huawei.com> > Subject: [PATCH] vhost-user: avoid misaligned access > > We can't pass a pointer to memory field directly since it's within a packed > structure, so isn't aligned. > Pass a pointer on stack and copy. > > Fixes: 30c4cc7 ("vhost: used_memslots refactoring") > Cc: Jay Zhou <jianjay.zhou@huawei.com> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > --- > > I had to apply this to fix make check errors with clang. > Pls review, test and ack. > > Thanks! > > hw/virtio/vhost-user.c | 14 +++++++++++--- > 1 file changed, 11 insertions(+), 3 deletions(-) > > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c index > c12fdd9..a44ee7f 100644 > --- a/hw/virtio/vhost-user.c > +++ b/hw/virtio/vhost-user.c > @@ -396,6 +396,7 @@ static int vhost_user_set_mem_table_postcopy(struct > vhost_dev *dev, > bool reply_supported = virtio_has_feature(dev->protocol_features, > > VHOST_USER_PROTOCOL_F_REPLY_ACK); > VhostUserMsg msg_reply; > + VhostUserMemory memory = {}; > int region_i, msg_i; > > VhostUserMsg msg = { > @@ -407,10 +408,11 @@ static int vhost_user_set_mem_table_postcopy(struct > vhost_dev *dev, > msg.hdr.flags |= VHOST_USER_NEED_REPLY_MASK; > } > > - if (vhost_user_prepare_msg(dev, &msg.payload.memory, fds) < 0) { > + if (vhost_user_prepare_msg(dev, &memory, fds) < 0) { > error_report("Failed preparing vhost-user memory table msg"); > return -1; > } > + msg.payload.memory = memory; > > fd_num = msg.payload.memory.nregions; > > @@ -549,16 +551,19 @@ static int vhost_user_set_mem_table(struct vhost_dev > *dev, > .hdr.request = VHOST_USER_SET_MEM_TABLE, > .hdr.flags = VHOST_USER_VERSION, > }; > + VhostUserMemory memory = {}; > > if (reply_supported) { > msg.hdr.flags |= VHOST_USER_NEED_REPLY_MASK; > } > > - if (vhost_user_prepare_msg(dev, &msg.payload.memory, fds) < 0) { > + if (vhost_user_prepare_msg(dev, &memory, fds) < 0) { > error_report("Failed preparing vhost-user memory table msg"); > return -1; > } > > + msg.payload.memory = memory; > + > fd_num = msg.payload.memory.nregions; > > if (!fd_num) { > @@ -1575,8 +1580,11 @@ static void vhost_user_set_used_memslots(struct > vhost_dev *dev) { > int fds[VHOST_MEMORY_MAX_NREGIONS]; > VhostUserMsg msg; > + VhostUserMemory memory = {}; > + > + vhost_user_prepare_msg(dev, &memory, fds); > > - vhost_user_prepare_msg(dev, &msg.payload.memory, fds); > + msg.payload.memory = memory; > } Hi Michael, here should be like this: static void vhost_user_set_used_memslots(struct vhost_dev *dev) { int fds[VHOST_MEMORY_MAX_NREGIONS]; - VhostUserMsg msg; + VhostUserMemory memory = {}; - vhost_user_prepare_msg(dev, &msg.payload.memory, fds); + vhost_user_prepare_msg(dev, &memory, fds); } Regards, Jay > > const VhostOps user_ops = { > -- > MST ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] vhost-user: avoid misaligned access 2018-03-20 2:01 ` Zhoujian (jay) @ 2018-03-20 2:36 ` Michael S. Tsirkin 2018-03-20 7:05 ` Igor Mammedov 0 siblings, 1 reply; 5+ messages in thread From: Michael S. Tsirkin @ 2018-03-20 2:36 UTC (permalink / raw) To: Zhoujian (jay); +Cc: qemu-devel@nongnu.org On Tue, Mar 20, 2018 at 02:01:07AM +0000, Zhoujian (jay) wrote: > > > > -----Original Message----- > > From: Michael S. Tsirkin [mailto:mst@redhat.com] > > Sent: Saturday, March 17, 2018 2:20 AM > > To: qemu-devel@nongnu.org > > Cc: Zhoujian (jay) <jianjay.zhou@huawei.com> > > Subject: [PATCH] vhost-user: avoid misaligned access > > > > We can't pass a pointer to memory field directly since it's within a packed > > structure, so isn't aligned. > > Pass a pointer on stack and copy. > > > > Fixes: 30c4cc7 ("vhost: used_memslots refactoring") > > Cc: Jay Zhou <jianjay.zhou@huawei.com> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > --- > > > > I had to apply this to fix make check errors with clang. > > Pls review, test and ack. > > > > Thanks! > > > > hw/virtio/vhost-user.c | 14 +++++++++++--- > > 1 file changed, 11 insertions(+), 3 deletions(-) > > > > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c index > > c12fdd9..a44ee7f 100644 > > --- a/hw/virtio/vhost-user.c > > +++ b/hw/virtio/vhost-user.c > > @@ -396,6 +396,7 @@ static int vhost_user_set_mem_table_postcopy(struct > > vhost_dev *dev, > > bool reply_supported = virtio_has_feature(dev->protocol_features, > > > > VHOST_USER_PROTOCOL_F_REPLY_ACK); > > VhostUserMsg msg_reply; > > + VhostUserMemory memory = {}; > > int region_i, msg_i; > > > > VhostUserMsg msg = { > > @@ -407,10 +408,11 @@ static int vhost_user_set_mem_table_postcopy(struct > > vhost_dev *dev, > > msg.hdr.flags |= VHOST_USER_NEED_REPLY_MASK; > > } > > > > - if (vhost_user_prepare_msg(dev, &msg.payload.memory, fds) < 0) { > > + if (vhost_user_prepare_msg(dev, &memory, fds) < 0) { > > error_report("Failed preparing vhost-user memory table msg"); > > return -1; > > } > > + msg.payload.memory = memory; > > > > fd_num = msg.payload.memory.nregions; > > > > @@ -549,16 +551,19 @@ static int vhost_user_set_mem_table(struct vhost_dev > > *dev, > > .hdr.request = VHOST_USER_SET_MEM_TABLE, > > .hdr.flags = VHOST_USER_VERSION, > > }; > > + VhostUserMemory memory = {}; > > > > if (reply_supported) { > > msg.hdr.flags |= VHOST_USER_NEED_REPLY_MASK; > > } > > > > - if (vhost_user_prepare_msg(dev, &msg.payload.memory, fds) < 0) { > > + if (vhost_user_prepare_msg(dev, &memory, fds) < 0) { > > error_report("Failed preparing vhost-user memory table msg"); > > return -1; > > } > > > > + msg.payload.memory = memory; > > + > > fd_num = msg.payload.memory.nregions; > > > > if (!fd_num) { > > @@ -1575,8 +1580,11 @@ static void vhost_user_set_used_memslots(struct > > vhost_dev *dev) { > > int fds[VHOST_MEMORY_MAX_NREGIONS]; > > VhostUserMsg msg; > > + VhostUserMemory memory = {}; > > + > > + vhost_user_prepare_msg(dev, &memory, fds); > > > > - vhost_user_prepare_msg(dev, &msg.payload.memory, fds); > > + msg.payload.memory = memory; > > } FYI I think it's better to pass pointer to msg to avoid alignment issues. > Hi Michael, here should be like this: > > > static void vhost_user_set_used_memslots(struct vhost_dev *dev) > { > int fds[VHOST_MEMORY_MAX_NREGIONS]; > - VhostUserMsg msg; > + VhostUserMemory memory = {}; > > - vhost_user_prepare_msg(dev, &msg.payload.memory, fds); > + vhost_user_prepare_msg(dev, &memory, fds); > } > > > Regards, > Jay But what's the point of all this? The structure is discarded after being initialized. Doesn't look right to me. > > > > const VhostOps user_ops = { > > -- > > MST ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] vhost-user: avoid misaligned access 2018-03-20 2:36 ` Michael S. Tsirkin @ 2018-03-20 7:05 ` Igor Mammedov 2018-03-20 11:55 ` Michael S. Tsirkin 0 siblings, 1 reply; 5+ messages in thread From: Igor Mammedov @ 2018-03-20 7:05 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: Zhoujian (jay), qemu-devel@nongnu.org On Tue, 20 Mar 2018 04:36:48 +0200 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Tue, Mar 20, 2018 at 02:01:07AM +0000, Zhoujian (jay) wrote: > > > > > > > -----Original Message----- > > > From: Michael S. Tsirkin [mailto:mst@redhat.com] > > > Sent: Saturday, March 17, 2018 2:20 AM > > > To: qemu-devel@nongnu.org > > > Cc: Zhoujian (jay) <jianjay.zhou@huawei.com> > > > Subject: [PATCH] vhost-user: avoid misaligned access > > > > > > We can't pass a pointer to memory field directly since it's within a packed > > > structure, so isn't aligned. > > > Pass a pointer on stack and copy. > > > > > > Fixes: 30c4cc7 ("vhost: used_memslots refactoring") > > > Cc: Jay Zhou <jianjay.zhou@huawei.com> > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > > --- > > > > > > I had to apply this to fix make check errors with clang. > > > Pls review, test and ack. > > > > > > Thanks! > > > > > > hw/virtio/vhost-user.c | 14 +++++++++++--- > > > 1 file changed, 11 insertions(+), 3 deletions(-) > > > > > > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c index > > > c12fdd9..a44ee7f 100644 > > > --- a/hw/virtio/vhost-user.c > > > +++ b/hw/virtio/vhost-user.c > > > @@ -396,6 +396,7 @@ static int vhost_user_set_mem_table_postcopy(struct > > > vhost_dev *dev, > > > bool reply_supported = virtio_has_feature(dev->protocol_features, > > > > > > VHOST_USER_PROTOCOL_F_REPLY_ACK); > > > VhostUserMsg msg_reply; > > > + VhostUserMemory memory = {}; > > > int region_i, msg_i; > > > > > > VhostUserMsg msg = { > > > @@ -407,10 +408,11 @@ static int vhost_user_set_mem_table_postcopy(struct > > > vhost_dev *dev, > > > msg.hdr.flags |= VHOST_USER_NEED_REPLY_MASK; > > > } > > > > > > - if (vhost_user_prepare_msg(dev, &msg.payload.memory, fds) < 0) { > > > + if (vhost_user_prepare_msg(dev, &memory, fds) < 0) { > > > error_report("Failed preparing vhost-user memory table msg"); > > > return -1; > > > } > > > + msg.payload.memory = memory; > > > > > > fd_num = msg.payload.memory.nregions; > > > > > > @@ -549,16 +551,19 @@ static int vhost_user_set_mem_table(struct vhost_dev > > > *dev, > > > .hdr.request = VHOST_USER_SET_MEM_TABLE, > > > .hdr.flags = VHOST_USER_VERSION, > > > }; > > > + VhostUserMemory memory = {}; > > > > > > if (reply_supported) { > > > msg.hdr.flags |= VHOST_USER_NEED_REPLY_MASK; > > > } > > > > > > - if (vhost_user_prepare_msg(dev, &msg.payload.memory, fds) < 0) { > > > + if (vhost_user_prepare_msg(dev, &memory, fds) < 0) { > > > error_report("Failed preparing vhost-user memory table msg"); > > > return -1; > > > } > > > > > > + msg.payload.memory = memory; > > > + > > > fd_num = msg.payload.memory.nregions; > > > > > > if (!fd_num) { > > > @@ -1575,8 +1580,11 @@ static void vhost_user_set_used_memslots(struct > > > vhost_dev *dev) { > > > int fds[VHOST_MEMORY_MAX_NREGIONS]; > > > VhostUserMsg msg; > > > + VhostUserMemory memory = {}; > > > + > > > + vhost_user_prepare_msg(dev, &memory, fds); > > > > > > - vhost_user_prepare_msg(dev, &msg.payload.memory, fds); > > > + msg.payload.memory = memory; > > > } > > FYI I think it's better to pass pointer to msg to > avoid alignment issues. > > > Hi Michael, here should be like this: > > > > > > static void vhost_user_set_used_memslots(struct vhost_dev *dev) > > { > > int fds[VHOST_MEMORY_MAX_NREGIONS]; > > - VhostUserMsg msg; > > + VhostUserMemory memory = {}; > > > > - vhost_user_prepare_msg(dev, &msg.payload.memory, fds); > > + vhost_user_prepare_msg(dev, &memory, fds); > > } > > > > > > Regards, > > Jay > > > But what's the point of all this? The structure is discarded > after being initialized. Doesn't look right to me. I think it was my idea, point is to share vhost_user_prepare_msg() between _set_used_memslots and _set_mem_table* instead of duplicating code, of cause at the cost of discarding results in vhost_user_set_used_memslots() > > > > > > > const VhostOps user_ops = { > > > -- > > > MST > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] vhost-user: avoid misaligned access 2018-03-20 7:05 ` Igor Mammedov @ 2018-03-20 11:55 ` Michael S. Tsirkin 0 siblings, 0 replies; 5+ messages in thread From: Michael S. Tsirkin @ 2018-03-20 11:55 UTC (permalink / raw) To: Igor Mammedov; +Cc: Zhoujian (jay), qemu-devel@nongnu.org On Tue, Mar 20, 2018 at 08:05:38AM +0100, Igor Mammedov wrote: > On Tue, 20 Mar 2018 04:36:48 +0200 > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > On Tue, Mar 20, 2018 at 02:01:07AM +0000, Zhoujian (jay) wrote: > > > > > > > > > > -----Original Message----- > > > > From: Michael S. Tsirkin [mailto:mst@redhat.com] > > > > Sent: Saturday, March 17, 2018 2:20 AM > > > > To: qemu-devel@nongnu.org > > > > Cc: Zhoujian (jay) <jianjay.zhou@huawei.com> > > > > Subject: [PATCH] vhost-user: avoid misaligned access > > > > > > > > We can't pass a pointer to memory field directly since it's within a packed > > > > structure, so isn't aligned. > > > > Pass a pointer on stack and copy. > > > > > > > > Fixes: 30c4cc7 ("vhost: used_memslots refactoring") > > > > Cc: Jay Zhou <jianjay.zhou@huawei.com> > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > > > --- > > > > > > > > I had to apply this to fix make check errors with clang. > > > > Pls review, test and ack. > > > > > > > > Thanks! > > > > > > > > hw/virtio/vhost-user.c | 14 +++++++++++--- > > > > 1 file changed, 11 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c index > > > > c12fdd9..a44ee7f 100644 > > > > --- a/hw/virtio/vhost-user.c > > > > +++ b/hw/virtio/vhost-user.c > > > > @@ -396,6 +396,7 @@ static int vhost_user_set_mem_table_postcopy(struct > > > > vhost_dev *dev, > > > > bool reply_supported = virtio_has_feature(dev->protocol_features, > > > > > > > > VHOST_USER_PROTOCOL_F_REPLY_ACK); > > > > VhostUserMsg msg_reply; > > > > + VhostUserMemory memory = {}; > > > > int region_i, msg_i; > > > > > > > > VhostUserMsg msg = { > > > > @@ -407,10 +408,11 @@ static int vhost_user_set_mem_table_postcopy(struct > > > > vhost_dev *dev, > > > > msg.hdr.flags |= VHOST_USER_NEED_REPLY_MASK; > > > > } > > > > > > > > - if (vhost_user_prepare_msg(dev, &msg.payload.memory, fds) < 0) { > > > > + if (vhost_user_prepare_msg(dev, &memory, fds) < 0) { > > > > error_report("Failed preparing vhost-user memory table msg"); > > > > return -1; > > > > } > > > > + msg.payload.memory = memory; > > > > > > > > fd_num = msg.payload.memory.nregions; > > > > > > > > @@ -549,16 +551,19 @@ static int vhost_user_set_mem_table(struct vhost_dev > > > > *dev, > > > > .hdr.request = VHOST_USER_SET_MEM_TABLE, > > > > .hdr.flags = VHOST_USER_VERSION, > > > > }; > > > > + VhostUserMemory memory = {}; > > > > > > > > if (reply_supported) { > > > > msg.hdr.flags |= VHOST_USER_NEED_REPLY_MASK; > > > > } > > > > > > > > - if (vhost_user_prepare_msg(dev, &msg.payload.memory, fds) < 0) { > > > > + if (vhost_user_prepare_msg(dev, &memory, fds) < 0) { > > > > error_report("Failed preparing vhost-user memory table msg"); > > > > return -1; > > > > } > > > > > > > > + msg.payload.memory = memory; > > > > + > > > > fd_num = msg.payload.memory.nregions; > > > > > > > > if (!fd_num) { > > > > @@ -1575,8 +1580,11 @@ static void vhost_user_set_used_memslots(struct > > > > vhost_dev *dev) { > > > > int fds[VHOST_MEMORY_MAX_NREGIONS]; > > > > VhostUserMsg msg; > > > > + VhostUserMemory memory = {}; > > > > + > > > > + vhost_user_prepare_msg(dev, &memory, fds); > > > > > > > > - vhost_user_prepare_msg(dev, &msg.payload.memory, fds); > > > > + msg.payload.memory = memory; > > > > } > > > > FYI I think it's better to pass pointer to msg to > > avoid alignment issues. > > > > > Hi Michael, here should be like this: > > > > > > > > > static void vhost_user_set_used_memslots(struct vhost_dev *dev) > > > { > > > int fds[VHOST_MEMORY_MAX_NREGIONS]; > > > - VhostUserMsg msg; > > > + VhostUserMemory memory = {}; > > > > > > - vhost_user_prepare_msg(dev, &msg.payload.memory, fds); > > > + vhost_user_prepare_msg(dev, &memory, fds); > > > } > > > > > > > > > Regards, > > > Jay > > > > > > But what's the point of all this? The structure is discarded > > after being initialized. Doesn't look right to me. > I think it was my idea, > point is to share vhost_user_prepare_msg() between > _set_used_memslots and _set_mem_table* instead of > duplicating code, of cause at the cost of discarding > results in vhost_user_set_used_memslots() So vhost_user_prepare_msg has a side effect of setting a global flag, that is why it's called here. Pls add a comment both near vhost_user_prepare_msg and where it's called. > > > > > > > > > > const VhostOps user_ops = { > > > > -- > > > > MST > > ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-03-20 11:55 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-03-16 18:20 [Qemu-devel] [PATCH] vhost-user: avoid misaligned access Michael S. Tsirkin 2018-03-20 2:01 ` Zhoujian (jay) 2018-03-20 2:36 ` Michael S. Tsirkin 2018-03-20 7:05 ` Igor Mammedov 2018-03-20 11:55 ` Michael S. Tsirkin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).