Re: [Qemu-devel] [PATCH] hw/scsi/mptendian: Avoid taking address of fields in packed structs

[Paolo Bonzini]

On 27/09/2018 15:48, Peter Maydell wrote:
> Taking the address of a field in a packed struct is a bad idea, because
> it might not be actually aligned enough for that pointer type (and
> thus cause a crash on dereference on some host architectures). Newer
> versions of clang warn about this. Avoid the bug by not using the
> "modify in place" byte swapping functions.
> 
> This patch was produced with the following simple spatch script:
> @@
> expression E;
> @@
> -le16_to_cpus(&E);
> +E = le16_to_cpu(E);
> @@
> expression E;
> @@
> -le32_to_cpus(&E);
> +E = le32_to_cpu(E);
> @@
> expression E;
> @@
> -le64_to_cpus(&E);
> +E = le64_to_cpu(E);
> @@
> expression E;
> @@
> -cpu_to_le16s(&E);
> +E = cpu_to_le16(E);
> @@
> expression E;
> @@
> -cpu_to_le32s(&E);
> +E = cpu_to_le32(E);
> @@
> expression E;
> @@
> -cpu_to_le64s(&E);
> +E = cpu_to_le64(E);
> 
> followed by some minor tidying of overlong lines and bad indent.
> 
> Signed-off-by: Peter Maydell <address@hidden>
> ---
> NB: tested with qemu-system-x86_64 -cpu host -enable-kvm -m 4096
>  -device mptsas1068 -drive id=mydisk,if=none,file=harddisk.qcow2
>  -device scsi-disk,drive=mydisk
> which behaves no differently before or after the patch, though
> the guest I have (an x86 ubuntu) seems to detect the controller
> but not find any disks attached to it. Maybe my command line is wrong?

I think you need to specify the WWN, port WWN and port index of the disk.

Paolo

>  hw/scsi/mptendian.c | 163 ++++++++++++++++++++++----------------------
>  1 file changed, 83 insertions(+), 80 deletions(-)
> 
> diff --git a/hw/scsi/mptendian.c b/hw/scsi/mptendian.c
> index 8ae39a76f42..79f99734d21 100644
> --- a/hw/scsi/mptendian.c
> +++ b/hw/scsi/mptendian.c
> @@ -35,152 +35,155 @@
>  
>  static void mptsas_fix_sgentry_endianness(MPISGEntry *sge)
>  {
> -    le32_to_cpus(&sge->FlagsLength);
> +    sge->FlagsLength = le32_to_cpu(sge->FlagsLength);
>      if (sge->FlagsLength & MPI_SGE_FLAGS_64_BIT_ADDRESSING) {
> -       le64_to_cpus(&sge->u.Address64);
> +        sge->u.Address64 = le64_to_cpu(sge->u.Address64);
>      } else {
> -       le32_to_cpus(&sge->u.Address32);
> +        sge->u.Address32 = le32_to_cpu(sge->u.Address32);
>      }
>  }
>  
>  static void mptsas_fix_sgentry_endianness_reply(MPISGEntry *sge)
>  {
>      if (sge->FlagsLength & MPI_SGE_FLAGS_64_BIT_ADDRESSING) {
> -       cpu_to_le64s(&sge->u.Address64);
> +        sge->u.Address64 = cpu_to_le64(sge->u.Address64);
>      } else {
> -       cpu_to_le32s(&sge->u.Address32);
> +        sge->u.Address32 = cpu_to_le32(sge->u.Address32);
>      }
> -    cpu_to_le32s(&sge->FlagsLength);
> +    sge->FlagsLength = cpu_to_le32(sge->FlagsLength);
>  }
>  
>  void mptsas_fix_scsi_io_endianness(MPIMsgSCSIIORequest *req)
>  {
> -    le32_to_cpus(&req->MsgContext);
> -    le32_to_cpus(&req->Control);
> -    le32_to_cpus(&req->DataLength);
> -    le32_to_cpus(&req->SenseBufferLowAddr);
> +    req->MsgContext = le32_to_cpu(req->MsgContext);
> +    req->Control = le32_to_cpu(req->Control);
> +    req->DataLength = le32_to_cpu(req->DataLength);
> +    req->SenseBufferLowAddr = le32_to_cpu(req->SenseBufferLowAddr);
>  }
>  
>  void mptsas_fix_scsi_io_reply_endianness(MPIMsgSCSIIOReply *reply)
>  {
> -    cpu_to_le32s(&reply->MsgContext);
> -    cpu_to_le16s(&reply->IOCStatus);
> -    cpu_to_le32s(&reply->IOCLogInfo);
> -    cpu_to_le32s(&reply->TransferCount);
> -    cpu_to_le32s(&reply->SenseCount);
> -    cpu_to_le32s(&reply->ResponseInfo);
> -    cpu_to_le16s(&reply->TaskTag);
> +    reply->MsgContext = cpu_to_le32(reply->MsgContext);
> +    reply->IOCStatus = cpu_to_le16(reply->IOCStatus);
> +    reply->IOCLogInfo = cpu_to_le32(reply->IOCLogInfo);
> +    reply->TransferCount = cpu_to_le32(reply->TransferCount);
> +    reply->SenseCount = cpu_to_le32(reply->SenseCount);
> +    reply->ResponseInfo = cpu_to_le32(reply->ResponseInfo);
> +    reply->TaskTag = cpu_to_le16(reply->TaskTag);
>  }
>  
>  void mptsas_fix_scsi_task_mgmt_endianness(MPIMsgSCSITaskMgmt *req)
>  {
> -    le32_to_cpus(&req->MsgContext);
> -    le32_to_cpus(&req->TaskMsgContext);
> +    req->MsgContext = le32_to_cpu(req->MsgContext);
> +    req->TaskMsgContext = le32_to_cpu(req->TaskMsgContext);
>  }
>  
>  void mptsas_fix_scsi_task_mgmt_reply_endianness(MPIMsgSCSITaskMgmtReply 
> *reply)
>  {
> -    cpu_to_le32s(&reply->MsgContext);
> -    cpu_to_le16s(&reply->IOCStatus);
> -    cpu_to_le32s(&reply->IOCLogInfo);
> -    cpu_to_le32s(&reply->TerminationCount);
> +    reply->MsgContext = cpu_to_le32(reply->MsgContext);
> +    reply->IOCStatus = cpu_to_le16(reply->IOCStatus);
> +    reply->IOCLogInfo = cpu_to_le32(reply->IOCLogInfo);
> +    reply->TerminationCount = cpu_to_le32(reply->TerminationCount);
>  }
>  
>  void mptsas_fix_ioc_init_endianness(MPIMsgIOCInit *req)
>  {
> -    le32_to_cpus(&req->MsgContext);
> -    le16_to_cpus(&req->ReplyFrameSize);
> -    le32_to_cpus(&req->HostMfaHighAddr);
> -    le32_to_cpus(&req->SenseBufferHighAddr);
> -    le32_to_cpus(&req->ReplyFifoHostSignalingAddr);
> +    req->MsgContext = le32_to_cpu(req->MsgContext);
> +    req->ReplyFrameSize = le16_to_cpu(req->ReplyFrameSize);
> +    req->HostMfaHighAddr = le32_to_cpu(req->HostMfaHighAddr);
> +    req->SenseBufferHighAddr = le32_to_cpu(req->SenseBufferHighAddr);
> +    req->ReplyFifoHostSignalingAddr =
> +        le32_to_cpu(req->ReplyFifoHostSignalingAddr);
>      mptsas_fix_sgentry_endianness(&req->HostPageBufferSGE);
> -    le16_to_cpus(&req->MsgVersion);
> -    le16_to_cpus(&req->HeaderVersion);
> +    req->MsgVersion = le16_to_cpu(req->MsgVersion);
> +    req->HeaderVersion = le16_to_cpu(req->HeaderVersion);
>  }
>  
>  void mptsas_fix_ioc_init_reply_endianness(MPIMsgIOCInitReply *reply)
>  {
> -    cpu_to_le32s(&reply->MsgContext);
> -    cpu_to_le16s(&reply->IOCStatus);
> -    cpu_to_le32s(&reply->IOCLogInfo);
> +    reply->MsgContext = cpu_to_le32(reply->MsgContext);
> +    reply->IOCStatus = cpu_to_le16(reply->IOCStatus);
> +    reply->IOCLogInfo = cpu_to_le32(reply->IOCLogInfo);
>  }
>  
>  void mptsas_fix_ioc_facts_endianness(MPIMsgIOCFacts *req)
>  {
> -    le32_to_cpus(&req->MsgContext);
> +    req->MsgContext = le32_to_cpu(req->MsgContext);
>  }
>  
>  void mptsas_fix_ioc_facts_reply_endianness(MPIMsgIOCFactsReply *reply)
>  {
> -    cpu_to_le16s(&reply->MsgVersion);
> -    cpu_to_le16s(&reply->HeaderVersion);
> -    cpu_to_le32s(&reply->MsgContext);
> -    cpu_to_le16s(&reply->IOCExceptions);
> -    cpu_to_le16s(&reply->IOCStatus);
> -    cpu_to_le32s(&reply->IOCLogInfo);
> -    cpu_to_le16s(&reply->ReplyQueueDepth);
> -    cpu_to_le16s(&reply->RequestFrameSize);
> -    cpu_to_le16s(&reply->ProductID);
> -    cpu_to_le32s(&reply->CurrentHostMfaHighAddr);
> -    cpu_to_le16s(&reply->GlobalCredits);
> -    cpu_to_le32s(&reply->CurrentSenseBufferHighAddr);
> -    cpu_to_le16s(&reply->CurReplyFrameSize);
> -    cpu_to_le32s(&reply->FWImageSize);
> -    cpu_to_le32s(&reply->IOCCapabilities);
> -    cpu_to_le16s(&reply->HighPriorityQueueDepth);
> +    reply->MsgVersion = cpu_to_le16(reply->MsgVersion);
> +    reply->HeaderVersion = cpu_to_le16(reply->HeaderVersion);
> +    reply->MsgContext = cpu_to_le32(reply->MsgContext);
> +    reply->IOCExceptions = cpu_to_le16(reply->IOCExceptions);
> +    reply->IOCStatus = cpu_to_le16(reply->IOCStatus);
> +    reply->IOCLogInfo = cpu_to_le32(reply->IOCLogInfo);
> +    reply->ReplyQueueDepth = cpu_to_le16(reply->ReplyQueueDepth);
> +    reply->RequestFrameSize = cpu_to_le16(reply->RequestFrameSize);
> +    reply->ProductID = cpu_to_le16(reply->ProductID);
> +    reply->CurrentHostMfaHighAddr = 
> cpu_to_le32(reply->CurrentHostMfaHighAddr);
> +    reply->GlobalCredits = cpu_to_le16(reply->GlobalCredits);
> +    reply->CurrentSenseBufferHighAddr =
> +        cpu_to_le32(reply->CurrentSenseBufferHighAddr);
> +    reply->CurReplyFrameSize = cpu_to_le16(reply->CurReplyFrameSize);
> +    reply->FWImageSize = cpu_to_le32(reply->FWImageSize);
> +    reply->IOCCapabilities = cpu_to_le32(reply->IOCCapabilities);
> +    reply->HighPriorityQueueDepth = 
> cpu_to_le16(reply->HighPriorityQueueDepth);
>      mptsas_fix_sgentry_endianness_reply(&reply->HostPageBufferSGE);
> -    cpu_to_le32s(&reply->ReplyFifoHostSignalingAddr);
> +    reply->ReplyFifoHostSignalingAddr =
> +        cpu_to_le32(reply->ReplyFifoHostSignalingAddr);
>  }
>  
>  void mptsas_fix_config_endianness(MPIMsgConfig *req)
>  {
> -    le16_to_cpus(&req->ExtPageLength);
> -    le32_to_cpus(&req->MsgContext);
> -    le32_to_cpus(&req->PageAddress);
> +    req->ExtPageLength = le16_to_cpu(req->ExtPageLength);
> +    req->MsgContext = le32_to_cpu(req->MsgContext);
> +    req->PageAddress = le32_to_cpu(req->PageAddress);
>      mptsas_fix_sgentry_endianness(&req->PageBufferSGE);
>  }
>  
>  void mptsas_fix_config_reply_endianness(MPIMsgConfigReply *reply)
>  {
> -    cpu_to_le16s(&reply->ExtPageLength);
> -    cpu_to_le32s(&reply->MsgContext);
> -    cpu_to_le16s(&reply->IOCStatus);
> -    cpu_to_le32s(&reply->IOCLogInfo);
> +    reply->ExtPageLength = cpu_to_le16(reply->ExtPageLength);
> +    reply->MsgContext = cpu_to_le32(reply->MsgContext);
> +    reply->IOCStatus = cpu_to_le16(reply->IOCStatus);
> +    reply->IOCLogInfo = cpu_to_le32(reply->IOCLogInfo);
>  }
>  
>  void mptsas_fix_port_facts_endianness(MPIMsgPortFacts *req)
>  {
> -    le32_to_cpus(&req->MsgContext);
> +    req->MsgContext = le32_to_cpu(req->MsgContext);
>  }
>  
>  void mptsas_fix_port_facts_reply_endianness(MPIMsgPortFactsReply *reply)
>  {
> -    cpu_to_le32s(&reply->MsgContext);
> -    cpu_to_le16s(&reply->IOCStatus);
> -    cpu_to_le32s(&reply->IOCLogInfo);
> -    cpu_to_le16s(&reply->MaxDevices);
> -    cpu_to_le16s(&reply->PortSCSIID);
> -    cpu_to_le16s(&reply->ProtocolFlags);
> -    cpu_to_le16s(&reply->MaxPostedCmdBuffers);
> -    cpu_to_le16s(&reply->MaxPersistentIDs);
> -    cpu_to_le16s(&reply->MaxLanBuckets);
> +    reply->MsgContext = cpu_to_le32(reply->MsgContext);
> +    reply->IOCStatus = cpu_to_le16(reply->IOCStatus);
> +    reply->IOCLogInfo = cpu_to_le32(reply->IOCLogInfo);
> +    reply->MaxDevices = cpu_to_le16(reply->MaxDevices);
> +    reply->PortSCSIID = cpu_to_le16(reply->PortSCSIID);
> +    reply->ProtocolFlags = cpu_to_le16(reply->ProtocolFlags);
> +    reply->MaxPostedCmdBuffers = cpu_to_le16(reply->MaxPostedCmdBuffers);
> +    reply->MaxPersistentIDs = cpu_to_le16(reply->MaxPersistentIDs);
> +    reply->MaxLanBuckets = cpu_to_le16(reply->MaxLanBuckets);
>  }
>  
>  void mptsas_fix_port_enable_endianness(MPIMsgPortEnable *req)
>  {
> -    le32_to_cpus(&req->MsgContext);
> +    req->MsgContext = le32_to_cpu(req->MsgContext);
>  }
>  
>  void mptsas_fix_port_enable_reply_endianness(MPIMsgPortEnableReply *reply)
>  {
> -    cpu_to_le32s(&reply->MsgContext);
> -    cpu_to_le16s(&reply->IOCStatus);
> -    cpu_to_le32s(&reply->IOCLogInfo);
> +    reply->MsgContext = cpu_to_le32(reply->MsgContext);
> +    reply->IOCStatus = cpu_to_le16(reply->IOCStatus);
> +    reply->IOCLogInfo = cpu_to_le32(reply->IOCLogInfo);
>  }
>  
>  void mptsas_fix_event_notification_endianness(MPIMsgEventNotify *req)
>  {
> -    le32_to_cpus(&req->MsgContext);
> +    req->MsgContext = le32_to_cpu(req->MsgContext);
>  }
>  
>  void mptsas_fix_event_notification_reply_endianness(MPIMsgEventNotifyReply 
> *reply)
> @@ -188,16 +191,16 @@ void 
> mptsas_fix_event_notification_reply_endianness(MPIMsgEventNotifyReply *repl
>      int length = reply->EventDataLength;
>      int i;
>  
> -    cpu_to_le16s(&reply->EventDataLength);
> -    cpu_to_le32s(&reply->MsgContext);
> -    cpu_to_le16s(&reply->IOCStatus);
> -    cpu_to_le32s(&reply->IOCLogInfo);
> -    cpu_to_le32s(&reply->Event);
> -    cpu_to_le32s(&reply->EventContext);
> +    reply->EventDataLength = cpu_to_le16(reply->EventDataLength);
> +    reply->MsgContext = cpu_to_le32(reply->MsgContext);
> +    reply->IOCStatus = cpu_to_le16(reply->IOCStatus);
> +    reply->IOCLogInfo = cpu_to_le32(reply->IOCLogInfo);
> +    reply->Event = cpu_to_le32(reply->Event);
> +    reply->EventContext = cpu_to_le32(reply->EventContext);
>  
>      /* Really depends on the event kind.  This will do for now.  */
>      for (i = 0; i < length; i++) {
> -        cpu_to_le32s(&reply->Data[i]);
> +        reply->Data[i] = cpu_to_le32(reply->Data[i]);
>      }
>  }
>  
>