Re: [Xen-devel] [PATCH 7/7] xen/mm: Clean up share_xen_page_with_guest() API

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Fri, Mar 09, 2018 at 01:18:42PM +0000, Andrew Cooper wrote:
> The share_xen_page_with_guest() functions are used by common code, and are
> implemented the same by each arch.  Move the declarations into the common mm.h
> rather than duplicating them in each arch/mm.h
> 
> Turn an int readonly into a boolean enum, to retain ro/rw context at the
> callsites, but use shorter labels which avoids a large number of split lines.
> 
> Implement share_xen_page_with_privileged_guests() as a static inline wrapper
> around share_xen_page_with_guest() to avoid having a call into a separate
> translation unit whose only purpose is to shuffle function arguments.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> CC: Julien Grall <julien.grall@xxxxxxx>
> CC: George Dunlap <george.dunlap@xxxxxxxxxxxxx>
> CC: Tim Deegan <tim@xxxxxxx>
> CC: Wei Liu <wei.liu2@xxxxxxxxxx>
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> ---
>  xen/arch/arm/domain.c             |  3 +--
>  xen/arch/arm/mm.c                 | 13 ++++---------
>  xen/arch/x86/domain.c             |  3 +--
>  xen/arch/x86/hvm/dom0_build.c     |  2 +-
>  xen/arch/x86/hvm/vmx/vmx.c        |  2 +-
>  xen/arch/x86/mm.c                 | 20 +++++++-------------
>  xen/arch/x86/pv/shim.c            |  6 ++----
>  xen/arch/x86/x86_64/mm.c          | 16 ++++++----------
>  xen/common/trace.c                |  9 +++------
>  xen/common/xenoprof.c             |  3 +--
>  xen/include/asm-arm/grant_table.h |  3 +--
>  xen/include/asm-arm/mm.h          |  7 -------
>  xen/include/asm-x86/grant_table.h |  6 ++----
>  xen/include/asm-x86/mm.h          |  8 --------
>  xen/include/xen/mm.h              | 14 ++++++++++++++
>  15 files changed, 44 insertions(+), 71 deletions(-)
> 
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 4b45fad..23dac5d 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -602,8 +602,7 @@ int arch_domain_create(struct domain *d,
>          goto fail;
>  
>      clear_page(d->shared_info);
> -    share_xen_page_with_guest(
> -        virt_to_page(d->shared_info), d, XENSHARE_writable);
> +    share_xen_page_with_guest(virt_to_page(d->shared_info), d, SHARE_rw);
>  
>      switch ( config->config.gic_version )
>      {
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index a09bea2..baa3b0d 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -1187,8 +1187,8 @@ unsigned long domain_get_maximum_gpfn(struct domain *d)
>      return gfn_x(d->arch.p2m.max_mapped_gfn);
>  }
>  
> -void share_xen_page_with_guest(struct page_info *page,
> -                          struct domain *d, int readonly)
> +void share_xen_page_with_guest(struct page_info *page, struct domain *d,
> +                               enum XENSHARE_flags flags)

Naming this _flags feels wrong to me, I would assume flags to be
something which can be used as (SHARE_r | SHARE_w) (ie: stacked) and
so on. I would maybe name this XENSHARE_options rather than flags.

TBH I would be OK with renaming the parameter to "bool ro/readonly"
and let the callers use true and false directly. It seems like
over-engineering to use an enum for this, or maybe you have further
changes in mind that are going to expand the set of options?

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel