| Message ID | 155137678223.44753.5438092367451176318.stgit@bahia.lan |
|---|---|
| State | New |
| Headers | show |
| Series | virtio-scsi: Fix build with gcc 9 | expand |
On Thu, 28 Feb 2019 at 17:59, Greg Kurz <groug@kaod.org> wrote: > > Build fails with gcc 9: > > CC ppc64-softmmu/hw/scsi/virtio-scsi.o > hw/scsi/virtio-scsi.c: In function ‘virtio_scsi_do_tmf’: > hw/scsi/virtio-scsi.c:265:39: error: taking address of packed member of ‘struct virtio_scsi_ctrl_tmf_req’ may result in an unaligned pointer value [-Werror=address-of-packed-member] > 265 | virtio_tswap32s(VIRTIO_DEVICE(s), &req->req.tmf.subtype); > | ^~~~~~~~~~~~~~~~~~~~~ > cc1: all warnings being treated as errors > > All the fields in struct virtio_scsi_ctrl_tmf_req are naturally aligned, > so we could in theory drop QEMU_PACKED. Unfortunately, the header file > is imported from linux which already has the packed attribute. Trying to > fix that in the update-linux-headers.sh script is likely to produce > ugliness. Turn the call to virtio_tswap32s() into an assignment instead. > > Signed-off-by: Greg Kurz <groug@kaod.org> > --- > hw/scsi/virtio-scsi.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c > index ce99d288b035..839f1202567d 100644 > --- a/hw/scsi/virtio-scsi.c > +++ b/hw/scsi/virtio-scsi.c > @@ -262,7 +262,13 @@ static int virtio_scsi_do_tmf(VirtIOSCSI *s, VirtIOSCSIReq *req) > /* Here VIRTIO_SCSI_S_OK means "FUNCTION COMPLETE". */ > req->resp.tmf.response = VIRTIO_SCSI_S_OK; > > - virtio_tswap32s(VIRTIO_DEVICE(s), &req->req.tmf.subtype); > + /* > + * req->req.tmf has the QEMU_PACKED attribute. Don't use virtio_tswap32s() > + * to avoid compiler errors. > + */ > + req->req.tmf.subtype = > + virtio_tswap32(VIRTIO_DEVICE(s), req->req.tmf.subtype); > + > switch (req->req.tmf.subtype) { > case VIRTIO_SCSI_T_TMF_ABORT_TASK: > case VIRTIO_SCSI_T_TMF_QUERY_TASK: Reviewed-by: Peter Maydell <peter.maydell@linaro.org> I haven't generally bothered to add comments about packed structs when I've switched away from the *s versions of byteswap functions, but it doesn't hurt. We should consider just replacing the other dozen uses of virtio_tswap*s() with the non-s versions and dropping the s functions entirely... thanks -- PMM
On 28/02/19 19:31, Peter Maydell wrote: > On Thu, 28 Feb 2019 at 17:59, Greg Kurz <groug@kaod.org> wrote: >> >> Build fails with gcc 9: >> >> CC ppc64-softmmu/hw/scsi/virtio-scsi.o >> hw/scsi/virtio-scsi.c: In function ‘virtio_scsi_do_tmf’: >> hw/scsi/virtio-scsi.c:265:39: error: taking address of packed member of ‘struct virtio_scsi_ctrl_tmf_req’ may result in an unaligned pointer value [-Werror=address-of-packed-member] >> 265 | virtio_tswap32s(VIRTIO_DEVICE(s), &req->req.tmf.subtype); >> | ^~~~~~~~~~~~~~~~~~~~~ >> cc1: all warnings being treated as errors >> >> All the fields in struct virtio_scsi_ctrl_tmf_req are naturally aligned, >> so we could in theory drop QEMU_PACKED. Unfortunately, the header file >> is imported from linux which already has the packed attribute. Trying to >> fix that in the update-linux-headers.sh script is likely to produce >> ugliness. Turn the call to virtio_tswap32s() into an assignment instead. >> >> Signed-off-by: Greg Kurz <groug@kaod.org> >> --- >> hw/scsi/virtio-scsi.c | 8 +++++++- >> 1 file changed, 7 insertions(+), 1 deletion(-) >> >> diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c >> index ce99d288b035..839f1202567d 100644 >> --- a/hw/scsi/virtio-scsi.c >> +++ b/hw/scsi/virtio-scsi.c >> @@ -262,7 +262,13 @@ static int virtio_scsi_do_tmf(VirtIOSCSI *s, VirtIOSCSIReq *req) >> /* Here VIRTIO_SCSI_S_OK means "FUNCTION COMPLETE". */ >> req->resp.tmf.response = VIRTIO_SCSI_S_OK; >> >> - virtio_tswap32s(VIRTIO_DEVICE(s), &req->req.tmf.subtype); >> + /* >> + * req->req.tmf has the QEMU_PACKED attribute. Don't use virtio_tswap32s() >> + * to avoid compiler errors. >> + */ >> + req->req.tmf.subtype = >> + virtio_tswap32(VIRTIO_DEVICE(s), req->req.tmf.subtype); >> + >> switch (req->req.tmf.subtype) { >> case VIRTIO_SCSI_T_TMF_ABORT_TASK: >> case VIRTIO_SCSI_T_TMF_QUERY_TASK: > > Reviewed-by: Peter Maydell <peter.maydell@linaro.org> > > I haven't generally bothered to add comments about packed > structs when I've switched away from the *s versions of byteswap > functions, but it doesn't hurt. > > We should consider just replacing the other dozen uses > of virtio_tswap*s() with the non-s versions and dropping the > s functions entirely... Applied, thanks. Paolo
diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c index ce99d288b035..839f1202567d 100644 --- a/hw/scsi/virtio-scsi.c +++ b/hw/scsi/virtio-scsi.c @@ -262,7 +262,13 @@ static int virtio_scsi_do_tmf(VirtIOSCSI *s, VirtIOSCSIReq *req) /* Here VIRTIO_SCSI_S_OK means "FUNCTION COMPLETE". */ req->resp.tmf.response = VIRTIO_SCSI_S_OK; - virtio_tswap32s(VIRTIO_DEVICE(s), &req->req.tmf.subtype); + /* + * req->req.tmf has the QEMU_PACKED attribute. Don't use virtio_tswap32s() + * to avoid compiler errors. + */ + req->req.tmf.subtype = + virtio_tswap32(VIRTIO_DEVICE(s), req->req.tmf.subtype); + switch (req->req.tmf.subtype) { case VIRTIO_SCSI_T_TMF_ABORT_TASK: case VIRTIO_SCSI_T_TMF_QUERY_TASK:
Build fails with gcc 9: CC ppc64-softmmu/hw/scsi/virtio-scsi.o hw/scsi/virtio-scsi.c: In function ‘virtio_scsi_do_tmf’: hw/scsi/virtio-scsi.c:265:39: error: taking address of packed member of ‘struct virtio_scsi_ctrl_tmf_req’ may result in an unaligned pointer value [-Werror=address-of-packed-member] 265 | virtio_tswap32s(VIRTIO_DEVICE(s), &req->req.tmf.subtype); | ^~~~~~~~~~~~~~~~~~~~~ cc1: all warnings being treated as errors All the fields in struct virtio_scsi_ctrl_tmf_req are naturally aligned, so we could in theory drop QEMU_PACKED. Unfortunately, the header file is imported from linux which already has the packed attribute. Trying to fix that in the update-linux-headers.sh script is likely to produce ugliness. Turn the call to virtio_tswap32s() into an assignment instead. Signed-off-by: Greg Kurz <groug@kaod.org> --- hw/scsi/virtio-scsi.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)