Re: RFC/Proposal: Partial `libxenctrl` API/ABI stabilisation

[Ian Campbell <ian.campbell@citrix.com>]

On Mon, 2015-05-18 at 16:55 +0100, Andrew Cooper wrote:
> On 18/05/15 16:30, Ian Campbell wrote:
> > # Major External Consumers of `libxenctrl`
> >
> > * qemu
> > * kexec tools
> > * in guest tools e.g. users of `libxenstore`, `libvchan`, and by
> >   extension `grant table` and `event channel` functionality. NB:
> >   `libxenstore` is already `SSU` or `SSS` (XXX?)
> >
> > # `libxenctrl` symbols
> >
> > Gathered by:
> >
> >     nm tools/libxc/libxenctrl.so | grep ' [Tt] ' | cut -f 3 -d \ | sort -u
> >
> > `libxenctrl` today exposes many symbols which look to be internal. We
> > should consider also reducing that set by using
> > `__attribute__((visibility("hidden")))`.
> 
> Don't forget libxenguest.so which is built sharing some of the same
> source.  I suspect that quite a few of the libxenctrl symbols could move to

I added a similar section listing things exported by libxenguest.

I don't think libxenguest needs API/ABI stability adding to it at this
point, but it could certainly do with some rationalisation regarding the
split with libxenctrl and the use of its services.

> > The following proposes some functional groupings via some proposed
> > split library names. In some cases we may also wish to consider
> > replacing an API with one which can be properly maintained going
> > forwards. e.g.:
> >
> > - perhaps replacing domctl's used by qemu with new stable
> >   hypercall ABIs and reflecting that in new library APIs.
> > - perhaps exposing more constrained versions of some broad interfaces
> >   for external users.
> >
> > XXX: Change `xc_*` namespacing as well as library names?
> >
> > ## `libxenhypercall`
> >
> > Core open/close interface, "make a hypercall" functionality, hypercall
> > buffers.
> >
> > All other libraries likely depend on this. Applications do as well in
> > order to access open/close interface at least.
> >
> >     - xc_interface_close
> >     - xc_interface_is_fake (???)
> 
> This is used when running something wanting libxc on a non xen system. 
> e.g. readnotes which invokes the Xen elf parsing on a binary, but
> doesn't have any actual hypervisor interaction.

The ??? was whether this needs to be a stable symbol or not. I suspect
it may as well be regardless of whether it needs to be.

> 
> >     - xc_interface_open
> >     - xc_hypercall_buffer_array_create
> >     - xc_hypercall_buffer_array_destroy
> 
> There are surely more than this when it comes to hypercall buffers?

Yes. From the introduction to the document:
        XXX: I haven't yet done a full pass over the list of symbols in
        libxenctrl to categorise them and decide where they belong. I
        thought I would get some early feedback first and just picked a
        few representative examples for each library.

> One issue I have found with libxc in general is that a surprising
> quantity of code is in static inlines in header files, which ends up
> moving into the includee's code. (I had a particular problem with
> do_domctl() and the Xen Interface Version for a project I did a while back)

Yes. I think where possible we should either make these private or
expose them via non-inline functions, I don't think anything really
needs to be inlined.

> > ## `libxengnttab`
> >
> > Interacting with `/dev/xen/gnt{shr,alloc}`
> >
> > XXX two libs or one?
> >
> >     - xc_gntshr_close
> >     - xc_gntshr_munmap
> >     - xc_gntshr_open
> >     - xc_gntshr_share_page_notify
> >     - xc_gntshr_share_pages
> >     - xc_gnttab_close
> >     - xc_gnttab_get_version
> >     - xc_gnttab_map_domain_grant_refs
> >     - xc_gnttab_map_grant_ref
> >     - xc_gnttab_map_grant_ref_notify
> >     - xc_gnttab_map_grant_refs
> >     - xc_gnttab_map_table_v1
> >     - xc_gnttab_map_table_v2
> >     - xc_gnttab_munmap
> >     - xc_gnttab_op
> >     - xc_gnttab_open
> >     - xc_gnttab_set_max_grants
> 
> One probably, given how small it would be.

