On Wed, Mar 30, 2022, 6:31 AM Jan Beulich <[email protected]> wrote:
> On 29.03.2022 16:03, Tamas K Lengyel wrote:
> > --- a/xen/arch/x86/include/asm/mem_sharing.h
> > +++ b/xen/arch/x86/include/asm/mem_sharing.h
> > @@ -85,6 +85,9 @@ static inline bool mem_sharing_is_fork(const struct
> domain *d)
> > int mem_sharing_fork_page(struct domain *d, gfn_t gfn,
> > bool unsharing);
> >
> > +int mem_sharing_fork_reset(struct domain *d, bool reset_state,
> > + bool reset_memory);
>
> Please avoid passing multiple booleans, even more so when you already
> derive them from a single "flags" value. This would likely be easiest
> if you re-used the VM_EVENT_FLAG_RESET_FORK_* values also for
> XENMEM_FORK_RESET_*, with suitable BUILD_BUG_ON() put in place to
> prove they're the same.
>
I don't see why that would be an improvement in any way. I also don't want
to make VM_EVENT flags tied to the XENMEM ones as they are separate
interfaces. I rather just drop the changes to the XENMEM interface then do
that.
> > --- a/xen/arch/x86/mm/mem_sharing.c
> > +++ b/xen/arch/x86/mm/mem_sharing.c
> > @@ -1890,15 +1890,24 @@ static int fork(struct domain *cd, struct domain
> *d, uint16_t flags)
> > * footprints the hypercall continuation should be implemented (or if
> this
> > * feature needs to be become "stable").
> > */
> > -static int mem_sharing_fork_reset(struct domain *d)
> > +int mem_sharing_fork_reset(struct domain *d, bool reset_state,
> > + bool reset_memory)
> > {
> > - int rc;
> > + int rc = 0;
> > struct domain *pd = d->parent;
> > struct p2m_domain *p2m = p2m_get_hostp2m(d);
> > struct page_info *page, *tmp;
> >
> > + ASSERT(reset_state || reset_memory);
> > +
> > + if ( !d->arch.hvm.mem_sharing.fork_complete )
> > + return -ENOSYS;
>
> Either EOPNOTSUPP (in case you think this operation could make sense
> to implement for incomplete forks) or e.g. EINVAL, but please not
> ENOSYS.
>
> > @@ -394,6 +399,16 @@ static int vm_event_resume(struct domain *d, struct
> vm_event_domain *ved)
> > if ( rsp.reason == VM_EVENT_REASON_MEM_PAGING )
> > p2m_mem_paging_resume(d, &rsp);
> > #endif
> > +#ifdef CONFIG_MEM_SHARING
> > + if ( mem_sharing_is_fork(d) )
> > + {
> > + bool reset_state = rsp.flags &
> VM_EVENT_FLAG_RESET_FORK_STATE;
> > + bool reset_mem = rsp.flags &
> VM_EVENT_FLAG_RESET_FORK_MEMORY;
> > +
> > + if ( reset_state || reset_mem )
> > + ASSERT(!mem_sharing_fork_reset(d, reset_state,
> reset_mem));
> > + }
> > +#endif
>
> Should the two flags be rejected in the "else" case, rather than
> being silently ignored?
>
What do you mean by rejected? There is no feasible "rejection" that could
be done in this path other then skipping it.
> > --- a/xen/include/public/memory.h
> > +++ b/xen/include/public/memory.h
> > @@ -541,12 +541,14 @@ struct xen_mem_sharing_op {
> > uint32_t gref; /* IN: gref to debug */
> > } u;
> > } debug;
> > - struct mem_sharing_op_fork { /* OP_FORK */
> > + struct mem_sharing_op_fork { /* OP_FORK/_RESET */
>
> I consider the notation in the comment misleading - I would read it to
> mean OP_FORK and OP_RESET, supported by the earlier
> OP_SHARE/ADD_PHYSMAP. Commonly we write OP_FORK{,_RESET} in such cases.
Ack.
Tamas
>
