Re: [Qemu-devel] [PATCH] virtio-scsi: Fix build with gcc 9

[Paolo Bonzini]

On 28/02/19 19:31, Peter Maydell wrote:
> On Thu, 28 Feb 2019 at 17:59, Greg Kurz <address@hidden> 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 <address@hidden>
>> ---
>>  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 <address@hidden>
> 
> 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