I agree.

My current diff vs. draft A is below, I'll await a bit more feedback
before posting a proper update.

Ian.

diff --git a/docs/misc/libxenctrl-functions.pandoc b/docs/misc/libxenctrl-functions.pandoc
index ef5ec49..56d93fd 100644
--- a/docs/misc/libxenctrl-functions.pandoc
+++ b/docs/misc/libxenctrl-functions.pandoc
@@ -89,6 +89,13 @@ libxenctrl first.
   extension `grant table` and `event channel` functionality. NB:
   `libxenstore` is already `SSU` or `SSS` (XXX?)
 
+# `libxenguest`
+
+Uses some of the same sources as `libxenctrl`. Could do with some
+rationalisation of what lives where.
+
+Not a candidate for API/ABI stabilith at this point.
+
 # `libxenctrl` symbols
 
 Gathered by:
@@ -97,7 +104,7 @@ Gathered by:
 
 `libxenctrl` today exposes many symbols which look to be internal. We
 should consider also reducing that set by using
-`__attribute__((visibility("hidden")))`.
+`__attribute__((visibility("hidden")))` or a version script.
 
 The following proposes some functional groupings via some proposed
 split library names. In some cases we may also wish to consider
@@ -111,6 +118,10 @@ forwards. e.g.:
 
 XXX: Change `xc_*` namespacing as well as library names?
 
+In additition to these there are many inlines in `xenctrl.h`. Where
+possible these should become either private functions or regular
+functions exported by the library.
+
 ## `libxenhypercall`
 
 Core open/close interface, "make a hypercall" functionality, hypercall
@@ -120,7 +131,7 @@ All other libraries likely depend on this. Applications do as well in
 order to access open/close interface at least.
 
     - xc_interface_close
-    - xc_interface_is_fake (???)
+    - xc_interface_is_fake
     - xc_interface_open
     - xc_hypercall_buffer_array_create
     - xc_hypercall_buffer_array_destroy
@@ -147,8 +158,6 @@ Interacting with `/dev/xen/evtchn`
 
 Interacting with `/dev/xen/gnt{shr,alloc}`
 
-XXX two libs or one?
-
     - xc_gntshr_close
     - xc_gntshr_munmap
     - xc_gntshr_open
@@ -627,6 +636,399 @@ XXX Lots of this should be internal/hidden
     - xc_interface_close_common
     - xc_interface_open_common
 
