Re: Kexec and libxenctlr.so

[Wei Liu <wl@xxxxxxx>]

On Thu, Jul 02, 2020 at 06:34:48PM +0100, Julien Grall wrote:
> Hi Wei,
> 
> On 26/06/2020 12:08, Wei Liu wrote:
> > On Thu, Jun 11, 2020 at 03:57:37PM +0100, Julien Grall wrote:
> > > Hi all,
> > > 
> > > kexec-tools has an option to load dynamically libxenctlr.so (not .so.4.x)
> > > (see [1]).
> > > 
> > > Given that the library has never been considered stable, it is probably a
> > > disaster that is waiting to happen.
> > > 
> > > Looking at the tree kexec is using the follow libxc functions:
> > >     - xc_kexec_get_range()
> > >     - xc_kexec_load()
> > >     - xc_kexec_unload()
> > >     - xc_kexec_status()
> > >     - xc_kexec_exec()
> > >     - xc_version()
> > >     - xc_interface_open()
> > >     - xc_interface_close()
> > >     - xc_get_max_cpus()
> > >     - xc_get_machine_memory_map()
> > > 
> > > I think it is uncontroversial that we want a new stable library for all 
> > > the
> > > xc_kexec_* functions (maybe libxenexec)?
> > 
> > That sounds fine to me.
> > 
> > Looking at the list of functions, all the xc_kexec_* ones are probably
> > already rather stable.
> 
> That's my understanding as well.
> 
> Although, we may want to rethink some of the hypercalls (such as
> KEXEC_cmd_kexec_get_range) in the future as they have different layout
> between 32-bit and 64-bit. Thanksfully this wasn't exposed outside of libxc,
> so it shouldn't be an issue to have a stable library.
> 

Oh, that's good to hear.

> > 
> > For xc_interface_open / close, they are perhaps used only to obtain an
> > xc handle such that it can be used to make hypercalls. You new kexec
> > library is going to expose its own handle with a xencall handle wrapped
> > inside, so you can do away with an xc handle.
> 
> I have already a PoC for the new library. I had to tweak a bit the list of
> helpers as some use hypercalls arguments directly. Below, the proposed
> interface:
> 
> /* Callers who don't care don't need to #include <xentoollog.h> */
> struct xentoollog_logger;
> 
> typedef struct xenkexec_handle xenkexec_handle;
> 
> typedef struct xenkexec_segments xenkexec_segments;
> 
> xenkexec_handle *xenkexec_open(struct xentoollog_logger *logger,
>                                unsigned int open_flags);
> int xenkexec_close(xenkexec_handle *khdl);
> 
> int xenkexec_exec(xenkexec_handle *khdl, int type);
> int xenkexec_get_range(xenkexec_handle *khdl, int range, int nr,
>                        uint64_t *size, uint64_t *start);
> int xenkexec_load(xenkexec_handle *khdl, uint8_t type, uint16_t arch,
>                   uint64_t entry_maddr, uint32_t nr_segments,
>                   xenkexec_segments *segments);
> int xenkexec_unload(xenkexec_handle *khdl, int type);
> int xenkexec_status(xenkexec_handle *khdl, int type);
> 
> xenkexec_segments *xenkexec_allocate_segments(xenkexec_handle *khdl,
>                                               unsigned int nr);
> void xenkexec_free_segments(xenkexec_handle *khdl, xenkexec_segments *segs);
> 
> int xenkexec_update_segment(xenkexec_handle *khdl, xenkexec_segments *segs,
>                             unsigned int idx, void *buffer, size_t
> buffer_size,
>                             uint64_t dest_maddr, size_t dest_size);
> 

You definitely have more experience in kexec than I do. This list looks
sensible.

> 
> > 
> > > 
> > > However I am not entirely sure where to put the others.
> > > 
> > > I am thinking to introduce libxensysctl for xc_get_max_cpus() as it is a
> > > XEN_SYSCTL. We could possibly include xc_get_machine_memory_map() (despite
> > > it is a XENMEM_).
> > > 
> > 
> > Introducing an libxensysctl before we stabilise sysctl interface seems
> > wrong to me. We can bury the call inside libxenkexec itself for the time
> > being.
> 
> That would work for me.
> 
> > 
> > > For xc_version(), I am thinking to extend libxentoolcore to also include
> > > "stable xen API".
> > > 
> > 
> > If you can do without an xc handle, do you still need to call
> > xc_version?
> 
> Looking at kexec, xc_version() is used by crashdump to determine which
> architecture is used by Xen (in this case 32-bit x86 vs 64-bit x86).
> 
> The was introcuded by commit:
> 
> commit cdbc9b011fe43407908632d842e3a39e495e48d9
> Author: Ian Campbell <ian.campbell@xxxxxxxxxxxxx>
> Date:   Fri Mar 16 10:10:24 2007 +0000
> 
>     Set crash dump ELF header e_machine field based on underlying hypervisor
> architecture.
> 
>     This is necessary when running Xen with a 64 bit hypervisor and 32 bit
>     domain 0 since the CPU crash notes will be 64 bit.
> 
>     Detecting the hypervisor archiecture requires libxenctrl and therefore
> this
>     support is optional and disabled by default.
> 
>     Signed-off-by: Ian Campbell <ian.campbell@xxxxxxxxxxxxx>
>     Acked-by: Magnus Damm <magnus@xxxxxxxxxxxxx>
>     Signed-off-by: Simon Horman <horms@xxxxxxxxxxxx>
> 
> As we drop support for 32-bit Xen quite a long time ago, we may be able to
> remove the call to xc_version().
> 

Does Arm care about the bitness of the hypervisor?

Wei.

> Cheers,
> 
> -- 
> Julien Grall