While this patch is very simple, and I hope without any objection, it is
RFC for the reason that our crash ABI is private.
The comments for XEN_ELFNOTE_CRASH_REGS does state that it is
architecture specific, and makes no indication about the size or
contents of the crash note. However, any code trying to use one of
these types of notes has to make an assumption that it if the note desc
length is 4*8 bytes long, it is representing CR{0,2-4}.
I guess my question boils down to whether it is acceptable to change a
private ABI which is not really so private, or whether we should make a
formal public ABI for all of the inards of the crash notes and use that.
I have some upcoming plans to put quite a lot more information into this
(or an equivalent) structure for extra analysis of the environment at
the point of a crash, so perhaps putting in the proper work to make a
public ABI would be the best course of action.
--
Andrew Cooper - Dom0 Kernel Engineer, Citrix XenServer
T: +44 (0)1223 225 900, http://www.citrix.com
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
On Tue, 2012-09-25 at 15:18 +0100, Andrew Cooper wrote:> While this patch is very simple, and I hope without any objection, it is > RFC for the reason that our crash ABI is private. > > The comments for XEN_ELFNOTE_CRASH_REGS does state that it is > architecture specific, and makes no indication about the size or > contents of the crash note. However, any code trying to use one of > these types of notes has to make an assumption that it if the note desc > length is 4*8 bytes long, it is representing CR{0,2-4}. > > I guess my question boils down to whether it is acceptable to change a > private ABI which is not really so private, or whether we should make a > formal public ABI for all of the inards of the crash notes and use that.Is it really private? (or even "not so private"), don''t external tools like crash support it? I suppose the size field in the notes is a sort of rudimentary version field. Remember you can always add a new note type though. Ian.
On 25/09/12 15:33, Ian Campbell wrote:> On Tue, 2012-09-25 at 15:18 +0100, Andrew Cooper wrote: >> While this patch is very simple, and I hope without any objection, it is >> RFC for the reason that our crash ABI is private. >> >> The comments for XEN_ELFNOTE_CRASH_REGS does state that it is >> architecture specific, and makes no indication about the size or >> contents of the crash note. However, any code trying to use one of >> these types of notes has to make an assumption that it if the note desc >> length is 4*8 bytes long, it is representing CR{0,2-4}. >> >> I guess my question boils down to whether it is acceptable to change a >> private ABI which is not really so private, or whether we should make a >> formal public ABI for all of the inards of the crash notes and use that. > Is it really private? (or even "not so private"), don''t external tools > like crash support it?This is my point. It is not in xen/public but is used by crash/kdump and similar utilities, effectively making it a public ABI. include/public/elfnote.h even explicitly refers to include/xen/elfcore.h, which is not public by our definition.> > I suppose the size field in the notes is a sort of rudimentary version > field. Remember you can always add a new note type though.Yes, although looking through my code, I do raise an error if sizeof(note->desc) != sizeof(my structure representing this note), which was put in with the best of intentions, but will break with the this RFC change. On the other hand, adding a new crash note for every new register will not scale well, as it is per PCU. I guess the only sensible way to continue is to present a formal public ABI.> > Ian. > >-- Andrew Cooper - Dom0 Kernel Engineer, Citrix XenServer T: +44 (0)1223 225 900, http://www.citrix.com
On 25/09/2012 15:53, "Andrew Cooper" <andrew.cooper3@citrix.com> wrote:>> I suppose the size field in the notes is a sort of rudimentary version >> field. Remember you can always add a new note type though. > > Yes, although looking through my code, I do raise an error if > sizeof(note->desc) != sizeof(my structure representing this note), which > was put in with the best of intentions, but will break with the this RFC > change. > > On the other hand, adding a new crash note for every new register will > not scale well, as it is per PCU. > > I guess the only sensible way to continue is to present a formal public ABI.That seems a good idea. -- Keir
On Tue, 2012-09-25 at 15:53 +0100, Andrew Cooper wrote:> On 25/09/12 15:33, Ian Campbell wrote: > > On Tue, 2012-09-25 at 15:18 +0100, Andrew Cooper wrote: > >> While this patch is very simple, and I hope without any objection, it is > >> RFC for the reason that our crash ABI is private. > >> > >> The comments for XEN_ELFNOTE_CRASH_REGS does state that it is > >> architecture specific, and makes no indication about the size or > >> contents of the crash note. However, any code trying to use one of > >> these types of notes has to make an assumption that it if the note desc > >> length is 4*8 bytes long, it is representing CR{0,2-4}. > >> > >> I guess my question boils down to whether it is acceptable to change a > >> private ABI which is not really so private, or whether we should make a > >> formal public ABI for all of the inards of the crash notes and use that. > > Is it really private? (or even "not so private"), don''t external tools > > like crash support it? > > This is my point. It is not in xen/public but is used by crash/kdump > and similar utilities, effectively making it a public ABI. > > include/public/elfnote.h even explicitly refers to > include/xen/elfcore.h, which is not public by our definition.Hrm, I think it is public despite its location.> > > > > I suppose the size field in the notes is a sort of rudimentary version > > field. Remember you can always add a new note type though. > > Yes, although looking through my code, I do raise an error if > sizeof(note->desc) != sizeof(my structure representing this note), which > was put in with the best of intentions, but will break with the this RFC > change. > > On the other hand, adding a new crash note for every new register will > not scale well, as it is per PCU.A new note for every batch of registers is what I was thinking. Or a new note with a more extensible structure.> > I guess the only sensible way to continue is to present a formal public ABI. > > > > > Ian. > > > >
>>> On 25.09.12 at 16:53, Andrew Cooper <andrew.cooper3@citrix.com> wrote:> On 25/09/12 15:33, Ian Campbell wrote: >> On Tue, 2012-09-25 at 15:18 +0100, Andrew Cooper wrote: >>> While this patch is very simple, and I hope without any objection, it is >>> RFC for the reason that our crash ABI is private. >>> >>> The comments for XEN_ELFNOTE_CRASH_REGS does state that it is >>> architecture specific, and makes no indication about the size or >>> contents of the crash note. However, any code trying to use one of >>> these types of notes has to make an assumption that it if the note desc >>> length is 4*8 bytes long, it is representing CR{0,2-4}. >>> >>> I guess my question boils down to whether it is acceptable to change a >>> private ABI which is not really so private, or whether we should make a >>> formal public ABI for all of the inards of the crash notes and use that. >> Is it really private? (or even "not so private"), don''t external tools >> like crash support it? > > This is my point. It is not in xen/public but is used by crash/kdump > and similar utilities, effectively making it a public ABI. > > include/public/elfnote.h even explicitly refers to > include/xen/elfcore.h, which is not public by our definition. > >> >> I suppose the size field in the notes is a sort of rudimentary version >> field. Remember you can always add a new note type though. > > Yes, although looking through my code, I do raise an error if > sizeof(note->desc) != sizeof(my structure representing this note), which > was put in with the best of intentions, but will break with the this RFC > change. > > On the other hand, adding a new crash note for every new register will > not scale well, as it is per PCU.You don''t need a not per addition - if you add a new note now clearly identifying it as "might grow", then subsequently adding to it shouldn''t be a problem. Adding to an existing one that wasn''t considered extensible, as you see with your own example above, is not an option unless you have control over _all_ consumers. Jan