Messages in this thread | | | Date | Wed, 28 Sep 2022 17:33:53 +0200 | Subject | Re: [RFC PATCH 0/9] kvm: implement atomic memslot updates | From | David Hildenbrand <> |
| |
On 28.09.22 17:07, Paolo Bonzini wrote: > On 9/27/22 17:58, Sean Christopherson wrote: >> On Tue, Sep 27, 2022, Emanuele Giuseppe Esposito wrote: >>> >>> Am 26/09/2022 um 23:28 schrieb Sean Christopherson: >>>> On Mon, Sep 26, 2022, David Hildenbrand wrote: >>>>> As Sean said "This is an awful lot of a complexity to take on for something >>>>> that appears to be solvable in userspace." >>>> >>>> And if the userspace solution is unpalatable for whatever reason, I'd like to >>>> understand exactly what KVM behavior is problematic for userspace. E.g. the >>>> above RHBZ bug should no longer be an issue as the buggy commit has since been >>>> reverted. >>> >>> It still is because I can reproduce the bug, as also pointed out in >>> multiple comments below. >> >> You can reproduce _a_ bug, but it's obviously not the original bug, because the >> last comment says: >> >> Second, indeed the patch was reverted and somehow accepted without generating >> too much noise: >> >> ... >> >> The underlying issue of course as we both know is still there. >> >> You might have luck reproducing it with this bug >> >> https://bugzilla.redhat.com/show_bug.cgi?id=1855298 >> >> But for me it looks like it is 'working' as well, so you might have >> to write a unit test to trigger the issue. >> >>>> If the issue is KVM doing something nonsensical on a code fetch to MMIO, then I'd >>>> much rather fix _that_ bug and improve KVM's user exit ABI to let userspace handle >>>> the race _if_ userspace chooses not to pause vCPUs. >>>> >>> >>> Also on the BZ they all seem (Paolo included) to agree that the issue is >>> non-atomic memslots update. >> >> Yes, non-atomic memslot likely results in the guest fetching from a GPA without a >> memslot. I'm asking for an explanation of exactly what happens when that occurs, >> because it should be possible to adjust KVM and/or QEMU to play nice with the >> fetch, e.g. to resume the guest until the new memslot is installed, in which case >> an atomic update isn't needed. >> >> I assume the issue is that KVM exits with KVM_EXIT_INTERNAL_ERROR because the >> guest is running at CPL=0, and QEMU kills the guest in response. If that's correct, >> then that problem can be solved by exiting to userspace with KVM_EXIT_MMIO instead >> of KVM_EXIT_INTERNAL_ERROR so that userspace can do something sane in response to >> the MMIO code fetch. >> >> I'm pretty sure this patch will Just Work for QEMU, because QEMU simply resumes >> the vCPU if mmio.len==0. It's a bit of a hack, but I don't think it violates KVM's >> ABI in any way, and it can even become "official" behavior since KVM x86 doesn't >> otherwise exit with mmio.len==0. > > I think this patch is not a good idea for two reasons: > > 1) we don't know how userspace behaves if mmio.len is zero. It is of > course reasonable to do nothing, but an assertion failure is also a > valid behavior > > 2) more important, there is no way to distinguish a failure due to the > guest going in the weeds (and then KVM_EXIT_INTERNAL_ERROR is fine) from > one due to the KVM_SET_USER_MEMORY_REGION race condition. So this will > cause a guest that correctly caused an internal error to loop forever. > > While the former could be handled in a "wait and see" manner, the latter > in particular is part of the KVM_RUN contract. Of course it is possible > for a guest to just loop forever, but in general all of KVM, QEMU and > upper userspace layers want a crashed guest to be detected and stopped > forever. > > Yes, QEMU could loop only if memslot updates are in progress, but > honestly all the alternatives I have seen to atomic memslot updates are > really *awful*. David's patches even invent a new kind of mutex for > which I have absolutely no idea what kind of deadlocks one should worry > about and why they should not exist; QEMU's locking is already pretty > crappy, it's certainly not on my wishlist to make it worse!
Just to comment on that (I'm happy as long as this gets fixed), a simple mutex with trylock should get the thing done as well -- kicking the VCPU if the trylock fails. But I did not look further into locking alternatives.
-- Thanks,
David / dhildenb
| |