Re: [Xen-devel] [PATCH 3/3] dump-core take 5: elf formatify and added PFN-GMFN table

[John Levon <levon@xxxxxxxxxxxxxxxxx>]

On Fri, Feb 02, 2007 at 09:42:28PM +0900, Isaku Yamahata wrote:

+ *  |section headers                                         |
+ *  |    null section header                                 |
+ *  |    .shstrtab                                           |
+ *  |    .note.Xen                                           |
+ *  |    .Xen.p2m or .Xen.pfn                                |
+ *  |    .Xen.pages                                          |

.xen_p2m as names would be more typical (e.g. Sun use .SUNW_ctf, etc.).

(This wasn't really what I meant by documentation, I was talking about a
document like the ELF standard that fully defines the content of each
section etc.)

+    live_p2m_frame_list_list =
+        xc_map_foreign_range(xc_handle, dom, PAGE_SIZE, PROT_READ,
+                             live_shinfo->arch.pfn_to_mfn_frame_list_list);

We should check for a zero value for this, and not even try, give some
warning like "domain hasn't booted/resumed enough to be able to dump
core".

+memory_map_get_old_hvm(int xc_handle, xc_dominfo_t *info, 

Can't we have all the ia64 stuff in a separate xc_core_ia64? It's always
been painfully difficult to read the old xc_linux_build.c, for example,
because of this.

+    rc = xc_version(xc_handle, XENVER_pagesize, NULL);

Great, but are we also storing PAE vs. non-PAE? Currently there's no way
to even find out if a core is PAE or not (!)

+static int
+elfnote_fill_format_version(struct xen_elfnote_dumpcore_format_version_desc
+                            *format_version)
+{
+    format_version->major = XEN_DUMPCORE_FORMAT_MAJOR_VERSION;
+    format_version->minor = XEN_DUMPCORE_FORMAT_MINOR_VERSION;
+    format_version->extra = XEN_DUMPCORE_FORMAT_EXTRA_VERSION;

For what purpose do you see the "extra version" being used for? Why
isn't this just one number? If we're bumping a minor version, it means
we must be /adding/ something - can't the client code just check if that
new thing exists?

+                /* try to map page to determin wheter it has underlying page */

'whether'

+        sizeof(struct xen_elfnote_dumpcore_none) +           /* none */
+        sizeof(struct xen_elfnote_dumpcore_header) +         /* core header */
+        sizeof(struct xen_elfnote_dumpcore_xen_version) +    /* xen version */
+        sizeof(struct xen_elfnote_dumpcore_format_version) + /* format version 
*/
+        sizeof(struct xen_elfnote_dumpcore_prstatus) + sizeof(ctxt[0]) * 
nr_vcpus; /* vcpu context */

Wouldn't prstatus make more sense as a section (entsize and all that)

regards
john

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel