[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v11 1/3] xen/mem_sharing: VM forking
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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |