Re: [PATCH 5/5] hw/scsi/spapr_vscsi: Do not mix SRP IU size with DMA buffer size

[Philippe Mathieu-Daudé]

On 3/5/20 1:43 AM, David Gibson wrote:
On Wed, Mar 04, 2020 at 04:33:11PM +0100, Philippe Mathieu-Daudé wrote:
The 'union srp_iu' is meant as a pointer to any SRP Information
Unit type, it is not related to the size of a VIO DMA buffer.
Use a plain buffer for the VIO DMA read/write calls.
We can remove the reserved buffer from the 'union srp_iu'.

This issue was noticed when replacing the zero-length arrays
from hw/scsi/srp.h with flexible array member,
'clang -fsanitize=undefined' reported:

   hw/scsi/spapr_vscsi.c:69:29: error: field 'iu' with variable sized type 
'union viosrp_iu' not at the end of a struct or class is a GNU extension 
[-Werror,-Wgnu-variable-sized-type-not-at-end]
        union viosrp_iu         iu;
                                ^

Signed-off-by: Philippe Mathieu-Daudé <phi...@redhat.com>
---
  hw/scsi/viosrp.h      |  2 +-
  hw/scsi/spapr_vscsi.c | 10 +++++-----
  2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/hw/scsi/viosrp.h b/hw/scsi/viosrp.h
index 25676c2383..aba3203028 100644
--- a/hw/scsi/viosrp.h
+++ b/hw/scsi/viosrp.h
@@ -49,8 +49,8 @@ union srp_iu {
      struct srp_tsk_mgmt tsk_mgmt;
      struct srp_cmd cmd;
      struct srp_rsp rsp;
-    uint8_t reserved[SRP_MAX_IU_LEN];
  };
+_Static_assert(sizeof(union srp_iu) <= SRP_MAX_IU_LEN, "srp_iu size 
incorrect");

Hrm.  Given that srp_iu will be a variably sized type, is this
assertion actually testing anything meaningful?

I wanted to assert that if another SRP IU is added, the DMA buffer will 
be big enough to hold it. I'll simply remove the check.

Otherwise, LGTM.

Thanks for reviewing the series,

Phil.


  
  enum viosrp_crq_formats {
      VIOSRP_SRP_FORMAT = 0x01,
diff --git a/hw/scsi/spapr_vscsi.c b/hw/scsi/spapr_vscsi.c
index f1a0bbdc31..f9be68e44e 100644
--- a/hw/scsi/spapr_vscsi.c
+++ b/hw/scsi/spapr_vscsi.c
@@ -66,7 +66,7 @@ typedef union vscsi_crq {
  
  typedef struct vscsi_req {
      vscsi_crq               crq;
-    union viosrp_iu         iu;
+    uint8_t                 viosrp_iu_buf[SRP_MAX_IU_LEN];
  
      /* SCSI request tracking */
      SCSIRequest             *sreq;
@@ -99,7 +99,7 @@ typedef struct {
  
  static union viosrp_iu *req_iu(vscsi_req *req)
  {
-    return (union viosrp_iu *)req->iu.srp.reserved;
+    return (union viosrp_iu *)req->viosrp_iu_buf;
  }
  
  
@@ -184,7 +184,7 @@ static int vscsi_send_iu(VSCSIState *s, vscsi_req *req,
  
      /* First copy the SRP */
      rc = spapr_vio_dma_write(&s->vdev, req->crq.s.IU_data_ptr,
-                             &req->iu, length);
+                             &req->viosrp_iu_buf, length);
      if (rc) {
          fprintf(stderr, "vscsi_send_iu: DMA write failure !\n");
      }
@@ -603,7 +603,7 @@ static const VMStateDescription vmstate_spapr_vscsi_req = {
      .minimum_version_id = 1,
      .fields = (VMStateField[]) {
          VMSTATE_BUFFER(crq.raw, vscsi_req),
-        VMSTATE_BUFFER(iu.srp.reserved, vscsi_req),
+        VMSTATE_BUFFER(viosrp_iu_buf, vscsi_req),
          VMSTATE_UINT32(qtag, vscsi_req),
          VMSTATE_BOOL(active, vscsi_req),
          VMSTATE_UINT32(data_len, vscsi_req),
@@ -1104,7 +1104,7 @@ static void vscsi_got_payload(VSCSIState *s, vscsi_crq 
*crq)
      }
  
      /* XXX Handle failure differently ? */
-    if (spapr_vio_dma_read(&s->vdev, crq->s.IU_data_ptr, &req->iu,
+    if (spapr_vio_dma_read(&s->vdev, crq->s.IU_data_ptr, &req->viosrp_iu_buf,
                             crq->s.IU_length)) {
          fprintf(stderr, "vscsi_got_payload: DMA read failure !\n");
          vscsi_put_req(req);