Re: [PATCH v4 for-4.14] x86/monitor: revert default behavior when monitoring register write events

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Mon, Jun 8, 2020 at 2:14 PM Razvan Cojocaru
<rcojocaru@xxxxxxxxxxxxxxx> wrote:
>
> On 6/8/20 10:54 PM, Tamas K Lengyel wrote:
> > On Mon, Jun 8, 2020 at 12:58 PM Razvan Cojocaru
> >> And last but not least, the proper sequence is: 1. unsubscribe from
> >> register write events, 2. process all events "still in the chamber"
> >> (keep checking the ring buffer for a while), 3. detach from the guest
> >> (disable the vm_event subsystem). Not ideal perhaps (in that it's not
> >> guaranteed that a VCPU won't resume after a longer period than our
> >> timeout), but if the sequence is followed there should be no guest hangs
> >> or crashes (at least none that we or our clients have observed so far).
> >
> > Incorrect. That's not enough. You also have to wait for all the vCPUs
> > to get scheduled before disabling vm_event or otherwise the emulation
> > is skipped entirely. Please read the patch message. If the user
> > decides to disable vm_event after getting a CR3 event delivered, the
> > CR3 never gets updated and results in the guest crashing in
> > unpredictable ways. Same happens with all the other registers.
>
> I did read the patch message. As I've previously stated ("it's not
> guaranteed that a VCPU won't resume after a longer period than our
> timeout"), it's not ideal, and I've made no claim that the problem does
> not exist or that it shouldn't be fixed - but really if you've got a
> VCPU that will wait more than a couple of seconds to get scheduled then
> you've got a bigger problem with the VM.

Sorry, missed the part where you say you knew about this issue. We
didn't and it was absolutely not documented anywhere and certainly not
mentioned in the original patch that added the feature. It literally
took us years to figure out what the hell is going on. While as you it
can be a couple seconds before its safe to disable it can be a lot
longer depending on the system state, how many VMs are running and how
many vCPUs are active on the VM. There is absolutely necessary
use-cases where you want to keep the VM paused after a CR3 event is
received and exit vm_event afterwards. This bug totally blocks those
use-cases. Not to mention that it's a total mess having an interface
where the user has to guess when its safe to do something. If this was
pointed out when the patch was made I would have objected to that
being merged.

>
> >> So in short, I think there's a better fix for this by simply not
> >> allocating the vm_event memory on-demand anymore and never having to
> >> deal with lost pending emulations again. It should also decrease code
> >> complexity by a tiny bit. Then again, as stated at the beginning of this
> >> message, that's just a recommendation.is
> >
> > Since only you guys use this feature I'm going to wait for a fix.
> > Until then, the default behavior should be restored so this buggy
> > behavior doesn't affect anyone else. You can still turn it on, its
> > just not going to be on for vm_event by default. I don't particularly
> > care what fix is there since only you guys use it. If you don't mind
> > that there is this buggy behavior because you never disable vm_events
> > once you activate it then that's that.
>
> Indeed, our usual scenario is that vm_event is always on on the machine.
> It's only rarely being disabled while the guest is running, and when it
> is, it's always with waiting sufficiently long that we've not seen any
> unexplainable hung VMs.
>
> Fair enough, as long as the previous behaviour is optionally available I
> see no reason why this patch shouldn't make it in - though, of course,
> as also previously stated, I'm just trying to shed as much light as
> possible on this from what I remember, and what happens next is not my
> call. My colleagues should be able to chime in tomorrow.

Looking forward to it.

Thanks,
Tamas