Re: [Xen-devel] [PATCH 1/3] kexec: extend hypercall with improved load/unload ops

[Daniel Kiper <daniel.kiper@xxxxxxxxxx>]

On Thu, Jan 17, 2013 at 02:50:26PM +0000, David Vrabel wrote:
> Can you trim your replies?  It's too hard to find your comments otherwise.

OK.

> On 17/01/13 12:28, Daniel Kiper wrote:
> > On Wed, Jan 16, 2013 at 04:29:04PM +0000, David Vrabel wrote:
> >> From: David Vrabel <david.vrabel@xxxxxxxxxx>
> >>
> >> In the existing kexec hypercall, the load and unload ops depend on
> >> internals of the Linux kernel (the page list and code page provided by
> >> the kernel).  The code page is used to transition between Xen context
> >> and the image so using kernel code doesn't make sense and will not
> >> work for PVH guests.
> >>
> >> Add replacement KEXEC_CMD_kexec_load_v2 and KEXEC_CMD_kexec_unload_v2
> >> ops that no longer require a code page to be provided by the guest --
> >> Xen now provides the code for calling the image directly.
> >>
> [...]
> >> +    if ( image->class == KEXEC_CLASS_32 )
> >> +        compat_machine_kexec(image->entry_maddr);
> >
> > Why do you need that?
>
> image->class controls whether the processor is in 32-bit or 64-bit mode
> when calling the image.  The current implementation only allows images
> to be executed with the same class as dom0.
>
> It's called class because that's the term ELF uses in the ELF header.

As I correctly understand this sets processor mode before new kernel exection.
If yes then it is not needed. Purgatory code (from kexec-tools) does all
needed things. Please check.

> >> +        if ( seg.dest_maddr < kexec_crash_area.start
> >> +             || seg.dest_maddr + seg.size > kexec_crash_area.start + 
> >> kexec_crash_area.size)
> >> +            return -EINVAL;
> >
> > This way you are breaking regular kexec support which
> > does not use prealocated area. As I said earlier you
> > should use kexec code from Linux Kernel (with relevant
> > changes). It has all needed stuff and you do not need
> > to reinvent the wheel.
>
> Yeah. I did say it was a prototype.

OK. I could not find any comment that this feature will be implemented too.
I prefer to say about my thoughts know than later break your work.

> >> +#define KEXEC_CMD_kexec_load_v2        4
> >> +typedef struct xen_kexec_load_v2 {
> >> +    uint8_t type;  /* One of KEXEC_TYPE_* */
> >> +    uint8_t class; /* One of KEXEC_CLASS_* */
> >
> > Why do not use one member called flags (uint32_t or uint64_t)?
> > This way you could add quite easily new flags in the future.
>
> Neither type nor class are flags.

type could be passed as flags (as kexec Linux Kernel implementation does)
which gives us more flexibility if we need more flags in the future.
class is not needed as I stated above.

Daniel

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