On 30.03.2022 15:19, Tamas K Lengyel wrote:
> 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.
If the function gained two or three more flags, how would that look to
you? And how would you easily identify the correct order of all the
booleans?
>>> @@ -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.
IOW no return value being handed back to the requester? The function
does have an error return path, so I don't see why you couldn't return
-EINVAL.
Jan
