Re: [Qemu-devel] [qemu-s390x] [PATCH 12/14] hw/s390/css: avoid taking address members in packed structs

Re: [Qemu-devel] [qemu-s390x] [PATCH 12/14] hw/s390/css: avoid taking address members in packed structs

[Halil Pasic]

On Fri, 29 Mar 2019 11:11:02 +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/s390x/css.c: In function ‘sch_handle_clear_func’:
> hw/s390x/css.c:698:15: warning: taking address of packed member of ‘struct SCHIB’ may result in an unaligned pointer val\
> ue [-Waddress-of-packed-member]
>   698 |     PMCW *p = &sch->curr_status.pmcw;
>       |               ^~~~~~~~~~~~~~~~~~~~~~
> hw/s390x/css.c:699:15: warning: taking address of packed member of ‘struct SCHIB’ may result in an unaligned pointer val\
> ue [-Waddress-of-packed-member]
>   699 |     SCSW *s = &sch->curr_status.scsw;
>       |               ^~~~~~~~~~~~~~~~~~~~~~
> 
> ...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>

Thanks!

Re: [Qemu-devel] [PATCH 13/14] hw/s390x/ipl: avoid taking address of fields in packed struct

[Farhan Ali]

On 03/29/2019 07:11 AM, Daniel P. Berrangé wrote:
> Compiling with GCC 9 complains
> 
> hw/s390x/ipl.c: In function ‘s390_ipl_set_boot_menu’:
> hw/s390x/ipl.c:256:25: warning: taking address of packed member of ‘struct QemuIplParameters’ may result in an unaligned pointer value [-Waddress-of-packed-member]
>    256 |     uint32_t *timeout = &ipl->qipl.boot_menu_timeout;
>        |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> This local variable is only present to save a little bit of
> typing when setting the field later. Get rid of this to avoid
> the warning about unaligned accesses.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   hw/s390x/ipl.c | 12 +++++-------
>   1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
> index 896888bf8f..51b272e190 100644
> --- a/hw/s390x/ipl.c
> +++ b/hw/s390x/ipl.c
> @@ -252,8 +252,6 @@ static void s390_ipl_set_boot_menu(S390IPLState *ipl)
>   {
>       QemuOptsList *plist = qemu_find_opts("boot-opts");
>       QemuOpts *opts = QTAILQ_FIRST(&plist->head);
> -    uint8_t *flags = &ipl->qipl.qipl_flags;
> -    uint32_t *timeout = &ipl->qipl.boot_menu_timeout;
>       const char *tmp;
>       unsigned long splash_time = 0;
>   
> @@ -269,7 +267,7 @@ static void s390_ipl_set_boot_menu(S390IPLState *ipl)
>       case S390_IPL_TYPE_CCW:
>           /* In the absence of -boot menu, use zipl parameters */
>           if (!qemu_opt_get(opts, "menu")) {
> -            *flags |= QIPL_FLAG_BM_OPTS_ZIPL;
> +            ipl->qipl.qipl_flags |= QIPL_FLAG_BM_OPTS_ZIPL;
>               return;
>           }
>           break;
> @@ -286,23 +284,23 @@ static void s390_ipl_set_boot_menu(S390IPLState *ipl)
>           return;
>       }
>   
> -    *flags |= QIPL_FLAG_BM_OPTS_CMD;
> +    ipl->qipl.qipl_flags |= QIPL_FLAG_BM_OPTS_CMD;
>   
>       tmp = qemu_opt_get(opts, "splash-time");
>   
>       if (tmp && qemu_strtoul(tmp, NULL, 10, &splash_time)) {
>           error_report("splash-time is invalid, forcing it to 0");
> -        *timeout = 0;
> +        ipl->qipl.boot_menu_timeout = 0;
>           return;
>       }
>   
>       if (splash_time > 0xffffffff) {
>           error_report("splash-time is too large, forcing it to max value");
> -        *timeout = 0xffffffff;
> +        ipl->qipl.boot_menu_timeout = 0xffffffff;
>           return;
>       }
>   
> -    *timeout = cpu_to_be32(splash_time);
> +    ipl->qipl.boot_menu_timeout = cpu_to_be32(splash_time);
>   }
>   
>   static CcwDevice *s390_get_ccw_device(DeviceState *dev_st)
> 

Reviewed-by: Farhan Ali <alifm@linux.ibm.com>

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

[Halil Pasic]

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 

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

[Farhan Ali]

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>

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

[Cornelia Huck]

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?

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

[Thomas Huth]

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

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

