Re: [Xen-devel] [PATCH v11 1/3] xen/mem_sharing: VM forking

[Jan Beulich <jbeulich@xxxxxxxx>]

On 28.02.2020 19:40, Tamas K Lengyel wrote:
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -509,6 +509,12 @@ mfn_t __get_gfn_type_access(struct p2m_domain *p2m, 
> unsigned long gfn_l,
>  
>      mfn = p2m->get_entry(p2m, gfn, t, a, q, page_order, NULL);
>  
> +    /* Check if we need to fork the page */
> +    if ( (q & P2M_ALLOC) && p2m_is_hole(*t) &&
> +         !mem_sharing_fork_page(p2m->domain, gfn, !!(q & P2M_UNSHARE)) )

No need for !! here.

> @@ -588,7 +594,8 @@ struct page_info *p2m_get_page_from_gfn(
>              return page;
>  
>          /* Error path: not a suitable GFN at all */
> -        if ( !p2m_is_ram(*t) && !p2m_is_paging(*t) && !p2m_is_pod(*t) )
> +        if ( !p2m_is_ram(*t) && !p2m_is_paging(*t) && !p2m_is_pod(*t) &&
> +             !mem_sharing_is_fork(p2m->domain) )
>              return NULL;

This looks pretty broad a condition, i.e. all possible types would
make it through here for a fork. Wouldn't it make sense to limit
to to p2m_is_hole() page types, like you check for in
__get_gfn_type_access()?

> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -1269,6 +1269,9 @@ int map_vcpu_info(struct vcpu *v, unsigned long gfn, 
> unsigned offset)
>  
>      v->vcpu_info = new_info;
>      v->vcpu_info_mfn = page_to_mfn(page);
> +#ifdef CONFIG_MEM_SHARING
> +    v->vcpu_info_offset = offset;
> +#endif

Seeing something like this makes me wonder whether forking shouldn't
have its own Kconfig control.

> --- a/xen/include/asm-x86/mem_sharing.h
> +++ b/xen/include/asm-x86/mem_sharing.h
> @@ -39,6 +39,9 @@ struct mem_sharing_domain
>  
>  #define mem_sharing_enabled(d) ((d)->arch.hvm.mem_sharing.enabled)
>  
> +#define mem_sharing_is_fork(d) \
> +    (mem_sharing_enabled(d) && !!((d)->parent))

Again not need for !! (for a different reason).

Also, does the build break if you made this an inline function
(as we generally prefer)?

> @@ -141,6 +148,16 @@ static inline int mem_sharing_notify_enomem(struct 
> domain *d, unsigned long gfn,
>      return -EOPNOTSUPP;
>  }
>  
> +static inline int mem_sharing_fork(struct domain *d, struct domain *cd, bool 
> vcpu)
> +{
> +    return -EOPNOTSUPP;
> +}
> +
> +static inline int mem_sharing_fork_page(struct domain *d, gfn_t gfn, bool 
> lock)
> +{
> +    return -EOPNOTSUPP;
> +}

Can these be reached? If not, please add ASSERT_UNREACHABLE().

> @@ -532,6 +533,10 @@ struct xen_mem_sharing_op {
>                  uint32_t gref;     /* IN: gref to debug         */
>              } u;
>          } debug;
> +        struct mem_sharing_op_fork {      /* OP_FORK */
> +            domid_t parent_domain;        /* IN: parent's domain id */
> +            uint16_t _pad[3];             /* Must be set to 0 */

Especially in the public interface - no new name space
violations please. I.e. please drop the leading underscore.
I also struggle to see why this is an array of three
elements. In fact I don't see why the padding field would be
needed at all - one other union member only gets padded to
its alignment (which is what I'd expect), while others
(presumably older ones) don't have any padding at all. Here
there's no implicit structure's alignment padding that wants
making explicit.

> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -248,6 +248,9 @@ struct vcpu
>  
>      /* Guest-specified relocation of vcpu_info. */
>      mfn_t            vcpu_info_mfn;
> +#ifdef CONFIG_MEM_SHARING
> +    uint32_t         vcpu_info_offset;

There's no need for a fixed width type here afaics - unsigned
int and probably even unsigned short would seem to both be
fine.

Jan

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