Re: [Qemu-devel] [qemu-s390x] [PATCH 11/14] hw/vfio/ccw: avoid taking address members in packed structs

[Eric Farman]

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