[Cornelia Huck]

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.

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

[Daniel P. Berrangé]

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
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

[Cornelia Huck]

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 :)

Re: [Qemu-devel] [PATCH 08/14] qxl: avoid unaligned pointer reads/writes

[Marc-André Lureau]

Hi

On Fri, Mar 29, 2019 at 12:20 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> The SPICE_RING_PROD_ITEM() macro is initializing a local
> 'uint64_t *' variable to point to the 'el' field inside
> the QXLReleaseRing struct. This uint64_t field is not
> guaranteed aligned as the struct is packed.
>
> Code should not take the address of fields within a
> packed struct. Changing the SPICE_RING_PROD_ITEM()
> macro to avoid taking the address of the field is
> impractical. It is clearer to just remove the macro
> and inline its functionality in the three call sites
> that need it.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

I didn't notice your patch, I sent a different one which didn't inline
as much code:

https://patchew.org/QEMU/20190408201203.28924-1-marcandre.lureau@redhat.com/

> ---
>  hw/display/qxl.c | 55 +++++++++++++++++++++---------------------------
>  1 file changed, 24 insertions(+), 31 deletions(-)
>
> diff --git a/hw/display/qxl.c b/hw/display/qxl.c
> index c8ce5781e0..5c38e6e906 100644
> --- a/hw/display/qxl.c
> +++ b/hw/display/qxl.c
> @@ -33,24 +33,6 @@
>
>  #include "qxl.h"
>
> -/*
> - * NOTE: SPICE_RING_PROD_ITEM accesses memory on the pci bar and as
> - * such can be changed by the guest, so to avoid a guest trigerrable
> - * abort we just qxl_set_guest_bug and set the return to NULL. Still
> - * it may happen as a result of emulator bug as well.
> - */
> -#undef SPICE_RING_PROD_ITEM
> -#define SPICE_RING_PROD_ITEM(qxl, r, ret) {                             \
> -        uint32_t prod = (r)->prod & SPICE_RING_INDEX_MASK(r);           \
> -        if (prod >= ARRAY_SIZE((r)->items)) {                           \
> -            qxl_set_guest_bug(qxl, "SPICE_RING_PROD_ITEM indices mismatch " \
> -                          "%u >= %zu", prod, ARRAY_SIZE((r)->items));   \
> -            ret = NULL;                                                 \
> -        } else {                                                        \
> -            ret = &(r)->items[prod].el;                                 \
> -        }                                                               \
> -    }
> -
>  #undef SPICE_RING_CONS_ITEM
>  #define SPICE_RING_CONS_ITEM(qxl, r, ret) {                             \
>          uint32_t cons = (r)->cons & SPICE_RING_INDEX_MASK(r);           \
> @@ -414,7 +396,8 @@ static void init_qxl_rom(PCIQXLDevice *d)
>  static void init_qxl_ram(PCIQXLDevice *d)
>  {
>      uint8_t *buf;
> -    uint64_t *item;
> +    uint32_t prod;
> +    QXLReleaseRing *ring;
>
>      buf = d->vga.vram_ptr;
>      d->ram = (QXLRam *)(buf + le32_to_cpu(d->shadow_rom.ram_header_offset));
> @@ -426,9 +409,12 @@ static void init_qxl_ram(PCIQXLDevice *d)
>      SPICE_RING_INIT(&d->ram->cmd_ring);
>      SPICE_RING_INIT(&d->ram->cursor_ring);
>      SPICE_RING_INIT(&d->ram->release_ring);
> -    SPICE_RING_PROD_ITEM(d, &d->ram->release_ring, item);
> -    assert(item);
> -    *item = 0;
> +
> +    ring = &d->ram->release_ring;
> +    prod = ring->prod & SPICE_RING_INDEX_MASK(ring);
> +    assert(prod < ARRAY_SIZE(ring->items));
> +    ring->items[prod].el = 0;
> +
>      qxl_ring_set_dirty(d);
>  }
>
> @@ -732,7 +718,7 @@ static int interface_req_cmd_notification(QXLInstance *sin)
>  static inline void qxl_push_free_res(PCIQXLDevice *d, int flush)
>  {
>      QXLReleaseRing *ring = &d->ram->release_ring;
> -    uint64_t *item;
> +    uint32_t prod;
>      int notify;
>
>  #define QXL_FREE_BUNCH_SIZE 32
> @@ -759,11 +745,15 @@ static inline void qxl_push_free_res(PCIQXLDevice *d, int flush)
>      if (notify) {
>          qxl_send_events(d, QXL_INTERRUPT_DISPLAY);
>      }
> -    SPICE_RING_PROD_ITEM(d, ring, item);
> -    if (!item) {
> +
> +    ring = &d->ram->release_ring;
> +    prod = ring->prod & SPICE_RING_INDEX_MASK(ring);
> +    if (prod >= ARRAY_SIZE(ring->items)) {
> +        qxl_set_guest_bug(d, "SPICE_RING_PROD_ITEM indices mismatch "
> +                          "%u >= %zu", prod, ARRAY_SIZE(ring->items));
>          return;
>      }
> -    *item = 0;
> +    ring->items[prod].el = 0;
>      d->num_free_res = 0;
>      d->last_release = NULL;
>      qxl_ring_set_dirty(d);
> @@ -775,7 +765,8 @@ static void interface_release_resource(QXLInstance *sin,
>  {
>      PCIQXLDevice *qxl = container_of(sin, PCIQXLDevice, ssd.qxl);
>      QXLReleaseRing *ring;
> -    uint64_t *item, id;
> +    uint32_t prod;
> +    uint64_t id;
>
>      if (ext.group_id == MEMSLOT_GROUP_HOST) {
>          /* host group -> vga mode update request */
> @@ -792,16 +783,18 @@ static void interface_release_resource(QXLInstance *sin,
>       * pci bar 0, $command.release_info
>       */
>      ring = &qxl->ram->release_ring;
> -    SPICE_RING_PROD_ITEM(qxl, ring, item);
> -    if (!item) {
> +    prod = ring->prod & SPICE_RING_INDEX_MASK(ring);
> +    if (prod >= ARRAY_SIZE(ring->items)) {
> +        qxl_set_guest_bug(qxl, "SPICE_RING_PROD_ITEM indices mismatch "
> +                          "%u >= %zu", prod, ARRAY_SIZE(ring->items));
>          return;
>      }
> -    if (*item == 0) {
> +    if (ring->items[prod].el == 0) {
>          /* stick head into the ring */
>          id = ext.info->id;
>          ext.info->next = 0;
>          qxl_ram_set_dirty(qxl, &ext.info->next);
> -        *item = id;
> +        ring->items[prod].el = id;
>          qxl_ring_set_dirty(qxl);
>      } else {
>          /* append item to the list */
> --
> 2.20.1
>
>


-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH 08/14] qxl: avoid unaligned pointer reads/writes

[Marc-André Lureau]

Hi

On Fri, Mar 29, 2019 at 12:20 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> The SPICE_RING_PROD_ITEM() macro is initializing a local
> 'uint64_t *' variable to point to the 'el' field inside
> the QXLReleaseRing struct. This uint64_t field is not
> guaranteed aligned as the struct is packed.
>
> Code should not take the address of fields within a
> packed struct. Changing the SPICE_RING_PROD_ITEM()
> macro to avoid taking the address of the field is
> impractical. It is clearer to just remove the macro
> and inline its functionality in the three call sites
> that need it.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

I didn't notice your patch, I sent a different one which didn't inline
as much code:

https://patchew.org/QEMU/20190408201203.28924-1-marcandre.lureau@redhat.com/

> ---
>  hw/display/qxl.c | 55 +++++++++++++++++++++---------------------------
>  1 file changed, 24 insertions(+), 31 deletions(-)
>
> diff --git a/hw/display/qxl.c b/hw/display/qxl.c
> index c8ce5781e0..5c38e6e906 100644
> --- a/hw/display/qxl.c
> +++ b/hw/display/qxl.c
> @@ -33,24 +33,6 @@
>
>  #include "qxl.h"
>
> -/*
> - * NOTE: SPICE_RING_PROD_ITEM accesses memory on the pci bar and as
> - * such can be changed by the guest, so to avoid a guest trigerrable
> - * abort we just qxl_set_guest_bug and set the return to NULL. Still
> - * it may happen as a result of emulator bug as well.
> - */
> -#undef SPICE_RING_PROD_ITEM
> -#define SPICE_RING_PROD_ITEM(qxl, r, ret) {                             \
> -        uint32_t prod = (r)->prod & SPICE_RING_INDEX_MASK(r);           \
> -        if (prod >= ARRAY_SIZE((r)->items)) {                           \
> -            qxl_set_guest_bug(qxl, "SPICE_RING_PROD_ITEM indices mismatch " \
> -                          "%u >= %zu", prod, ARRAY_SIZE((r)->items));   \
> -            ret = NULL;                                                 \
> -        } else {                                                        \
> -            ret = &(r)->items[prod].el;                                 \
> -        }                                                               \
> -    }
> -
>  #undef SPICE_RING_CONS_ITEM
>  #define SPICE_RING_CONS_ITEM(qxl, r, ret) {                             \
>          uint32_t cons = (r)->cons & SPICE_RING_INDEX_MASK(r);           \
> @@ -414,7 +396,8 @@ static void init_qxl_rom(PCIQXLDevice *d)
>  static void init_qxl_ram(PCIQXLDevice *d)
>  {
>      uint8_t *buf;
> -    uint64_t *item;
> +    uint32_t prod;
> +    QXLReleaseRing *ring;
>
>      buf = d->vga.vram_ptr;
>      d->ram = (QXLRam *)(buf + le32_to_cpu(d->shadow_rom.ram_header_offset));
> @@ -426,9 +409,12 @@ static void init_qxl_ram(PCIQXLDevice *d)
>      SPICE_RING_INIT(&d->ram->cmd_ring);
>      SPICE_RING_INIT(&d->ram->cursor_ring);
>      SPICE_RING_INIT(&d->ram->release_ring);
> -    SPICE_RING_PROD_ITEM(d, &d->ram->release_ring, item);
> -    assert(item);
> -    *item = 0;
> +
> +    ring = &d->ram->release_ring;
> +    prod = ring->prod & SPICE_RING_INDEX_MASK(ring);
> +    assert(prod < ARRAY_SIZE(ring->items));
> +    ring->items[prod].el = 0;
> +
>      qxl_ring_set_dirty(d);
>  }
>
> @@ -732,7 +718,7 @@ static int interface_req_cmd_notification(QXLInstance *sin)
>  static inline void qxl_push_free_res(PCIQXLDevice *d, int flush)
>  {
>      QXLReleaseRing *ring = &d->ram->release_ring;
> -    uint64_t *item;
> +    uint32_t prod;
>      int notify;
>
>  #define QXL_FREE_BUNCH_SIZE 32
> @@ -759,11 +745,15 @@ static inline void qxl_push_free_res(PCIQXLDevice *d, int flush)
>      if (notify) {
>          qxl_send_events(d, QXL_INTERRUPT_DISPLAY);
>      }
> -    SPICE_RING_PROD_ITEM(d, ring, item);
> -    if (!item) {
> +
> +    ring = &d->ram->release_ring;
> +    prod = ring->prod & SPICE_RING_INDEX_MASK(ring);
> +    if (prod >= ARRAY_SIZE(ring->items)) {
> +        qxl_set_guest_bug(d, "SPICE_RING_PROD_ITEM indices mismatch "
> +                          "%u >= %zu", prod, ARRAY_SIZE(ring->items));
>          return;
>      }
> -    *item = 0;
> +    ring->items[prod].el = 0;
>      d->num_free_res = 0;
>      d->last_release = NULL;
>      qxl_ring_set_dirty(d);
> @@ -775,7 +765,8 @@ static void interface_release_resource(QXLInstance *sin,
>  {
>      PCIQXLDevice *qxl = container_of(sin, PCIQXLDevice, ssd.qxl);
>      QXLReleaseRing *ring;
> -    uint64_t *item, id;
> +    uint32_t prod;
> +    uint64_t id;
>
>      if (ext.group_id == MEMSLOT_GROUP_HOST) {
>          /* host group -> vga mode update request */
> @@ -792,16 +783,18 @@ static void interface_release_resource(QXLInstance *sin,
>       * pci bar 0, $command.release_info
>       */
>      ring = &qxl->ram->release_ring;
> -    SPICE_RING_PROD_ITEM(qxl, ring, item);
> -    if (!item) {
> +    prod = ring->prod & SPICE_RING_INDEX_MASK(ring);
> +    if (prod >= ARRAY_SIZE(ring->items)) {
> +        qxl_set_guest_bug(qxl, "SPICE_RING_PROD_ITEM indices mismatch "
> +                          "%u >= %zu", prod, ARRAY_SIZE(ring->items));
>          return;
>      }
> -    if (*item == 0) {
> +    if (ring->items[prod].el == 0) {
>          /* stick head into the ring */
>          id = ext.info->id;
>          ext.info->next = 0;
>          qxl_ram_set_dirty(qxl, &ext.info->next);
> -        *item = id;
> +        ring->items[prod].el = id;
>          qxl_ring_set_dirty(qxl);
>      } else {
>          /* append item to the list */
> --
> 2.20.1
>
>


-- 
Marc-André Lureau