| Message ID | 20190329111104.17223-12-berrange@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | misc set of fixes for warnings under GCC 9 | expand |
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
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)); > } > >
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
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>
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?
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
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
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.
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 :)
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)); }
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(-)