[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v4 1/2] x86/mem_sharing: make fork_reset more configurable
- To: Julien Grall <julien@xxxxxxx>
- From: Tamas K Lengyel <tamas@xxxxxxxxxxxxx>
- Date: Tue, 26 Apr 2022 06:45:26 -0400
- Cc: Roger Pau Monné <roger.pau@xxxxxxxxxx>, Tamas K Lengyel <tamas.lengyel@xxxxxxxxx>, Xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Anthony PERARD <anthony.perard@xxxxxxxxxx>, Juergen Gross <jgross@xxxxxxxx>, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, George Dunlap <george.dunlap@xxxxxxxxxx>, Jan Beulich <jbeulich@xxxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, Alexandru Isaila <aisaila@xxxxxxxxxxxxxxx>, Petre Pircalabu <ppircalabu@xxxxxxxxxxxxxxx>
- Delivery-date: Tue, 26 Apr 2022 10:45:44 +0000
- List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
Hi,
On 26/04/2022 09:17, Roger Pau Monné wrote:
> On Mon, Apr 25, 2022 at 11:24:37AM -0400, Tamas K Lengyel wrote:
>> On Mon, Apr 25, 2022 at 10:12 AM Roger Pau Monné <roger.pau@xxxxxxxxxx> wrote:
>>>> diff --git a/xen/common/vm_event.c b/xen/common/vm_event.c
>>>> index 84cf52636b..d26a6699fc 100644
>>>> --- a/xen/common/vm_event.c
>>>> +++ b/xen/common/vm_event.c
>>>> @@ -28,6 +28,11 @@
>>>> #include <asm/p2m.h>
>>>> #include <asm/monitor.h>
>>>> #include <asm/vm_event.h>
>>>> +
>>>> +#ifdef CONFIG_MEM_SHARING
>>>> +#include <asm/mem_sharing.h>
>>>> +#endif
>>>> +
>>>> #include <xsm/xsm.h>
>>>> #include <public/hvm/params.h>
>>>>
>>>> @@ -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));
>>>
>>> Might be appropriate to destroy the domain in case fork reset fails?
>>> ASSERT will only help in debug builds.
>>
>> No, I would prefer not destroying the domain here. If it ever becomes
>> necessary the right way would be to introduce a new monitor event to
>> signal an error and let the listener decide what to do. At the moment
>> I don't see that being necessary as there are no known scenarios where
>> we would be able to setup a fork but fail to reset is.
>
> My concern for raising this was what would happen on non-debug
> builds if mem_sharing_fork_reset() failed, and hence my request to
> crash the domain. I would have used something like:
>
> if ( (reset_state || reset_mem) &&
> mem_sharing_fork_reset(d, reset_state, reset_mem) )
> {
> ASSERT_UNREACHABLE();
> domain_crash(d);
> break;
> }
>
> But if you and other vm_event maintainers are fine with the current
> approach and don't think it's a problem that's OK with me.
The current approach is actually not correct. On production build,
ASSERT() will turn to NOP. IOW mem_sharing_fork_reset() *will* not be
called.
So the call needs to move outside of the ASSERT() and use a construct
similar to what you suggested:
if ( .... && mem_sharing_fork_reset(...) )
{
ASSERT_UNREACHABLE();
break;
}
Ah, good call. Thanks!
Tamas
|