Re: [Xen-devel] [PATCH v2 11/20] x86/mem_sharing: Convert MEM_SHARING_DESTROY_GFN to a bool

[Julien Grall <julien@xxxxxxx>]

Hi Tamas,

On 18/12/2019 19:40, Tamas K Lengyel wrote:
> MEM_SHARING_DESTROY_GFN is used on the 'flags' bitfield during unsharing.
> However, the bitfield is not used for anything else, so just convert it to a
> bool instead.
>
> Signed-off-by: Tamas K Lengyel <tamas.lengyel@xxxxxxxxx>
> ---
>   xen/arch/x86/mm/mem_sharing.c     | 7 +++----
>   xen/arch/x86/mm/p2m.c             | 1 +
>   xen/common/memory.c               | 2 +-
>   xen/include/asm-x86/mem_sharing.h | 5 ++---
>   4 files changed, 7 insertions(+), 8 deletions(-)
>
> diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
> index fc1d8be1eb..6e81e1a895 100644
> --- a/xen/arch/x86/mm/mem_sharing.c
> +++ b/xen/arch/x86/mm/mem_sharing.c
> @@ -1175,7 +1175,7 @@ err_out:
>    */
>   int __mem_sharing_unshare_page(struct domain *d,
>                                  unsigned long gfn,
> -                               uint16_t flags)
> +                               bool destroy)
>   {
>       p2m_type_t p2mt;
>       mfn_t mfn;
> @@ -1231,7 +1231,7 @@ int __mem_sharing_unshare_page(struct domain *d,
>        * If the GFN is getting destroyed drop the references to MFN
>        * (possibly freeing the page), and exit early.
>        */
> -    if ( flags & MEM_SHARING_DESTROY_GFN )
> +    if ( destroy )
>       {
>           if ( !last_gfn )
>               mem_sharing_gfn_destroy(page, d, gfn_info);
> @@ -1321,8 +1321,7 @@ int relinquish_shared_pages(struct domain *d)
>           if ( mfn_valid(mfn) && p2m_is_shared(t) )
>           {
>               /* Does not fail with ENOMEM given the DESTROY flag */
> -            BUG_ON(__mem_sharing_unshare_page(d, gfn,
> -                   MEM_SHARING_DESTROY_GFN));
> +            BUG_ON(__mem_sharing_unshare_page(d, gfn, true));
>               /*
>                * Clear out the p2m entry so no one else may try to
>                * unshare.  Must succeed: we just read the old entry and
> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> index baea632acc..53ea44fe3c 100644
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -517,6 +517,7 @@ mfn_t __get_gfn_type_access(struct p2m_domain *p2m, 
> unsigned long gfn_l,
>            */
>           if ( mem_sharing_unshare_page(p2m->domain, gfn_l) < 0 )
>               mem_sharing_notify_enomem(p2m->domain, gfn_l, false);
> +
This line looks spurious.

>           mfn = p2m->get_entry(p2m, gfn, t, a, q, page_order, NULL);
>       }
>   
> diff --git a/xen/common/memory.c b/xen/common/memory.c
> index 309e872edf..c7d2bac452 100644
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -352,7 +352,7 @@ int guest_remove_page(struct domain *d, unsigned long gmfn)
>            * might be the only one using this shared page, and we need to
>            * trigger proper cleanup. Once done, this is like any other page.
>            */
> -        rc = mem_sharing_unshare_page(d, gmfn, 0);
> +        rc = mem_sharing_unshare_page(d, gmfn);
AFAICT, this patch does not reduce the number of parameters for 
mem_sharing_unshare_page(). Did you intend to make this change in 
another patch?
>           if ( rc )
>           {
>               mem_sharing_notify_enomem(d, gmfn, false);
> diff --git a/xen/include/asm-x86/mem_sharing.h 
> b/xen/include/asm-x86/mem_sharing.h
> index 89cdaccea0..4b982a4803 100644
> --- a/xen/include/asm-x86/mem_sharing.h
> +++ b/xen/include/asm-x86/mem_sharing.h
> @@ -76,17 +76,16 @@ struct page_sharing_info
>   unsigned int mem_sharing_get_nr_saved_mfns(void);
>   unsigned int mem_sharing_get_nr_shared_mfns(void);
>   
> -#define MEM_SHARING_DESTROY_GFN       (1<<1)
>   /* Only fails with -ENOMEM. Enforce it with a BUG_ON wrapper. */
>   int __mem_sharing_unshare_page(struct domain *d,
>                                  unsigned long gfn,
> -                               uint16_t flags);
> +                               bool destroy);
>   
>   static inline
>   int mem_sharing_unshare_page(struct domain *d,
>                                unsigned long gfn)
>   {
> -    int rc = __mem_sharing_unshare_page(d, gfn, 0);
> +    int rc = __mem_sharing_unshare_page(d, gfn, false);
>       BUG_ON(rc && (rc != -ENOMEM));
>       return rc;
>   }
Cheers,

--
Julien Grall

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