+# `libxenguest` symbols
+
+Gathered by:
+
+    $ nm tools/libxc/libxenguest.so | grep ' [Tt] ' | cut -f 3 -d \ | sort -u
+
+    - add_full_page
+    - add_to_batch
+    - alloc_magic_pages
+    - alloc_str
+    - alloc_superpage_mfns
+    - amd_xc_cpuid_policy
+    - analysis_phase
+    - apply_batch
+    - arch_setup_bootearly
+    - arch_setup_bootlate
+    - arch_setup_meminit
+    - backup_ptes
+    - bitmap_alloc
+    - bitmap_clear
+    - bitmap_or
+    - bitmap_size
+    - buffer_qemu
+    - buffer_tail
+    - buffer_tail_hvm
+    - buffer_tail_pv
+    - build_assertions
+    - build_hvm_info
+    - call_gmon_start
+    - canonicalize_pagetable
+    - change_pte
+    - check_elf_kernel
+    - check_magic
+    - check_mmio_hole
+    - clear_bit
+    - clear_page
+    - __clear_pte
+    - clear_pte
+    - compat_buffer_qemu
+    - compress_page
+    - copy_mfns_from_guest
+    - count_pgtables
+    - count_pgtables_x86_32_pae
+    - count_pgtables_x86_64
+    - cpuid
+    - cr3_to_mfn
+    - csum_page
+    - deregister_tm_clones
+    - dhdr_type_to_str
+    - do_domctl
+    - __do_global_dtors_aux
+    - __do_global_dtors_aux_fini_array_entry
+    - dump_bad_pseudophysmap_entry
+    - dump_qemu
+    - elf_access_ok
+    - elf_access_unsigned
+    - ELF_ADVANCE_DEST
+    - elf_call_log_callback
+    - elf_check_broken
+    - elf_get_ptr
+    - elf_init
+    - elf_is_elfbinary
+    - elf_load_binary
+    - elf_load_bsdsyms
+    - elf_load_image
+    - elf_lookup_addr
+    - elf_mark_broken
+    - elf_memcpy_safe
+    - elf_memcpy_unchecked
+    - elf_memmove_unchecked
+    - elf_memset_safe
+    - elf_memset_unchecked
+    - elf_note_desc
+    - elf_note_name
+    - elf_note_next
+    - elf_note_numeric
+    - elf_note_numeric_array
+    - elf_parse_binary
+    - elf_parse_bsdsyms
+    - elf_phdr_by_index
+    - elf_phdr_count
+    - elf_phdr_is_loadable
+    - elf_ptrval_in_range
+    - elf_round_up
+    - elf_section_end
+    - elf_section_name
+    - elf_section_start
+    - elf_segment_end
+    - elf_segment_start
+    - elf_set_log
+    - elf_shdr_by_index
+    - elf_shdr_by_name
+    - elf_shdr_count
+    - elf_strfmt
+    - elf_strval
+    - elf_sym_by_index
+    - elf_sym_by_name
+    - elf_xen_addr_calc_check
+    - elf_xen_feature_get
+    - elf_xen_feature_set
+    - elf_xen_note_check
+    - elf_xen_parse
+    - elf_xen_parse_features
+    - elf_xen_parse_guest_info
+    - elf_xen_parse_note
+    - elf_xen_parse_notes
+    - enable_logdirty
+    - expand_p2m
+    - find_table
+    - _fini
+    - flush_batch
+    - frame_dummy
+    - __frame_dummy_init_array_entry
+    - fstat
+    - __fstat
+    - get_cache_page
+    - get_platform_info
+    - get_suspend_file
+    - get_unaligned_le16
+    - get_unaligned_le32
+    - handle_hvm_context
+    - handle_hvm_params
+    - handle_page_data
+    - handle_qemu
+    - handle_shared_info
+    - handle_toolstack
+    - handle_tsc_info
+    - handle_x86_pv_info
+    - handle_x86_pv_p2m_frames
+    - handle_x86_pv_vcpu_blob
+    - _init
+    - intel_xc_cpuid_policy
+    - invalidate_cache_page
+    - is_page_exchangable
+    - launch_vm
+    - le16_to_cpup
+    - le32_to_cpup
+    - llgettimeofday
+    - loadelfimage
+    - loadmodules
+    - load_p2m_frame_list
+    - lock_suspend_event
+    - log_callback
+    - lz4_decompress_unknownoutputsize
+    - lz4_uncompress_unknownoutputsize
+    - map_and_save_p2m_table
+    - map_frame_list_list
+    - map_grant_table_frames
+    - map_p2m
+    - map_shinfo
+    - merge_pte
+    - mfn_in_pseudophysmap
+    - mfn_to_cr3
+    - mfn_to_pfn
+    - modules_init
+    - move_l3_below_4G
+    - noncached_write
+    - normalise_pagetable
+    - nr_page_tables
+    - outbuf_flush
+    - outbuf_free
+    - outbuf_hardwrite
+    - outbuf_init
+    - outbuf_write
+    - pagebuf_free
+    - pagebuf_get
+    - pagebuf_get_one
+    - pagebuf_init
+    - pfn_is_populated
+    - pfn_set_populated
+    - pfn_to_mfn
+    - pin_pagetables
+    - pin_table
+    - populate_pfns
+    - print_mem
+    - print_stats
+    - process_page_data
+    - process_start_info
+    - process_vcpu_basic
+    - process_vcpu_extended
+    - process_vcpu_msrs
+    - process_vcpu_xsave
+    - pte_to_frame
+    - rdexact
+    - read_headers
+    - read_record
+    - rec_type_to_str
+    - register_arch_hooks
+    - register_loader
+    - register_tm_clones
+    - restore
+    - save
+    - save_tsc_info
+    - send_all_pages
+    - send_domain_memory_live
+    - send_domain_memory_nonlive
+    - send_some_pages
+    - set_bit
+    - setup_guest
+    - setup_hypercall_page
+    - setup_pgtables_x86_32_pae
+    - setup_pgtables_x86_64
+    - shared_info_x86_32
+    - shared_info_x86_64
+    - start_info_x86_32
+    - start_info_x86_64
+    - stat
+    - __stat
+    - suspend_and_state
+    - suspend_domain
+    - tailbuf_free
+    - tailbuf_free_hvm
+    - tailbuf_free_pv
+    - test_bit
+    - tv_delta
+    - tv_to_us
+    - uncanonicalize_pagetable
+    - unlock_suspend_event
+    - update_guest_p2m
+    - update_progress_string
+    - __update_pte
+    - update_pte
+    - update_vcpu_context
+    - VALGRIND_PRINTF
+    - VALGRIND_PRINTF_BACKTRACE
+    - vcpu_x86_32
+    - vcpu_x86_64
+    - write_all_vcpu_information
+    - write_batch
+    - write_buffer
+    - write_compressed
+    - write_end_record
+    - write_headers
+    - write_hvm_context
+    - write_hvm_params
+    - write_one_vcpu_basic
+    - write_one_vcpu_extended
+    - write_one_vcpu_msrs
+    - write_one_vcpu_xsave
+    - write_record
+    - write_shared_info
+    - write_split_record
+    - write_toolstack
+    - write_tsc_info
+    - write_uncached
+    - write_x86_pv_info
+    - write_x86_pv_p2m_frames
+    - x86_compat
+    - x86_hvm_cleanup
+    - x86_hvm_end_of_stream
+    - x86_hvm_localise_page
+    - x86_hvm_normalise_page
+    - x86_hvm_pfn_is_valid
+    - x86_hvm_pfn_to_gfn
+    - x86_hvm_process_record
+    - x86_hvm_set_gfn
+    - x86_hvm_set_page_type
+    - x86_hvm_setup
+    - x86_hvm_start_of_stream
+    - x86_hvm_stream_complete
+    - x86_pv_cleanup
+    - x86_pv_domain_info
+    - x86_pv_end_of_stream
+    - x86_pv_localise_page
+    - x86_pv_map_m2p
+    - x86_pv_normalise_page
+    - x86_pv_pfn_is_valid
+    - x86_pv_pfn_to_gfn
+    - x86_pv_process_record
+    - x86_pv_set_gfn
+    - x86_pv_set_page_type
+    - x86_pv_setup
+    - x86_pv_start_of_stream
+    - x86_pv_stream_complete
+    - x86_shadow
+    - xc_await_suspend
+    - xc_clear_domain_page
+    - xc_compression_add_page
+    - xc_compression_compress_pages
+    - xc_compression_create_context
+    - xc_compression_free_context
+    - xc_compression_reset_pagebuf
+    - xc_compression_uncompress_page
+    - xc_cpuid_apply_policy
+    - xc_cpuid_brand_get
+    - xc_cpuid_check
+    - xc_cpuid_config_xsave
+    - xc_cpuid_do_domctl
+    - xc_cpuid_hvm_policy
+    - xc_cpuid_policy
+    - xc_cpuid_pv_policy
+    - xc_cpuid_set
+    - xc_cpuid_to_str
+    - xc_domain_get_native_protocol
+    - xc_domain_restore
+    - xc_domain_restore2
+    - xc_domain_save
+    - xc_domain_save2
+    - xc_dom_allocate
+    - xc_dom_alloc_page
+    - xc_dom_alloc_segment
+    - xc_dom_boot_domU_map
+    - xc_dom_boot_image
+    - xc_dom_boot_mem_init
+    - xc_dom_boot_xen_init
+    - xc_dom_build_image
+    - xc_dom_check_gzip
+    - xc_dom_compat_check
+    - xc_dom_devicetree_file
+    - xc_dom_devicetree_max_size
+    - xc_dom_devicetree_mem
+    - xc_dom_do_gunzip
+    - xc_dom_feature_translated
+    - xc_dom_find_arch_hooks
+    - xc_dom_find_loader
+    - xc_dom_free_all
+    - xc_dom_gnttab_hvm_seed
+    - xc_dom_gnttab_init
+    - xc_dom_gnttab_seed
+    - xc_dom_gnttab_setup
+    - xc_dom_guest_type
+    - xc_dom_kernel_check_size
+    - xc_dom_kernel_file
+    - xc_dom_kernel_max_size
+    - xc_dom_kernel_mem
+    - xc_dom_linux_build
+    - xc_dom_load_bin_kernel
+    - xc_dom_load_bzimage_kernel
+    - xc_dom_load_elf_kernel
+    - xc_dom_load_elf_symtab
+    - xc_dom_loginit
+    - xc_dom_log_memory_footprint
+    - xc_dom_malloc
+    - xc_dom_malloc_filemap
+    - xc_dom_malloc_page_aligned
+    - xc_dom_mem_init
+    - xc_dom_p2m_guest
+    - xc_dom_p2m_host
+    - xc_dom_panic_func
+    - xc_dom_parse_bin_kernel
+    - xc_dom_parse_bzimage_kernel
+    - xc_dom_parse_elf_kernel
+    - xc_dom_parse_image
+    - xc_dom_pfn_to_ptr
+    - xc_dom_pfn_to_ptr_retcount
+    - xc_dom_printf
+    - xc_dom_probe_bin_kernel
+    - xc_dom_probe_bzimage_kernel
+    - xc_dom_probe_elf_kernel
+    - xc_dom_rambase_init
+    - xc_dom_ramdisk_check_size
+    - xc_dom_ramdisk_file
+    - xc_dom_ramdisk_max_size
+    - xc_dom_ramdisk_mem
+    - xc_dom_register_arch_hooks
+    - xc_dom_register_external
+    - xc_dom_register_loader
+    - xc_dom_release
+    - xc_dom_seg_to_ptr
+    - xc_dom_seg_to_ptr_pages
+    - xc_dom_strdup
+    - xc_dom_try_gunzip
+    - xc_dom_unmap_all
+    - xc_dom_unmap_one
+    - xc_dom_update_guest_p2m
+    - xc_dom_vaddr_to_ptr
+    - xc_elf_set_logfile
+    - xc_exchange_page
+    - xc_get_bit_size
+    - xc_hvm_build
+    - xc_hvm_build_target_mem
+    - xc_inflate_buffer
+    - xc_is_page_granted_v1
+    - xc_is_page_granted_v2
+    - xc_linux_build
+    - xc_linux_build_internal
+    - xc_linux_build_mem
+    - xc_map_m2p
+    - xc_mark_page_offline
+    - xc_mark_page_online
+    - xc_pfn_to_mfn
+    - xc_query_page_offline_status
+    - xc_read_image
+    - xc_suspend_evtchn_init_exclusive
+    - xc_suspend_evtchn_init_sane
+    - xc_suspend_evtchn_release
+    - xc_try_bzip2_decode
+    - xc_try_lz4_decode
+    - _xc_try_lzma_decode
+    - xc_try_lzma_decode
+    - xc_try_lzo1x_decode
+    - xc_try_xz_decode
+
 # Symbols used by qemu
 
     $ nm tools/qemu-xen-dir-remote/i386-softmmu/qemu-system-i386 | grep \\bU.xc_