Unaligned writes on the kexec path

Unaligned writes on the kexec path

[Andrew Cooper]

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

Re: Unaligned writes on the kexec path

[Keir Fraser]

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

Re: Unaligned writes on the kexec path

[Simon Horman]

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.

Re: Unaligned writes on the kexec path

[Andrew Cooper]

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

Re: Unaligned writes on the kexec path

[Jan Beulich]

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

Re: Unaligned writes on the kexec path

[Andrew Cooper]

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

Re: Unaligned writes on the kexec path

[Simon Horman]

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.