Hello, In c/s 7493bb48d89f, you change the internals of kexec_crash_save_info() to reduce unaligned writes, but pass the resulting pointer back to machine_crash_shutdown() which performs writes on the possibly unaligned data structure. There are also plenty of other writes on the kexec path which are possibly or certainly unaligned. What is the reason for wanting to reduce unaligned writes? Would a better solution be to just ensure that the crash note itself is properly aligned? -- Andrew Cooper - Dom0 Kernel Engineer, Citrix XenServer T: +44 (0)1223 225 900, http://www.citrix.com
On 28/11/2011 14:24, "Andrew Cooper" <andrew.cooper3@citrix.com> wrote:> Hello, > > In c/s 7493bb48d89f, you change the internals of kexec_crash_save_info()The patch is by Simon Horman, cc''ed, not me.> to reduce unaligned writes, but pass the resulting pointer back to > machine_crash_shutdown() which performs writes on the possibly unaligned > data structure. There are also plenty of other writes on the kexec path > which are possibly or certainly unaligned. > > What is the reason for wanting to reduce unaligned writes?The patch is solving an IA64 issue. Machine_crash_shutdown is arch specific.> Would a better solution be to just ensure that the crash note itself is > properly aligned?Could be. -- Keir
Hi Keir, Hi Andrew, On Mon, Nov 28, 2011 at 08:42:16PM +0000, Keir Fraser wrote:> On 28/11/2011 14:24, "Andrew Cooper" <andrew.cooper3@citrix.com> wrote: > > > Hello, > > > > In c/s 7493bb48d89f, you change the internals of kexec_crash_save_info() > > The patch is by Simon Horman, cc''ed, not me. > > > to reduce unaligned writes, but pass the resulting pointer back to > > machine_crash_shutdown() which performs writes on the possibly unaligned > > data structure. There are also plenty of other writes on the kexec path > > which are possibly or certainly unaligned. > > > > What is the reason for wanting to reduce unaligned writes? > > The patch is solving an IA64 issue. Machine_crash_shutdown is arch specific. > > > Would a better solution be to just ensure that the crash note itself is > > properly aligned? > > Could be.Its a while since I wrote that change, but I believe that aligning the crash note would resolve the problem that I observed. As Keir mentions, machine_crash_shutdown() is architecture specific and I am not sure that I see the ia64 version making any unaligned writes.
On 29/11/11 05:51, Simon Horman wrote:> Hi Keir, Hi Andrew, > > On Mon, Nov 28, 2011 at 08:42:16PM +0000, Keir Fraser wrote: >> On 28/11/2011 14:24, "Andrew Cooper" <andrew.cooper3@citrix.com> wrote: >> >>> Hello, >>> >>> In c/s 7493bb48d89f, you change the internals of kexec_crash_save_info() >> The patch is by Simon Horman, cc''ed, not me.Right. Sorry. I should have remembered that basing "who wrote the patch" on a simple hg log was not a safe bet.>>> to reduce unaligned writes, but pass the resulting pointer back to >>> machine_crash_shutdown() which performs writes on the possibly unaligned >>> data structure. There are also plenty of other writes on the kexec path >>> which are possibly or certainly unaligned. >>> >>> What is the reason for wanting to reduce unaligned writes? >> The patch is solving an IA64 issue. Machine_crash_shutdown is arch specific. >> >>> Would a better solution be to just ensure that the crash note itself is >>> properly aligned? >> Could be. > Its a while since I wrote that change, but I believe that aligning > the crash note would resolve the problem that I observed. > > As Keir mentions, machine_crash_shutdown() is architecture specific > and I am not sure that I see the ia64 version making any unaligned writes.Fair enough. I will see what I can do about guaranteeing that the crash note is aligned in the first place. Thanks, -- Andrew Cooper - Dom0 Kernel Engineer, Citrix XenServer T: +44 (0)1223 225 900, http://www.citrix.com
>>> On 29.11.11 at 11:49, Andrew Cooper <andrew.cooper3@citrix.com> wrote:> > On 29/11/11 05:51, Simon Horman wrote: >> Hi Keir, Hi Andrew, >> >> On Mon, Nov 28, 2011 at 08:42:16PM +0000, Keir Fraser wrote: >>> On 28/11/2011 14:24, "Andrew Cooper" <andrew.cooper3@citrix.com> wrote: >>> >>>> Hello, >>>> >>>> In c/s 7493bb48d89f, you change the internals of kexec_crash_save_info() >>> The patch is by Simon Horman, cc''ed, not me. > > Right. Sorry. I should have remembered that basing "who wrote the > patch" on a simple hg log was not a safe bet.I don''t think there''s much room for improvement - all members of crash_xen_info_t are of "unsigned long" type, but ELF note handling will only ever guarantee 4-byte alignment. Jan
On 29/11/11 11:35, Jan Beulich wrote:>>>> On 29.11.11 at 11:49, Andrew Cooper <andrew.cooper3@citrix.com> wrote: >> On 29/11/11 05:51, Simon Horman wrote: >>> Hi Keir, Hi Andrew, >>> >>> On Mon, Nov 28, 2011 at 08:42:16PM +0000, Keir Fraser wrote: >>>> On 28/11/2011 14:24, "Andrew Cooper" <andrew.cooper3@citrix.com> wrote: >>>> >>>>> Hello, >>>>> >>>>> In c/s 7493bb48d89f, you change the internals of kexec_crash_save_info() >>>> The patch is by Simon Horman, cc''ed, not me. >> Right. Sorry. I should have remembered that basing "who wrote the >> patch" on a simple hg log was not a safe bet. > I don''t think there''s much room for improvement - all members of > crash_xen_info_t are of "unsigned long" type, but ELF note handling > will only ever guarantee 4-byte alignment. > > Jan >Depending on how flexible we want to be, we can either specify that the name field should be 2n words long plus 1-4 bytes, which will cause it to align to an odd number of 4 bytes, which will cause the desc field to be aligned to 8 bytes when the type field in the note header is taken into account. Then, the desc field should be constrained to be (2n+1) + 1-4bytes which would cause it to have 8 byte alignment, and subsequently 8 byte align the next note. Alternatively, we could artificially extend the name up to an odd 4 byte alignment, and desc field up to 8 byte alignment with trailing \0''s and include this as part of their length fields. All names should be processed as Null terminating strings (which wont suffer from having extra Nulls at the end) and I have yet to see processing of a note which doesn''t take the buffer and cast it to a structure pointer. This also wont suffer from from trailing data. Then again, this does sound like quite a lot of work for not a lot, and there is no guarantee that we wont break some of the more special code which works with elf files in ''special'' ways. (What really should have happened was for ELF64 to specify 64bit alignment of things like this, but we live and learn) -- Andrew Cooper - Dom0 Kernel Engineer, Citrix XenServer T: +44 (0)1223 225 900, http://www.citrix.com
On Tue, Nov 29, 2011 at 12:54:09PM +0000, Andrew Cooper wrote:> On 29/11/11 11:35, Jan Beulich wrote: > >>>> On 29.11.11 at 11:49, Andrew Cooper <andrew.cooper3@citrix.com> wrote: > >> On 29/11/11 05:51, Simon Horman wrote: > >>> Hi Keir, Hi Andrew, > >>> > >>> On Mon, Nov 28, 2011 at 08:42:16PM +0000, Keir Fraser wrote: > >>>> On 28/11/2011 14:24, "Andrew Cooper" <andrew.cooper3@citrix.com> wrote: > >>>> > >>>>> Hello, > >>>>> > >>>>> In c/s 7493bb48d89f, you change the internals of kexec_crash_save_info() > >>>> The patch is by Simon Horman, cc''ed, not me. > >> Right. Sorry. I should have remembered that basing "who wrote the > >> patch" on a simple hg log was not a safe bet. > > I don''t think there''s much room for improvement - all members of > > crash_xen_info_t are of "unsigned long" type, but ELF note handling > > will only ever guarantee 4-byte alignment. > > > > Jan > > > Depending on how flexible we want to be, we can either specify that the > name field should be 2n words long plus 1-4 bytes, which will cause it > to align to an odd number of 4 bytes, which will cause the desc field to > be aligned to 8 bytes when the type field in the note header is taken > into account. Then, the desc field should be constrained to be (2n+1) + > 1-4bytes which would cause it to have 8 byte alignment, and subsequently > 8 byte align the next note. > > Alternatively, we could artificially extend the name up to an odd 4 byte > alignment, and desc field up to 8 byte alignment with trailing \0''s and > include this as part of their length fields. All names should be > processed as Null terminating strings (which wont suffer from having > extra Nulls at the end) and I have yet to see processing of a note which > doesn''t take the buffer and cast it to a structure pointer. This also > wont suffer from from trailing data. > > Then again, this does sound like quite a lot of work for not a lot, and > there is no guarantee that we wont break some of the more special code > which works with elf files in ''special'' ways.I believe that the scheme you suggest would work. But elf parsing does tend to be a bit special. So I lean towards not changing things.> (What really should have happened was for ELF64 to specify 64bit > alignment of things like this, but we live and learn)Agreed.