| Message ID | 20181210135803.20208-1-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | hw/s390/ccw.c: Don't take address of packed members | expand |
On Mon, 10 Dec 2018 13:58:03 +0000 Peter Maydell <peter.maydell@linaro.org> wrote: > Taking the address of a field in a packed struct is a bad idea, because > it might not be actually aligned enough for that pointer type (and > thus cause a crash on dereference on some host architectures). Newer > versions of clang warn about this. > > Avoid the problem by using local copies of the PMCW and SCSW > struct fields in copy_schib_from_guest() and copy_schib_to_guest(). > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > This seemed like a not totally ugly and reasonably localised fix > that satisfies clang. Oddly, this makes the generated object file > 15K smaller (421K vs 406K), so it might even be better code... Nice :) > > hw/s390x/css.c | 20 ++++++++++++++++---- > 1 file changed, 16 insertions(+), 4 deletions(-) > > diff --git a/hw/s390x/css.c b/hw/s390x/css.c > index 04ec5cc9705..ef07691e36b 100644 > --- a/hw/s390x/css.c > +++ b/hw/s390x/css.c > @@ -1290,9 +1290,15 @@ void copy_scsw_to_guest(SCSW *dest, const SCSW *src) > static void copy_schib_to_guest(SCHIB *dest, const SCHIB *src) > { > int i; > + PMCW srcpmcw, destpmcw; > + SCSW srcscsw, destscsw; <bikeshed> I would find src_pmcw etc. easier to read. Other opinions? </bikeshed> > > - copy_pmcw_to_guest(&dest->pmcw, &src->pmcw); > - copy_scsw_to_guest(&dest->scsw, &src->scsw); > + srcpmcw = src->pmcw; > + copy_pmcw_to_guest(&destpmcw, &srcpmcw); > + dest->pmcw = destpmcw; > + srcscsw = src->scsw; > + copy_scsw_to_guest(&destscsw, &srcscsw); > + dest->scsw = destscsw; > dest->mba = cpu_to_be64(src->mba); > for (i = 0; i < ARRAY_SIZE(dest->mda); i++) { > dest->mda[i] = src->mda[i]; > @@ -1339,9 +1345,15 @@ static void copy_scsw_from_guest(SCSW *dest, const SCSW *src) > static void copy_schib_from_guest(SCHIB *dest, const SCHIB *src) > { > int i; > + PMCW srcpmcw, destpmcw; > + SCSW srcscsw, destscsw; > > - copy_pmcw_from_guest(&dest->pmcw, &src->pmcw); > - copy_scsw_from_guest(&dest->scsw, &src->scsw); > + srcpmcw = src->pmcw; > + copy_pmcw_from_guest(&destpmcw, &srcpmcw); > + dest->pmcw = destpmcw; > + srcscsw = src->scsw; > + copy_scsw_from_guest(&destscsw, &srcscsw); > + dest->scsw = destscsw; > dest->mba = be64_to_cpu(src->mba); > for (i = 0; i < ARRAY_SIZE(dest->mda); i++) { > dest->mda[i] = src->mda[i];
On Mon, 10 Dec 2018 at 14:13, Cornelia Huck <cohuck@redhat.com> wrote: > > On Mon, 10 Dec 2018 13:58:03 +0000 > Peter Maydell <peter.maydell@linaro.org> wrote: > > > Taking the address of a field in a packed struct is a bad idea, because > > it might not be actually aligned enough for that pointer type (and > > thus cause a crash on dereference on some host architectures). Newer > > versions of clang warn about this. > > > > Avoid the problem by using local copies of the PMCW and SCSW > > struct fields in copy_schib_from_guest() and copy_schib_to_guest(). > > > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > > --- > > This seemed like a not totally ugly and reasonably localised fix > > that satisfies clang. Oddly, this makes the generated object file > > 15K smaller (421K vs 406K), so it might even be better code... > > Nice :) > > > > > hw/s390x/css.c | 20 ++++++++++++++++---- > > 1 file changed, 16 insertions(+), 4 deletions(-) > > > > diff --git a/hw/s390x/css.c b/hw/s390x/css.c > > index 04ec5cc9705..ef07691e36b 100644 > > --- a/hw/s390x/css.c > > +++ b/hw/s390x/css.c > > @@ -1290,9 +1290,15 @@ void copy_scsw_to_guest(SCSW *dest, const SCSW *src) > > static void copy_schib_to_guest(SCHIB *dest, const SCHIB *src) > > { > > int i; > > + PMCW srcpmcw, destpmcw; > > + SCSW srcscsw, destscsw; > > <bikeshed> > I would find src_pmcw etc. easier to read. Other opinions? > </bikeshed> CODING_STYLE's "Naming" section agrees with you... thanks -- PMM
On 12/10/2018 08:58 AM, Peter Maydell wrote: > Taking the address of a field in a packed struct is a bad idea, because > it might not be actually aligned enough for that pointer type (and > thus cause a crash on dereference on some host architectures). Newer > versions of clang warn about this. > > Avoid the problem by using local copies of the PMCW and SCSW > struct fields in copy_schib_from_guest() and copy_schib_to_guest(). > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > This seemed like a not totally ugly and reasonably localised fix > that satisfies clang. Oddly, this makes the generated object file > 15K smaller (421K vs 406K), so it might even be better code... > > hw/s390x/css.c | 20 ++++++++++++++++---- > 1 file changed, 16 insertions(+), 4 deletions(-) > > diff --git a/hw/s390x/css.c b/hw/s390x/css.c > index 04ec5cc9705..ef07691e36b 100644 > --- a/hw/s390x/css.c > +++ b/hw/s390x/css.c > @@ -1290,9 +1290,15 @@ void copy_scsw_to_guest(SCSW *dest, const SCSW *src) > static void copy_schib_to_guest(SCHIB *dest, const SCHIB *src) > { > int i; > + PMCW srcpmcw, destpmcw; > + SCSW srcscsw, destscsw; > > - copy_pmcw_to_guest(&dest->pmcw, &src->pmcw); > - copy_scsw_to_guest(&dest->scsw, &src->scsw); > + srcpmcw = src->pmcw; > + copy_pmcw_to_guest(&destpmcw, &srcpmcw); > + dest->pmcw = destpmcw; > + srcscsw = src->scsw; > + copy_scsw_to_guest(&destscsw, &srcscsw); > + dest->scsw = destscsw; > dest->mba = cpu_to_be64(src->mba); > for (i = 0; i < ARRAY_SIZE(dest->mda); i++) { > dest->mda[i] = src->mda[i]; > @@ -1339,9 +1345,15 @@ static void copy_scsw_from_guest(SCSW *dest, const SCSW *src) > static void copy_schib_from_guest(SCHIB *dest, const SCHIB *src) > { > int i; > + PMCW srcpmcw, destpmcw; > + SCSW srcscsw, destscsw; > > - copy_pmcw_from_guest(&dest->pmcw, &src->pmcw); > - copy_scsw_from_guest(&dest->scsw, &src->scsw); > + srcpmcw = src->pmcw; > + copy_pmcw_from_guest(&destpmcw, &srcpmcw); > + dest->pmcw = destpmcw; > + srcscsw = src->scsw; > + copy_scsw_from_guest(&destscsw, &srcscsw); > + dest->scsw = destscsw; > dest->mba = be64_to_cpu(src->mba); > for (i = 0; i < ARRAY_SIZE(dest->mda); i++) { > dest->mda[i] = src->mda[i]; > Reviewed-by: Farhan Ali <alifm@linux.ibm.com>
On Mon, 10 Dec 2018 14:23:15 +0000 Peter Maydell <peter.maydell@linaro.org> wrote: > On Mon, 10 Dec 2018 at 14:13, Cornelia Huck <cohuck@redhat.com> wrote: > > > > On Mon, 10 Dec 2018 13:58:03 +0000 > > Peter Maydell <peter.maydell@linaro.org> wrote: > > > > > Taking the address of a field in a packed struct is a bad idea, because > > > it might not be actually aligned enough for that pointer type (and > > > thus cause a crash on dereference on some host architectures). Newer > > > versions of clang warn about this. > > > > > > Avoid the problem by using local copies of the PMCW and SCSW > > > struct fields in copy_schib_from_guest() and copy_schib_to_guest(). > > > > > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > > > --- > > > This seemed like a not totally ugly and reasonably localised fix > > > that satisfies clang. Oddly, this makes the generated object file > > > 15K smaller (421K vs 406K), so it might even be better code... > > > > Nice :) > > > > > > > > hw/s390x/css.c | 20 ++++++++++++++++---- > > > 1 file changed, 16 insertions(+), 4 deletions(-) > > > > > > diff --git a/hw/s390x/css.c b/hw/s390x/css.c > > > index 04ec5cc9705..ef07691e36b 100644 > > > --- a/hw/s390x/css.c > > > +++ b/hw/s390x/css.c > > > @@ -1290,9 +1290,15 @@ void copy_scsw_to_guest(SCSW *dest, const SCSW *src) > > > static void copy_schib_to_guest(SCHIB *dest, const SCHIB *src) > > > { > > > int i; > > > + PMCW srcpmcw, destpmcw; > > > + SCSW srcscsw, destscsw; > > > > <bikeshed> > > I would find src_pmcw etc. easier to read. Other opinions? > > </bikeshed> > > CODING_STYLE's "Naming" section agrees with you... Do you plan to send a v2, or should I just rename and apply?
On Wed, 12 Dec 2018 at 17:34, Cornelia Huck <cohuck@redhat.com> wrote: > > On Mon, 10 Dec 2018 14:23:15 +0000 > Peter Maydell <peter.maydell@linaro.org> wrote: > > > On Mon, 10 Dec 2018 at 14:13, Cornelia Huck <cohuck@redhat.com> wrote: > > > > > > On Mon, 10 Dec 2018 13:58:03 +0000 > > > Peter Maydell <peter.maydell@linaro.org> wrote: > > > > > > > Taking the address of a field in a packed struct is a bad idea, because > > > > it might not be actually aligned enough for that pointer type (and > > > > thus cause a crash on dereference on some host architectures). Newer > > > > versions of clang warn about this. > > > > > > > > Avoid the problem by using local copies of the PMCW and SCSW > > > > struct fields in copy_schib_from_guest() and copy_schib_to_guest(). > > > > > > > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > > > > --- > > > > This seemed like a not totally ugly and reasonably localised fix > > > > that satisfies clang. Oddly, this makes the generated object file > > > > 15K smaller (421K vs 406K), so it might even be better code... > > > > > > Nice :) > > > > > > > > > > > hw/s390x/css.c | 20 ++++++++++++++++---- > > > > 1 file changed, 16 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/hw/s390x/css.c b/hw/s390x/css.c > > > > index 04ec5cc9705..ef07691e36b 100644 > > > > --- a/hw/s390x/css.c > > > > +++ b/hw/s390x/css.c > > > > @@ -1290,9 +1290,15 @@ void copy_scsw_to_guest(SCSW *dest, const SCSW *src) > > > > static void copy_schib_to_guest(SCHIB *dest, const SCHIB *src) > > > > { > > > > int i; > > > > + PMCW srcpmcw, destpmcw; > > > > + SCSW srcscsw, destscsw; > > > > > > <bikeshed> > > > I would find src_pmcw etc. easier to read. Other opinions? > > > </bikeshed> > > > > CODING_STYLE's "Naming" section agrees with you... > > Do you plan to send a v2, or should I just rename and apply? If you want to rename and apply that would be great; I can send a v2 if that's easier for you. thanks -- PMM
On 2018-12-10 14:58, Peter Maydell wrote: > Taking the address of a field in a packed struct is a bad idea, because > it might not be actually aligned enough for that pointer type (and > thus cause a crash on dereference on some host architectures). Newer > versions of clang warn about this. > > Avoid the problem by using local copies of the PMCW and SCSW > struct fields in copy_schib_from_guest() and copy_schib_to_guest(). > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > This seemed like a not totally ugly and reasonably localised fix > that satisfies clang. Oddly, this makes the generated object file > 15K smaller (421K vs 406K), so it might even be better code... > > hw/s390x/css.c | 20 ++++++++++++++++---- > 1 file changed, 16 insertions(+), 4 deletions(-) > > diff --git a/hw/s390x/css.c b/hw/s390x/css.c > index 04ec5cc9705..ef07691e36b 100644 > --- a/hw/s390x/css.c > +++ b/hw/s390x/css.c > @@ -1290,9 +1290,15 @@ void copy_scsw_to_guest(SCSW *dest, const SCSW *src) > static void copy_schib_to_guest(SCHIB *dest, const SCHIB *src) > { > int i; > + PMCW srcpmcw, destpmcw; > + SCSW srcscsw, destscsw; > > - copy_pmcw_to_guest(&dest->pmcw, &src->pmcw); > - copy_scsw_to_guest(&dest->scsw, &src->scsw); > + srcpmcw = src->pmcw; > + copy_pmcw_to_guest(&destpmcw, &srcpmcw); > + dest->pmcw = destpmcw; > + srcscsw = src->scsw; > + copy_scsw_to_guest(&destscsw, &srcscsw); > + dest->scsw = destscsw; > dest->mba = cpu_to_be64(src->mba); > for (i = 0; i < ARRAY_SIZE(dest->mda); i++) { > dest->mda[i] = src->mda[i]; > @@ -1339,9 +1345,15 @@ static void copy_scsw_from_guest(SCSW *dest, const SCSW *src) > static void copy_schib_from_guest(SCHIB *dest, const SCHIB *src) > { > int i; > + PMCW srcpmcw, destpmcw; > + SCSW srcscsw, destscsw; > > - copy_pmcw_from_guest(&dest->pmcw, &src->pmcw); > - copy_scsw_from_guest(&dest->scsw, &src->scsw); > + srcpmcw = src->pmcw; > + copy_pmcw_from_guest(&destpmcw, &srcpmcw); > + dest->pmcw = destpmcw; > + srcscsw = src->scsw; > + copy_scsw_from_guest(&destscsw, &srcscsw); > + dest->scsw = destscsw; > dest->mba = be64_to_cpu(src->mba); > for (i = 0; i < ARRAY_SIZE(dest->mda); i++) { > dest->mda[i] = src->mda[i]; > May I suggest to add a comment to the code here a la: /* Use a local copy to avoid unaligned access to packed structs */ or something similar? Otherwise, I'm pretty sure somebody will revert this in a couple of years because they thinks the local copy is not really necessary here... Thomas
On Wed, 12 Dec 2018 21:15:29 +0000 Peter Maydell <peter.maydell@linaro.org> wrote: > On Wed, 12 Dec 2018 at 17:34, Cornelia Huck <cohuck@redhat.com> wrote: > > > > On Mon, 10 Dec 2018 14:23:15 +0000 > > Peter Maydell <peter.maydell@linaro.org> wrote: > > > > > On Mon, 10 Dec 2018 at 14:13, Cornelia Huck <cohuck@redhat.com> wrote: > > > > > > > > On Mon, 10 Dec 2018 13:58:03 +0000 > > > > Peter Maydell <peter.maydell@linaro.org> wrote: > > > > > > > > > Taking the address of a field in a packed struct is a bad idea, because > > > > > it might not be actually aligned enough for that pointer type (and > > > > > thus cause a crash on dereference on some host architectures). Newer > > > > > versions of clang warn about this. > > > > > > > > > > Avoid the problem by using local copies of the PMCW and SCSW > > > > > struct fields in copy_schib_from_guest() and copy_schib_to_guest(). > > > > > > > > > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > > > > > --- > > > > > This seemed like a not totally ugly and reasonably localised fix > > > > > that satisfies clang. Oddly, this makes the generated object file > > > > > 15K smaller (421K vs 406K), so it might even be better code... > > > > > > > > Nice :) > > > > > > > > > > > > > > hw/s390x/css.c | 20 ++++++++++++++++---- > > > > > 1 file changed, 16 insertions(+), 4 deletions(-) > > > > > > > > > > diff --git a/hw/s390x/css.c b/hw/s390x/css.c > > > > > index 04ec5cc9705..ef07691e36b 100644 > > > > > --- a/hw/s390x/css.c > > > > > +++ b/hw/s390x/css.c > > > > > @@ -1290,9 +1290,15 @@ void copy_scsw_to_guest(SCSW *dest, const SCSW *src) > > > > > static void copy_schib_to_guest(SCHIB *dest, const SCHIB *src) > > > > > { > > > > > int i; > > > > > + PMCW srcpmcw, destpmcw; > > > > > + SCSW srcscsw, destscsw; > > > > > > > > <bikeshed> > > > > I would find src_pmcw etc. easier to read. Other opinions? > > > > </bikeshed> > > > > > > CODING_STYLE's "Naming" section agrees with you... > > > > Do you plan to send a v2, or should I just rename and apply? > > If you want to rename and apply that would be great; I can > send a v2 if that's easier for you. > > thanks > -- PMM Given that Thomas also wanted a comment added, a v2 would be easier for me.
diff --git a/hw/s390x/css.c b/hw/s390x/css.c index 04ec5cc9705..ef07691e36b 100644 --- a/hw/s390x/css.c +++ b/hw/s390x/css.c @@ -1290,9 +1290,15 @@ void copy_scsw_to_guest(SCSW *dest, const SCSW *src) static void copy_schib_to_guest(SCHIB *dest, const SCHIB *src) { int i; + PMCW srcpmcw, destpmcw; + SCSW srcscsw, destscsw; - copy_pmcw_to_guest(&dest->pmcw, &src->pmcw); - copy_scsw_to_guest(&dest->scsw, &src->scsw); + srcpmcw = src->pmcw; + copy_pmcw_to_guest(&destpmcw, &srcpmcw); + dest->pmcw = destpmcw; + srcscsw = src->scsw; + copy_scsw_to_guest(&destscsw, &srcscsw); + dest->scsw = destscsw; dest->mba = cpu_to_be64(src->mba); for (i = 0; i < ARRAY_SIZE(dest->mda); i++) { dest->mda[i] = src->mda[i]; @@ -1339,9 +1345,15 @@ static void copy_scsw_from_guest(SCSW *dest, const SCSW *src) static void copy_schib_from_guest(SCHIB *dest, const SCHIB *src) { int i; + PMCW srcpmcw, destpmcw; + SCSW srcscsw, destscsw; - copy_pmcw_from_guest(&dest->pmcw, &src->pmcw); - copy_scsw_from_guest(&dest->scsw, &src->scsw); + srcpmcw = src->pmcw; + copy_pmcw_from_guest(&destpmcw, &srcpmcw); + dest->pmcw = destpmcw; + srcscsw = src->scsw; + copy_scsw_from_guest(&destscsw, &srcscsw); + dest->scsw = destscsw; dest->mba = be64_to_cpu(src->mba); for (i = 0; i < ARRAY_SIZE(dest->mda); i++) { dest->mda[i] = src->mda[i];
Taking the address of a field in a packed struct is a bad idea, because it might not be actually aligned enough for that pointer type (and thus cause a crash on dereference on some host architectures). Newer versions of clang warn about this. Avoid the problem by using local copies of the PMCW and SCSW struct fields in copy_schib_from_guest() and copy_schib_to_guest(). Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- This seemed like a not totally ugly and reasonably localised fix that satisfies clang. Oddly, this makes the generated object file 15K smaller (421K vs 406K), so it might even be better code... hw/s390x/css.c | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) -- 2.19.2