| Message ID | 20200311083304.3725276-1-borntraeger@de.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: s390: mark sie block as 512 byte aligned | expand |
On 11.03.20 09:33, Christian Borntraeger wrote: > The sie block must be aligned to 512 bytes. Mark it as such. > > Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> > --- > arch/s390/include/asm/kvm_host.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h > index 0ea82152d2f7..2d50f6c432e2 100644 > --- a/arch/s390/include/asm/kvm_host.h > +++ b/arch/s390/include/asm/kvm_host.h > @@ -344,7 +344,7 @@ struct kvm_s390_sie_block { > __u64 itdba; /* 0x01e8 */ > __u64 riccbd; /* 0x01f0 */ > __u64 gvrd; /* 0x01f8 */ > -} __attribute__((packed)); > +} __packed __aligned(512); > I guess there is no change in the code/behavior, because we always place into well defined spots within a page. Reviewed-by: David Hildenbrand <david@redhat.com>
On 11.03.20 09:41, David Hildenbrand wrote: > On 11.03.20 09:33, Christian Borntraeger wrote: >> The sie block must be aligned to 512 bytes. Mark it as such. >> >> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> >> --- >> arch/s390/include/asm/kvm_host.h | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h >> index 0ea82152d2f7..2d50f6c432e2 100644 >> --- a/arch/s390/include/asm/kvm_host.h >> +++ b/arch/s390/include/asm/kvm_host.h >> @@ -344,7 +344,7 @@ struct kvm_s390_sie_block { >> __u64 itdba; /* 0x01e8 */ >> __u64 riccbd; /* 0x01f0 */ >> __u64 gvrd; /* 0x01f8 */ >> -} __attribute__((packed)); >> +} __packed __aligned(512); >> > > I guess there is no change in the code/behavior, because we always place > into well defined spots within a page. In theory gcc can generate code that relies on that alignment. For example load relative long requires double word alignment. Or some atomic instructions. This could - in theory - generate better code. > > Reviewed-by: David Hildenbrand <david@redhat.com> > >
On Wed, 11 Mar 2020 09:33:04 +0100 Christian Borntraeger <borntraeger@de.ibm.com> wrote: > The sie block must be aligned to 512 bytes. Mark it as such. > > Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> > --- > arch/s390/include/asm/kvm_host.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/s390/include/asm/kvm_host.h > b/arch/s390/include/asm/kvm_host.h index 0ea82152d2f7..2d50f6c432e2 > 100644 --- a/arch/s390/include/asm/kvm_host.h > +++ b/arch/s390/include/asm/kvm_host.h > @@ -344,7 +344,7 @@ struct kvm_s390_sie_block { > __u64 itdba; /* 0x01e8 */ > __u64 riccbd; /* 0x01f0 */ > __u64 gvrd; /* 0x01f8 */ > -} __attribute__((packed)); > +} __packed __aligned(512); > > struct kvm_s390_itdb { > __u8 data[256]; I agree with the addition of aligned, but did you really have to remove packed? it makes me a little uncomfortable. do we have any compile-time assertions that the size of the block will be the one we expect?
On 16.03.20 13:10, Claudio Imbrenda wrote: > On Wed, 11 Mar 2020 09:33:04 +0100 > Christian Borntraeger <borntraeger@de.ibm.com> wrote: > >> The sie block must be aligned to 512 bytes. Mark it as such. >> >> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> >> --- >> arch/s390/include/asm/kvm_host.h | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/arch/s390/include/asm/kvm_host.h >> b/arch/s390/include/asm/kvm_host.h index 0ea82152d2f7..2d50f6c432e2 >> 100644 --- a/arch/s390/include/asm/kvm_host.h >> +++ b/arch/s390/include/asm/kvm_host.h >> @@ -344,7 +344,7 @@ struct kvm_s390_sie_block { >> __u64 itdba; /* 0x01e8 */ >> __u64 riccbd; /* 0x01f0 */ >> __u64 gvrd; /* 0x01f8 */ >> -} __attribute__((packed)); >> +} __packed __aligned(512); >> >> struct kvm_s390_itdb { >> __u8 data[256]; > > I agree with the addition of aligned, but did you really have to remove > packed? it makes me a little uncomfortable. There is still "__packed".
On Mon, 16 Mar 2020 13:11:30 +0100 David Hildenbrand <david@redhat.com> wrote: > On 16.03.20 13:10, Claudio Imbrenda wrote: > > On Wed, 11 Mar 2020 09:33:04 +0100 > > Christian Borntraeger <borntraeger@de.ibm.com> wrote: > > > >> The sie block must be aligned to 512 bytes. Mark it as such. > >> > >> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> > >> --- > >> arch/s390/include/asm/kvm_host.h | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/arch/s390/include/asm/kvm_host.h > >> b/arch/s390/include/asm/kvm_host.h index 0ea82152d2f7..2d50f6c432e2 > >> 100644 --- a/arch/s390/include/asm/kvm_host.h > >> +++ b/arch/s390/include/asm/kvm_host.h > >> @@ -344,7 +344,7 @@ struct kvm_s390_sie_block { > >> __u64 itdba; /* 0x01e8 */ > >> __u64 riccbd; /* 0x01f0 */ > >> __u64 gvrd; /* 0x01f8 */ > >> -} __attribute__((packed)); > >> +} __packed __aligned(512); > >> > >> struct kvm_s390_itdb { > >> __u8 data[256]; > > > > I agree with the addition of aligned, but did you really have to > > remove packed? it makes me a little uncomfortable. > > There is still "__packed". > I had somehow totally missed it this is what happens when you start working before actually waking up :D
diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h index 0ea82152d2f7..2d50f6c432e2 100644 --- a/arch/s390/include/asm/kvm_host.h +++ b/arch/s390/include/asm/kvm_host.h @@ -344,7 +344,7 @@ struct kvm_s390_sie_block { __u64 itdba; /* 0x01e8 */ __u64 riccbd; /* 0x01f0 */ __u64 gvrd; /* 0x01f8 */ -} __attribute__((packed)); +} __packed __aligned(512); struct kvm_s390_itdb { __u8 data[256];
The sie block must be aligned to 512 bytes. Mark it as such. Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> --- arch/s390/include/asm/kvm_host.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)