[11/14] hw/vfio/ccw: avoid taking address members in packed structs
diff mbox series

Message ID 20190329111104.17223-12-berrange@redhat.com
State New
Headers show
Series
  • misc set of fixes for warnings under GCC 9
Related show

Commit Message

Daniel P. Berrangé March 29, 2019, 11:11 a.m. UTC
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é <berrange@redhat.com>
---
 hw/vfio/ccw.c | 42 ++++++++++++++++++++++--------------------
 1 file changed, 22 insertions(+), 20 deletions(-)

Comments

Thomas Huth March 29, 2019, 1:53 p.m. UTC | #1
On 29/03/2019 12.11, 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é <berrange@redhat.com>
> ---
>  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;

You could adjust the alignment of the last line here.

>              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

dito

>              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;

dito

>          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;

I think this "s = schib->scsw" is not needed here - s is completely
populated by the copy_scsw_to_guest function.

> +    copy_scsw_to_guest(&s, &irb.scsw);
> +    schib->scsw = s;
>  
>      /* 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));
>      }
>  
> 

 Thomas
Eric Farman March 29, 2019, 2:40 p.m. UTC | #2
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é <berrange@redhat.com>
> ---
>   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 <farman@linux.ibm.com>

>   
>       /* 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));
>       }
>   
>
Halil Pasic April 1, 2019, 2:07 p.m. UTC | #3
On Fri, 29 Mar 2019 11:11:01 +0000
Daniel P. Berrangé <berrange@redhat.com> 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é <berrange@redhat.com>

Reviewed-by: Halil Pasic <pasic@linux.ibm.com>

BTW the reason for SCHIB being packed is the padding that
would emerge at the end of the struct (sizeof(SCHIB) == 52
and non-packed sizeof(SCHIB) == 54). So getting rid of packed
seems to be viable as long as we write guest memory
with the correct size (52 bytes).

Regards,
Halil
Farhan Ali April 2, 2019, 2:16 p.m. UTC | #4
On 03/29/2019 07: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é <berrange@redhat.com>
> ---
>   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 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));
>       }
>   
> 

Reviewed-by: Farhan Ali <alifm@linux.ibm.com>
Cornelia Huck April 2, 2019, 4 p.m. UTC | #5
On Fri, 29 Mar 2019 11:11:01 +0000
Daniel P. Berrangé <berrange@redhat.com> 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é <berrange@redhat.com>
> ---
>  hw/vfio/ccw.c | 42 ++++++++++++++++++++++--------------------
>  1 file changed, 22 insertions(+), 20 deletions(-)

I'm currently in the process of queuing this and the other three s390x
fixes, but I'm inclined to do so for 4.1 (it feels a bit late in the
cycle for 4.0.)

Other opinions?
Thomas Huth April 2, 2019, 4:05 p.m. UTC | #6
On 02/04/2019 18.00, Cornelia Huck wrote:
> On Fri, 29 Mar 2019 11:11:01 +0000
> Daniel P. Berrangé <berrange@redhat.com> 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é <berrange@redhat.com>
>> ---
>>  hw/vfio/ccw.c | 42 ++++++++++++++++++++++--------------------
>>  1 file changed, 22 insertions(+), 20 deletions(-)
> 
> I'm currently in the process of queuing this and the other three s390x
> fixes, but I'm inclined to do so for 4.1 (it feels a bit late in the
> cycle for 4.0.)
> 
> Other opinions?

IMHO it would be nice to get rid of the compiler warnings for the
release. Multiple people reviewed the patches, so I think it should
still be fine to include them.

 Thomas
Daniel P. Berrangé April 2, 2019, 4:11 p.m. UTC | #7
On Tue, Apr 02, 2019 at 06:00:33PM +0200, Cornelia Huck wrote:
> On Fri, 29 Mar 2019 11:11:01 +0000
> Daniel P. Berrangé <berrange@redhat.com> 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é <berrange@redhat.com>
> > ---
> >  hw/vfio/ccw.c | 42 ++++++++++++++++++++++--------------------
> >  1 file changed, 22 insertions(+), 20 deletions(-)
> 
> I'm currently in the process of queuing this and the other three s390x
> fixes, but I'm inclined to do so for 4.1 (it feels a bit late in the
> cycle for 4.0.)
> 
> Other opinions?

It would be nice to be warning free for 4.0, but I agree that it feels
kind of late to be making these changes. They're not fixing real world
bugs, and even if you queue the s390 bits we're unlikely to get all the
others merged, especially the usb-mtp one is a nasty mess. So we'll
not be 100% warning free.

Regards,
Daniel
Cornelia Huck April 2, 2019, 4:12 p.m. UTC | #8
On Tue, 2 Apr 2019 18:05:15 +0200
Thomas Huth <thuth@redhat.com> wrote:

> On 02/04/2019 18.00, Cornelia Huck wrote:
> > On Fri, 29 Mar 2019 11:11:01 +0000
> > Daniel P. Berrangé <berrange@redhat.com> 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é <berrange@redhat.com>
> >> ---
> >>  hw/vfio/ccw.c | 42 ++++++++++++++++++++++--------------------
> >>  1 file changed, 22 insertions(+), 20 deletions(-)  
> > 
> > I'm currently in the process of queuing this and the other three s390x
> > fixes, but I'm inclined to do so for 4.1 (it feels a bit late in the
> > cycle for 4.0.)
> > 
> > Other opinions?  
> 
> IMHO it would be nice to get rid of the compiler warnings for the
> release. Multiple people reviewed the patches, so I think it should
> still be fine to include them.

Still need to run my smoke tests, but I can send a pull req tomorrow
for -rc3.
Cornelia Huck April 3, 2019, 9:33 a.m. UTC | #9
On Tue, 2 Apr 2019 17:11:29 +0100
Daniel P. Berrangé <berrange@redhat.com> wrote:

> On Tue, Apr 02, 2019 at 06:00:33PM +0200, Cornelia Huck wrote:
> > On Fri, 29 Mar 2019 11:11:01 +0000
> > Daniel P. Berrangé <berrange@redhat.com> 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é <berrange@redhat.com>
> > > ---
> > >  hw/vfio/ccw.c | 42 ++++++++++++++++++++++--------------------
> > >  1 file changed, 22 insertions(+), 20 deletions(-)  
> > 
> > I'm currently in the process of queuing this and the other three s390x
> > fixes, but I'm inclined to do so for 4.1 (it feels a bit late in the
> > cycle for 4.0.)
> > 
> > Other opinions?  
> 
> It would be nice to be warning free for 4.0, but I agree that it feels
> kind of late to be making these changes. They're not fixing real world
> bugs, and even if you queue the s390 bits we're unlikely to get all the
> others merged, especially the usb-mtp one is a nasty mess. So we'll
> not be 100% warning free.

Yeah, but OTOH, the s390x changes are straightforward and have been
reviewed by several people.

So I changed my mind and queued them to s390-fixes :)

Patch
diff mbox series

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 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));
     }