thr3ads.net - Xen devel - [PATCH 3/8] kexec: add public interface for improved load/unload sub-ops [Feb 2013]

If this information is useful, please help other people find it:
Share via:

David Vrabel

2013-Feb-21 17:48 UTC

[PATCH 3/8] kexec: add public interface for improved load/unload sub-ops

From: David Vrabel <david.vrabel@citrix.com>

Add replacement KEXEC_CMD_load and KEXEC_CMD_unload sub-ops to the
kexec hypercall.  These new sub-ops allow a priviledged guest to
provide the image data to be loaded into Xen memory or the crash
region instead of guests loading the image data themselves and
providing the relocation code and metadata.

The old interface is provided to guests requesting an interface
version prior to 4.3.

Signed-off: David Vrabel <david.vrabel@citrix.com>
---
 xen/common/kexec.c         |   12 ++++----
 xen/include/public/kexec.h |   66 +++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 68 insertions(+), 10 deletions(-)

diff --git a/xen/common/kexec.c b/xen/common/kexec.c
index 6dd20c6..2cbb62c 100644
--- a/xen/common/kexec.c
+++ b/xen/common/kexec.c
@@ -732,7 +732,7 @@ static void crash_save_vmcoreinfo(void)
 #endif
 }
 
-static int kexec_load_unload_internal(unsigned long op, xen_kexec_load_t *load)
+static int kexec_load_unload_internal(unsigned long op, xen_kexec_load_v1_t
*load)
 {
     xen_kexec_image_t *image;
     int base, bit, pos;
@@ -779,7 +779,7 @@ static int kexec_load_unload_internal(unsigned long op,
xen_kexec_load_t *load)
 
 static int kexec_load_unload(unsigned long op, XEN_GUEST_HANDLE_PARAM(void)
uarg)
 {
-    xen_kexec_load_t load;
+    xen_kexec_load_v1_t load;
 
     if ( unlikely(copy_from_guest(&load, uarg, 1)) )
         return -EFAULT;
@@ -791,8 +791,8 @@ static int kexec_load_unload_compat(unsigned long op,
                                     XEN_GUEST_HANDLE_PARAM(void) uarg)
 {
 #ifdef CONFIG_COMPAT
-    compat_kexec_load_t compat_load;
-    xen_kexec_load_t load;
+    compat_kexec_load_v1_t compat_load;
+    xen_kexec_load_v1_t load;
 
     if ( unlikely(copy_from_guest(&compat_load, uarg, 1)) )
         return -EFAULT;
@@ -864,8 +864,8 @@ static int do_kexec_op_internal(unsigned long op,
         else
                 ret = kexec_get_range(uarg);
         break;
-    case KEXEC_CMD_kexec_load:
-    case KEXEC_CMD_kexec_unload:
+    case KEXEC_CMD_kexec_load_v1:
+    case KEXEC_CMD_kexec_unload_v1:
         spin_lock_irqsave(&kexec_lock, flags);
         if (!test_bit(KEXEC_FLAG_IN_PROGRESS, &kexec_flags))
         {
diff --git a/xen/include/public/kexec.h b/xen/include/public/kexec.h
index 61a8d7d..5259446 100644
--- a/xen/include/public/kexec.h
+++ b/xen/include/public/kexec.h
@@ -116,12 +116,12 @@ typedef struct xen_kexec_exec {
  * type  == KEXEC_TYPE_DEFAULT or KEXEC_TYPE_CRASH [in]
  * image == relocation information for kexec (ignored for unload) [in]
  */
-#define KEXEC_CMD_kexec_load            1
-#define KEXEC_CMD_kexec_unload          2
-typedef struct xen_kexec_load {
+#define KEXEC_CMD_kexec_load_v1         1 /* obsolete since 0x00040300 */
+#define KEXEC_CMD_kexec_unload_v1       2 /* obsolete since 0x00040300 */
+typedef struct xen_kexec_load_v1 {
     int type;
     xen_kexec_image_t image;
-} xen_kexec_load_t;
+} xen_kexec_load_v1_t;
 
 #define KEXEC_RANGE_MA_CRASH      0 /* machine address and size of crash area
*/
 #define KEXEC_RANGE_MA_XEN        1 /* machine address and size of Xen itself
*/
@@ -152,6 +152,64 @@ typedef struct xen_kexec_range {
     unsigned long start;
 } xen_kexec_range_t;
 
+#if __XEN_INTERFACE_VERSION__ >= 0x00040300
+/*
+ * A contiguous chunk of a kexec image and it''s destination machine
+ * address.
+ */
+typedef struct xen_kexec_segment {
+    XEN_GUEST_HANDLE_64(const_void) buf;
+    uint64_t buf_size;
+    uint64_t dest_maddr;
+    uint64_t dest_size;
+} xen_kexec_segment_t;
+DEFINE_XEN_GUEST_HANDLE(xen_kexec_segment_t);
+
+/*
+ * Load a kexec image into memory.
+ *
+ * For KEXEC_TYPE_DEFAULT images, the segments may be anywhere in RAM.
+ * The image is relocated prior to being executed.
+ *
+ * For KEXEC_TYPE_CRASH images, each segment of the image must reside
+ * in the memory region reserved for kexec (KEXEC_RANGE_MA_CRASH) and
+ * the entry point must be within the image. The caller is responsible
+ * for ensuring that multiple images do not overlap.
+ */
+
+#define KEXEC_CMD_kexec_load 4
+typedef struct xen_kexec_load {
+    uint8_t  type;        /* One of KEXEC_TYPE_* */
+    uint16_t arch;        /* ELF machine type (EM_*). */
+    uint32_t __pad;
+    uint64_t entry_maddr; /* image entry point machine address. */
+    uint32_t nr_segments;
+    XEN_GUEST_HANDLE_64(xen_kexec_segment_t) segments;
+} xen_kexec_load_t;
+DEFINE_XEN_GUEST_HANDLE(xen_kexec_load_t);
+
+/*
+ * Unload a kexec image.
+ *
+ * Type must be one of KEXEC_TYPE_DEFAULT or KEXEC_TYPE_CRASH.
+ */
+#define KEXEC_CMD_kexec_unload 5
+typedef struct xen_kexec_unload {
+    uint8_t type;
+} xen_kexec_unload_t;
+DEFINE_XEN_GUEST_HANDLE(xen_kexec_unload_t);
+
+#else /* __XEN_INTERFACE_VERSION__ < 0x00040300 */
+
+#undef KEXEC_CMD_kexec_load
+#undef KEXEC_CMD_kexec_unload
+#define KEXEC_CMD_kexec_load KEXEC_CMD_kexec_load_v1
+#define KEXEC_CMD_kexec_unload KEXEC_CMD_kexec_unload_v1
+
+typedef struct xen_kexec_load_v1_t xen_kexec_load_t;
+
+#endif
+
 #endif /* _XEN_PUBLIC_KEXEC_H */
 
 /*
-- 
1.7.2.5

Daniel Kiper

2013-Feb-21 22:29 UTC

head link

Re: [PATCH 3/8] kexec: add public interface for improved load/unload sub-ops

On Thu, Feb 21, 2013 at 05:48:09PM +0000, David Vrabel
wrote:> From: David Vrabel <david.vrabel@citrix.com>
>
> Add replacement KEXEC_CMD_load and KEXEC_CMD_unload sub-ops to the
> kexec hypercall.  These new sub-ops allow a priviledged guest to
> provide the image data to be loaded into Xen memory or the crash
> region instead of guests loading the image data themselves and
> providing the relocation code and metadata.
>
> The old interface is provided to guests requesting an interface
> version prior to 4.3.
>
> Signed-off: David Vrabel <david.vrabel@citrix.com>
> ---
>  xen/common/kexec.c         |   12 ++++----
>  xen/include/public/kexec.h |   66
+++++++++++++++++++++++++++++++++++++++++--
>  2 files changed, 68 insertions(+), 10 deletions(-)
>
> diff --git a/xen/common/kexec.c b/xen/common/kexec.c
> index 6dd20c6..2cbb62c 100644
> --- a/xen/common/kexec.c
> +++ b/xen/common/kexec.c
> @@ -732,7 +732,7 @@ static void crash_save_vmcoreinfo(void)
>  #endif
>  }
>
> -static int kexec_load_unload_internal(unsigned long op, xen_kexec_load_t
*load)
> +static int kexec_load_unload_internal(unsigned long op,
xen_kexec_load_v1_t *load)
>  {
>      xen_kexec_image_t *image;
>      int base, bit, pos;
> @@ -779,7 +779,7 @@ static int kexec_load_unload_internal(unsigned long op,
xen_kexec_load_t *load)
>
>  static int kexec_load_unload(unsigned long op,
XEN_GUEST_HANDLE_PARAM(void) uarg)
>  {
> -    xen_kexec_load_t load;
> +    xen_kexec_load_v1_t load;
>
>      if ( unlikely(copy_from_guest(&load, uarg, 1)) )
>          return -EFAULT;
> @@ -791,8 +791,8 @@ static int kexec_load_unload_compat(unsigned long op,
>                                      XEN_GUEST_HANDLE_PARAM(void) uarg)
>  {
>  #ifdef CONFIG_COMPAT
> -    compat_kexec_load_t compat_load;
> -    xen_kexec_load_t load;
> +    compat_kexec_load_v1_t compat_load;
> +    xen_kexec_load_v1_t load;
>
>      if ( unlikely(copy_from_guest(&compat_load, uarg, 1)) )
>          return -EFAULT;
> @@ -864,8 +864,8 @@ static int do_kexec_op_internal(unsigned long op,
>          else
>                  ret = kexec_get_range(uarg);
>          break;
> -    case KEXEC_CMD_kexec_load:
> -    case KEXEC_CMD_kexec_unload:
> +    case KEXEC_CMD_kexec_load_v1:
> +    case KEXEC_CMD_kexec_unload_v1:
>          spin_lock_irqsave(&kexec_lock, flags);
>          if (!test_bit(KEXEC_FLAG_IN_PROGRESS, &kexec_flags))
>          {
> diff --git a/xen/include/public/kexec.h b/xen/include/public/kexec.h
> index 61a8d7d..5259446 100644
> --- a/xen/include/public/kexec.h
> +++ b/xen/include/public/kexec.h
> @@ -116,12 +116,12 @@ typedef struct xen_kexec_exec {
>   * type  == KEXEC_TYPE_DEFAULT or KEXEC_TYPE_CRASH [in]
>   * image == relocation information for kexec (ignored for unload) [in]
>   */
> -#define KEXEC_CMD_kexec_load            1
> -#define KEXEC_CMD_kexec_unload          2
> -typedef struct xen_kexec_load {
> +#define KEXEC_CMD_kexec_load_v1         1 /* obsolete since 0x00040300 */
> +#define KEXEC_CMD_kexec_unload_v1       2 /* obsolete since 0x00040300 */
> +typedef struct xen_kexec_load_v1 {
>      int type;
>      xen_kexec_image_t image;
> -} xen_kexec_load_t;
> +} xen_kexec_load_v1_t;
>
>  #define KEXEC_RANGE_MA_CRASH      0 /* machine address and size of crash
area */
>  #define KEXEC_RANGE_MA_XEN        1 /* machine address and size of Xen
itself */
> @@ -152,6 +152,64 @@ typedef struct xen_kexec_range {
>      unsigned long start;
>  } xen_kexec_range_t;
>
> +#if __XEN_INTERFACE_VERSION__ >= 0x00040300
> +/*
> + * A contiguous chunk of a kexec image and it''s destination
machine
> + * address.
> + */
> +typedef struct xen_kexec_segment {
> +    XEN_GUEST_HANDLE_64(const_void) buf;
> +    uint64_t buf_size;
> +    uint64_t dest_maddr;
> +    uint64_t dest_size;
> +} xen_kexec_segment_t;
> +DEFINE_XEN_GUEST_HANDLE(xen_kexec_segment_t);
> +
> +/*
> + * Load a kexec image into memory.
> + *
> + * For KEXEC_TYPE_DEFAULT images, the segments may be anywhere in RAM.
> + * The image is relocated prior to being executed.
> + *
> + * For KEXEC_TYPE_CRASH images, each segment of the image must reside
> + * in the memory region reserved for kexec (KEXEC_RANGE_MA_CRASH) and
> + * the entry point must be within the image. The caller is responsible
> + * for ensuring that multiple images do not overlap.
What do you mean by "The caller is responsible for ensuring
that multiple images do not overlap."?
> + */
> +
> +#define KEXEC_CMD_kexec_load 4
> +typedef struct xen_kexec_load {
> +    uint8_t  type;        /* One of KEXEC_TYPE_* */
> +    uint16_t arch;        /* ELF machine type (EM_*). */
> +    uint32_t __pad;
Why do you need __pad here?
> +    uint64_t entry_maddr; /* image entry point machine address. */
> +    uint32_t nr_segments;
> +    XEN_GUEST_HANDLE_64(xen_kexec_segment_t) segments;
> +} xen_kexec_load_t;
> +DEFINE_XEN_GUEST_HANDLE(xen_kexec_load_t);
> +
> +/*
> + * Unload a kexec image.
> + *
> + * Type must be one of KEXEC_TYPE_DEFAULT or KEXEC_TYPE_CRASH.
> + */
> +#define KEXEC_CMD_kexec_unload 5
> +typedef struct xen_kexec_unload {
> +    uint8_t type;
> +} xen_kexec_unload_t;
> +DEFINE_XEN_GUEST_HANDLE(xen_kexec_unload_t);
> +
> +#else /* __XEN_INTERFACE_VERSION__ < 0x00040300 */
> +
> +#undef KEXEC_CMD_kexec_load
> +#undef KEXEC_CMD_kexec_unload
> +#define KEXEC_CMD_kexec_load KEXEC_CMD_kexec_load_v1
> +#define KEXEC_CMD_kexec_unload KEXEC_CMD_kexec_unload_v1
Could you define all constants in one place at the
beginning of this file? It is very difficult to
see what is going on. Especially those undefs are
crazy for me.

Daniel

Jan Beulich

2013-Feb-22 08:33 UTC

head link

Re: [PATCH 3/8] kexec: add public interface for improved load/unload sub-ops

>>> On 21.02.13 at 18:48, David Vrabel <david.vrabel@citrix.com>
wrote:
> --- a/xen/include/public/kexec.h
> +++ b/xen/include/public/kexec.h
> @@ -116,12 +116,12 @@ typedef struct xen_kexec_exec {
>   * type  == KEXEC_TYPE_DEFAULT or KEXEC_TYPE_CRASH [in]
>   * image == relocation information for kexec (ignored for unload) [in]
>   */
> -#define KEXEC_CMD_kexec_load            1
> -#define KEXEC_CMD_kexec_unload          2
> -typedef struct xen_kexec_load {
> +#define KEXEC_CMD_kexec_load_v1         1 /* obsolete since 0x00040300 */
> +#define KEXEC_CMD_kexec_unload_v1       2 /* obsolete since 0x00040300 */
> +typedef struct xen_kexec_load_v1 {
>      int type;
>      xen_kexec_image_t image;
> -} xen_kexec_load_t;
> +} xen_kexec_load_v1_t;
You can''t change type names like this without also guarding them
with a __XEN_INTERFACE_VERSION__ conditional or providing
backward compatibility #define-s.
>  
>  #define KEXEC_RANGE_MA_CRASH      0 /* machine address and size of crash
area */
>  #define KEXEC_RANGE_MA_XEN        1 /* machine address and size of Xen
itself */
> @@ -152,6 +152,64 @@ typedef struct xen_kexec_range {
>      unsigned long start;
>  } xen_kexec_range_t;
>  
> +#if __XEN_INTERFACE_VERSION__ >= 0x00040300
> +/*
> + * A contiguous chunk of a kexec image and it''s destination
machine
> + * address.
> + */
> +typedef struct xen_kexec_segment {
> +    XEN_GUEST_HANDLE_64(const_void) buf;
> +    uint64_t buf_size;
> +    uint64_t dest_maddr;
> +    uint64_t dest_size;
> +} xen_kexec_segment_t;
> +DEFINE_XEN_GUEST_HANDLE(xen_kexec_segment_t);
> +
> +/*
> + * Load a kexec image into memory.
> + *
> + * For KEXEC_TYPE_DEFAULT images, the segments may be anywhere in RAM.
> + * The image is relocated prior to being executed.
> + *
> + * For KEXEC_TYPE_CRASH images, each segment of the image must reside
> + * in the memory region reserved for kexec (KEXEC_RANGE_MA_CRASH) and
> + * the entry point must be within the image. The caller is responsible
> + * for ensuring that multiple images do not overlap.
> + */
> +
> +#define KEXEC_CMD_kexec_load 4
> +typedef struct xen_kexec_load {
> +    uint8_t  type;        /* One of KEXEC_TYPE_* */
uint8_t __pad1;
> +    uint16_t arch;        /* ELF machine type (EM_*). */
> +    uint32_t __pad;
Put nr_segments here instead?
> +    uint64_t entry_maddr; /* image entry point machine address. */
> +    uint32_t nr_segments;
> +    XEN_GUEST_HANDLE_64(xen_kexec_segment_t) segments;
> +} xen_kexec_load_t;
> +DEFINE_XEN_GUEST_HANDLE(xen_kexec_load_t);
Jan

David Vrabel

2013-Feb-22 11:49 UTC

head link

Re: [PATCH 3/8] kexec: add public interface for improved load/unload sub-ops

On 21/02/13 22:29, Daniel Kiper wrote:> On Thu, Feb 21, 2013 at 05:48:09PM +0000, David Vrabel wrote:
>> From: David Vrabel <david.vrabel@citrix.com>
>>
>> Add replacement KEXEC_CMD_load and KEXEC_CMD_unload sub-ops to the
>> kexec hypercall.  These new sub-ops allow a priviledged guest to
>> provide the image data to be loaded into Xen memory or the crash
>> region instead of guests loading the image data themselves and
>> providing the relocation code and metadata.
>>
>> The old interface is provided to guests requesting an interface
>> version prior to 4.3.
>>
[...]>> +/*
>> + * Load a kexec image into memory.
>> + *
>> + * For KEXEC_TYPE_DEFAULT images, the segments may be anywhere in RAM.
>> + * The image is relocated prior to being executed.
>> + *
>> + * For KEXEC_TYPE_CRASH images, each segment of the image must reside
>> + * in the memory region reserved for kexec (KEXEC_RANGE_MA_CRASH) and
>> + * the entry point must be within the image. The caller is responsible
>> + * for ensuring that multiple images do not overlap.
> 
> What do you mean by "The caller is responsible for ensuring
> that multiple images do not overlap."?
The intention here is to allow for safe replacement of a crash image by
loading the second image at a different location in the crash region.

This won''t actually work however, as the control pages (also allocated
from the crash region) will conflict.

This is the behaviour of the Linux implementation.  It''s less than
ideal
and something I plan to look at later on (it''s low priority as
replacing
crash images isn''t an interesting use case).
>> + */
>> +
>> +#define KEXEC_CMD_kexec_load 4
>> +typedef struct xen_kexec_load {
>> +    uint8_t  type;        /* One of KEXEC_TYPE_* */
>> +    uint16_t arch;        /* ELF machine type (EM_*). */
>> +    uint32_t __pad;
> 
> Why do you need __pad here?
To ensure that the following uint64_t is aligned to 8 bytes in both 32
and 64-bit.

Annoyingly uint64_t only has 4 byte alignment on 32-bit architectures.
>> +    uint64_t entry_maddr; /* image entry point machine address. */
>> +    uint32_t nr_segments;
>> +    XEN_GUEST_HANDLE_64(xen_kexec_segment_t) segments;
>> +} xen_kexec_load_t;
>> +DEFINE_XEN_GUEST_HANDLE(xen_kexec_load_t);
>> +
>> +/*
>> + * Unload a kexec image.
>> + *
>> + * Type must be one of KEXEC_TYPE_DEFAULT or KEXEC_TYPE_CRASH.
>> + */
>> +#define KEXEC_CMD_kexec_unload 5
>> +typedef struct xen_kexec_unload {
>> +    uint8_t type;
>> +} xen_kexec_unload_t;
>> +DEFINE_XEN_GUEST_HANDLE(xen_kexec_unload_t);
>> +
>> +#else /* __XEN_INTERFACE_VERSION__ < 0x00040300 */
>> +
>> +#undef KEXEC_CMD_kexec_load
>> +#undef KEXEC_CMD_kexec_unload
>> +#define KEXEC_CMD_kexec_load KEXEC_CMD_kexec_load_v1
>> +#define KEXEC_CMD_kexec_unload KEXEC_CMD_kexec_unload_v1
> 
> Could you define all constants in one place at the
> beginning of this file? It is very difficult to
> see what is going on. Especially those undefs are
> crazy for me.
I was copying the style used for sched_op_compat.

David

David Vrabel

2013-Feb-22 11:50 UTC

head link

Re: [PATCH 3/8] kexec: add public interface for improved load/unload sub-ops

On 22/02/13 08:33, Jan Beulich wrote:>>>> On 21.02.13 at 18:48, David Vrabel
<david.vrabel@citrix.com> wrote:
>> --- a/xen/include/public/kexec.h
>> +++ b/xen/include/public/kexec.h
>> @@ -116,12 +116,12 @@ typedef struct xen_kexec_exec {
>>   * type  == KEXEC_TYPE_DEFAULT or KEXEC_TYPE_CRASH [in]
>>   * image == relocation information for kexec (ignored for unload) [in]
>>   */
>> -#define KEXEC_CMD_kexec_load            1
>> -#define KEXEC_CMD_kexec_unload          2
>> -typedef struct xen_kexec_load {
>> +#define KEXEC_CMD_kexec_load_v1         1 /* obsolete since 0x00040300
*/
>> +#define KEXEC_CMD_kexec_unload_v1       2 /* obsolete since 0x00040300
*/
>> +typedef struct xen_kexec_load_v1 {
>>      int type;
>>      xen_kexec_image_t image;
>> -} xen_kexec_load_t;
>> +} xen_kexec_load_v1_t;
> 
> You can''t change type names like this without also guarding them
> with a __XEN_INTERFACE_VERSION__ conditional or providing
> backward compatibility #define-s.
There are backward compatible definitions provided at the end of the
header file.

I will probably refactor this as it confused Daniel as well.

David

Jan Beulich

2013-Feb-22 13:09 UTC

head link

Re: [PATCH 3/8] kexec: add public interface for improved load/unload sub-ops

>>> On 22.02.13 at 12:50, David Vrabel <david.vrabel@citrix.com>
wrote:
> On 22/02/13 08:33, Jan Beulich wrote:
>>>>> On 21.02.13 at 18:48, David Vrabel
<david.vrabel@citrix.com> wrote:
>>> --- a/xen/include/public/kexec.h
>>> +++ b/xen/include/public/kexec.h
>>> @@ -116,12 +116,12 @@ typedef struct xen_kexec_exec {
>>>   * type  == KEXEC_TYPE_DEFAULT or KEXEC_TYPE_CRASH [in]
>>>   * image == relocation information for kexec (ignored for unload)
[in]
>>>   */
>>> -#define KEXEC_CMD_kexec_load            1
>>> -#define KEXEC_CMD_kexec_unload          2
>>> -typedef struct xen_kexec_load {
>>> +#define KEXEC_CMD_kexec_load_v1         1 /* obsolete since
0x00040300 */
>>> +#define KEXEC_CMD_kexec_unload_v1       2 /* obsolete since
0x00040300 */
>>> +typedef struct xen_kexec_load_v1 {
>>>      int type;
>>>      xen_kexec_image_t image;
>>> -} xen_kexec_load_t;
>>> +} xen_kexec_load_v1_t;
>> 
>> You can''t change type names like this without also guarding
them
>> with a __XEN_INTERFACE_VERSION__ conditional or providing
>> backward compatibility #define-s.
> 
> There are backward compatible definitions provided at the end of the
> header file.
There''s a typedef producing xen_kexec_load_t, but there''s no
way for a consumer to use struct xen_kexec_load afaics.

Jan

Daniel Kiper

2013-Mar-08 10:50 UTC

head link

Re: [PATCH 3/8] kexec: add public interface for improved load/unload sub-ops

On Thu, Feb 21, 2013 at 05:48:09PM +0000, David Vrabel
wrote:> From: David Vrabel <david.vrabel@citrix.com>
>
> Add replacement KEXEC_CMD_load and KEXEC_CMD_unload sub-ops to the
> kexec hypercall.  These new sub-ops allow a priviledged guest to
> provide the image data to be loaded into Xen memory or the crash
> region instead of guests loading the image data themselves and
> providing the relocation code and metadata.
>
> The old interface is provided to guests requesting an interface
> version prior to 4.3.
>
> Signed-off: David Vrabel <david.vrabel@citrix.com>
[...]
> diff --git a/xen/include/public/kexec.h b/xen/include/public/kexec.h
> index 61a8d7d..5259446 100644
> --- a/xen/include/public/kexec.h
> +++ b/xen/include/public/kexec.h
> @@ -116,12 +116,12 @@ typedef struct xen_kexec_exec {
>   * type  == KEXEC_TYPE_DEFAULT or KEXEC_TYPE_CRASH [in]
>   * image == relocation information for kexec (ignored for unload) [in]
>   */
> -#define KEXEC_CMD_kexec_load            1
> -#define KEXEC_CMD_kexec_unload          2
> -typedef struct xen_kexec_load {
> +#define KEXEC_CMD_kexec_load_v1         1 /* obsolete since 0x00040300 */
> +#define KEXEC_CMD_kexec_unload_v1       2 /* obsolete since 0x00040300 */
> +typedef struct xen_kexec_load_v1 {
>      int type;
>      xen_kexec_image_t image;
> -} xen_kexec_load_t;
> +} xen_kexec_load_v1_t;
I think that this is not good idea to redefine meaning of constants,
types, structures, etc. IMO it is comparable to redefining meaning
of words in any laguage (e.g. English). It will be very confusing
and may easily lead to stupid bugs. I think that old interface should
stay as is (with its bad behavior). New interface should be introduced
with "_v2" suffix, e.g. KEXEC_CMD_kexec_load_v2, ...
This would not confuse our descendants.

Daniel

David Vrabel

2013-Mar-08 11:52 UTC

head link

Re: [PATCH 3/8] kexec: add public interface for improved load/unload sub-ops

On 08/03/13 10:50, Daniel Kiper wrote:> On Thu, Feb 21, 2013 at 05:48:09PM +0000, David Vrabel wrote:
>> From: David Vrabel <david.vrabel@citrix.com>
>>
>> Add replacement KEXEC_CMD_load and KEXEC_CMD_unload sub-ops to the
>> kexec hypercall.  These new sub-ops allow a priviledged guest to
>> provide the image data to be loaded into Xen memory or the crash
>> region instead of guests loading the image data themselves and
>> providing the relocation code and metadata.
>>
>> The old interface is provided to guests requesting an interface
>> version prior to 4.3.
>>
>> Signed-off: David Vrabel <david.vrabel@citrix.com>
> 
> [...]
> 
>> diff --git a/xen/include/public/kexec.h b/xen/include/public/kexec.h
>> index 61a8d7d..5259446 100644
>> --- a/xen/include/public/kexec.h
>> +++ b/xen/include/public/kexec.h
>> @@ -116,12 +116,12 @@ typedef struct xen_kexec_exec {
>>   * type  == KEXEC_TYPE_DEFAULT or KEXEC_TYPE_CRASH [in]
>>   * image == relocation information for kexec (ignored for unload) [in]
>>   */
>> -#define KEXEC_CMD_kexec_load            1
>> -#define KEXEC_CMD_kexec_unload          2
>> -typedef struct xen_kexec_load {
>> +#define KEXEC_CMD_kexec_load_v1         1 /* obsolete since 0x00040300
*/
>> +#define KEXEC_CMD_kexec_unload_v1       2 /* obsolete since 0x00040300
*/
>> +typedef struct xen_kexec_load_v1 {
>>      int type;
>>      xen_kexec_image_t image;
>> -} xen_kexec_load_t;
>> +} xen_kexec_load_v1_t;
> 
> I think that this is not good idea to redefine meaning of constants,
> types, structures, etc. IMO it is comparable to redefining meaning
> of words in any laguage (e.g. English). It will be very confusing
> and may easily lead to stupid bugs. I think that old interface should
> stay as is (with its bad behavior). New interface should be introduced
> with "_v2" suffix, e.g. KEXEC_CMD_kexec_load_v2, ...
> This would not confuse our descendants.
This is something that was requested (by Ian C) as the Xen way of doing it.

David

Daniel Kiper

2013-Mar-08 12:28 UTC

head link

Re: [PATCH 3/8] kexec: add public interface for improved load/unload sub-ops

On Fri, Mar 08, 2013 at 11:52:21AM +0000, David Vrabel
wrote:> On 08/03/13 10:50, Daniel Kiper wrote:
> > On Thu, Feb 21, 2013 at 05:48:09PM +0000, David Vrabel wrote:
> >> From: David Vrabel <david.vrabel@citrix.com>
> >>
> >> Add replacement KEXEC_CMD_load and KEXEC_CMD_unload sub-ops to the
> >> kexec hypercall.  These new sub-ops allow a priviledged guest to
> >> provide the image data to be loaded into Xen memory or the crash
> >> region instead of guests loading the image data themselves and
> >> providing the relocation code and metadata.
> >>
> >> The old interface is provided to guests requesting an interface
> >> version prior to 4.3.
> >>
> >> Signed-off: David Vrabel <david.vrabel@citrix.com>
> >
> > [...]
> >
> >> diff --git a/xen/include/public/kexec.h
b/xen/include/public/kexec.h
> >> index 61a8d7d..5259446 100644
> >> --- a/xen/include/public/kexec.h
> >> +++ b/xen/include/public/kexec.h
> >> @@ -116,12 +116,12 @@ typedef struct xen_kexec_exec {
> >>   * type  == KEXEC_TYPE_DEFAULT or KEXEC_TYPE_CRASH [in]
> >>   * image == relocation information for kexec (ignored for unload)
[in]
> >>   */
> >> -#define KEXEC_CMD_kexec_load            1
> >> -#define KEXEC_CMD_kexec_unload          2
> >> -typedef struct xen_kexec_load {
> >> +#define KEXEC_CMD_kexec_load_v1         1 /* obsolete since
0x00040300 */
> >> +#define KEXEC_CMD_kexec_unload_v1       2 /* obsolete since
0x00040300 */
> >> +typedef struct xen_kexec_load_v1 {
> >>      int type;
> >>      xen_kexec_image_t image;
> >> -} xen_kexec_load_t;
> >> +} xen_kexec_load_v1_t;
> >
> > I think that this is not good idea to redefine meaning of constants,
> > types, structures, etc. IMO it is comparable to redefining meaning
> > of words in any laguage (e.g. English). It will be very confusing
> > and may easily lead to stupid bugs. I think that old interface should
> > stay as is (with its bad behavior). New interface should be introduced
> > with "_v2" suffix, e.g. KEXEC_CMD_kexec_load_v2, ...
> > This would not confuse our descendants.
>
> This is something that was requested (by Ian C) as the Xen way of doing it.
Yes, I remember but still do not agree with that idea in general.
Maybe discussion on kexec interface is good point to change
that Xen community behavior? Ian?

Daniel

Jan Beulich

2013-Mar-08 12:36 UTC

head link

Re: [PATCH 3/8] kexec: add public interface for improved load/unload sub-ops

>>> On 08.03.13 at 13:28, Daniel Kiper <daniel.kiper@oracle.com>
wrote:
> On Fri, Mar 08, 2013 at 11:52:21AM +0000, David Vrabel wrote:
>> On 08/03/13 10:50, Daniel Kiper wrote:
>> > On Thu, Feb 21, 2013 at 05:48:09PM +0000, David Vrabel wrote:
>> >> From: David Vrabel <david.vrabel@citrix.com>
>> >>
>> >> Add replacement KEXEC_CMD_load and KEXEC_CMD_unload sub-ops to
the
>> >> kexec hypercall.  These new sub-ops allow a priviledged guest
to
>> >> provide the image data to be loaded into Xen memory or the
crash
>> >> region instead of guests loading the image data themselves and
>> >> providing the relocation code and metadata.
>> >>
>> >> The old interface is provided to guests requesting an
interface
>> >> version prior to 4.3.
>> >>
>> >> Signed-off: David Vrabel <david.vrabel@citrix.com>
>> >
>> > [...]
>> >
>> >> diff --git a/xen/include/public/kexec.h
b/xen/include/public/kexec.h
>> >> index 61a8d7d..5259446 100644
>> >> --- a/xen/include/public/kexec.h
>> >> +++ b/xen/include/public/kexec.h
>> >> @@ -116,12 +116,12 @@ typedef struct xen_kexec_exec {
>> >>   * type  == KEXEC_TYPE_DEFAULT or KEXEC_TYPE_CRASH [in]
>> >>   * image == relocation information for kexec (ignored for
unload) [in]
>> >>   */
>> >> -#define KEXEC_CMD_kexec_load            1
>> >> -#define KEXEC_CMD_kexec_unload          2
>> >> -typedef struct xen_kexec_load {
>> >> +#define KEXEC_CMD_kexec_load_v1         1 /* obsolete since
0x00040300 */
>> >> +#define KEXEC_CMD_kexec_unload_v1       2 /* obsolete since
0x00040300 */
>> >> +typedef struct xen_kexec_load_v1 {
>> >>      int type;
>> >>      xen_kexec_image_t image;
>> >> -} xen_kexec_load_t;
>> >> +} xen_kexec_load_v1_t;
>> >
>> > I think that this is not good idea to redefine meaning of
constants,
>> > types, structures, etc. IMO it is comparable to redefining meaning
>> > of words in any laguage (e.g. English). It will be very confusing
>> > and may easily lead to stupid bugs. I think that old interface
should
>> > stay as is (with its bad behavior). New interface should be
introduced
>> > with "_v2" suffix, e.g. KEXEC_CMD_kexec_load_v2, ...
>> > This would not confuse our descendants.
>>
>> This is something that was requested (by Ian C) as the Xen way of doing
it.
> 
> Yes, I remember but still do not agree with that idea in general.
> Maybe discussion on kexec interface is good point to change
> that Xen community behavior? Ian?
Together with all consumers being expected to properly make
use of __XEN_INTERFACE_VERSION__ (or else they get the
lowest possible definitions), I don''t think there''s a big
issue here
as long as all old definitions retain their meaning when said
symbol is set low enough.

Jan

Daniel Kiper

2013-Mar-08 15:34 UTC

head link

Re: [PATCH 3/8] kexec: add public interface for improved load/unload sub-ops

On Fri, Mar 08, 2013 at 12:36:16PM +0000, Jan Beulich
wrote:> >>> On 08.03.13 at 13:28, Daniel Kiper
<daniel.kiper@oracle.com> wrote:
> > On Fri, Mar 08, 2013 at 11:52:21AM +0000, David Vrabel wrote:
> >> On 08/03/13 10:50, Daniel Kiper wrote:
> >> > On Thu, Feb 21, 2013 at 05:48:09PM +0000, David Vrabel wrote:
> >> >> From: David Vrabel <david.vrabel@citrix.com>
> >> >>
> >> >> Add replacement KEXEC_CMD_load and KEXEC_CMD_unload
sub-ops to the
> >> >> kexec hypercall.  These new sub-ops allow a priviledged
guest to
> >> >> provide the image data to be loaded into Xen memory or
the crash
> >> >> region instead of guests loading the image data
themselves and
> >> >> providing the relocation code and metadata.
> >> >>
> >> >> The old interface is provided to guests requesting an
interface
> >> >> version prior to 4.3.
> >> >>
> >> >> Signed-off: David Vrabel <david.vrabel@citrix.com>
> >> >
> >> > [...]
> >> >
> >> >> diff --git a/xen/include/public/kexec.h
b/xen/include/public/kexec.h
> >> >> index 61a8d7d..5259446 100644
> >> >> --- a/xen/include/public/kexec.h
> >> >> +++ b/xen/include/public/kexec.h
> >> >> @@ -116,12 +116,12 @@ typedef struct xen_kexec_exec {
> >> >>   * type  == KEXEC_TYPE_DEFAULT or KEXEC_TYPE_CRASH [in]
> >> >>   * image == relocation information for kexec (ignored
for unload) [in]
> >> >>   */
> >> >> -#define KEXEC_CMD_kexec_load            1
> >> >> -#define KEXEC_CMD_kexec_unload          2
> >> >> -typedef struct xen_kexec_load {
> >> >> +#define KEXEC_CMD_kexec_load_v1         1 /* obsolete
since 0x00040300 */
> >> >> +#define KEXEC_CMD_kexec_unload_v1       2 /* obsolete
since 0x00040300 */
> >> >> +typedef struct xen_kexec_load_v1 {
> >> >>      int type;
> >> >>      xen_kexec_image_t image;
> >> >> -} xen_kexec_load_t;
> >> >> +} xen_kexec_load_v1_t;
> >> >
> >> > I think that this is not good idea to redefine meaning of
constants,
> >> > types, structures, etc. IMO it is comparable to redefining
meaning
> >> > of words in any laguage (e.g. English). It will be very
confusing
> >> > and may easily lead to stupid bugs. I think that old
interface should
> >> > stay as is (with its bad behavior). New interface should be
introduced
> >> > with "_v2" suffix, e.g. KEXEC_CMD_kexec_load_v2,
...
> >> > This would not confuse our descendants.
> >>
> >> This is something that was requested (by Ian C) as the Xen way of
doing it.
> >
> > Yes, I remember but still do not agree with that idea in general.
> > Maybe discussion on kexec interface is good point to change
> > that Xen community behavior? Ian?
>
> Together with all consumers being expected to properly make
> use of __XEN_INTERFACE_VERSION__ (or else they get the
> lowest possible definitions), I don''t think there''s a big
issue here
> as long as all old definitions retain their meaning when said
> symbol is set low enough.
It is good for dropping or adding some functionalities. However, I still
do not agree that it is sufficient solution in this case. Here we are
redefining interface. I know that was done many times but it does not
mean this is good. Additionally, compilter does not care. It reads everything.
However, for me (I suppose at least) it is difficult read code which have
same "words" with meaning depending only on placement. I would like to
read
sources without thinking where I am. That is all.

Daniel

Xen devel - Feb 2013 - [PATCH 3/8] kexec: add public interface for improved load/unload sub-ops

[PATCH 3/8] kexec: add public interface for improved load/unload sub-ops

Re: [PATCH 3/8] kexec: add public interface for improved load/unload sub-ops

Re: [PATCH 3/8] kexec: add public interface for improved load/unload sub-ops

Re: [PATCH 3/8] kexec: add public interface for improved load/unload sub-ops

Re: [PATCH 3/8] kexec: add public interface for improved load/unload sub-ops

Re: [PATCH 3/8] kexec: add public interface for improved load/unload sub-ops

Re: [PATCH 3/8] kexec: add public interface for improved load/unload sub-ops

Re: [PATCH 3/8] kexec: add public interface for improved load/unload sub-ops

Re: [PATCH 3/8] kexec: add public interface for improved load/unload sub-ops

Re: [PATCH 3/8] kexec: add public interface for improved load/unload sub-ops

Re: [PATCH 3/8] kexec: add public interface for improved load/unload sub-ops