| Message ID | 1537947527-3336-2-git-send-email-thuth@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fix migration problems of s390x guests on Sparc hosts | expand |
On 26/09/2018 09:38, Thomas Huth wrote: > The IplParameterBlock and QemuIplParameters structures are declared > with QEMU_PACKED, so the compiler assumes that the structures do not > need to be aligned in memory. Since the are listed after a "bool" > within the S390IPLState, the IplParameterBlock and QemuIplParameters > are also indeed mis-aligned in memory. This causes problems on Sparc > during migration, since we use VMSTATE_UINT16 in vmstate_iplb to access > the devno member for example, and the corresponding migration functions > (like qemu_get_be16s) then try to access a 16-bit value from a mis- > aligned memory address. > The easiest solution to fix this problem is to move the packed structures > to the beginning of the S390IPLState. Also add some additional comments > here to prevent that this problem will be introduced again in the future. The last sentence no longer applies. Reviewed-by: David Hildenbrand <david@redhat.com> > > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- > hw/s390x/ipl.h | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h > index 4e87b89..b3a07a1 100644 > --- a/hw/s390x/ipl.h > +++ b/hw/s390x/ipl.h > @@ -132,15 +132,15 @@ typedef struct QemuIplParameters QemuIplParameters; > struct S390IPLState { > /*< private >*/ > DeviceState parent_obj; > + IplParameterBlock iplb; > + QemuIplParameters qipl; > uint64_t start_addr; > uint64_t compat_start_addr; > uint64_t bios_start_addr; > uint64_t compat_bios_start_addr; > bool enforce_bios; > - IplParameterBlock iplb; > bool iplb_valid; > bool netboot; > - QemuIplParameters qipl; > /* reset related properties don't have to be migrated or reset */ > enum s390_reset reset_type; > int reset_cpu_index; > @@ -157,6 +157,7 @@ struct S390IPLState { > bool iplbext_migration; > }; > typedef struct S390IPLState S390IPLState; > +QEMU_BUILD_BUG_MSG(offsetof(S390IPLState, iplb) & 3, "alignment of iplb wrong"); > > #define S390_IPL_TYPE_FCP 0x00 > #define S390_IPL_TYPE_CCW 0x02 >
On 2018-09-26 09:56, David Hildenbrand wrote: > On 26/09/2018 09:38, Thomas Huth wrote: >> The IplParameterBlock and QemuIplParameters structures are declared >> with QEMU_PACKED, so the compiler assumes that the structures do not >> need to be aligned in memory. Since the are listed after a "bool" >> within the S390IPLState, the IplParameterBlock and QemuIplParameters >> are also indeed mis-aligned in memory. This causes problems on Sparc >> during migration, since we use VMSTATE_UINT16 in vmstate_iplb to access >> the devno member for example, and the corresponding migration functions >> (like qemu_get_be16s) then try to access a 16-bit value from a mis- >> aligned memory address. >> The easiest solution to fix this problem is to move the packed structures >> to the beginning of the S390IPLState. Also add some additional comments >> here to prevent that this problem will be introduced again in the future. > > The last sentence no longer applies. Oops. Cornelia, could you please fix it up when picking up the patch (in case there are no other reasons for respinning the series)? Thanks, Thomas
On Wed, 26 Sep 2018 09:38:45 +0200 Thomas Huth <thuth@redhat.com> wrote: > The IplParameterBlock and QemuIplParameters structures are declared > with QEMU_PACKED, so the compiler assumes that the structures do not > need to be aligned in memory. Since the are listed after a "bool" > within the S390IPLState, the IplParameterBlock and QemuIplParameters > are also indeed mis-aligned in memory. This causes problems on Sparc > during migration, since we use VMSTATE_UINT16 in vmstate_iplb to access > the devno member for example, and the corresponding migration functions > (like qemu_get_be16s) then try to access a 16-bit value from a mis- > aligned memory address. > The easiest solution to fix this problem is to move the packed structures > to the beginning of the S390IPLState. Also add some additional comments > here to prevent that this problem will be introduced again in the future. > > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- > hw/s390x/ipl.h | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h > index 4e87b89..b3a07a1 100644 > --- a/hw/s390x/ipl.h > +++ b/hw/s390x/ipl.h > @@ -132,15 +132,15 @@ typedef struct QemuIplParameters QemuIplParameters; > struct S390IPLState { > /*< private >*/ > DeviceState parent_obj; > + IplParameterBlock iplb; > + QemuIplParameters qipl; Hm... this is not quite the beginning of the structure; what am I missing? > uint64_t start_addr; > uint64_t compat_start_addr; > uint64_t bios_start_addr; > uint64_t compat_bios_start_addr; > bool enforce_bios; > - IplParameterBlock iplb; > bool iplb_valid; > bool netboot; > - QemuIplParameters qipl; > /* reset related properties don't have to be migrated or reset */ > enum s390_reset reset_type; > int reset_cpu_index; > @@ -157,6 +157,7 @@ struct S390IPLState { > bool iplbext_migration; > }; > typedef struct S390IPLState S390IPLState; > +QEMU_BUILD_BUG_MSG(offsetof(S390IPLState, iplb) & 3, "alignment of iplb wrong"); > > #define S390_IPL_TYPE_FCP 0x00 > #define S390_IPL_TYPE_CCW 0x02
On 2018-09-26 11:42, Cornelia Huck wrote: > On Wed, 26 Sep 2018 09:38:45 +0200 > Thomas Huth <thuth@redhat.com> wrote: > >> The IplParameterBlock and QemuIplParameters structures are declared >> with QEMU_PACKED, so the compiler assumes that the structures do not >> need to be aligned in memory. Since the are listed after a "bool" >> within the S390IPLState, the IplParameterBlock and QemuIplParameters >> are also indeed mis-aligned in memory. This causes problems on Sparc >> during migration, since we use VMSTATE_UINT16 in vmstate_iplb to access >> the devno member for example, and the corresponding migration functions >> (like qemu_get_be16s) then try to access a 16-bit value from a mis- >> aligned memory address. >> The easiest solution to fix this problem is to move the packed structures >> to the beginning of the S390IPLState. Also add some additional comments >> here to prevent that this problem will be introduced again in the future. >> >> Signed-off-by: Thomas Huth <thuth@redhat.com> >> --- >> hw/s390x/ipl.h | 5 +++-- >> 1 file changed, 3 insertions(+), 2 deletions(-) >> >> diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h >> index 4e87b89..b3a07a1 100644 >> --- a/hw/s390x/ipl.h >> +++ b/hw/s390x/ipl.h >> @@ -132,15 +132,15 @@ typedef struct QemuIplParameters QemuIplParameters; >> struct S390IPLState { >> /*< private >*/ >> DeviceState parent_obj; >> + IplParameterBlock iplb; >> + QemuIplParameters qipl; > > Hm... this is not quite the beginning of the structure; what am I > missing? DeviceState of course has to stay first for QOM reasons. But since it is a non-packed struct, we can be sure that it will be padded to the correct alignment at the end. If not, the QEMU_BUILD_BUG_MSG in this patch will tell us. Thomas
On Wed, 26 Sep 2018 11:46:22 +0200 Thomas Huth <thuth@redhat.com> wrote: > On 2018-09-26 11:42, Cornelia Huck wrote: > > On Wed, 26 Sep 2018 09:38:45 +0200 > > Thomas Huth <thuth@redhat.com> wrote: > > > >> The IplParameterBlock and QemuIplParameters structures are declared > >> with QEMU_PACKED, so the compiler assumes that the structures do not > >> need to be aligned in memory. Since the are listed after a "bool" > >> within the S390IPLState, the IplParameterBlock and QemuIplParameters > >> are also indeed mis-aligned in memory. This causes problems on Sparc > >> during migration, since we use VMSTATE_UINT16 in vmstate_iplb to access > >> the devno member for example, and the corresponding migration functions > >> (like qemu_get_be16s) then try to access a 16-bit value from a mis- > >> aligned memory address. > >> The easiest solution to fix this problem is to move the packed structures > >> to the beginning of the S390IPLState. Also add some additional comments > >> here to prevent that this problem will be introduced again in the future. > >> > >> Signed-off-by: Thomas Huth <thuth@redhat.com> > >> --- > >> hw/s390x/ipl.h | 5 +++-- > >> 1 file changed, 3 insertions(+), 2 deletions(-) > >> > >> diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h > >> index 4e87b89..b3a07a1 100644 > >> --- a/hw/s390x/ipl.h > >> +++ b/hw/s390x/ipl.h > >> @@ -132,15 +132,15 @@ typedef struct QemuIplParameters QemuIplParameters; > >> struct S390IPLState { > >> /*< private >*/ > >> DeviceState parent_obj; > >> + IplParameterBlock iplb; > >> + QemuIplParameters qipl; > > > > Hm... this is not quite the beginning of the structure; what am I > > missing? > > DeviceState of course has to stay first for QOM reasons. But since it is > a non-packed struct, we can be sure that it will be padded to the > correct alignment at the end. If not, the QEMU_BUILD_BUG_MSG in this > patch will tell us. What about adding that explanation to the commit message? [I can do that.]
On 2018-09-26 11:55, Cornelia Huck wrote: > On Wed, 26 Sep 2018 11:46:22 +0200 > Thomas Huth <thuth@redhat.com> wrote: > >> On 2018-09-26 11:42, Cornelia Huck wrote: >>> On Wed, 26 Sep 2018 09:38:45 +0200 >>> Thomas Huth <thuth@redhat.com> wrote: >>> >>>> The IplParameterBlock and QemuIplParameters structures are declared >>>> with QEMU_PACKED, so the compiler assumes that the structures do not >>>> need to be aligned in memory. Since the are listed after a "bool" >>>> within the S390IPLState, the IplParameterBlock and QemuIplParameters >>>> are also indeed mis-aligned in memory. This causes problems on Sparc >>>> during migration, since we use VMSTATE_UINT16 in vmstate_iplb to access >>>> the devno member for example, and the corresponding migration functions >>>> (like qemu_get_be16s) then try to access a 16-bit value from a mis- >>>> aligned memory address. >>>> The easiest solution to fix this problem is to move the packed structures >>>> to the beginning of the S390IPLState. Also add some additional comments >>>> here to prevent that this problem will be introduced again in the future. >>>> >>>> Signed-off-by: Thomas Huth <thuth@redhat.com> >>>> --- >>>> hw/s390x/ipl.h | 5 +++-- >>>> 1 file changed, 3 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h >>>> index 4e87b89..b3a07a1 100644 >>>> --- a/hw/s390x/ipl.h >>>> +++ b/hw/s390x/ipl.h >>>> @@ -132,15 +132,15 @@ typedef struct QemuIplParameters QemuIplParameters; >>>> struct S390IPLState { >>>> /*< private >*/ >>>> DeviceState parent_obj; >>>> + IplParameterBlock iplb; >>>> + QemuIplParameters qipl; >>> >>> Hm... this is not quite the beginning of the structure; what am I >>> missing? >> >> DeviceState of course has to stay first for QOM reasons. But since it is >> a non-packed struct, we can be sure that it will be padded to the >> correct alignment at the end. If not, the QEMU_BUILD_BUG_MSG in this >> patch will tell us. > > What about adding that explanation to the commit message? > > [I can do that.] Sure, if you could do that, that would be great! Thanks, Thomas
On 26 September 2018 at 08:38, Thomas Huth <thuth@redhat.com> wrote: > The IplParameterBlock and QemuIplParameters structures are declared > with QEMU_PACKED, so the compiler assumes that the structures do not > need to be aligned in memory. Since the are listed after a "bool" > within the S390IPLState, the IplParameterBlock and QemuIplParameters > are also indeed mis-aligned in memory. This causes problems on Sparc > during migration, since we use VMSTATE_UINT16 in vmstate_iplb to access > the devno member for example, and the corresponding migration functions > (like qemu_get_be16s) then try to access a 16-bit value from a mis- > aligned memory address. > The easiest solution to fix this problem is to move the packed structures > to the beginning of the S390IPLState. Also add some additional comments > here to prevent that this problem will be introduced again in the future. > +QEMU_BUILD_BUG_MSG(offsetof(S390IPLState, iplb) & 3, "alignment of iplb wrong"); Incidentally, new gcc has an attribute "warn_if_not_aligned" so you can say struct S390IPLState { ... IplParameterBlock iplb __attribute__((warn_if_not_aligned(4))); ... }; and then the compiler will warn if the alignment isn't as specified. But that needs such a new version of gcc we're better off with our QEMU_BUILD_BUG_MSG. thanks -- PMM
diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h index 4e87b89..b3a07a1 100644 --- a/hw/s390x/ipl.h +++ b/hw/s390x/ipl.h @@ -132,15 +132,15 @@ typedef struct QemuIplParameters QemuIplParameters; struct S390IPLState { /*< private >*/ DeviceState parent_obj; + IplParameterBlock iplb; + QemuIplParameters qipl; uint64_t start_addr; uint64_t compat_start_addr; uint64_t bios_start_addr; uint64_t compat_bios_start_addr; bool enforce_bios; - IplParameterBlock iplb; bool iplb_valid; bool netboot; - QemuIplParameters qipl; /* reset related properties don't have to be migrated or reset */ enum s390_reset reset_type; int reset_cpu_index; @@ -157,6 +157,7 @@ struct S390IPLState { bool iplbext_migration; }; typedef struct S390IPLState S390IPLState; +QEMU_BUILD_BUG_MSG(offsetof(S390IPLState, iplb) & 3, "alignment of iplb wrong"); #define S390_IPL_TYPE_FCP 0x00 #define S390_IPL_TYPE_CCW 0x02
The IplParameterBlock and QemuIplParameters structures are declared with QEMU_PACKED, so the compiler assumes that the structures do not need to be aligned in memory. Since the are listed after a "bool" within the S390IPLState, the IplParameterBlock and QemuIplParameters are also indeed mis-aligned in memory. This causes problems on Sparc during migration, since we use VMSTATE_UINT16 in vmstate_iplb to access the devno member for example, and the corresponding migration functions (like qemu_get_be16s) then try to access a 16-bit value from a mis- aligned memory address. The easiest solution to fix this problem is to move the packed structures to the beginning of the S390IPLState. Also add some additional comments here to prevent that this problem will be introduced again in the future. Signed-off-by: Thomas Huth <thuth@redhat.com> --- hw/s390x/ipl.h | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)