From mboxrd@z Thu Jan 1 00:00:00 1970 From: Don Slutz Subject: Re: [PATCH 3/9] kexec: add infrastructure for handling kexec images Date: Tue, 5 Nov 2013 17:39:15 -0500 Message-ID: <52797393.3030704@terremark.com> References: <1381251310-29449-1-git-send-email-david.vrabel@citrix.com> <1381251310-29449-4-git-send-email-david.vrabel@citrix.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============3785677814549833431==" Return-path: In-Reply-To: <1381251310-29449-4-git-send-email-david.vrabel@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: David Vrabel , xen-devel@lists.xen.org Cc: Keir Fraser , Jan Beulich List-Id: xen-devel@lists.xenproject.org --===============3785677814549833431== Content-Type: multipart/alternative; boundary="------------020804090904080207050209" --------------020804090904080207050209 Content-Type: text/plain; charset="ISO-8859-1"; format=flowed Content-Transfer-Encoding: 7bit On 10/08/13 12:55, David Vrabel wrote: > From: David Vrabel > > 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 --------------020804090904080207050209 Content-Type: text/html; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit
On 10/08/13 12:55, David Vrabel wrote:
From: David Vrabel <david.vrabel@citrix.com>

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
--------------020804090904080207050209-- --===============3785677814549833431== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel --===============3785677814549833431==--