qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [qemu-s390x] [PATCH 11/14] hw/vfio/ccw: avoid taking ad


From: Eric Farman
Subject: Re: [Qemu-devel] [qemu-s390x] [PATCH 11/14] hw/vfio/ccw: avoid taking address members in packed structs
Date: Fri, 29 Mar 2019 10:40:21 -0400
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.5.0



On 3/29/19 7:11 AM, Daniel P. Berrangé wrote:
The GCC 9 compiler complains about many places in s390 code
that take the address of members of the 'struct SCHIB' which
is marked packed:

hw/vfio/ccw.c: In function ‘vfio_ccw_io_notifier_handler’:
hw/vfio/ccw.c:133:15: warning: taking address of packed member of ‘struct 
SCHIB’ may result in an unaligned pointer value \
[-Waddress-of-packed-member]
   133 |     SCSW *s = &sch->curr_status.scsw;
       |               ^~~~~~~~~~~~~~~~~~~~~~
hw/vfio/ccw.c:134:15: warning: taking address of packed member of ‘struct 
SCHIB’ may result in an unaligned pointer value \
[-Waddress-of-packed-member]
   134 |     PMCW *p = &sch->curr_status.pmcw;
       |               ^~~~~~~~~~~~~~~~~~~~~~

...snip many more...

Almost all of these are just done for convenience to avoid
typing out long variable/field names when referencing struct
members. We can get most of this convenience by taking the
address of the 'struct SCHIB' instead, avoiding triggering
the compiler warnings.

In a couple of places we copy via a local variable which is
a technique already applied elsewhere in s390 code for this
problem.

Signed-off-by: Daniel P. Berrangé <address@hidden>
---
  hw/vfio/ccw.c | 42 ++++++++++++++++++++++--------------------
  1 file changed, 22 insertions(+), 20 deletions(-)

diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
index 9246729a75..c44d13cc50 100644
--- a/hw/vfio/ccw.c
+++ b/hw/vfio/ccw.c
@@ -130,8 +130,8 @@ static void vfio_ccw_io_notifier_handler(void *opaque)
      S390CCWDevice *cdev = S390_CCW_DEVICE(vcdev);
      CcwDevice *ccw_dev = CCW_DEVICE(cdev);
      SubchDev *sch = ccw_dev->sch;
-    SCSW *s = &sch->curr_status.scsw;
-    PMCW *p = &sch->curr_status.pmcw;
+    SCHIB *schib = &sch->curr_status;
+    SCSW s;
      IRB irb;
      int size;
@@ -145,33 +145,33 @@ static void vfio_ccw_io_notifier_handler(void *opaque)
          switch (errno) {
          case ENODEV:
              /* Generate a deferred cc 3 condition. */
-            s->flags |= SCSW_FLAGS_MASK_CC;
-            s->ctrl &= ~SCSW_CTRL_MASK_STCTL;
-            s->ctrl |= (SCSW_STCTL_ALERT | SCSW_STCTL_STATUS_PEND);
+            schib->scsw.flags |= SCSW_FLAGS_MASK_CC;
+            schib->scsw.ctrl &= ~SCSW_CTRL_MASK_STCTL;
+            schib->scsw.ctrl |= (SCSW_STCTL_ALERT | SCSW_STCTL_STATUS_PEND);
              goto read_err;
          case EFAULT:
              /* Memory problem, generate channel data check. */
-            s->ctrl &= ~SCSW_ACTL_START_PEND;
-            s->cstat = SCSW_CSTAT_DATA_CHECK;
-            s->ctrl &= ~SCSW_CTRL_MASK_STCTL;
-            s->ctrl |= SCSW_STCTL_PRIMARY | SCSW_STCTL_SECONDARY |
+            schib->scsw.ctrl &= ~SCSW_ACTL_START_PEND;
+            schib->scsw.cstat = SCSW_CSTAT_DATA_CHECK;
+            schib->scsw.ctrl &= ~SCSW_CTRL_MASK_STCTL;
+            schib->scsw.ctrl |= SCSW_STCTL_PRIMARY | SCSW_STCTL_SECONDARY |
                         SCSW_STCTL_ALERT | SCSW_STCTL_STATUS_PEND;
              goto read_err;
          default:
              /* Error, generate channel program check. */
-            s->ctrl &= ~SCSW_ACTL_START_PEND;
-            s->cstat = SCSW_CSTAT_PROG_CHECK;
-            s->ctrl &= ~SCSW_CTRL_MASK_STCTL;
-            s->ctrl |= SCSW_STCTL_PRIMARY | SCSW_STCTL_SECONDARY |
+            schib->scsw.ctrl &= ~SCSW_ACTL_START_PEND;
+            schib->scsw.cstat = SCSW_CSTAT_PROG_CHECK;
+            schib->scsw.ctrl &= ~SCSW_CTRL_MASK_STCTL;
+            schib->scsw.ctrl |= SCSW_STCTL_PRIMARY | SCSW_STCTL_SECONDARY |
                         SCSW_STCTL_ALERT | SCSW_STCTL_STATUS_PEND;
              goto read_err;
          }
      } else if (size != vcdev->io_region_size) {
          /* Information transfer error, generate channel-control check. */
-        s->ctrl &= ~SCSW_ACTL_START_PEND;
-        s->cstat = SCSW_CSTAT_CHN_CTRL_CHK;
-        s->ctrl &= ~SCSW_CTRL_MASK_STCTL;
-        s->ctrl |= SCSW_STCTL_PRIMARY | SCSW_STCTL_SECONDARY |
+        schib->scsw.ctrl &= ~SCSW_ACTL_START_PEND;
+        schib->scsw.cstat = SCSW_CSTAT_CHN_CTRL_CHK;
+        schib->scsw.ctrl &= ~SCSW_CTRL_MASK_STCTL;
+        schib->scsw.ctrl |= SCSW_STCTL_PRIMARY | SCSW_STCTL_SECONDARY |
                     SCSW_STCTL_ALERT | SCSW_STCTL_STATUS_PEND;
          goto read_err;
      }
@@ -179,11 +179,13 @@ static void vfio_ccw_io_notifier_handler(void *opaque)
      memcpy(&irb, region->irb_area, sizeof(IRB));
/* Update control block via irb. */
-    copy_scsw_to_guest(s, &irb.scsw);
+    s = schib->scsw;
+    copy_scsw_to_guest(&s, &irb.scsw);
+    schib->scsw = s;

If the backup/restore of the schib->scsw is part of the GCC9 check, then okay, but it seems unnecessary. Either way,

Reviewed-by: Eric Farman <address@hidden>

/* If a uint check is pending, copy sense data. */
-    if ((s->dstat & SCSW_DSTAT_UNIT_CHECK) &&
-        (p->chars & PMCW_CHARS_MASK_CSENSE)) {
+    if ((schib->scsw.dstat & SCSW_DSTAT_UNIT_CHECK) &&
+        (schib->pmcw.chars & PMCW_CHARS_MASK_CSENSE)) {
          memcpy(sch->sense_data, irb.ecw, sizeof(irb.ecw));
      }




reply via email to

[Prev in Thread] Current Thread [Next in Thread]