RE: [PATCH] tools/libs/ctrl: add and export xc_memory_op

["Lengyel, Tamas" <tamas.lengyel@xxxxxxxxx>]

> -----Original Message-----
> From: Juergen Gross <jgross@xxxxxxxx>
> Sent: Thursday, May 19, 2022 10:31 AM
> To: Lengyel, Tamas <tamas.lengyel@xxxxxxxxx>; xen-
> devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Wei Liu <wl@xxxxxxx>; Anthony PERARD <anthony.perard@xxxxxxxxxx>;
> Cooper, Andrew <andrew.cooper3@xxxxxxxxxx>
> Subject: Re: [PATCH] tools/libs/ctrl: add and export xc_memory_op
> 
> On 19.05.22 15:59, Lengyel, Tamas wrote:
> >
> >
> >> -----Original Message-----
> >> From: Xen-devel <xen-devel-bounces@xxxxxxxxxxxxxxxxxxxx> On Behalf Of
> >> Juergen Gross
> >> Sent: Thursday, May 19, 2022 9:33 AM
> >> To: Lengyel, Tamas <tamas.lengyel@xxxxxxxxx>;
> >> xen-devel@xxxxxxxxxxxxxxxxxxxx
> >> Cc: Wei Liu <wl@xxxxxxx>; Anthony PERARD
> <anthony.perard@xxxxxxxxxx>;
> >> Cooper, Andrew <andrew.cooper3@xxxxxxxxxx>
> >> Subject: Re: [PATCH] tools/libs/ctrl: add and export xc_memory_op
> >>
> >> On 19.05.22 15:27, Tamas K Lengyel wrote:
> >>> Add and export xc_memory_op so that do_memory_op can be used by
> >>> tools linking with libxc. This is effectively in the same spirit as
> >>> the existing xc_domctl and xc_sysctl functions, which are already
> exported.
> >>>
> >>> In this patch we move do_memory_op into xc_private.h as a static
> >>> inline function and convert its 'cmd' input from int to unsigned int
> >>> to accurately reflect what the hypervisor expects. No other changes
> >>> are made
> >> to the function.
> >>>
> >>> Signed-off-by: Tamas K Lengyel <tamas.lengyel@xxxxxxxxx>
> >>> ---
> >>>    tools/include/xenctrl.h      |  1 +
> >>>    tools/libs/ctrl/xc_private.c | 63 +++---------------------------------
> >>>    tools/libs/ctrl/xc_private.h | 58 ++++++++++++++++++++++++++++++++-
> >>>    3 files changed, 63 insertions(+), 59 deletions(-)
> >>>
> >>> diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h index
> >>> 95bd5eca67..484e354412 100644
> >>> --- a/tools/include/xenctrl.h
> >>> +++ b/tools/include/xenctrl.h
> >>> @@ -1597,6 +1597,7 @@ int xc_vmtrace_set_option(xc_interface *xch,
> >>> uint32_t domid,
> >>>
> >>>    int xc_domctl(xc_interface *xch, struct xen_domctl *domctl);
> >>>    int xc_sysctl(xc_interface *xch, struct xen_sysctl *sysctl);
> >>> +long xc_memory_op(xc_interface *xch, unsigned int cmd, void *arg,
> >>> +size_t len);
> >>>
> >>>    int xc_version(xc_interface *xch, int cmd, void *arg);
> >>>
> >>> diff --git a/tools/libs/ctrl/xc_private.c
> >>> b/tools/libs/ctrl/xc_private.c index c0422662f0..6a247d2b1f 100644
> >>> --- a/tools/libs/ctrl/xc_private.c
> >>> +++ b/tools/libs/ctrl/xc_private.c
> >>> @@ -326,64 +326,6 @@ int xc_flush_mmu_updates(xc_interface *xch,
> >>> struct
> >> xc_mmu *mmu)
> >>>        return flush_mmu_updates(xch, mmu);
> >>>    }
> >>>
> >>> -long do_memory_op(xc_interface *xch, int cmd, void *arg, size_t
> >>> len)
> >>
> >> Why don't you just rename this function and modify the users to use
> >> the new name?
> >
> > For two reasons:
> > 1) having the do_memory_op as a static inline is consistent with how
> do_domctl and do_sysctl are implemented, so logically that's what I would
> expect to see for the memory_op hypercall as well.
> 
> It is much more complicated than the do_domctl and do_sysctl inlines.
> 
> Additionally it is being used by libxenguest, so making it an inline would
> expose lots of libxenctrl internals to libxenguest.
> 
> > 2) the patch itself is cleaner because there is no churn in all the files 
> > that
> previously called do_memory_op.
> 
> OTOH all callers are in Xen, so its no deal to change those.

I'm fine with going the pure rename route  if that's what's preferred.

Tamas