Re: [Xen-devel] [PATCH 3/9] kexec: add infrastructure for handling kexec images

[Don Slutz <dslutz@xxxxxxxxxxx>]

On 10/08/13 12:55, David Vrabel wrote:

From: David Vrabel <david.vrabel@xxxxxxxxxx>

Add the code needed to handle and load kexec images into Xen memory or
into the crash region.  This is needed for the new KEXEC_CMD_load and
KEXEC_CMD_unload hypercall sub-ops.

[...]

+static int kimage_crash_alloc(struct kexec_image **rimage, paddr_t entry,
+                              unsigned long nr_segments,
+                              xen_kexec_segment_t *segments)
+{
+    unsigned long i;
+    int result;
+
+    /* Verify we have a valid entry point */
+    if ( (entry < kexec_crash_area.start)
+         || (entry > kexec_crash_area.start + kexec_crash_area.size))
+        return -EADDRNOTAVAIL;
+
+    /*
+     * Verify we have good destination addresses.  Normally
+     * the caller is responsible for making certain we don't
+     * attempt to load the new image into invalid or reserved
+     * areas of RAM.  But crash kernels are preloaded into a
+     * reserved area of ram.  We must ensure the addresses
+     * are in the reserved area otherwise preloading the
+     * kernel could corrupt things.
+     */
+    for ( i = 0; i < nr_segments; i++ )
+    {
+        paddr_t mstart, mend;
+
+        if ( guest_handle_is_null(segments[i].buf.h) )
+            continue;
+
+        mstart = segments[i].dest_maddr;
+        mend = mstart + segments[i].dest_size - 1;

It is safer and matches the rest of the use to drop the -1 here, and use "mend >=" below.  (The more I think on it, "mend >" below is still correct after removing the "- 1".)

+        /* Ensure we are within the crash kernel limits. */
+        if ( (mstart < kexec_crash_area.start )
+             || (mend > kexec_crash_area.start + kexec_crash_area.size))
+            return -EADDRNOTAVAIL;
+    }
+
+    /* Allocate and initialize a controlling structure. */
+    result = do_kimage_alloc(rimage, entry, nr_segments, segments,
+                             KEXEC_TYPE_CRASH);
+    if ( result )
+        return result;
+
+    return 0;

It can be proved that result === 0.  So "return result" and "return 0" are the same.  This means that the if above is not needed.  When it is removed the only use of result is to be returned, so changing to:

return do_kimage_alloc(rimage, entry, nr_segments, segments, KEXEC_TYPE_CRASH);

makes sense to me.

+}

[...]

+static int kimage_add_entry(struct kexec_image *image, kimage_entry_t entry)
+{
+    kimage_entry_t *entries;
+
+    if ( image->next_entry == KIMAGE_LAST_ENTRY )
+    {
+        struct page_info *page;
+
+        page = kimage_alloc_page(image, KIMAGE_NO_DEST);
+        if ( !page )
+            return -ENOMEM;
+
+        entries = __map_domain_page(image->entry_page);

Not sure if entries needs to be checked for NULL.  Best guess is that it cannot be NULL.

+        entries[image->next_entry] = page_to_maddr(page) | IND_INDIRECTION;
+        unmap_domain_page(entries);
+

  -Don Slutz

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel